| Message ID | 20200301164138.8542-1-adajunjin@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | drivers/of/of_mdio.c:fix of_mdiobus_register() | expand |
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
>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
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
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?
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; } } }
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(+)