| Message ID | 1611488647-12478-4-git-send-email-stefanc@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: mvpp2: Add TX Flow Control support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 5 this patch: 5 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 143 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 5 this patch: 5 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sun, Jan 24, 2021 at 01:43:52PM +0200, stefanc@marvell.com wrote: > +static int mvpp2_get_sram(struct platform_device *pdev, > + struct mvpp2 *priv) > +{ > + struct device_node *dn = pdev->dev.of_node; > + static bool defer_once; > + struct resource *res; > + > + if (has_acpi_companion(&pdev->dev)) { > + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); > + if (!res) { > + dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n"); > + return 0; > + } > + priv->cm3_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(priv->cm3_base)) > + return PTR_ERR(priv->cm3_base); > + } else { > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0); > + if (!priv->sram_pool) { > + if (!defer_once) { > + defer_once = true; > + /* Try defer once */ > + return -EPROBE_DEFER; > + } > + dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n"); > + return -ENOMEM; > + } Above, priv->sram_pool will only be non-NULL if the pool has been initialised. > +err_cm3: > + if (!has_acpi_companion(&pdev->dev) && priv->cm3_base) > + gen_pool_free(priv->sram_pool, (unsigned long)priv->cm3_base, > + MSS_SRAM_SIZE); So wouldn't: if (priv->sram_pool && priv->cm3_base) be more appropriate here? > + if (!has_acpi_companion(&pdev->dev) && priv->cm3_base) { > + gen_pool_free(priv->sram_pool, (unsigned long)priv->cm3_base, > + MSS_SRAM_SIZE); > + gen_pool_destroy(priv->sram_pool); > + } Same here. Why is it correct to call gen_pool_destroy() in the remove path but not the error path? I think you want to drop this - the pool is created and destroyed by the SRAM driver, users of it should not be destroying it.
On Sun, Jan 24, 2021 at 01:43:52PM +0200, stefanc@marvell.com wrote: > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0); > + if (!priv->sram_pool) { > + if (!defer_once) { > + defer_once = true; > + /* Try defer once */ > + return -EPROBE_DEFER; > + } > + dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n"); > + return -ENOMEM; > + } > + priv->cm3_base = (void __iomem *)gen_pool_alloc(priv->sram_pool, > + MSS_SRAM_SIZE); > + if (!priv->cm3_base) > + return -ENOMEM; This probably could do with a comment indicating that it is reliant on this allocation happening at offset zero into the SRAM. The only reason that is guaranteed _at the moment_ is because the SRAM mapping is 0x800 bytes in size, and you are requesting 0x800 bytes in this allocation, so allocating the full size.
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index a07cf60..c969a2d 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -25,6 +25,7 @@ > #include <linux/of_net.h> > #include <linux/of_address.h> > #include <linux/of_device.h> > +#include <linux/genalloc.h> > #include <linux/phy.h> > #include <linux/phylink.h> > #include <linux/phy/phy.h> > @@ -91,6 +92,18 @@ static inline u32 mvpp2_cpu_to_thread(struct mvpp2 *priv, int cpu) > return cpu % priv->nthreads; > } > > +static inline > +void mvpp2_cm3_write(struct mvpp2 *priv, u32 offset, u32 data) > +{ > + writel(data, priv->cm3_base + offset); > +} > + > +static inline > +u32 mvpp2_cm3_read(struct mvpp2 *priv, u32 offset) > +{ > + return readl(priv->cm3_base + offset); > +} No inline functions in .c file please. Let the compiler decide. Andrew
On Sun, Jan 24, 2021 at 12:44:43PM +0000, Russell King - ARM Linux admin wrote: > On Sun, Jan 24, 2021 at 01:43:52PM +0200, stefanc@marvell.com wrote: > > + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0); > > + if (!priv->sram_pool) { > > + if (!defer_once) { > > + defer_once = true; > > + /* Try defer once */ > > + return -EPROBE_DEFER; > > + } > > + dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n"); > > + return -ENOMEM; > > + } > > + priv->cm3_base = (void __iomem *)gen_pool_alloc(priv->sram_pool, > > + MSS_SRAM_SIZE); > > + if (!priv->cm3_base) > > + return -ENOMEM; > > This probably could do with a comment indicating that it is reliant on > this allocation happening at offset zero into the SRAM. The only reason > that is guaranteed _at the moment_ is because the SRAM mapping is 0x800 > bytes in size, and you are requesting 0x800 bytes in this allocation, > so allocating the full size. Hi Russell I'm wondering if using a pool even makes sense. The ACPI case just ioremap() the memory region. Either this memory is dedicated, and then there is no need to use a pool, or the memory is shared, and at some point the ACPI code is going to run into problems when some other driver also wants access. Andrew
> > #include <linux/of_net.h> > > #include <linux/of_address.h> > > #include <linux/of_device.h> > > +#include <linux/genalloc.h> > > #include <linux/phy.h> > > #include <linux/phylink.h> > > #include <linux/phy/phy.h> > > @@ -91,6 +92,18 @@ static inline u32 mvpp2_cpu_to_thread(struct > mvpp2 *priv, int cpu) > > return cpu % priv->nthreads; > > } > > > > +static inline > > +void mvpp2_cm3_write(struct mvpp2 *priv, u32 offset, u32 data) { > > + writel(data, priv->cm3_base + offset); } > > + > > +static inline > > +u32 mvpp2_cm3_read(struct mvpp2 *priv, u32 offset) { > > + return readl(priv->cm3_base + offset); } > > No inline functions in .c file please. Let the compiler decide. > > Andrew Ok, I would fix. Thanks, Stefan.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h index 6bd7e40..aec9179 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h @@ -748,6 +748,9 @@ #define MVPP2_TX_FIFO_THRESHOLD(kb) \ ((kb) * 1024 - MVPP2_TX_FIFO_THRESHOLD_MIN) +/* MSS Flow control */ +#define MSS_SRAM_SIZE 0x800 + /* RX buffer constants */ #define MVPP2_SKB_SHINFO_SIZE \ SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) @@ -925,6 +928,7 @@ struct mvpp2 { /* Shared registers' base addresses */ void __iomem *lms_base; void __iomem *iface_base; + void __iomem *cm3_base; /* On PPv2.2, each "software thread" can access the base * register through a separate address space, each 64 KB apart @@ -996,6 +1000,9 @@ struct mvpp2 { /* page_pool allocator */ struct page_pool *page_pool[MVPP2_PORT_MAX_RXQ]; + + /* CM3 SRAM pool */ + struct gen_pool *sram_pool; }; struct mvpp2_pcpu_stats { diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index a07cf60..c969a2d 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -25,6 +25,7 @@ #include <linux/of_net.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/genalloc.h> #include <linux/phy.h> #include <linux/phylink.h> #include <linux/phy/phy.h> @@ -91,6 +92,18 @@ static inline u32 mvpp2_cpu_to_thread(struct mvpp2 *priv, int cpu) return cpu % priv->nthreads; } +static inline +void mvpp2_cm3_write(struct mvpp2 *priv, u32 offset, u32 data) +{ + writel(data, priv->cm3_base + offset); +} + +static inline +u32 mvpp2_cm3_read(struct mvpp2 *priv, u32 offset) +{ + return readl(priv->cm3_base + offset); +} + static struct page_pool * mvpp2_create_page_pool(struct device *dev, int num, int len, enum dma_data_direction dma_dir) @@ -6846,6 +6859,41 @@ static int mvpp2_init(struct platform_device *pdev, struct mvpp2 *priv) return 0; } +static int mvpp2_get_sram(struct platform_device *pdev, + struct mvpp2 *priv) +{ + struct device_node *dn = pdev->dev.of_node; + static bool defer_once; + struct resource *res; + + if (has_acpi_companion(&pdev->dev)) { + res = platform_get_resource(pdev, IORESOURCE_MEM, 2); + if (!res) { + dev_warn(&pdev->dev, "ACPI is too old, Flow control not supported\n"); + return 0; + } + priv->cm3_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(priv->cm3_base)) + return PTR_ERR(priv->cm3_base); + } else { + priv->sram_pool = of_gen_pool_get(dn, "cm3-mem", 0); + if (!priv->sram_pool) { + if (!defer_once) { + defer_once = true; + /* Try defer once */ + return -EPROBE_DEFER; + } + dev_warn(&pdev->dev, "DT is too old, Flow control not supported\n"); + return -ENOMEM; + } + priv->cm3_base = (void __iomem *)gen_pool_alloc(priv->sram_pool, + MSS_SRAM_SIZE); + if (!priv->cm3_base) + return -ENOMEM; + } + return 0; +} + static int mvpp2_probe(struct platform_device *pdev) { const struct acpi_device_id *acpi_id; @@ -6902,6 +6950,13 @@ static int mvpp2_probe(struct platform_device *pdev) priv->iface_base = devm_ioremap_resource(&pdev->dev, res); if (IS_ERR(priv->iface_base)) return PTR_ERR(priv->iface_base); + + /* Map CM3 SRAM */ + err = mvpp2_get_sram(pdev, priv); + if (err == -EPROBE_DEFER) + return err; + else if (err) + dev_warn(&pdev->dev, "Fail to alloc CM3 SRAM\n"); } if (priv->hw_version == MVPP22 && dev_of_node(&pdev->dev)) { @@ -6947,11 +7002,13 @@ static int mvpp2_probe(struct platform_device *pdev) if (dev_of_node(&pdev->dev)) { priv->pp_clk = devm_clk_get(&pdev->dev, "pp_clk"); - if (IS_ERR(priv->pp_clk)) - return PTR_ERR(priv->pp_clk); + if (IS_ERR(priv->pp_clk)) { + err = PTR_ERR(priv->pp_clk); + goto err_cm3; + } err = clk_prepare_enable(priv->pp_clk); if (err < 0) - return err; + goto err_cm3; priv->gop_clk = devm_clk_get(&pdev->dev, "gop_clk"); if (IS_ERR(priv->gop_clk)) { @@ -7087,6 +7144,11 @@ static int mvpp2_probe(struct platform_device *pdev) clk_disable_unprepare(priv->gop_clk); err_pp_clk: clk_disable_unprepare(priv->pp_clk); +err_cm3: + if (!has_acpi_companion(&pdev->dev) && priv->cm3_base) + gen_pool_free(priv->sram_pool, (unsigned long)priv->cm3_base, + MSS_SRAM_SIZE); + return err; } @@ -7127,6 +7189,12 @@ static int mvpp2_remove(struct platform_device *pdev) aggr_txq->descs_dma); } + if (!has_acpi_companion(&pdev->dev) && priv->cm3_base) { + gen_pool_free(priv->sram_pool, (unsigned long)priv->cm3_base, + MSS_SRAM_SIZE); + gen_pool_destroy(priv->sram_pool); + } + if (is_acpi_node(port_fwnode)) return 0;