[PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
There, of_mdiobus_register was called even for the fixed-link representing
the SPI-connected switch PHY, with the result that the driver attempts to
enumerate PHYs on a non-existent MDIO bus:

	libphy: MACB_mii_bus: probed
	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
        [snip]
	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
	macb f0028000.ethernet: broken fixed-link specification

Cc: <stable@vger.kernel.org>
Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++---------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index a6c911bb5ce2..d202a03c42ed 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev)
 
 	if (np) {
 		if (of_phy_is_fixed_link(np)) {
-			if (of_phy_register_fixed_link(np) < 0) {
-				dev_err(&bp->pdev->dev,
-					"broken fixed-link specification\n");
-				return -ENODEV;
-			}
 			bp->phy_node = of_node_get(np);
 		} else {
 			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
@@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp)
 {
 	struct macb_platform_data *pdata;
 	struct device_node *np;
-	int err;
+	int err = -ENXIO;
 
 	/* Enable management port */
 	macb_writel(bp, NCR, MACB_BIT(MPE));
@@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp)
 	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
 
 	np = bp->pdev->dev.of_node;
-	if (pdata)
-		bp->mii_bus->phy_mask = pdata->phy_mask;
+	if (np && of_phy_is_fixed_link(np)) {
+		if (of_phy_register_fixed_link(np) < 0) {
+			dev_err(&bp->pdev->dev,
+					"broken fixed-link specification\n");
+			goto err_out_free_mdiobus;
+		}
+
+		err = mdiobus_register(bp->mii_bus);
+	} else {
+		if (pdata)
+			bp->mii_bus->phy_mask = pdata->phy_mask;
+
+		err = of_mdiobus_register(bp->mii_bus, np);
+	}
 
-	err = of_mdiobus_register(bp->mii_bus, np);
 	if (err)
 		goto err_out_free_mdiobus;
 
@@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp)
 
 err_out_unregister_bus:
 	mdiobus_unregister(bp->mii_bus);
+err_out_free_mdiobus:
 	if (np && of_phy_is_fixed_link(np))
 		of_phy_deregister_fixed_link(np);
-err_out_free_mdiobus:
 	of_node_put(bp->phy_node);
 	mdiobus_free(bp->mii_bus);
 err_out:
-- 
2.18.0

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Uwe Kleine-König]

Hello Ahmad,


On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> There, of_mdiobus_register was called even for the fixed-link representing
> the SPI-connected switch PHY, with the result that the driver attempts to
> enumerate PHYs on a non-existent MDIO bus:
> 
> 	libphy: MACB_mii_bus: probed
> 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
>         [snip]
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> 	macb f0028000.ethernet: broken fixed-link specification
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")

I added the people involved in 739de9a1563a to Cc. Maybe they want to
comment. So not stripping the remaining part of the original mail.

Best regards
Uwe

> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++---------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a6c911bb5ce2..d202a03c42ed 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev)
>  
>  	if (np) {
>  		if (of_phy_is_fixed_link(np)) {
> -			if (of_phy_register_fixed_link(np) < 0) {
> -				dev_err(&bp->pdev->dev,
> -					"broken fixed-link specification\n");
> -				return -ENODEV;
> -			}
>  			bp->phy_node = of_node_get(np);
>  		} else {
>  			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
> @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp)
>  {
>  	struct macb_platform_data *pdata;
>  	struct device_node *np;
> -	int err;
> +	int err = -ENXIO;
>  
>  	/* Enable management port */
>  	macb_writel(bp, NCR, MACB_BIT(MPE));
> @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp)
>  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
>  
>  	np = bp->pdev->dev.of_node;
> -	if (pdata)
> -		bp->mii_bus->phy_mask = pdata->phy_mask;
> +	if (np && of_phy_is_fixed_link(np)) {
> +		if (of_phy_register_fixed_link(np) < 0) {
> +			dev_err(&bp->pdev->dev,
> +					"broken fixed-link specification\n");
> +			goto err_out_free_mdiobus;
> +		}
> +
> +		err = mdiobus_register(bp->mii_bus);
> +	} else {
> +		if (pdata)
> +			bp->mii_bus->phy_mask = pdata->phy_mask;
> +
> +		err = of_mdiobus_register(bp->mii_bus, np);
> +	}
>  
> -	err = of_mdiobus_register(bp->mii_bus, np);
>  	if (err)
>  		goto err_out_free_mdiobus;
>  
> @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp)
>  
>  err_out_unregister_bus:
>  	mdiobus_unregister(bp->mii_bus);
> +err_out_free_mdiobus:
>  	if (np && of_phy_is_fixed_link(np))
>  		of_phy_deregister_fixed_link(np);
> -err_out_free_mdiobus:
>  	of_node_put(bp->phy_node);
>  	mdiobus_free(bp->mii_bus);
>  err_out:
> -- 
> 2.18.0
> 
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Uwe Kleine-König]

