| Message ID | 20230526074252.480200-3-maxime.chevallier@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add a regmap-based mdio driver and drop TSE PCS | expand |
On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote: > The newly introduced regmap-based MDIO driver allows for an easy mapping > of an mdiodevice onto the memory-mapped TSE PCS, which is actually a > Lynx PCS. > > Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which > is nothing more than a memory-mapped Lynx PCS. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> Hi Maxime, I have some concerns about the error paths in this patch. ... > @@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct platform_device *pdev) > const struct of_device_id *of_id = NULL; > struct altera_tse_private *priv; > struct resource *control_port; > + struct regmap *pcs_regmap; > struct resource *dma_res; > struct resource *pcs_res; > + struct mii_bus *pcs_bus; > struct net_device *ndev; > void __iomem *descmap; > - int pcs_reg_width = 2; > int ret = -ENODEV; > > + struct regmap_config pcs_regmap_cfg; nit: this probably belongs in with the bunch of declarations above it. > + > + struct mdio_regmap_config mrc = { > + .parent = &pdev->dev, > + .valid_addr = 0x0, > + }; nit: maybe this too. > + > ndev = alloc_etherdev(sizeof(struct altera_tse_private)); > if (!ndev) { > dev_err(&pdev->dev, "Could not allocate network device\n"); > @@ -1258,10 +1268,29 @@ static int altera_tse_probe(struct platform_device *pdev) > ret = request_and_map(pdev, "pcs", &pcs_res, > &priv->pcs_base); > if (ret) { > + /* If we can't find a dedicated resource for the PCS, fallback > + * to the internal PCS, that has a different address stride > + */ > priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0); > - pcs_reg_width = 4; > + pcs_regmap_cfg.reg_bits = 32; > + /* Values are MDIO-like values, on 16 bits */ > + pcs_regmap_cfg.val_bits = 16; > + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2); > + } else { > + pcs_regmap_cfg.reg_bits = 16; > + pcs_regmap_cfg.val_bits = 16; > + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1); > } > > + /* Create a regmap for the PCS so that it can be used by the PCS driver */ > + pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base, > + &pcs_regmap_cfg); > + if (IS_ERR(pcs_regmap)) { > + ret = PTR_ERR(pcs_regmap); > + goto err_free_netdev; > + } > + mrc.regmap = pcs_regmap; > + > /* Rx IRQ */ > priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq"); > if (priv->rx_irq == -ENXIO) { > @@ -1384,7 +1413,20 @@ static int altera_tse_probe(struct platform_device *pdev) > (unsigned long) control_port->start, priv->rx_irq, > priv->tx_irq); > > - priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, pcs_reg_width); > + snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name); > + pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc); > + if (IS_ERR(pcs_bus)) { > + ret = PTR_ERR(pcs_bus); > + goto err_init_phy; > + } > + > + priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0); mdio_device_create() can fail. Should that be handled here? > + > + priv->pcs = lynx_pcs_create(priv->pcs_mdiodev); > + if (!priv->pcs) { > + ret = -ENODEV; > + goto err_init_phy; Does this leak priv->pcs_mdiodev? > + } > > priv->phylink_config.dev = &ndev->dev; > priv->phylink_config.type = PHYLINK_NETDEV; > @@ -1407,11 +1449,12 @@ static int altera_tse_probe(struct platform_device *pdev) > if (IS_ERR(priv->phylink)) { > dev_err(&pdev->dev, "failed to create phylink\n"); > ret = PTR_ERR(priv->phylink); > - goto err_init_phy; > + goto err_pcs; Does this leak priv->pcs ? > } > > return 0; > - > +err_pcs: > + mdio_device_free(priv->pcs_mdiodev); > err_init_phy: > unregister_netdev(ndev); > err_register_netdev: > @@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct platform_device *pdev) > altera_tse_mdio_destroy(ndev); > unregister_netdev(ndev); > phylink_destroy(priv->phylink); > + mdio_device_free(priv->pcs_mdiodev); > + > free_netdev(ndev); > > return 0; > diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h > index b8508f152552..679d9069846b 100644 > --- a/include/linux/mdio/mdio-regmap.h > +++ b/include/linux/mdio/mdio-regmap.h > @@ -7,6 +7,8 @@ > #ifndef MDIO_REGMAP_H > #define MDIO_REGMAP_H > > +#include <linux/phy.h> > + > struct device; > struct regmap; > This hunk doesn't seem strictly related to the patch. Perhaps the include belongs elsewhere. Or the hunk belongs in another patch.
On Fri, May 26, 2023 at 10:39:08AM +0200, Simon Horman wrote: > On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote: > > The newly introduced regmap-based MDIO driver allows for an easy mapping > > of an mdiodevice onto the memory-mapped TSE PCS, which is actually a > > Lynx PCS. > > > > Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which > > is nothing more than a memory-mapped Lynx PCS. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > Hi Maxime, > > I have some concerns about the error paths in this patch. We've had similar problems with mdio_device_create() vs the XPCS driver. I think it's time that we made this easier for users. diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c index b87c69c4cdd7..802222581feb 100644 --- a/drivers/net/pcs/pcs-xpcs.c +++ b/drivers/net/pcs/pcs-xpcs.c @@ -1240,6 +1240,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, if (!xpcs) return ERR_PTR(-ENOMEM); + mdio_device_get(mdiodev); xpcs->mdiodev = mdiodev; xpcs_id = xpcs_get_id(xpcs); @@ -1272,6 +1273,7 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev, ret = -ENODEV; out: + mdio_device_put(mdiodev); kfree(xpcs); return ERR_PTR(ret); @@ -1280,8 +1282,33 @@ EXPORT_SYMBOL_GPL(xpcs_create); void xpcs_destroy(struct dw_xpcs *xpcs) { + mdio_device_put(mdiodev); kfree(xpcs); } EXPORT_SYMBOL_GPL(xpcs_destroy); +struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr, + phy_interface_t interface) +{ + struct mdio_device *mdiodev; + struct dw_xpcs *xpcs; + + mdiodev = mdio_device_create(bus, addr); + if (IS_ERR(mdiodev)) + return ERR_CAST(mdiodev); + + xpcs = xpcs_create(mdiodev, interface); + + /* xpcs_create() has taken a refcount on the mdiodev if it was + * successful. If xpcs_create() fails, this will free the mdio + * device here. In any case, we don't need to hold our reference + * anymore, and putting it here will allow mdio_device_put() in + * xpcs_destroy() to automatically free the mdio device. + */ + mdio_device_put(mdiodev); + + return xpcs; +} +EXPORT_SYMBOL_GPL(xpcs_create_mdiodev); + MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mdio.h b/include/linux/mdio.h index 1d7d550bbf1a..537b62330c90 100644 --- a/include/linux/mdio.h +++ b/include/linux/mdio.h @@ -108,6 +108,16 @@ int mdio_driver_register(struct mdio_driver *drv); void mdio_driver_unregister(struct mdio_driver *drv); int mdio_device_bus_match(struct device *dev, struct device_driver *drv); +static inline void mdio_device_get(struct mdio_device *mdiodev) +{ + get_device(&mdiodev->dev); +} + +static inline void mdio_device_put(struct mdio_device *mdiodev) +{ + mdio_device_free(mdiodev); +} + static inline bool mdio_phy_id_is_c45(int phy_id) { return (phy_id & MDIO_PHY_ID_C45) && !(phy_id & ~MDIO_PHY_ID_C45_MASK); The same for pcs-lynx. That way we remove the need for driver authors to get the creation and destruction of the mdio device correct without messing up the existing users - they only have to worry about creating the PCS via the xxx_create_mdiodev() function and destroying it via xxx_destroy().
On Fri, May 26, 2023 at 10:05:17AM +0100, Russell King (Oracle) wrote: > On Fri, May 26, 2023 at 10:39:08AM +0200, Simon Horman wrote: > > On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote: > > > The newly introduced regmap-based MDIO driver allows for an easy mapping > > > of an mdiodevice onto the memory-mapped TSE PCS, which is actually a > > > Lynx PCS. > > > > > > Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which > > > is nothing more than a memory-mapped Lynx PCS. > > > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > > > Hi Maxime, > > > > I have some concerns about the error paths in this patch. > > We've had similar problems with mdio_device_create() vs the XPCS > driver. > > I think it's time that we made this easier for users. Patch series here: https://lore.kernel.org/all/ZHCGZ8IgAAwr8bla@shell.armlinux.org.uk/
Hello Simon, On Fri, 26 May 2023 10:39:08 +0200 Simon Horman <simon.horman@corigine.com> wrote: > On Fri, May 26, 2023 at 09:42:50AM +0200, Maxime Chevallier wrote: > > The newly introduced regmap-based MDIO driver allows for an easy > > mapping of an mdiodevice onto the memory-mapped TSE PCS, which is > > actually a Lynx PCS. > > > > Convert Altera TSE to use this PCS instead of the pcs-altera-tse, > > which is nothing more than a memory-mapped Lynx PCS. > > > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > > Hi Maxime, > > I have some concerns about the error paths in this patch. Heh I didn't do a very good job in that regard indeed :/ I'll address these, but I'll wait for Russell series to make it through though. Thanks for spotting this. > ... > > > @@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct > > platform_device *pdev) const struct of_device_id *of_id = NULL; > > struct altera_tse_private *priv; > > struct resource *control_port; > > + struct regmap *pcs_regmap; > > struct resource *dma_res; > > struct resource *pcs_res; > > + struct mii_bus *pcs_bus; > > struct net_device *ndev; > > void __iomem *descmap; > > - int pcs_reg_width = 2; > > int ret = -ENODEV; > > > > + struct regmap_config pcs_regmap_cfg; > > nit: this probably belongs in with the bunch of declarations above it. > > > + > > + struct mdio_regmap_config mrc = { > > + .parent = &pdev->dev, > > + .valid_addr = 0x0, > > + }; > > nit: maybe this too. > > > + > > ndev = alloc_etherdev(sizeof(struct altera_tse_private)); > > if (!ndev) { > > dev_err(&pdev->dev, "Could not allocate network > > device\n"); @@ -1258,10 +1268,29 @@ static int > > altera_tse_probe(struct platform_device *pdev) ret = > > request_and_map(pdev, "pcs", &pcs_res, &priv->pcs_base); > > if (ret) { > > + /* If we can't find a dedicated resource for the > > PCS, fallback > > + * to the internal PCS, that has a different > > address stride > > + */ > > priv->pcs_base = priv->mac_dev + > > tse_csroffs(mdio_phy0); > > - pcs_reg_width = 4; > > + pcs_regmap_cfg.reg_bits = 32; > > + /* Values are MDIO-like values, on 16 bits */ > > + pcs_regmap_cfg.val_bits = 16; > > + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2); > > + } else { > > + pcs_regmap_cfg.reg_bits = 16; > > + pcs_regmap_cfg.val_bits = 16; > > + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1); > > } > > > > + /* Create a regmap for the PCS so that it can be used by > > the PCS driver */ > > + pcs_regmap = devm_regmap_init_mmio(&pdev->dev, > > priv->pcs_base, > > + &pcs_regmap_cfg); > > + if (IS_ERR(pcs_regmap)) { > > + ret = PTR_ERR(pcs_regmap); > > + goto err_free_netdev; > > + } > > + mrc.regmap = pcs_regmap; > > + > > /* Rx IRQ */ > > priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq"); > > if (priv->rx_irq == -ENXIO) { > > @@ -1384,7 +1413,20 @@ static int altera_tse_probe(struct > > platform_device *pdev) (unsigned long) control_port->start, > > priv->rx_irq, priv->tx_irq); > > > > - priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, > > pcs_reg_width); > > + snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", > > ndev->name); > > + pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc); > > + if (IS_ERR(pcs_bus)) { > > + ret = PTR_ERR(pcs_bus); > > + goto err_init_phy; > > + } > > + > > + priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0); > > mdio_device_create() can fail. Should that be handled here? > > > + > > + priv->pcs = lynx_pcs_create(priv->pcs_mdiodev); > > + if (!priv->pcs) { > > + ret = -ENODEV; > > + goto err_init_phy; > > Does this leak priv->pcs_mdiodev? > > > + } > > > > priv->phylink_config.dev = &ndev->dev; > > priv->phylink_config.type = PHYLINK_NETDEV; > > @@ -1407,11 +1449,12 @@ static int altera_tse_probe(struct > > platform_device *pdev) if (IS_ERR(priv->phylink)) { > > dev_err(&pdev->dev, "failed to create phylink\n"); > > ret = PTR_ERR(priv->phylink); > > - goto err_init_phy; > > + goto err_pcs; > > Does this leak priv->pcs ? > > > } > > > > return 0; > > - > > +err_pcs: > > + mdio_device_free(priv->pcs_mdiodev); > > err_init_phy: > > unregister_netdev(ndev); > > err_register_netdev: > > @@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct > > platform_device *pdev) altera_tse_mdio_destroy(ndev); > > unregister_netdev(ndev); > > phylink_destroy(priv->phylink); > > + mdio_device_free(priv->pcs_mdiodev); > > + > > free_netdev(ndev); > > > > return 0; > > diff --git a/include/linux/mdio/mdio-regmap.h > > b/include/linux/mdio/mdio-regmap.h index b8508f152552..679d9069846b > > 100644 --- a/include/linux/mdio/mdio-regmap.h > > +++ b/include/linux/mdio/mdio-regmap.h > > @@ -7,6 +7,8 @@ > > #ifndef MDIO_REGMAP_H > > #define MDIO_REGMAP_H > > > > +#include <linux/phy.h> > > + > > struct device; > > struct regmap; > > > > This hunk doesn't seem strictly related to the patch. > Perhaps the include belongs elsewhere. > Or the hunk belongs in another patch. Indeed, I had the same issue in another patch in this series. I'll re-split everything correctly. Thank you for the review, Maxime
diff --git a/drivers/net/ethernet/altera/altera_tse.h b/drivers/net/ethernet/altera/altera_tse.h index db5eed06e92d..d50cf440d01b 100644 --- a/drivers/net/ethernet/altera/altera_tse.h +++ b/drivers/net/ethernet/altera/altera_tse.h @@ -477,6 +477,7 @@ struct altera_tse_private { struct phylink *phylink; struct phylink_config phylink_config; struct phylink_pcs *pcs; + struct mdio_device *pcs_mdiodev; }; /* Function prototypes diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c index 190ff1bcd94e..66db6a7d0b22 100644 --- a/drivers/net/ethernet/altera/altera_tse_main.c +++ b/drivers/net/ethernet/altera/altera_tse_main.c @@ -27,14 +27,16 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/mii.h> +#include <linux/mdio/mdio-regmap.h> #include <linux/netdevice.h> #include <linux/of_device.h> #include <linux/of_mdio.h> #include <linux/of_net.h> #include <linux/of_platform.h> -#include <linux/pcs-altera-tse.h> +#include <linux/pcs-lynx.h> #include <linux/phy.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/skbuff.h> #include <asm/cacheflush.h> @@ -1134,13 +1136,21 @@ static int altera_tse_probe(struct platform_device *pdev) const struct of_device_id *of_id = NULL; struct altera_tse_private *priv; struct resource *control_port; + struct regmap *pcs_regmap; struct resource *dma_res; struct resource *pcs_res; + struct mii_bus *pcs_bus; struct net_device *ndev; void __iomem *descmap; - int pcs_reg_width = 2; int ret = -ENODEV; + struct regmap_config pcs_regmap_cfg; + + struct mdio_regmap_config mrc = { + .parent = &pdev->dev, + .valid_addr = 0x0, + }; + ndev = alloc_etherdev(sizeof(struct altera_tse_private)); if (!ndev) { dev_err(&pdev->dev, "Could not allocate network device\n"); @@ -1258,10 +1268,29 @@ static int altera_tse_probe(struct platform_device *pdev) ret = request_and_map(pdev, "pcs", &pcs_res, &priv->pcs_base); if (ret) { + /* If we can't find a dedicated resource for the PCS, fallback + * to the internal PCS, that has a different address stride + */ priv->pcs_base = priv->mac_dev + tse_csroffs(mdio_phy0); - pcs_reg_width = 4; + pcs_regmap_cfg.reg_bits = 32; + /* Values are MDIO-like values, on 16 bits */ + pcs_regmap_cfg.val_bits = 16; + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(2); + } else { + pcs_regmap_cfg.reg_bits = 16; + pcs_regmap_cfg.val_bits = 16; + pcs_regmap_cfg.reg_shift = REGMAP_UPSHIFT(1); } + /* Create a regmap for the PCS so that it can be used by the PCS driver */ + pcs_regmap = devm_regmap_init_mmio(&pdev->dev, priv->pcs_base, + &pcs_regmap_cfg); + if (IS_ERR(pcs_regmap)) { + ret = PTR_ERR(pcs_regmap); + goto err_free_netdev; + } + mrc.regmap = pcs_regmap; + /* Rx IRQ */ priv->rx_irq = platform_get_irq_byname(pdev, "rx_irq"); if (priv->rx_irq == -ENXIO) { @@ -1384,7 +1413,20 @@ static int altera_tse_probe(struct platform_device *pdev) (unsigned long) control_port->start, priv->rx_irq, priv->tx_irq); - priv->pcs = alt_tse_pcs_create(ndev, priv->pcs_base, pcs_reg_width); + snprintf(mrc.name, MII_BUS_ID_SIZE, "%s-pcs-mii", ndev->name); + pcs_bus = devm_mdio_regmap_register(&pdev->dev, &mrc); + if (IS_ERR(pcs_bus)) { + ret = PTR_ERR(pcs_bus); + goto err_init_phy; + } + + priv->pcs_mdiodev = mdio_device_create(pcs_bus, 0); + + priv->pcs = lynx_pcs_create(priv->pcs_mdiodev); + if (!priv->pcs) { + ret = -ENODEV; + goto err_init_phy; + } priv->phylink_config.dev = &ndev->dev; priv->phylink_config.type = PHYLINK_NETDEV; @@ -1407,11 +1449,12 @@ static int altera_tse_probe(struct platform_device *pdev) if (IS_ERR(priv->phylink)) { dev_err(&pdev->dev, "failed to create phylink\n"); ret = PTR_ERR(priv->phylink); - goto err_init_phy; + goto err_pcs; } return 0; - +err_pcs: + mdio_device_free(priv->pcs_mdiodev); err_init_phy: unregister_netdev(ndev); err_register_netdev: @@ -1433,6 +1476,8 @@ static int altera_tse_remove(struct platform_device *pdev) altera_tse_mdio_destroy(ndev); unregister_netdev(ndev); phylink_destroy(priv->phylink); + mdio_device_free(priv->pcs_mdiodev); + free_netdev(ndev); return 0; diff --git a/include/linux/mdio/mdio-regmap.h b/include/linux/mdio/mdio-regmap.h index b8508f152552..679d9069846b 100644 --- a/include/linux/mdio/mdio-regmap.h +++ b/include/linux/mdio/mdio-regmap.h @@ -7,6 +7,8 @@ #ifndef MDIO_REGMAP_H #define MDIO_REGMAP_H +#include <linux/phy.h> + struct device; struct regmap;
The newly introduced regmap-based MDIO driver allows for an easy mapping of an mdiodevice onto the memory-mapped TSE PCS, which is actually a Lynx PCS. Convert Altera TSE to use this PCS instead of the pcs-altera-tse, which is nothing more than a memory-mapped Lynx PCS. Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> --- V2->V3 : No changes V1->V2 : No changes drivers/net/ethernet/altera/altera_tse.h | 1 + drivers/net/ethernet/altera/altera_tse_main.c | 57 +++++++++++++++++-- include/linux/mdio/mdio-regmap.h | 2 + 3 files changed, 54 insertions(+), 6 deletions(-)