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(-)
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;
--
2.40.1
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.
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
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(). -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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/ -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
© 2016 - 2024 Red Hat, Inc.