Hello Ahmad,


On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> There, of_mdiobus_register was called even for the fixed-link representing
> the SPI-connected switch PHY, with the result that the driver attempts to
> enumerate PHYs on a non-existent MDIO bus:
> 
> 	libphy: MACB_mii_bus: probed
> 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
>         [snip]
> 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> 	macb f0028000.ethernet: broken fixed-link specification
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")

I added the people involved in 739de9a1563a to Cc. Maybe they want to
comment. So not stripping the remaining part of the original mail.

Best regards
Uwe

> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++---------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index a6c911bb5ce2..d202a03c42ed 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev)
>  
>  	if (np) {
>  		if (of_phy_is_fixed_link(np)) {
> -			if (of_phy_register_fixed_link(np) < 0) {
> -				dev_err(&bp->pdev->dev,
> -					"broken fixed-link specification\n");
> -				return -ENODEV;
> -			}
>  			bp->phy_node = of_node_get(np);
>  		} else {
>  			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
> @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp)
>  {
>  	struct macb_platform_data *pdata;
>  	struct device_node *np;
> -	int err;
> +	int err = -ENXIO;
>  
>  	/* Enable management port */
>  	macb_writel(bp, NCR, MACB_BIT(MPE));
> @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp)
>  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
>  
>  	np = bp->pdev->dev.of_node;
> -	if (pdata)
> -		bp->mii_bus->phy_mask = pdata->phy_mask;
> +	if (np && of_phy_is_fixed_link(np)) {
> +		if (of_phy_register_fixed_link(np) < 0) {
> +			dev_err(&bp->pdev->dev,
> +					"broken fixed-link specification\n");
> +			goto err_out_free_mdiobus;
> +		}
> +
> +		err = mdiobus_register(bp->mii_bus);
> +	} else {
> +		if (pdata)
> +			bp->mii_bus->phy_mask = pdata->phy_mask;
> +
> +		err = of_mdiobus_register(bp->mii_bus, np);
> +	}
>  
> -	err = of_mdiobus_register(bp->mii_bus, np);
>  	if (err)
>  		goto err_out_free_mdiobus;
>  
> @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp)
>  
>  err_out_unregister_bus:
>  	mdiobus_unregister(bp->mii_bus);
> +err_out_free_mdiobus:
>  	if (np && of_phy_is_fixed_link(np))
>  		of_phy_deregister_fixed_link(np);
> -err_out_free_mdiobus:
>  	of_node_put(bp->phy_node);
>  	mdiobus_free(bp->mii_bus);
>  err_out:
> -- 
> 2.18.0
> 
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-K�nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Brad Mouring]

Hello Ahmed, Uwe,

On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-König wrote:
> Hello Ahmad,
> 
> 
> On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> > There, of_mdiobus_register was called even for the fixed-link representing
> > the SPI-connected switch PHY, with the result that the driver attempts to
> > enumerate PHYs on a non-existent MDIO bus:
> > 
> > 	libphy: MACB_mii_bus: probed
> > 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
> >         [snip]
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> > 	macb f0028000.ethernet: broken fixed-link specification
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> 
> I added the people involved in 739de9a1563a to Cc. Maybe they want to
> comment. So not stripping the remaining part of the original mail.
> 
> Best regards
> Uwe

You should probably prod Andrew Lunn, he suggested that I move the fixed
link code from macb_mii_init() to _probe(). Here, you're at least partially
directly undoing that.

