[3/3] net: ethernet: ixp4xx: Use OF MDIO bus registration

Message ID 20210419225133.2005360-3-linus.walleij@linaro.org
State New
Headers show
Series
  • [1/3] net: ethernet: ixp4xx: Add DT bindings
Related show

Commit Message

Linus Walleij April 19, 2021, 10:51 p.m.
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

Comments

Linus Walleij April 19, 2021, 10:54 p.m. | #1
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
Andrew Lunn April 20, 2021, 1:47 a.m. | #2
> @@ -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
Linus Walleij April 20, 2021, 8:44 a.m. | #3
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
Andrew Lunn April 20, 2021, 12:53 p.m. | #4
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

Patch

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");