[PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Dajun Jin]

when registers a phy_device successful, should terminate the loop
or the phy_device would be registered in other addr.

Signed-off-by: Dajun Jin <adajunjin@gmail.com>
---
 drivers/of/of_mdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 8270bbf505fb..9f982c0627a0 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -306,6 +306,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 				rc = of_mdiobus_register_phy(mdio, child, addr);
 				if (rc && rc != -ENODEV)
 					goto unregister;
+				break;
 			}
 		}
 	}
-- 
2.17.1

Re: [PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Andrew Lunn]

On Mon, Mar 02, 2020 at 12:41:38AM +0800, Dajun Jin wrote:
> when registers a phy_device successful, should terminate the loop
> or the phy_device would be registered in other addr.
> 
> Signed-off-by: Dajun Jin <adajunjin@gmail.com>
> ---
>  drivers/of/of_mdio.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 8270bbf505fb..9f982c0627a0 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -306,6 +306,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  				rc = of_mdiobus_register_phy(mdio, child, addr);
>  				if (rc && rc != -ENODEV)
>  					goto unregister;
> +				break;
>  			}
>  		}
>  	}

Hi Dajun

What problem are you seeing? You explanation needs to be better.

I'm guessing you have two or more PHYs on the bus, without reg
properties?

	Andrew

Re: [PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Dajun Jin]

>On Mon, Mar 02, 2020 at 12:41:38AM +0800, Dajun Jin wrote:
>> when registers a phy_device successful, should terminate the loop
>> or the phy_device would be registered in other addr.
>> 
>> Signed-off-by: Dajun Jin <adajunjin@xxxxxxxxx>
>> ---
>>  drivers/of/of_mdio.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 8270bbf505fb..9f982c0627a0 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -306,6 +306,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>>  				rc = of_mdiobus_register_phy(mdio, child, addr);
>>  				if (rc && rc != -ENODEV)
>>  					goto unregister;
>> +				break;
>>  			}
>>  		}
>>  	}
>
>Hi Dajun
>
>What problem are you seeing? You explanation needs to be better.
>
>I'm guessing you have two or more PHYs on the bus, without reg
>properties?
>
>	Andrew

Hi Andrew

If a phy without reg property would be registered to all unoccupied addr.

This is my test in Xilinx zcu106 board.

dts is liks this:
ethernet@ff0e0000 {
    compatible = "cdns,zynqmp-gem", "cdns,gem";
    status = "okay";
    ...
    
    phy@0 {
        ti,rx-internal-delay = <0x8>;
        ti,tx-internal-delay = <0xa>;
        ti,fifo-depth = <0x1>;
        ti,rxctrl-strap-worka;
        linux,phandle = <0x12>;
        phandle = <0x12>;
    };
};

then when borad is booting,the dmesg is like this:
[    4.600035] mdio_bus ff0e0000.ethernet-ffffffff: /amba/ethernet@ff0e0000/phy@0 has invalid PHY address
[    4.600050] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 0
[    4.602076] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 1
[    4.603849] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 2
[    4.605574] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 4
[    4.607312] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 5
...
[    4.636155] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 28
[    4.637335] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 29
[    4.638504] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 30
[    4.639666] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 31

	Dajun

Re: [PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Andrew Lunn]

Hi Dajun

> This is my test in Xilinx zcu106 board.
> 
> dts is liks this:
> ethernet@ff0e0000 {
>     compatible = "cdns,zynqmp-gem", "cdns,gem";
>     status = "okay";
>     ...
>     
>     phy@0 {
>         ti,rx-internal-delay = <0x8>;
>         ti,tx-internal-delay = <0xa>;
>         ti,fifo-depth = <0x1>;
>         ti,rxctrl-strap-worka;
>         linux,phandle = <0x12>;
>         phandle = <0x12>;
>     };
> };
> 
> then when borad is booting,the dmesg is like this:
> [    4.600035] mdio_bus ff0e0000.ethernet-ffffffff: /amba/ethernet@ff0e0000/phy@0 has invalid PHY address
> [    4.600050] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 0
> [    4.602076] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 1
> [    4.603849] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 2
> [    4.605574] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 4
> [    4.607312] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 5
> ...
> [    4.636155] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 28
> [    4.637335] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 29
> [    4.638504] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 30
> [    4.639666] mdio_bus ff0e0000.ethernet-ffffffff: scan phy phy at address 31

For a single PHY without a reg properties, it should do the right
thing. But it will go wrong if there are multiple PHYs without reg
properties. In that case, the break helps.

However, as the comment suggests, you really should have a reg
property.

Please make the commit message better, and then i will give a
reviewed-by.

Thanks
	Andrew

Re: [PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Russell King - ARM Linux admin]

On Mon, Mar 02, 2020 at 06:57:59PM +0100, Andrew Lunn wrote:
> Hi Dajun
> 
> > This is my test in Xilinx zcu106 board.
> > 
> > dts is liks this:
> > ethernet@ff0e0000 {
> >     compatible = "cdns,zynqmp-gem", "cdns,gem";
> >     status = "okay";
> >     ...
> >     
> >     phy@0 {
> >         ti,rx-internal-delay = <0x8>;
> >         ti,tx-internal-delay = <0xa>;
> >         ti,fifo-depth = <0x1>;
> >         ti,rxctrl-strap-worka;
> >         linux,phandle = <0x12>;
> >         phandle = <0x12>;

Isn't dtc going to complain about this?  The node name has an address,
but there's no reg property.  If there's no reg property, shouldn't it
be just "phy" ?

The above doesn't look like the original .dts file itself either, but
a .dtb translated back to a .dts - note the numeric phandle properties
and presence of "linux,phandle".

ethernet-phy.yaml also says:

required:
  - reg

so arguably the above doesn't conform to what we expect?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

[PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Dajun Jin]

When registers a phy_device successful, should terminate the loop
or the phy_device would be registered in other addr. If there are
multiple PHYs without reg properties, it will go wrong.

Signed-off-by: Dajun Jin <adajunjin@gmail.com>
---
Hi Andrew, Thanks for your review.

 drivers/of/of_mdio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 8270bbf505fb..9f982c0627a0 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -306,6 +306,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 				rc = of_mdiobus_register_phy(mdio, child, addr);
 				if (rc && rc != -ENODEV)
 					goto unregister;
+				break;
 			}
 		}
 	}
-- 
2.17.1

Re: [PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[Andrew Lunn]

On Mon, Mar 02, 2020 at 08:24:21PM -0800, Dajun Jin wrote:
> When registers a phy_device successful, should terminate the loop
> or the phy_device would be registered in other addr. If there are
> multiple PHYs without reg properties, it will go wrong.
> 
> Signed-off-by: Dajun Jin <adajunjin@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH] drivers/of/of_mdio.c:fix of_mdiobus_register()

[David Miller]

From: Dajun Jin <adajunjin@gmail.com>
Date: Mon,  2 Mar 2020 20:24:21 -0800

> When registers a phy_device successful, should terminate the loop
> or the phy_device would be registered in other addr. If there are
> multiple PHYs without reg properties, it will go wrong.
> 
> Signed-off-by: Dajun Jin <adajunjin@gmail.com>

Applied, thank you.