(ref: https://www.mail-archive.com/netdev@vger.kernel.org/msg221018.html)

> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++---------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index a6c911bb5ce2..d202a03c42ed 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev)
> >  
> >  	if (np) {
> >  		if (of_phy_is_fixed_link(np)) {
> > -			if (of_phy_register_fixed_link(np) < 0) {
> > -				dev_err(&bp->pdev->dev,
> > -					"broken fixed-link specification\n");
> > -				return -ENODEV;
> > -			}
> >  			bp->phy_node = of_node_get(np);
> >  		} else {
> >  			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
> > @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp)
> >  {
> >  	struct macb_platform_data *pdata;
> >  	struct device_node *np;
> > -	int err;
> > +	int err = -ENXIO;
> >  
> >  	/* Enable management port */
> >  	macb_writel(bp, NCR, MACB_BIT(MPE));
> > @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp)
> >  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
> >  
> >  	np = bp->pdev->dev.of_node;
> > -	if (pdata)
> > -		bp->mii_bus->phy_mask = pdata->phy_mask;
> > +	if (np && of_phy_is_fixed_link(np)) {
> > +		if (of_phy_register_fixed_link(np) < 0) {
> > +			dev_err(&bp->pdev->dev,
> > +					"broken fixed-link specification\n");
> > +			goto err_out_free_mdiobus;
> > +		}
> > +
> > +		err = mdiobus_register(bp->mii_bus);
> > +	} else {
> > +		if (pdata)
> > +			bp->mii_bus->phy_mask = pdata->phy_mask;
> > +
> > +		err = of_mdiobus_register(bp->mii_bus, np);
> > +	}
> >  
> > -	err = of_mdiobus_register(bp->mii_bus, np);
> >  	if (err)
> >  		goto err_out_free_mdiobus;
> >  
> > @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp)
> >  
> >  err_out_unregister_bus:
> >  	mdiobus_unregister(bp->mii_bus);
> > +err_out_free_mdiobus:
> >  	if (np && of_phy_is_fixed_link(np))
> >  		of_phy_deregister_fixed_link(np);
> > -err_out_free_mdiobus:
> >  	of_node_put(bp->phy_node);
> >  	mdiobus_free(bp->mii_bus);
> >  err_out:
> > -- 
> > 2.18.0
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://urldefense.proofpoint.com/v2/url?u=http-3A__www.pengutronix.de_&d=DwIDAw&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=8iZb7YNOSMVIG_mTIHDL03ZcObgQI_gGlWrSewdGETA&m=IQAK1YsKs7Z2bvZxuajiSXw3asFiEztQKYkvy-LpBn8&s=XKrOhFxQshoEcDxMZVSATnJW2cbaD16mQofKdEbJVW0&e=  |

-- 
Brad Mouring
Senior Software Engineer
National Instruments
512-683-6610 / bmouring@ni.com

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Brad Mouring]

Hello Ahmed, Uwe,

On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-K�nig wrote:
> Hello Ahmad,
> 
> 
> On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> > There, of_mdiobus_register was called even for the fixed-link representing
> > the SPI-connected switch PHY, with the result that the driver attempts to
> > enumerate PHYs on a non-existent MDIO bus:
> > 
> > 	libphy: MACB_mii_bus: probed
> > 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
> >         [snip]
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> > 	macb f0028000.ethernet: broken fixed-link specification
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> 
> I added the people involved in 739de9a1563a to Cc. Maybe they want to
> comment. So not stripping the remaining part of the original mail.
> 
> Best regards
> Uwe

You should probably prod Andrew Lunn, he suggested that I move the fixed
link code from macb_mii_init() to _probe(). Here, you're at least partially
directly undoing that.

