| Message ID | 20210419225133.2005360-3-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] net: ethernet: ixp4xx: Add DT bindings | expand |
On Tue, Apr 20, 2021 at 12:51 AM Linus Walleij <linus.walleij@linaro.org> wrote: > This augments the IXP4xx to use the OF MDIO bus code when > registering the MDIO bus and when looking up the PHY > for the ethernet network device. > > Cc: Zoltan HERPAI <wigyori@uid0.hu> > Cc: Raylynn Knight <rayknight@me.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - New patch making use of more OF infrastructure. Realized after sending that the MDIO people wanna look at this too so adding them to CC, tell me if you want me to resend the patch. Yours, Linus Walleij
> @@ -1381,25 +1382,12 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev) > /* NPE ID 0x00, 0x10, 0x20... */ > plat->npe = (val << 4); > > - phy_np = of_parse_phandle(np, "phy-handle", 0); > - if (phy_np) { > - ret = of_property_read_u32(phy_np, "reg", &val); > - if (ret) { > - dev_err(dev, "cannot find phy reg\n"); > - return NULL; > - } > - of_node_put(phy_np); > - } else { > - dev_err(dev, "cannot find phy instance\n"); > - val = 0; > - } > - plat->phy = val; > - Isn't this code you just added in the previous patch? > /* Check if this device has an MDIO bus */ > mdio_np = of_get_child_by_name(np, "mdio"); > if (mdio_np) { > plat->has_mdio = true; > - of_node_put(mdio_np); > + mdio_bus_np = mdio_np; > + /* DO NOT put the mdio_np, it will be used */ > } > > /* Get the rx queue as a resource from queue manager */ > @@ -1539,10 +1527,14 @@ static int ixp4xx_eth_probe(struct platform_device *pdev) > __raw_writel(DEFAULT_CORE_CNTRL, &port->regs->core_control); > udelay(50); > > - snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > - mdio_bus->id, plat->phy); > - phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link, > - PHY_INTERFACE_MODE_MII); > + if (np) { > + phydev = of_phy_get_and_connect(ndev, np, ixp4xx_adjust_link); > + } else { > + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > + mdio_bus->id, plat->phy); mdiobus_get_phy() and phy_connect_direct() might be better. Andrew
On Tue, Apr 20, 2021 at 3:47 AM Andrew Lunn <andrew@lunn.ch> wrote: > > @@ -1381,25 +1382,12 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev) > > /* NPE ID 0x00, 0x10, 0x20... */ > > plat->npe = (val << 4); > > > > - phy_np = of_parse_phandle(np, "phy-handle", 0); > > - if (phy_np) { > > - ret = of_property_read_u32(phy_np, "reg", &val); > > - if (ret) { > > - dev_err(dev, "cannot find phy reg\n"); > > - return NULL; > > - } > > - of_node_put(phy_np); > > - } else { > > - dev_err(dev, "cannot find phy instance\n"); > > - val = 0; > > - } > > - plat->phy = val; > > - > > Isn't this code you just added in the previous patch? Yep. It's by the token of "one technical step per patch" but I suggested that maybe you prefer to take two technical steps in one big patch, then we can just squash patches 2 & 3. I'll fix it as you want it just tell me how :) > > - snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > > - mdio_bus->id, plat->phy); > > - phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link, > > - PHY_INTERFACE_MODE_MII); > > + if (np) { > > + phydev = of_phy_get_and_connect(ndev, np, ixp4xx_adjust_link); > > + } else { > > + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > > + mdio_bus->id, plat->phy); > > mdiobus_get_phy() and phy_connect_direct() might be better. Do you mean for the legacy code path (else clause), or the new code path with of_phy_get_and_connect() or both? I tried not to change the legacy code in order to not introduce regressions, so if I change that I suppose it should be a separate patch. On the other hand this driver has not been much maintained the recent years so we might need to be a bit rough when bringing it into shape. (After migrating all of IXP4xx to device tree a lot of the legacy code will eventually be deleted.) Yours, Linus Walleij
On Tue, Apr 20, 2021 at 10:44:16AM +0200, Linus Walleij wrote: > On Tue, Apr 20, 2021 at 3:47 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > @@ -1381,25 +1382,12 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev) > > > /* NPE ID 0x00, 0x10, 0x20... */ > > > plat->npe = (val << 4); > > > > > > - phy_np = of_parse_phandle(np, "phy-handle", 0); > > > - if (phy_np) { > > > - ret = of_property_read_u32(phy_np, "reg", &val); > > > - if (ret) { > > > - dev_err(dev, "cannot find phy reg\n"); > > > - return NULL; > > > - } > > > - of_node_put(phy_np); > > > - } else { > > > - dev_err(dev, "cannot find phy instance\n"); > > > - val = 0; > > > - } > > > - plat->phy = val; > > > - > > > > Isn't this code you just added in the previous patch? > > Yep. It's by the token of "one technical step per patch" I don't actually seeing it being a step, since it is actually broken and of_phy_get_and_connect() does pretty much everything it should do, which is what you replace it with. It is a long time since i converted a platform_data driver to DT. But i remember trying to fill in the platform_data structure from DT was often the wrong way to do it. They contain different data, and you cannot easily map one to the other. So you need to make bigger changes to the probe function. You have two intermingled code paths, one dealing with platform_data, and the other using DT. I've not looked in detail, but i guess my first step would be, cleanly register the MDIO bus. Second step would be to register the PHY. And it might need some refactoring patches just to make it easier to understand. > > > - snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > > > - mdio_bus->id, plat->phy); > > > - phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link, > > > - PHY_INTERFACE_MODE_MII); > > > + if (np) { > > > + phydev = of_phy_get_and_connect(ndev, np, ixp4xx_adjust_link); > > > + } else { > > > + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, > > > + mdio_bus->id, plat->phy); > > > > mdiobus_get_phy() and phy_connect_direct() might be better. > > Do you mean for the legacy code path (else clause), or the > new code path with of_phy_get_and_connect() or both? > > I tried not to change the legacy code in order to not introduce > regressions, so if I change that I suppose it should be a > separate patch. Yes, the legacy code. You don't often see this string parsing method. And since you have the bus, and the index, you can directly go to the PHY avoiding it all. A separate patch would be better. Andrew
diff --git a/drivers/net/ethernet/xscale/Kconfig b/drivers/net/ethernet/xscale/Kconfig index 7b83a6e5d894..468ffe3d1707 100644 --- a/drivers/net/ethernet/xscale/Kconfig +++ b/drivers/net/ethernet/xscale/Kconfig @@ -22,6 +22,7 @@ config IXP4XX_ETH tristate "Intel IXP4xx Ethernet support" depends on ARM && ARCH_IXP4XX && IXP4XX_NPE && IXP4XX_QMGR select PHYLIB + select OF_MDIO if OF select NET_PTP_CLASSIFY help Say Y here if you want to use built-in Ethernet ports diff --git a/drivers/net/ethernet/xscale/ixp4xx_eth.c b/drivers/net/ethernet/xscale/ixp4xx_eth.c index 758f820068b5..1e1779b53f22 100644 --- a/drivers/net/ethernet/xscale/ixp4xx_eth.c +++ b/drivers/net/ethernet/xscale/ixp4xx_eth.c @@ -250,6 +250,7 @@ static inline void memcpy_swab32(u32 *dest, u32 *src, int cnt) static DEFINE_SPINLOCK(mdio_lock); static struct eth_regs __iomem *mdio_regs; /* mdio command and status only */ static struct mii_bus *mdio_bus; +static struct device_node *mdio_bus_np; static int ports_open; static struct port *npe_port_tab[MAX_NPES]; static struct dma_pool *dma_pool; @@ -533,7 +534,8 @@ static int ixp4xx_mdio_register(struct eth_regs __iomem *regs) mdio_bus->write = &ixp4xx_mdio_write; snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "ixp4xx-eth-0"); - if ((err = mdiobus_register(mdio_bus))) + err = of_mdiobus_register(mdio_bus, mdio_bus_np); + if (err) mdiobus_free(mdio_bus); return err; } @@ -1364,7 +1366,6 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev) struct device_node *np = dev->of_node; struct of_phandle_args queue_spec; struct eth_plat_info *plat; - struct device_node *phy_np; struct device_node *mdio_np; u32 val; int ret; @@ -1381,25 +1382,12 @@ static struct eth_plat_info *ixp4xx_of_get_platdata(struct device *dev) /* NPE ID 0x00, 0x10, 0x20... */ plat->npe = (val << 4); - phy_np = of_parse_phandle(np, "phy-handle", 0); - if (phy_np) { - ret = of_property_read_u32(phy_np, "reg", &val); - if (ret) { - dev_err(dev, "cannot find phy reg\n"); - return NULL; - } - of_node_put(phy_np); - } else { - dev_err(dev, "cannot find phy instance\n"); - val = 0; - } - plat->phy = val; - /* Check if this device has an MDIO bus */ mdio_np = of_get_child_by_name(np, "mdio"); if (mdio_np) { plat->has_mdio = true; - of_node_put(mdio_np); + mdio_bus_np = mdio_np; + /* DO NOT put the mdio_np, it will be used */ } /* Get the rx queue as a resource from queue manager */ @@ -1539,10 +1527,14 @@ static int ixp4xx_eth_probe(struct platform_device *pdev) __raw_writel(DEFAULT_CORE_CNTRL, &port->regs->core_control); udelay(50); - snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, - mdio_bus->id, plat->phy); - phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link, - PHY_INTERFACE_MODE_MII); + if (np) { + phydev = of_phy_get_and_connect(ndev, np, ixp4xx_adjust_link); + } else { + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT, + mdio_bus->id, plat->phy); + phydev = phy_connect(ndev, phy_id, &ixp4xx_adjust_link, + PHY_INTERFACE_MODE_MII); + } if (!phydev) { err = -ENODEV; dev_err(dev, "no phydev\n");
This augments the IXP4xx to use the OF MDIO bus code when registering the MDIO bus and when looking up the PHY for the ethernet network device. Cc: Zoltan HERPAI <wigyori@uid0.hu> Cc: Raylynn Knight <rayknight@me.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - New patch making use of more OF infrastructure. --- drivers/net/ethernet/xscale/Kconfig | 1 + drivers/net/ethernet/xscale/ixp4xx_eth.c | 34 +++++++++--------------- 2 files changed, 14 insertions(+), 21 deletions(-) -- 2.29.2