(ref: https://www.mail-archive.com/netdev@vger.kernel.org/msg221018.html)

> > Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > ---
> >  drivers/net/ethernet/cadence/macb_main.c | 26 +++++++++++++++---------
> >  1 file changed, 16 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index a6c911bb5ce2..d202a03c42ed 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -481,11 +481,6 @@ static int macb_mii_probe(struct net_device *dev)
> >  
> >  	if (np) {
> >  		if (of_phy_is_fixed_link(np)) {
> > -			if (of_phy_register_fixed_link(np) < 0) {
> > -				dev_err(&bp->pdev->dev,
> > -					"broken fixed-link specification\n");
> > -				return -ENODEV;
> > -			}
> >  			bp->phy_node = of_node_get(np);
> >  		} else {
> >  			bp->phy_node = of_parse_phandle(np, "phy-handle", 0);
> > @@ -568,7 +563,7 @@ static int macb_mii_init(struct macb *bp)
> >  {
> >  	struct macb_platform_data *pdata;
> >  	struct device_node *np;
> > -	int err;
> > +	int err = -ENXIO;
> >  
> >  	/* Enable management port */
> >  	macb_writel(bp, NCR, MACB_BIT(MPE));
> > @@ -591,10 +586,21 @@ static int macb_mii_init(struct macb *bp)
> >  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
> >  
> >  	np = bp->pdev->dev.of_node;
> > -	if (pdata)
> > -		bp->mii_bus->phy_mask = pdata->phy_mask;
> > +	if (np && of_phy_is_fixed_link(np)) {
> > +		if (of_phy_register_fixed_link(np) < 0) {
> > +			dev_err(&bp->pdev->dev,
> > +					"broken fixed-link specification\n");
> > +			goto err_out_free_mdiobus;
> > +		}
> > +
> > +		err = mdiobus_register(bp->mii_bus);
> > +	} else {
> > +		if (pdata)
> > +			bp->mii_bus->phy_mask = pdata->phy_mask;
> > +
> > +		err = of_mdiobus_register(bp->mii_bus, np);
> > +	}
> >  
> > -	err = of_mdiobus_register(bp->mii_bus, np);
> >  	if (err)
> >  		goto err_out_free_mdiobus;
> >  
> > @@ -606,9 +612,9 @@ static int macb_mii_init(struct macb *bp)
> >  
> >  err_out_unregister_bus:
> >  	mdiobus_unregister(bp->mii_bus);
> > +err_out_free_mdiobus:
> >  	if (np && of_phy_is_fixed_link(np))
> >  		of_phy_deregister_fixed_link(np);
> > -err_out_free_mdiobus:
> >  	of_node_put(bp->phy_node);
> >  	mdiobus_free(bp->mii_bus);
> >  err_out:
> > -- 
> > 2.18.0
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-K�nig            |
> Industrial Linux Solutions                 | https://urldefense.proofpoint.com/v2/url?u=http-3A__www.pengutronix.de_&d=DwIDAw&c=I_0YwoKy7z5LMTVdyO6YCiE2uzI1jjZZuIPelcSjixA&r=8iZb7YNOSMVIG_mTIHDL03ZcObgQI_gGlWrSewdGETA&m=IQAK1YsKs7Z2bvZxuajiSXw3asFiEztQKYkvy-LpBn8&s=XKrOhFxQshoEcDxMZVSATnJW2cbaD16mQofKdEbJVW0&e=  |

-- 
Brad Mouring
Senior Software Engineer
National Instruments
512-683-6610 / bmouring@ni.com

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Andrew Lunn]

On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-König wrote:
> Hello Ahmad,
> 
> 
> On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> > There, of_mdiobus_register was called even for the fixed-link representing
> > the SPI-connected switch PHY, with the result that the driver attempts to
> > enumerate PHYs on a non-existent MDIO bus:
> > 
> > 	libphy: MACB_mii_bus: probed
> > 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
> >         [snip]
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> > 	macb f0028000.ethernet: broken fixed-link specification
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> 
> I added the people involved in 739de9a1563a to Cc. Maybe they want to
> comment. So not stripping the remaining part of the original mail.

Thanks Uwe for Cc: in my.

Ahmed, where is the device tree for the EVB-KSZ9477?

Thanks
	Andrew	

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Andrew Lunn]

On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-K�nig wrote:
> Hello Ahmad,
> 
> 
> On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> > There, of_mdiobus_register was called even for the fixed-link representing
> > the SPI-connected switch PHY, with the result that the driver attempts to
> > enumerate PHYs on a non-existent MDIO bus:
> > 
> > 	libphy: MACB_mii_bus: probed
> > 	mdio_bus f0028000.ethernet-ffffffff: fixed-link has invalid PHY address
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 0
> >         [snip]
> > 	mdio_bus f0028000.ethernet-ffffffff: scan phy fixed-link at address 31
> > 	macb f0028000.ethernet: broken fixed-link specification
> > 
> > Cc: <stable@vger.kernel.org>
> > Fixes: 739de9a1563a ("net: macb: Reorganize macb_mii bringup")
> 
> I added the people involved in 739de9a1563a to Cc. Maybe they want to
> comment. So not stripping the remaining part of the original mail.

Thanks Uwe for Cc: in my.

Ahmed, where is the device tree for the EVB-KSZ9477?

Thanks
	Andrew	

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Lad, Prabhakar]

Hi,

On Wed, Aug 15, 2018 at 3:35 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Aug 14, 2018 at 05:58:12PM +0200, Uwe Kleine-König wrote:
> > Hello Ahmad,
> >
> >
> > On Tue, Aug 14, 2018 at 04:12:40PM +0200, Ahmad Fatoum wrote:
> > > The referenced commit broke initializing macb on the EVB-KSZ9477 eval board.
> > > There, of_mdiobus_register was called even for the fixed-link representing
> > > the SPI-connected switch PHY, with the result that the driver attempts to
> > > enumerate PHYs on a non-existent MDIO bus:
> > >
I ran into a similar problem on v14.4 for davinci_mdio I had to patch
it with [1].
The cpsw has 2 phys one phy is connected to KSZ9031 and other to
ksz9897 Ethernet switch
which is treated as a fixed phy with no mdio lines because of which
mdio_read/write failed.
This didn’t happen in v4.9.x something in core has changed ?

[1]

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c
b/drivers/net/ethernet/ti/davinci_mdio.c
index 3e84107..197baa6 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -245,6 +245,13 @@ static int davinci_mdio_read(struct mii_bus *bus,
int phy_id, int phy_reg)
        u32 reg;
        int ret;

+       if (phy_id == 2)
+               return 0;
+
        if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
                return -EINVAL;

@@ -289,6 +296,13 @@ static int davinci_mdio_write(struct mii_bus
*bus, int phy_id,
        u32 reg;
        int ret;

+       if (phy_id == 2)
+               return 0;
+
        if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
                return -EINVAL;


Cheers,
--Prabhakar Lad

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

On 08/15/2018 04:32 AM, Andrew Lunn wrote:
> Ahmed, where is the device tree for the EVB-KSZ9477?

I've attached it [1]. It's still work-in-progress (DSA doesn't work yet for example), but Ethernet is usable with Linux v4.18 and my patch applied.

Cheers
Ahmad

[1]


=======================================
diff --git a/arch/arm/boot/dts/at91-sama5d3_xplained_ung8087.dts b/arch/arm/boot/dts/at91-sama5d3_xplained_ung8087.dts
new file mode 100644
index 000000000000..164b6a47d0bf
--- /dev/null
+++ b/arch/arm/boot/dts/at91-sama5d3_xplained_ung8087.dts
@@ -0,0 +1,393 @@
+/*
+ * at91-sama5d3_xplained_ung8087.dts - Device Tree file for the EVB-KSZ9477 board
+ *
+ *  Copyright (C) 2014 Atmel,
+ *		  2014 Nicolas Ferre <nicolas.ferre@atmel.com>
+ *		  2018 Ahmad Fatoum <a.fatoum@pengutronix.de>
+ *
+ * Licensed under GPLv2 or later.
+ */
+/dts-v1/;
+#include "sama5d36.dtsi"
+
+/ {
+	model = "SAMA5D3 Xplained";
+	compatible = "atmel,sama5d3-xplained", "atmel,sama5d3", "atmel,sama5";
+
+	chosen {
+		bootargs = "console=ttyS0,115200";
+	};
+
+	memory {
+		reg = <0x20000000 0x10000000>;
+	};
+
+	clocks {
+		slow_xtal {
+			clock-frequency = <32768>;
+		};
+
+		main_xtal {
+			clock-frequency = <12000000>;
+		};
+	};
+
+	ahb {
+		apb {
+			mmc0: mmc@f0000000 {
+				pinctrl-0 = <&pinctrl_mmc0_clk_cmd_dat0 &pinctrl_mmc0_dat1_3 &pinctrl_mmc0_dat4_7 &pinctrl_mmc0_cd>;
+				status = "okay";
+				slot@0 {
+					reg = <0>;
+					bus-width = <8>;
+					cd-gpios = <&pioE 0 GPIO_ACTIVE_LOW>;
+				};
+			};
+
+			spi0: spi@f0004000 {
+				cs-gpios = <&pioD 13 0>, <0>, <0>, <&pioD 16 0>;
+				id = <0>;
+				status = "okay";
+			};
+
+			can0: can@f000c000 {
+				status = "okay";
+			};
+
+			i2c0: i2c@f0014000 {
+				pinctrl-0 = <&pinctrl_i2c0_pu>;
+				status = "okay";
+			};
+
+			i2c1: i2c@f0018000 {
+				status = "okay";
+
+				pmic: act8865@5b {
+					compatible = "active-semi,act8865";
+					reg = <0x5b>;
+					status = "okay";
+
+					regulators {
+						vcc_1v8_reg: DCDC_REG1 {
+							regulator-name = "VCC_1V8";
+							regulator-min-microvolt = <1800000>;
+							regulator-max-microvolt = <1800000>;
+							regulator-always-on;
+						};
+
+						vcc_1v2_reg: DCDC_REG2 {
+							regulator-name = "VCC_1V2";
+							regulator-min-microvolt = <1200000>;
+							regulator-max-microvolt = <1200000>;
+							regulator-always-on;
+						};
+
+						vcc_3v3_reg: DCDC_REG3 {
+							regulator-name = "VCC_3V3";
+							regulator-min-microvolt = <3300000>;
+							regulator-max-microvolt = <3300000>;
+							regulator-always-on;
+						};
+
+						vddfuse_reg: LDO_REG1 {
+							regulator-name = "FUSE_2V5";
+							regulator-min-microvolt = <2500000>;
+							regulator-max-microvolt = <2500000>;
+						};
+
+						vddana_reg: LDO_REG2 {
+							regulator-name = "VDDANA";
+							regulator-min-microvolt = <3300000>;
+							regulator-max-microvolt = <3300000>;
+							regulator-always-on;
+						};
+					};
+				};
+			};
+
+			macb0: ethernet@f0028000 {
+				phy-mode = "rgmii";
+				gpios = <&pioB 28 GPIO_ACTIVE_LOW>;
+				reset-gpios = <&pioC 31 GPIO_ACTIVE_LOW>;
+				status = "okay";
+
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+
+			pwm0: pwm@f002c000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&pinctrl_pwm0_pwmh0_0 &pinctrl_pwm0_pwmh1_0>;
+				status = "okay";
+			};
+
+			usart0: serial@f001c000 {
+				status = "okay";
+			};
+
+			usart1: serial@f0020000 {
+				pinctrl-0 = <&pinctrl_usart1 &pinctrl_usart1_rts_cts>;
+				status = "okay";
+			};
+
+			uart0: serial@f0024000 {
+				status = "okay";
+			};
+
+			mmc1: mmc@f8000000 {
+				pinctrl-0 = <&pinctrl_mmc1_clk_cmd_dat0 &pinctrl_mmc1_dat1_3 &pinctrl_mmc1_cd>;
+				status = "okay";
+				slot@0 {
+					reg = <0>;
+					bus-width = <4>;
+					cd-gpios = <&pioE 1 GPIO_ACTIVE_HIGH>;
+				};
+			};
+
+			spi1: spi@f8008000 {
+				pinctrl-0 = <&pinctrl_spi_ksz>;
+				cs-gpios = <&pioC 25 0>;
+				id = <1>;
+				status = "okay";
+
+				ksz9477: ksz9477@0 {
+					compatible = "microchip,ksz9477", "microchip,ksz9893";
+					reg = <0>;
+
+					/* Bus clock is 132 MHz. */
+					spi-max-frequency = <44000000>;
+					spi-cpha;
+					spi-cpol;
+					gpios = <&pioB 28 GPIO_ACTIVE_LOW>;
+					status = "okay";
+
+					ports {
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						port@0 {
+							reg = <0>;
+							label = "lan0";
+						};
+
+						port@1 {
+							reg = <1>;
+							label = "lan1";
+						};
+
+						port@2 {
+							reg = <2>;
+							label = "lan2";
+						};
+
+						port@3 {
+							reg = <3>;
+							label = "lan3";
+						};
+
+						port@4 {
+							reg = <4>;
+							label = "lan4";
+						};
+
+						port@5 {
+							reg = <5>;
+							label = "cpu";
+							ethernet = <&macb0>;
+							phy-mode = "rgmii-id";
+
+							fixed-link {
+								speed = <0x3e8>;
+								full-duplex;
+							};
+						};
+
+						/* port 6 is connected to eth0 */
+					};
+				};
+			};
+
+			adc0: adc@f8018000 {
+				pinctrl-0 = <
+					&pinctrl_adc0_adtrg
+					&pinctrl_adc0_ad0
+					&pinctrl_adc0_ad1
+					&pinctrl_adc0_ad2
+					&pinctrl_adc0_ad3
+					&pinctrl_adc0_ad4
+					&pinctrl_adc0_ad5
+					&pinctrl_adc0_ad6
+					&pinctrl_adc0_ad7
+					&pinctrl_adc0_ad8
+					&pinctrl_adc0_ad9
+					>;
+				status = "okay";
+			};
+
+			i2c2: i2c@f801c000 {
+				dmas = <0>, <0>;	/* Do not use DMA for i2c2 */
+				pinctrl-0 = <&pinctrl_i2c2_pu>;
+				status = "okay";
+			};
+
+			macb1: ethernet@f802c000 {
+				phy-mode = "rmii";
+				status = "disabled";
+			};
+
+			dbgu: serial@ffffee00 {
+				status = "okay";
+			};
+
+			pinctrl@fffff200 {
+				board {
+					pinctrl_i2c0_pu: i2c0_pu {
+						atmel,pins =
+							<AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>,
+							<AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
+					};
+
+					pinctrl_i2c2_pu: i2c2_pu {
+						atmel,pins =
+							<AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_PULL_UP>,
+							<AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_PULL_UP>;
+					};
+
+					pinctrl_mmc0_cd: mmc0_cd {
+						atmel,pins =
+							<AT91_PIOE 0 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
+					};
+
+					pinctrl_mmc1_cd: mmc1_cd {
+						atmel,pins =
+							<AT91_PIOE 1 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
+					};
+
+					pinctrl_usba_vbus: usba_vbus {
+						atmel,pins =
+							<AT91_PIOE 9 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH>;	/* PE9, conflicts with A9 */
+					};
+
+					pinctrl_spi_ksz: spi_ksz {
+						atmel,pins =
+							<
+							AT91_PIOB 28 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH
+							AT91_PIOC 31 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH
+							>;
+					};
+				};
+			};
+
+			pmc: pmc@fffffc00 {
+				main: mainck {
+					clock-frequency = <12000000>;
+				};
+			};
+		};
+
+		ebi: ebi@10000000 {
+			pinctrl-0 = <&pinctrl_ebi_nand_addr>;
+			pinctrl-names = "default";
+			status = "okay";
+
+			nand_controller: nand-controller {
+				status = "okay";
+
+				nand@3 {
+					reg = <0x3 0x0 0x2>;
+					atmel,rb = <0>;
+					nand-bus-width = <8>;
+					nand-ecc-mode = "hw";
+					nand-ecc-strength = <4>;
+					nand-ecc-step-size = <512>;
+					nand-on-flash-bbt;
+					label = "atmel_nand";
+
+					partitions {
+						compatible = "fixed-partitions";
+						#address-cells = <1>;
+						#size-cells = <1>;
+
+						at91bootstrap@0 {
+							label = "at91bootstrap";
+							reg = <0x0 0x40000>;
+						};
+
+						bootloader@40000 {
+							label = "bootloader";
+							reg = <0x40000 0x80000>;
+						};
+
+						bootloaderenv@c0000 {
+							label = "bootloader env";
+							reg = <0xc0000 0xc0000>;
+						};
+
+						dtb@180000 {
+							label = "device tree";
+							reg = <0x180000 0x80000>;
+						};
+
+						kernel@200000 {
+							label = "kernel";
+							reg = <0x200000 0x600000>;
+						};
+
+						rootfs@800000 {
+							label = "rootfs";
+							reg = <0x800000 0x0f800000>;
+						};
+					};
+				};
+			};
+		};
+
+		usb0: gadget@500000 {
+			atmel,vbus-gpio = <&pioE 9 GPIO_ACTIVE_HIGH>;	/* PE9, conflicts with A9 */
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_usba_vbus>;
+			status = "okay";
+		};
+
+		usb1: ohci@600000 {
+			num-ports = <3>;
+			atmel,vbus-gpio = <0
+					   &pioE 3 GPIO_ACTIVE_LOW
+					   &pioE 4 GPIO_ACTIVE_LOW
+					  >;
+			status = "okay";
+		};
+
+		usb2: ehci@700000 {
+			status = "okay";
+		};
+	};
+
+	gpio_keys {
+		compatible = "gpio-keys";
+
+		bp3 {
+			label = "PB_USER";
+			gpios = <&pioE 29 GPIO_ACTIVE_LOW>;
+			linux,code = <0x104>;
+			gpio-key,wakeup;
+		};
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		d2 {
+			label = "d2";
+			gpios = <&pioE 23 GPIO_ACTIVE_LOW>;	/* PE23, conflicts with A23, CTS2 */
+			linux,default-trigger = "heartbeat";
+		};
+
+		d3 {
+			label = "d3";
+			gpios = <&pioE 24 GPIO_ACTIVE_HIGH>;
+		};
+	};
+};
-- 
2.18.0

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Andrew Lunn]

On Thu, Aug 16, 2018 at 08:54:40AM +0200, Ahmad Fatoum wrote:
> On 08/15/2018 04:32 AM, Andrew Lunn wrote:
> > Ahmed, where is the device tree for the EVB-KSZ9477?
> 
> I've attached it [1]. It's still work-in-progress (DSA doesn't work yet for example), but Ethernet is usable with Linux v4.18 and my patch applied.
> 

Thanks. 

So the problem is, macb does not put phy DT nodes inside an mdio
subnode. It places them directly in the MAC node. So
of_mdiobus_register() is being called with the MAC
np. of_mdiobus_register() then looks for children of the MAC node,
assuming they are phys. But when you have a fixed phy node, it is not
a phy, it does not have a reg property, and you get these warnings.

There are cases when you need both fixed-phy and a mdio bus. e.g. a
DSA switch hanging off MDIO.

So we have a few things here...

1) A regression. We should find a fix for that. Maybe we should
   special case a child node called 'fixed-link' in
   of_mdiobus_register(). I would suggest adding a single warning if
   such node is found.

2) Missing functionality. Add support for an mdio container node. 

        node = of_get_child_by_name(np, "mdio");
        if (node)
		err = of_mdiobus_register(bp->mii_bus, node);
        else 
		err = of_mdiobus_register(bp->mii_bus, np);

3) Modify the existing dts files to make use of this container.
   Because of backwards compatibility, we cannot force the use of it,
   but we can encourage it.

   Andrew

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

On 08/16/2018 04:24 PM, Andrew Lunn wrote:
> 1) A regression. We should find a fix for that. Maybe we should
>    special case a child node called 'fixed-link' in
>    of_mdiobus_register(). I would suggest adding a single warning if
>    such node is found.

I just sent out a v2, which warns if fixed-link is encountered
in of_mdiobus_register(). The actual fixed-link handling remains in the
macb driver, because I think it's would be out of scope for a regression
fix to change where fixed-links are handled for all using drivers...

> 2) Missing functionality. Add support for an mdio container node. 
> 
>         node = of_get_child_by_name(np, "mdio");
>         if (node)
> 		err = of_mdiobus_register(bp->mii_bus, node);
>         else 
> 		err = of_mdiobus_register(bp->mii_bus, np);

Done.
 
> 3) Modify the existing dts files to make use of this container.
>    Because of backwards compatibility, we cannot force the use of it,
>    but we can encourage it.

Done as well.

Cheers
Ahmad

Re: [PATCH] net: macb: Fix regression breaking non-MDIO fixed-link PHYs

[Ahmad Fatoum]

On 08/15/2018 03:59 PM, Lad, Prabhakar wrote:
> This didn’t happen in v4.9.x something in core has changed ?

In my case, it was the macb driver that changed. I found the offending commit
with git-bisect, you might want to give it a try.