| Message ID | 4aaa550bcc9566e14716309e3bf2c04f9c100b1d.1619440296.git.michal.simek@xilinx.com |
|---|---|
| State | Accepted |
| Commit | 0a9f0e0d00e0e8b2c5b6a2ae844c5ebca684616f |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | net: phy: xilinx: Break while loop over ethernet phy | expand |
Hi Michal, On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: > > The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF > tree") change driver behavior to while loop which wasn't correct because > the driver was looping over again and again. The reason was that > ofnode_valid() is taking 0 as correct value. I am still trying to understand the problem. The changes in 6c993815bbea sound correct from an fdtdec <=> OF API mapping perspective. If the new OF API does not work, the old fdtdec may fail too. Could you please explain a little bit? > Fix it by changing while loop to ofnode_for_each_subnode() which is only > loop over available nodes. > > Fixes: 6c993815bbea ("net: phy: xilinx: Be compatible with live OF tree") > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > drivers/net/phy/phy.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Regards, Bin
Hi Bin, On 4/27/21 7:17 AM, Bin Meng wrote: > Hi Michal, > > On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: >> >> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF >> tree") change driver behavior to while loop which wasn't correct because >> the driver was looping over again and again. The reason was that >> ofnode_valid() is taking 0 as correct value. > > I am still trying to understand the problem. The changes in > 6c993815bbea sound correct from an fdtdec <=> OF API mapping > perspective. If the new OF API does not work, the old fdtdec may fail > too. Could you please explain a little bit? here is behavior of origin code. ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id phy_connect_gmii2rgmii sn 11348 phy_connect_gmii2rgmii 1off -1 phy_connect_gmii2rgmii 2off -1 phy_connect_gmii2rgmii sn2 11752 phy_connect_gmii2rgmii 1off -1 phy_connect_gmii2rgmii 2off -1 phy_connect_gmii2rgmii sn2 -1 phy_connect_gmii2rgmii phydev 0000000000000000 eth0: ethernet@ff0e0000 Scanning disk mmc@ff170000.blk... Found 4 disks Hit any key to stop autoboot: 0 ZynqMP> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 89e3076bfd25..d0960d93ae08 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -956,22 +956,28 @@ static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus, int sn = dev_of_offset(dev); int off; + printf("%s sn %d\n", __func__, sn); while (sn > 0) { off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, "xlnx,gmii-to-rgmii-1.0"); + printf("%s 1off %d\n", __func__, off); if (off > 0) { phydev = phy_device_create(bus, off, PHY_GMII2RGMII_ID, false, interface); break; } - if (off == -FDT_ERR_NOTFOUND) + printf("%s 2off %d\n", __func__, off); + + if (off == -FDT_ERR_NOTFOUND) { sn = fdt_first_subnode(gd->fdt_blob, sn); - else + printf("%s sn2 %d\n", __func__, sn); + } else printf("%s: Error finding compat string:%d\n", __func__, off); } + printf("%s phydev %p\n", __func__, phydev); return phydev; } #endif With latest code and some prints ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset 2952 ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset -1 ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset 11348 phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset 11348 ofnode_valid: node.of_offset 11348 phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 ofnode_valid: node.of_offset -1 ofnode_valid: node.of_offset -1 ofnode_valid: node.of_offset 0 ofnode_valid: node.of_offset 0 phy_connect_gmii2rgmii 3valid 1 ofnode_valid: node.of_offset 0 ofnode_valid: node.of_offset 0 ofnode_valid: node.of_offset 0 phy_connect_gmii2rgmii 2valid 1 ofnode_valid: node.of_offset -1 ofnode_valid: node.of_offset -1 ofnode_valid: node.of_offset 0 ofnode_valid: node.of_offset 0 phy_connect_gmii2rgmii 3valid 1 ... [u-boot](debian-sent)$ git diff diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index dcdef9e661d6..56072ad55216 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -950,7 +950,10 @@ static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus, struct phy_device *phydev = NULL; ofnode node = dev_ofnode(dev); + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), ofnode_get_name(node)); + while (ofnode_valid(node)) { + printf("%s 2valid %d %s\n", __func__, ofnode_valid(node), ofnode_get_name(node)); node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); if (ofnode_valid(node)) { phydev = phy_device_create(bus, 0, @@ -962,6 +965,7 @@ static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus, } node = ofnode_first_subnode(node); + printf("%s 3valid %d %s\n", __func__, ofnode_valid(node), ofnode_get_name(node)); } return phydev; diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h index 2c0597c40739..7bfc06165e92 100644 --- a/include/dm/ofnode.h +++ b/include/dm/ofnode.h @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) { if (of_live_active()) return node.np != NULL; - else + else { + printf("ofnode_valid: node.of_offset %ld\n", node.of_offset); return node.of_offset >= 0; + } } /** It means ofnode_first_subnode is returning any node which has node.of_offset == 0 which is consider based on ofnode_valid() as valid node that's why it is looping over it. Thanks, Michal
Hi Michal, On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek@xilinx.com> wrote: > > Hi Bin, > > On 4/27/21 7:17 AM, Bin Meng wrote: > > Hi Michal, > > > > On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF > >> tree") change driver behavior to while loop which wasn't correct because > >> the driver was looping over again and again. The reason was that > >> ofnode_valid() is taking 0 as correct value. > > > > I am still trying to understand the problem. The changes in > > 6c993815bbea sound correct from an fdtdec <=> OF API mapping > > perspective. If the new OF API does not work, the old fdtdec may fail > > too. Could you please explain a little bit? > > here is behavior of origin code. > > ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id > phy_connect_gmii2rgmii sn 11348 > phy_connect_gmii2rgmii 1off -1 > phy_connect_gmii2rgmii 2off -1 > phy_connect_gmii2rgmii sn2 11752 > phy_connect_gmii2rgmii 1off -1 > phy_connect_gmii2rgmii 2off -1 > phy_connect_gmii2rgmii sn2 -1 > phy_connect_gmii2rgmii phydev 0000000000000000 > eth0: ethernet@ff0e0000 > Scanning disk mmc@ff170000.blk... > Found 4 disks > Hit any key to stop autoboot: 0 > ZynqMP> > > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index 89e3076bfd25..d0960d93ae08 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -956,22 +956,28 @@ static struct phy_device > *phy_connect_gmii2rgmii(struct mii_dev *bus, > int sn = dev_of_offset(dev); > int off; > > + printf("%s sn %d\n", __func__, sn); > while (sn > 0) { > off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, > > "xlnx,gmii-to-rgmii-1.0"); > + printf("%s 1off %d\n", __func__, off); > if (off > 0) { > phydev = phy_device_create(bus, off, > PHY_GMII2RGMII_ID, false, > interface); > break; > } > - if (off == -FDT_ERR_NOTFOUND) > + printf("%s 2off %d\n", __func__, off); > + > + if (off == -FDT_ERR_NOTFOUND) { > sn = fdt_first_subnode(gd->fdt_blob, sn); > - else > + printf("%s sn2 %d\n", __func__, sn); > + } else > printf("%s: Error finding compat string:%d\n", > __func__, off); > } > > + printf("%s phydev %p\n", __func__, phydev); > return phydev; > } > #endif > > > With latest code and some prints > ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset 2952 > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset -1 > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset 11348 > phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset 11348 > ofnode_valid: node.of_offset 11348 > phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 > ofnode_valid: node.of_offset -1 > ofnode_valid: node.of_offset -1 > ofnode_valid: node.of_offset 0 > ofnode_valid: node.of_offset 0 > phy_connect_gmii2rgmii 3valid 1 > ofnode_valid: node.of_offset 0 > ofnode_valid: node.of_offset 0 > ofnode_valid: node.of_offset 0 > phy_connect_gmii2rgmii 2valid 1 > ofnode_valid: node.of_offset -1 > ofnode_valid: node.of_offset -1 > ofnode_valid: node.of_offset 0 > ofnode_valid: node.of_offset 0 > phy_connect_gmii2rgmii 3valid 1 > ... > > > [u-boot](debian-sent)$ git diff > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > index dcdef9e661d6..56072ad55216 100644 > --- a/drivers/net/phy/phy.c > +++ b/drivers/net/phy/phy.c > @@ -950,7 +950,10 @@ static struct phy_device > *phy_connect_gmii2rgmii(struct mii_dev *bus, > struct phy_device *phydev = NULL; > ofnode node = dev_ofnode(dev); > > + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), > ofnode_get_name(node)); > + > while (ofnode_valid(node)) { > + printf("%s 2valid %d %s\n", __func__, > ofnode_valid(node), ofnode_get_name(node)); > node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); > if (ofnode_valid(node)) { > phydev = phy_device_create(bus, 0, > @@ -962,6 +965,7 @@ static struct phy_device > *phy_connect_gmii2rgmii(struct mii_dev *bus, > } > > node = ofnode_first_subnode(node); > + printf("%s 3valid %d %s\n", __func__, > ofnode_valid(node), ofnode_get_name(node)); > } > > return phydev; > > > diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h > index 2c0597c40739..7bfc06165e92 100644 > --- a/include/dm/ofnode.h > +++ b/include/dm/ofnode.h > @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) > { > if (of_live_active()) > return node.np != NULL; > - else > + else { > + printf("ofnode_valid: node.of_offset %ld\n", > node.of_offset); > return node.of_offset >= 0; > + } > } > > /** > > > It means ofnode_first_subnode is returning any node which has > node.of_offset == 0 which is consider based on ofnode_valid() as valid This sounds suspicious to me that ofnode_first_subnode() returns a node with node.of_offset being zero. I think it should return 11752? > node that's why it is looping over it. Regards, Bin
Hi Bin, On 4/28/21 4:37 PM, Bin Meng wrote: > Hi Michal, > > On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek@xilinx.com> wrote: >> >> Hi Bin, >> >> On 4/27/21 7:17 AM, Bin Meng wrote: >>> Hi Michal, >>> >>> On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: >>>> >>>> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF >>>> tree") change driver behavior to while loop which wasn't correct because >>>> the driver was looping over again and again. The reason was that >>>> ofnode_valid() is taking 0 as correct value. >>> >>> I am still trying to understand the problem. The changes in >>> 6c993815bbea sound correct from an fdtdec <=> OF API mapping >>> perspective. If the new OF API does not work, the old fdtdec may fail >>> too. Could you please explain a little bit? >> >> here is behavior of origin code. >> >> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id >> phy_connect_gmii2rgmii sn 11348 >> phy_connect_gmii2rgmii 1off -1 >> phy_connect_gmii2rgmii 2off -1 >> phy_connect_gmii2rgmii sn2 11752 >> phy_connect_gmii2rgmii 1off -1 >> phy_connect_gmii2rgmii 2off -1 >> phy_connect_gmii2rgmii sn2 -1 >> phy_connect_gmii2rgmii phydev 0000000000000000 >> eth0: ethernet@ff0e0000 >> Scanning disk mmc@ff170000.blk... >> Found 4 disks >> Hit any key to stop autoboot: 0 >> ZynqMP> >> >> >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index 89e3076bfd25..d0960d93ae08 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -956,22 +956,28 @@ static struct phy_device >> *phy_connect_gmii2rgmii(struct mii_dev *bus, >> int sn = dev_of_offset(dev); >> int off; >> >> + printf("%s sn %d\n", __func__, sn); >> while (sn > 0) { >> off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, >> >> "xlnx,gmii-to-rgmii-1.0"); >> + printf("%s 1off %d\n", __func__, off); >> if (off > 0) { >> phydev = phy_device_create(bus, off, >> PHY_GMII2RGMII_ID, false, >> interface); >> break; >> } >> - if (off == -FDT_ERR_NOTFOUND) >> + printf("%s 2off %d\n", __func__, off); >> + >> + if (off == -FDT_ERR_NOTFOUND) { >> sn = fdt_first_subnode(gd->fdt_blob, sn); >> - else >> + printf("%s sn2 %d\n", __func__, sn); >> + } else >> printf("%s: Error finding compat string:%d\n", >> __func__, off); >> } >> >> + printf("%s phydev %p\n", __func__, phydev); >> return phydev; >> } >> #endif >> >> >> With latest code and some prints >> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset 2952 >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset -1 >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset 11348 >> phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset 11348 >> ofnode_valid: node.of_offset 11348 >> phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 >> ofnode_valid: node.of_offset -1 >> ofnode_valid: node.of_offset -1 >> ofnode_valid: node.of_offset 0 >> ofnode_valid: node.of_offset 0 >> phy_connect_gmii2rgmii 3valid 1 >> ofnode_valid: node.of_offset 0 >> ofnode_valid: node.of_offset 0 >> ofnode_valid: node.of_offset 0 >> phy_connect_gmii2rgmii 2valid 1 >> ofnode_valid: node.of_offset -1 >> ofnode_valid: node.of_offset -1 >> ofnode_valid: node.of_offset 0 >> ofnode_valid: node.of_offset 0 >> phy_connect_gmii2rgmii 3valid 1 >> ... >> >> >> [u-boot](debian-sent)$ git diff >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >> index dcdef9e661d6..56072ad55216 100644 >> --- a/drivers/net/phy/phy.c >> +++ b/drivers/net/phy/phy.c >> @@ -950,7 +950,10 @@ static struct phy_device >> *phy_connect_gmii2rgmii(struct mii_dev *bus, >> struct phy_device *phydev = NULL; >> ofnode node = dev_ofnode(dev); >> >> + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), >> ofnode_get_name(node)); >> + >> while (ofnode_valid(node)) { >> + printf("%s 2valid %d %s\n", __func__, >> ofnode_valid(node), ofnode_get_name(node)); >> node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); >> if (ofnode_valid(node)) { >> phydev = phy_device_create(bus, 0, >> @@ -962,6 +965,7 @@ static struct phy_device >> *phy_connect_gmii2rgmii(struct mii_dev *bus, >> } >> >> node = ofnode_first_subnode(node); >> + printf("%s 3valid %d %s\n", __func__, >> ofnode_valid(node), ofnode_get_name(node)); >> } >> >> return phydev; >> >> >> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h >> index 2c0597c40739..7bfc06165e92 100644 >> --- a/include/dm/ofnode.h >> +++ b/include/dm/ofnode.h >> @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) >> { >> if (of_live_active()) >> return node.np != NULL; >> - else >> + else { >> + printf("ofnode_valid: node.of_offset %ld\n", >> node.of_offset); >> return node.of_offset >= 0; >> + } >> } >> >> /** >> >> >> It means ofnode_first_subnode is returning any node which has >> node.of_offset == 0 which is consider based on ofnode_valid() as valid > > This sounds suspicious to me that ofnode_first_subnode() returns a > node with node.of_offset being zero. I think it should return 11752? Gem driver is pointing via phy-handle directly to now which should be used. That's why there is no subnode but maybe it can be. Do you have any idea how to debug this to see that node content for OF_LIVE and !OF_LIVE? Anyway I need to get this fix sooner rather than later. One option is this patch and the second is disable this bridge for now. Thanks, Michal
Hi Michal, On Wed, Apr 28, 2021 at 11:03 PM Michal Simek <michal.simek@xilinx.com> wrote: > > Hi Bin, > > On 4/28/21 4:37 PM, Bin Meng wrote: > > Hi Michal, > > > > On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> Hi Bin, > >> > >> On 4/27/21 7:17 AM, Bin Meng wrote: > >>> Hi Michal, > >>> > >>> On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: > >>>> > >>>> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF > >>>> tree") change driver behavior to while loop which wasn't correct because > >>>> the driver was looping over again and again. The reason was that > >>>> ofnode_valid() is taking 0 as correct value. > >>> > >>> I am still trying to understand the problem. The changes in > >>> 6c993815bbea sound correct from an fdtdec <=> OF API mapping > >>> perspective. If the new OF API does not work, the old fdtdec may fail > >>> too. Could you please explain a little bit? > >> > >> here is behavior of origin code. > >> > >> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id > >> phy_connect_gmii2rgmii sn 11348 > >> phy_connect_gmii2rgmii 1off -1 > >> phy_connect_gmii2rgmii 2off -1 > >> phy_connect_gmii2rgmii sn2 11752 > >> phy_connect_gmii2rgmii 1off -1 > >> phy_connect_gmii2rgmii 2off -1 > >> phy_connect_gmii2rgmii sn2 -1 > >> phy_connect_gmii2rgmii phydev 0000000000000000 > >> eth0: ethernet@ff0e0000 > >> Scanning disk mmc@ff170000.blk... > >> Found 4 disks > >> Hit any key to stop autoboot: 0 > >> ZynqMP> > >> > >> > >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > >> index 89e3076bfd25..d0960d93ae08 100644 > >> --- a/drivers/net/phy/phy.c > >> +++ b/drivers/net/phy/phy.c > >> @@ -956,22 +956,28 @@ static struct phy_device > >> *phy_connect_gmii2rgmii(struct mii_dev *bus, > >> int sn = dev_of_offset(dev); > >> int off; > >> > >> + printf("%s sn %d\n", __func__, sn); > >> while (sn > 0) { > >> off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, > >> > >> "xlnx,gmii-to-rgmii-1.0"); > >> + printf("%s 1off %d\n", __func__, off); > >> if (off > 0) { > >> phydev = phy_device_create(bus, off, > >> PHY_GMII2RGMII_ID, false, > >> interface); > >> break; > >> } > >> - if (off == -FDT_ERR_NOTFOUND) > >> + printf("%s 2off %d\n", __func__, off); > >> + > >> + if (off == -FDT_ERR_NOTFOUND) { > >> sn = fdt_first_subnode(gd->fdt_blob, sn); > >> - else > >> + printf("%s sn2 %d\n", __func__, sn); > >> + } else > >> printf("%s: Error finding compat string:%d\n", > >> __func__, off); > >> } > >> > >> + printf("%s phydev %p\n", __func__, phydev); > >> return phydev; > >> } > >> #endif > >> > >> > >> With latest code and some prints > >> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset 2952 > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset -1 > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset 11348 > >> phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset 11348 > >> ofnode_valid: node.of_offset 11348 > >> phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 > >> ofnode_valid: node.of_offset -1 > >> ofnode_valid: node.of_offset -1 > >> ofnode_valid: node.of_offset 0 > >> ofnode_valid: node.of_offset 0 > >> phy_connect_gmii2rgmii 3valid 1 > >> ofnode_valid: node.of_offset 0 > >> ofnode_valid: node.of_offset 0 > >> ofnode_valid: node.of_offset 0 > >> phy_connect_gmii2rgmii 2valid 1 > >> ofnode_valid: node.of_offset -1 > >> ofnode_valid: node.of_offset -1 > >> ofnode_valid: node.of_offset 0 > >> ofnode_valid: node.of_offset 0 > >> phy_connect_gmii2rgmii 3valid 1 > >> ... > >> > >> > >> [u-boot](debian-sent)$ git diff > >> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > >> index dcdef9e661d6..56072ad55216 100644 > >> --- a/drivers/net/phy/phy.c > >> +++ b/drivers/net/phy/phy.c > >> @@ -950,7 +950,10 @@ static struct phy_device > >> *phy_connect_gmii2rgmii(struct mii_dev *bus, > >> struct phy_device *phydev = NULL; > >> ofnode node = dev_ofnode(dev); > >> > >> + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), > >> ofnode_get_name(node)); > >> + > >> while (ofnode_valid(node)) { > >> + printf("%s 2valid %d %s\n", __func__, > >> ofnode_valid(node), ofnode_get_name(node)); > >> node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); > >> if (ofnode_valid(node)) { > >> phydev = phy_device_create(bus, 0, > >> @@ -962,6 +965,7 @@ static struct phy_device > >> *phy_connect_gmii2rgmii(struct mii_dev *bus, > >> } > >> > >> node = ofnode_first_subnode(node); > >> + printf("%s 3valid %d %s\n", __func__, > >> ofnode_valid(node), ofnode_get_name(node)); > >> } > >> > >> return phydev; > >> > >> > >> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h > >> index 2c0597c40739..7bfc06165e92 100644 > >> --- a/include/dm/ofnode.h > >> +++ b/include/dm/ofnode.h > >> @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) > >> { > >> if (of_live_active()) > >> return node.np != NULL; > >> - else > >> + else { > >> + printf("ofnode_valid: node.of_offset %ld\n", > >> node.of_offset); > >> return node.of_offset >= 0; > >> + } > >> } > >> > >> /** > >> > >> > >> It means ofnode_first_subnode is returning any node which has > >> node.of_offset == 0 which is consider based on ofnode_valid() as valid > > > > This sounds suspicious to me that ofnode_first_subnode() returns a > > node with node.of_offset being zero. I think it should return 11752? > > Gem driver is pointing via phy-handle directly to now which should be > used. That's why there is no subnode but maybe it can be. > > Do you have any idea how to debug this to see that node content for > OF_LIVE and !OF_LIVE? I created a test case below, and reproduced the endless loop you mentioned, but on 2021.04, IOW, without my OF API change patch. diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index 2600360224..8543fbe497 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -1360,6 +1360,18 @@ mdio: mdio-test { compatible = "sandbox,mdio"; + #address-cells = <1>; + #size-cells = <0>; + + phy: ethernet-phy@0 { + reg = <0>; + }; + + gmiitorgmii: gmiitorgmii@8 { + compatible = "xlnx,gmii-to-rgmii-1.0"; + reg = <8>; + phy-handle = <&phy>; + }; }; pm-bus-test { diff --git a/test/dm/mdio.c b/test/dm/mdio.c index 64347e1275..19fa5a01e9 100644 --- a/test/dm/mdio.c +++ b/test/dm/mdio.c @@ -25,8 +25,9 @@ static int dm_test_mdio(struct unit_test_state *uts) { struct uclass *uc; - struct udevice *dev; + struct udevice *dev, *eth_dev; struct mdio_ops *ops; + struct phy_device *phy; u16 reg; ut_assertok(uclass_get(UCLASS_MDIO, &uc)); @@ -52,6 +53,10 @@ static int dm_test_mdio(struct unit_test_state *uts) SANDBOX_PHY_REG); ut_asserteq(reg, 0); + ut_assertok(uclass_first_device(UCLASS_ETH, ð_dev)); + phy = dm_mdio_phy_connect(dev, 0, eth_dev, PHY_INTERFACE_MODE_MII); + ut_assertnonnull(phy); + return 0; } So this means the original fdtdec_ version codes also have an issue, but I have no idea why you did not encounter such an issue before. > Anyway I need to get this fix sooner rather than later. One option is > this patch and the second is disable this bridge for now. > Regards, Bin
Hi Bin, On 4/28/21 5:57 PM, Bin Meng wrote: > Hi Michal, > > On Wed, Apr 28, 2021 at 11:03 PM Michal Simek <michal.simek@xilinx.com> wrote: >> >> Hi Bin, >> >> On 4/28/21 4:37 PM, Bin Meng wrote: >>> Hi Michal, >>> >>> On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek@xilinx.com> wrote: >>>> >>>> Hi Bin, >>>> >>>> On 4/27/21 7:17 AM, Bin Meng wrote: >>>>> Hi Michal, >>>>> >>>>> On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: >>>>>> >>>>>> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF >>>>>> tree") change driver behavior to while loop which wasn't correct because >>>>>> the driver was looping over again and again. The reason was that >>>>>> ofnode_valid() is taking 0 as correct value. >>>>> >>>>> I am still trying to understand the problem. The changes in >>>>> 6c993815bbea sound correct from an fdtdec <=> OF API mapping >>>>> perspective. If the new OF API does not work, the old fdtdec may fail >>>>> too. Could you please explain a little bit? >>>> >>>> here is behavior of origin code. >>>> >>>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id >>>> phy_connect_gmii2rgmii sn 11348 >>>> phy_connect_gmii2rgmii 1off -1 >>>> phy_connect_gmii2rgmii 2off -1 >>>> phy_connect_gmii2rgmii sn2 11752 >>>> phy_connect_gmii2rgmii 1off -1 >>>> phy_connect_gmii2rgmii 2off -1 >>>> phy_connect_gmii2rgmii sn2 -1 >>>> phy_connect_gmii2rgmii phydev 0000000000000000 >>>> eth0: ethernet@ff0e0000 >>>> Scanning disk mmc@ff170000.blk... >>>> Found 4 disks >>>> Hit any key to stop autoboot: 0 >>>> ZynqMP> >>>> >>>> >>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >>>> index 89e3076bfd25..d0960d93ae08 100644 >>>> --- a/drivers/net/phy/phy.c >>>> +++ b/drivers/net/phy/phy.c >>>> @@ -956,22 +956,28 @@ static struct phy_device >>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, >>>> int sn = dev_of_offset(dev); >>>> int off; >>>> >>>> + printf("%s sn %d\n", __func__, sn); >>>> while (sn > 0) { >>>> off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, >>>> >>>> "xlnx,gmii-to-rgmii-1.0"); >>>> + printf("%s 1off %d\n", __func__, off); >>>> if (off > 0) { >>>> phydev = phy_device_create(bus, off, >>>> PHY_GMII2RGMII_ID, false, >>>> interface); >>>> break; >>>> } >>>> - if (off == -FDT_ERR_NOTFOUND) >>>> + printf("%s 2off %d\n", __func__, off); >>>> + >>>> + if (off == -FDT_ERR_NOTFOUND) { >>>> sn = fdt_first_subnode(gd->fdt_blob, sn); >>>> - else >>>> + printf("%s sn2 %d\n", __func__, sn); >>>> + } else >>>> printf("%s: Error finding compat string:%d\n", >>>> __func__, off); >>>> } >>>> >>>> + printf("%s phydev %p\n", __func__, phydev); >>>> return phydev; >>>> } >>>> #endif >>>> >>>> >>>> With latest code and some prints >>>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset 2952 >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset -1 >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset 11348 >>>> phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset 11348 >>>> ofnode_valid: node.of_offset 11348 >>>> phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 >>>> ofnode_valid: node.of_offset -1 >>>> ofnode_valid: node.of_offset -1 >>>> ofnode_valid: node.of_offset 0 >>>> ofnode_valid: node.of_offset 0 >>>> phy_connect_gmii2rgmii 3valid 1 >>>> ofnode_valid: node.of_offset 0 >>>> ofnode_valid: node.of_offset 0 >>>> ofnode_valid: node.of_offset 0 >>>> phy_connect_gmii2rgmii 2valid 1 >>>> ofnode_valid: node.of_offset -1 >>>> ofnode_valid: node.of_offset -1 >>>> ofnode_valid: node.of_offset 0 >>>> ofnode_valid: node.of_offset 0 >>>> phy_connect_gmii2rgmii 3valid 1 >>>> ... >>>> >>>> >>>> [u-boot](debian-sent)$ git diff >>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >>>> index dcdef9e661d6..56072ad55216 100644 >>>> --- a/drivers/net/phy/phy.c >>>> +++ b/drivers/net/phy/phy.c >>>> @@ -950,7 +950,10 @@ static struct phy_device >>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, >>>> struct phy_device *phydev = NULL; >>>> ofnode node = dev_ofnode(dev); >>>> >>>> + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), >>>> ofnode_get_name(node)); >>>> + >>>> while (ofnode_valid(node)) { >>>> + printf("%s 2valid %d %s\n", __func__, >>>> ofnode_valid(node), ofnode_get_name(node)); >>>> node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); >>>> if (ofnode_valid(node)) { >>>> phydev = phy_device_create(bus, 0, >>>> @@ -962,6 +965,7 @@ static struct phy_device >>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, >>>> } >>>> >>>> node = ofnode_first_subnode(node); >>>> + printf("%s 3valid %d %s\n", __func__, >>>> ofnode_valid(node), ofnode_get_name(node)); >>>> } >>>> >>>> return phydev; >>>> >>>> >>>> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h >>>> index 2c0597c40739..7bfc06165e92 100644 >>>> --- a/include/dm/ofnode.h >>>> +++ b/include/dm/ofnode.h >>>> @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) >>>> { >>>> if (of_live_active()) >>>> return node.np != NULL; >>>> - else >>>> + else { >>>> + printf("ofnode_valid: node.of_offset %ld\n", >>>> node.of_offset); >>>> return node.of_offset >= 0; >>>> + } >>>> } >>>> >>>> /** >>>> >>>> >>>> It means ofnode_first_subnode is returning any node which has >>>> node.of_offset == 0 which is consider based on ofnode_valid() as valid >>> >>> This sounds suspicious to me that ofnode_first_subnode() returns a >>> node with node.of_offset being zero. I think it should return 11752? >> >> Gem driver is pointing via phy-handle directly to now which should be >> used. That's why there is no subnode but maybe it can be. >> >> Do you have any idea how to debug this to see that node content for >> OF_LIVE and !OF_LIVE? > > I created a test case below, and reproduced the endless loop you > mentioned, but on 2021.04, IOW, without my OF API change patch. > > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts > index 2600360224..8543fbe497 100644 > --- a/arch/sandbox/dts/test.dts > +++ b/arch/sandbox/dts/test.dts > @@ -1360,6 +1360,18 @@ > > mdio: mdio-test { > compatible = "sandbox,mdio"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + phy: ethernet-phy@0 { > + reg = <0>; > + }; > + > + gmiitorgmii: gmiitorgmii@8 { > + compatible = "xlnx,gmii-to-rgmii-1.0"; > + reg = <8>; > + phy-handle = <&phy>; > + }; > }; > > pm-bus-test { > diff --git a/test/dm/mdio.c b/test/dm/mdio.c > index 64347e1275..19fa5a01e9 100644 > --- a/test/dm/mdio.c > +++ b/test/dm/mdio.c > @@ -25,8 +25,9 @@ > static int dm_test_mdio(struct unit_test_state *uts) > { > struct uclass *uc; > - struct udevice *dev; > + struct udevice *dev, *eth_dev; > struct mdio_ops *ops; > + struct phy_device *phy; > u16 reg; > > ut_assertok(uclass_get(UCLASS_MDIO, &uc)); > @@ -52,6 +53,10 @@ static int dm_test_mdio(struct unit_test_state *uts) > SANDBOX_PHY_REG); > ut_asserteq(reg, 0); > > + ut_assertok(uclass_first_device(UCLASS_ETH, ð_dev)); > + phy = dm_mdio_phy_connect(dev, 0, eth_dev, PHY_INTERFACE_MODE_MII); > + ut_assertnonnull(phy); > + > return 0; > } > > So this means the original fdtdec_ version codes also have an issue, > but I have no idea why you did not encounter such an issue before. I think it is related that zynq gem driver has all the time pointing to bridge directly not generic mdio node with several subnodes. And when you point directly to one node without others you will never end up in that loop. And for that changes you have done it looks like they behaves differently in sense of accepting 0 as valid node which is causing that issue. Maybe because of direct pointing to phy we shouldn't really change any other node and remove that loop completely because it is not needed in our scenario. Thanks, Michal
Hi Michal, On Thu, Apr 29, 2021 at 12:17 AM Michal Simek <michal.simek@xilinx.com> wrote: > > Hi Bin, > > On 4/28/21 5:57 PM, Bin Meng wrote: > > Hi Michal, > > > > On Wed, Apr 28, 2021 at 11:03 PM Michal Simek <michal.simek@xilinx.com> wrote: > >> > >> Hi Bin, > >> > >> On 4/28/21 4:37 PM, Bin Meng wrote: > >>> Hi Michal, > >>> > >>> On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek@xilinx.com> wrote: > >>>> > >>>> Hi Bin, > >>>> > >>>> On 4/27/21 7:17 AM, Bin Meng wrote: > >>>>> Hi Michal, > >>>>> > >>>>> On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: > >>>>>> > >>>>>> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF > >>>>>> tree") change driver behavior to while loop which wasn't correct because > >>>>>> the driver was looping over again and again. The reason was that > >>>>>> ofnode_valid() is taking 0 as correct value. > >>>>> > >>>>> I am still trying to understand the problem. The changes in > >>>>> 6c993815bbea sound correct from an fdtdec <=> OF API mapping > >>>>> perspective. If the new OF API does not work, the old fdtdec may fail > >>>>> too. Could you please explain a little bit? > >>>> > >>>> here is behavior of origin code. > >>>> > >>>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id > >>>> phy_connect_gmii2rgmii sn 11348 > >>>> phy_connect_gmii2rgmii 1off -1 > >>>> phy_connect_gmii2rgmii 2off -1 > >>>> phy_connect_gmii2rgmii sn2 11752 > >>>> phy_connect_gmii2rgmii 1off -1 > >>>> phy_connect_gmii2rgmii 2off -1 > >>>> phy_connect_gmii2rgmii sn2 -1 > >>>> phy_connect_gmii2rgmii phydev 0000000000000000 > >>>> eth0: ethernet@ff0e0000 > >>>> Scanning disk mmc@ff170000.blk... > >>>> Found 4 disks > >>>> Hit any key to stop autoboot: 0 > >>>> ZynqMP> > >>>> > >>>> > >>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > >>>> index 89e3076bfd25..d0960d93ae08 100644 > >>>> --- a/drivers/net/phy/phy.c > >>>> +++ b/drivers/net/phy/phy.c > >>>> @@ -956,22 +956,28 @@ static struct phy_device > >>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, > >>>> int sn = dev_of_offset(dev); > >>>> int off; > >>>> > >>>> + printf("%s sn %d\n", __func__, sn); > >>>> while (sn > 0) { > >>>> off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, > >>>> > >>>> "xlnx,gmii-to-rgmii-1.0"); > >>>> + printf("%s 1off %d\n", __func__, off); > >>>> if (off > 0) { > >>>> phydev = phy_device_create(bus, off, > >>>> PHY_GMII2RGMII_ID, false, > >>>> interface); > >>>> break; > >>>> } > >>>> - if (off == -FDT_ERR_NOTFOUND) > >>>> + printf("%s 2off %d\n", __func__, off); > >>>> + > >>>> + if (off == -FDT_ERR_NOTFOUND) { > >>>> sn = fdt_first_subnode(gd->fdt_blob, sn); > >>>> - else > >>>> + printf("%s sn2 %d\n", __func__, sn); > >>>> + } else > >>>> printf("%s: Error finding compat string:%d\n", > >>>> __func__, off); > >>>> } > >>>> > >>>> + printf("%s phydev %p\n", __func__, phydev); > >>>> return phydev; > >>>> } > >>>> #endif > >>>> > >>>> > >>>> With latest code and some prints > >>>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset 2952 > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset -1 > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset 11348 > >>>> phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset 11348 > >>>> ofnode_valid: node.of_offset 11348 > >>>> phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 > >>>> ofnode_valid: node.of_offset -1 > >>>> ofnode_valid: node.of_offset -1 > >>>> ofnode_valid: node.of_offset 0 > >>>> ofnode_valid: node.of_offset 0 > >>>> phy_connect_gmii2rgmii 3valid 1 > >>>> ofnode_valid: node.of_offset 0 > >>>> ofnode_valid: node.of_offset 0 > >>>> ofnode_valid: node.of_offset 0 > >>>> phy_connect_gmii2rgmii 2valid 1 > >>>> ofnode_valid: node.of_offset -1 > >>>> ofnode_valid: node.of_offset -1 > >>>> ofnode_valid: node.of_offset 0 > >>>> ofnode_valid: node.of_offset 0 > >>>> phy_connect_gmii2rgmii 3valid 1 > >>>> ... > >>>> > >>>> > >>>> [u-boot](debian-sent)$ git diff > >>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > >>>> index dcdef9e661d6..56072ad55216 100644 > >>>> --- a/drivers/net/phy/phy.c > >>>> +++ b/drivers/net/phy/phy.c > >>>> @@ -950,7 +950,10 @@ static struct phy_device > >>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, > >>>> struct phy_device *phydev = NULL; > >>>> ofnode node = dev_ofnode(dev); > >>>> > >>>> + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), > >>>> ofnode_get_name(node)); > >>>> + > >>>> while (ofnode_valid(node)) { > >>>> + printf("%s 2valid %d %s\n", __func__, > >>>> ofnode_valid(node), ofnode_get_name(node)); > >>>> node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); > >>>> if (ofnode_valid(node)) { > >>>> phydev = phy_device_create(bus, 0, > >>>> @@ -962,6 +965,7 @@ static struct phy_device > >>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, > >>>> } > >>>> > >>>> node = ofnode_first_subnode(node); > >>>> + printf("%s 3valid %d %s\n", __func__, > >>>> ofnode_valid(node), ofnode_get_name(node)); > >>>> } > >>>> > >>>> return phydev; > >>>> > >>>> > >>>> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h > >>>> index 2c0597c40739..7bfc06165e92 100644 > >>>> --- a/include/dm/ofnode.h > >>>> +++ b/include/dm/ofnode.h > >>>> @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) > >>>> { > >>>> if (of_live_active()) > >>>> return node.np != NULL; > >>>> - else > >>>> + else { > >>>> + printf("ofnode_valid: node.of_offset %ld\n", > >>>> node.of_offset); > >>>> return node.of_offset >= 0; > >>>> + } > >>>> } > >>>> > >>>> /** > >>>> > >>>> > >>>> It means ofnode_first_subnode is returning any node which has > >>>> node.of_offset == 0 which is consider based on ofnode_valid() as valid > >>> > >>> This sounds suspicious to me that ofnode_first_subnode() returns a > >>> node with node.of_offset being zero. I think it should return 11752? > >> > >> Gem driver is pointing via phy-handle directly to now which should be > >> used. That's why there is no subnode but maybe it can be. > >> > >> Do you have any idea how to debug this to see that node content for > >> OF_LIVE and !OF_LIVE? > > > > I created a test case below, and reproduced the endless loop you > > mentioned, but on 2021.04, IOW, without my OF API change patch. > > > > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts > > index 2600360224..8543fbe497 100644 > > --- a/arch/sandbox/dts/test.dts > > +++ b/arch/sandbox/dts/test.dts > > @@ -1360,6 +1360,18 @@ > > > > mdio: mdio-test { > > compatible = "sandbox,mdio"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + phy: ethernet-phy@0 { > > + reg = <0>; > > + }; > > + > > + gmiitorgmii: gmiitorgmii@8 { > > + compatible = "xlnx,gmii-to-rgmii-1.0"; > > + reg = <8>; > > + phy-handle = <&phy>; > > + }; > > }; > > > > pm-bus-test { > > diff --git a/test/dm/mdio.c b/test/dm/mdio.c > > index 64347e1275..19fa5a01e9 100644 > > --- a/test/dm/mdio.c > > +++ b/test/dm/mdio.c > > @@ -25,8 +25,9 @@ > > static int dm_test_mdio(struct unit_test_state *uts) > > { > > struct uclass *uc; > > - struct udevice *dev; > > + struct udevice *dev, *eth_dev; > > struct mdio_ops *ops; > > + struct phy_device *phy; > > u16 reg; > > > > ut_assertok(uclass_get(UCLASS_MDIO, &uc)); > > @@ -52,6 +53,10 @@ static int dm_test_mdio(struct unit_test_state *uts) > > SANDBOX_PHY_REG); > > ut_asserteq(reg, 0); > > > > + ut_assertok(uclass_first_device(UCLASS_ETH, ð_dev)); > > + phy = dm_mdio_phy_connect(dev, 0, eth_dev, PHY_INTERFACE_MODE_MII); > > + ut_assertnonnull(phy); > > + > > return 0; > > } > > > > So this means the original fdtdec_ version codes also have an issue, > > but I have no idea why you did not encounter such an issue before. > > I think it is related that zynq gem driver has all the time pointing to > bridge directly not generic mdio node with several subnodes. > And when you point directly to one node without others you will never > end up in that loop. > > And for that changes you have done it looks like they behaves > differently in sense of accepting 0 as valid node which is causing that > issue. > > Maybe because of direct pointing to phy we shouldn't really change any > other node and remove that loop completely because it is not needed in > our scenario. I might know what is wrong. In original codes, fdt_node_offset_by_compatible() is used, and this API will go through each DT node one by one, so this API will always return OK and break the while() loop. if (off == -FDT_ERR_NOTFOUND) sn = fdt_first_subnode(gd->fdt_blob, sn); else printf("%s: Error finding compat string:%d\n", __func__, off); So above codes to call fdt_first_subnode() is never reachable. These are dead codes. With the new change, ofnode_by_compatible() only checks one node, and does not go through every DT node, so calls to ofnode_first_subnode() is now reachable and is causing issue if the first subnode is not the bridge. I will see if I can create a complete test case for this. For this patch, Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Regards, Bin
Hi Bin, On 4/28/21 6:36 PM, Bin Meng wrote: > Hi Michal, > > On Thu, Apr 29, 2021 at 12:17 AM Michal Simek <michal.simek@xilinx.com> wrote: >> >> Hi Bin, >> >> On 4/28/21 5:57 PM, Bin Meng wrote: >>> Hi Michal, >>> >>> On Wed, Apr 28, 2021 at 11:03 PM Michal Simek <michal.simek@xilinx.com> wrote: >>>> >>>> Hi Bin, >>>> >>>> On 4/28/21 4:37 PM, Bin Meng wrote: >>>>> Hi Michal, >>>>> >>>>> On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek@xilinx.com> wrote: >>>>>> >>>>>> Hi Bin, >>>>>> >>>>>> On 4/27/21 7:17 AM, Bin Meng wrote: >>>>>>> Hi Michal, >>>>>>> >>>>>>> On Mon, Apr 26, 2021 at 8:31 PM Michal Simek <michal.simek@xilinx.com> wrote: >>>>>>>> >>>>>>>> The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF >>>>>>>> tree") change driver behavior to while loop which wasn't correct because >>>>>>>> the driver was looping over again and again. The reason was that >>>>>>>> ofnode_valid() is taking 0 as correct value. >>>>>>> >>>>>>> I am still trying to understand the problem. The changes in >>>>>>> 6c993815bbea sound correct from an fdtdec <=> OF API mapping >>>>>>> perspective. If the new OF API does not work, the old fdtdec may fail >>>>>>> too. Could you please explain a little bit? >>>>>> >>>>>> here is behavior of origin code. >>>>>> >>>>>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id >>>>>> phy_connect_gmii2rgmii sn 11348 >>>>>> phy_connect_gmii2rgmii 1off -1 >>>>>> phy_connect_gmii2rgmii 2off -1 >>>>>> phy_connect_gmii2rgmii sn2 11752 >>>>>> phy_connect_gmii2rgmii 1off -1 >>>>>> phy_connect_gmii2rgmii 2off -1 >>>>>> phy_connect_gmii2rgmii sn2 -1 >>>>>> phy_connect_gmii2rgmii phydev 0000000000000000 >>>>>> eth0: ethernet@ff0e0000 >>>>>> Scanning disk mmc@ff170000.blk... >>>>>> Found 4 disks >>>>>> Hit any key to stop autoboot: 0 >>>>>> ZynqMP> >>>>>> >>>>>> >>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >>>>>> index 89e3076bfd25..d0960d93ae08 100644 >>>>>> --- a/drivers/net/phy/phy.c >>>>>> +++ b/drivers/net/phy/phy.c >>>>>> @@ -956,22 +956,28 @@ static struct phy_device >>>>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, >>>>>> int sn = dev_of_offset(dev); >>>>>> int off; >>>>>> >>>>>> + printf("%s sn %d\n", __func__, sn); >>>>>> while (sn > 0) { >>>>>> off = fdt_node_offset_by_compatible(gd->fdt_blob, sn, >>>>>> >>>>>> "xlnx,gmii-to-rgmii-1.0"); >>>>>> + printf("%s 1off %d\n", __func__, off); >>>>>> if (off > 0) { >>>>>> phydev = phy_device_create(bus, off, >>>>>> PHY_GMII2RGMII_ID, false, >>>>>> interface); >>>>>> break; >>>>>> } >>>>>> - if (off == -FDT_ERR_NOTFOUND) >>>>>> + printf("%s 2off %d\n", __func__, off); >>>>>> + >>>>>> + if (off == -FDT_ERR_NOTFOUND) { >>>>>> sn = fdt_first_subnode(gd->fdt_blob, sn); >>>>>> - else >>>>>> + printf("%s sn2 %d\n", __func__, sn); >>>>>> + } else >>>>>> printf("%s: Error finding compat string:%d\n", >>>>>> __func__, off); >>>>>> } >>>>>> >>>>>> + printf("%s phydev %p\n", __func__, phydev); >>>>>> return phydev; >>>>>> } >>>>>> #endif >>>>>> >>>>>> >>>>>> With latest code and some prints >>>>>> ZYNQ GEM: ff0e0000, mdio bus ff0e0000, phyaddr 12, interface rgmii-id >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset 2952 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset -1 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> phy_connect_gmii2rgmii 1valid 1 ethernet@ff0e0000 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> ofnode_valid: node.of_offset 11348 >>>>>> phy_connect_gmii2rgmii 2valid 1 ethernet@ff0e0000 >>>>>> ofnode_valid: node.of_offset -1 >>>>>> ofnode_valid: node.of_offset -1 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> phy_connect_gmii2rgmii 3valid 1 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> phy_connect_gmii2rgmii 2valid 1 >>>>>> ofnode_valid: node.of_offset -1 >>>>>> ofnode_valid: node.of_offset -1 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> ofnode_valid: node.of_offset 0 >>>>>> phy_connect_gmii2rgmii 3valid 1 >>>>>> ... >>>>>> >>>>>> >>>>>> [u-boot](debian-sent)$ git diff >>>>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c >>>>>> index dcdef9e661d6..56072ad55216 100644 >>>>>> --- a/drivers/net/phy/phy.c >>>>>> +++ b/drivers/net/phy/phy.c >>>>>> @@ -950,7 +950,10 @@ static struct phy_device >>>>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, >>>>>> struct phy_device *phydev = NULL; >>>>>> ofnode node = dev_ofnode(dev); >>>>>> >>>>>> + printf("%s 1valid %d %s\n", __func__, ofnode_valid(node), >>>>>> ofnode_get_name(node)); >>>>>> + >>>>>> while (ofnode_valid(node)) { >>>>>> + printf("%s 2valid %d %s\n", __func__, >>>>>> ofnode_valid(node), ofnode_get_name(node)); >>>>>> node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); >>>>>> if (ofnode_valid(node)) { >>>>>> phydev = phy_device_create(bus, 0, >>>>>> @@ -962,6 +965,7 @@ static struct phy_device >>>>>> *phy_connect_gmii2rgmii(struct mii_dev *bus, >>>>>> } >>>>>> >>>>>> node = ofnode_first_subnode(node); >>>>>> + printf("%s 3valid %d %s\n", __func__, >>>>>> ofnode_valid(node), ofnode_get_name(node)); >>>>>> } >>>>>> >>>>>> return phydev; >>>>>> >>>>>> >>>>>> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h >>>>>> index 2c0597c40739..7bfc06165e92 100644 >>>>>> --- a/include/dm/ofnode.h >>>>>> +++ b/include/dm/ofnode.h >>>>>> @@ -128,8 +128,10 @@ static inline bool ofnode_valid(ofnode node) >>>>>> { >>>>>> if (of_live_active()) >>>>>> return node.np != NULL; >>>>>> - else >>>>>> + else { >>>>>> + printf("ofnode_valid: node.of_offset %ld\n", >>>>>> node.of_offset); >>>>>> return node.of_offset >= 0; >>>>>> + } >>>>>> } >>>>>> >>>>>> /** >>>>>> >>>>>> >>>>>> It means ofnode_first_subnode is returning any node which has >>>>>> node.of_offset == 0 which is consider based on ofnode_valid() as valid >>>>> >>>>> This sounds suspicious to me that ofnode_first_subnode() returns a >>>>> node with node.of_offset being zero. I think it should return 11752? >>>> >>>> Gem driver is pointing via phy-handle directly to now which should be >>>> used. That's why there is no subnode but maybe it can be. >>>> >>>> Do you have any idea how to debug this to see that node content for >>>> OF_LIVE and !OF_LIVE? >>> >>> I created a test case below, and reproduced the endless loop you >>> mentioned, but on 2021.04, IOW, without my OF API change patch. >>> >>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts >>> index 2600360224..8543fbe497 100644 >>> --- a/arch/sandbox/dts/test.dts >>> +++ b/arch/sandbox/dts/test.dts >>> @@ -1360,6 +1360,18 @@ >>> >>> mdio: mdio-test { >>> compatible = "sandbox,mdio"; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + phy: ethernet-phy@0 { >>> + reg = <0>; >>> + }; >>> + >>> + gmiitorgmii: gmiitorgmii@8 { >>> + compatible = "xlnx,gmii-to-rgmii-1.0"; >>> + reg = <8>; >>> + phy-handle = <&phy>; >>> + }; >>> }; >>> >>> pm-bus-test { >>> diff --git a/test/dm/mdio.c b/test/dm/mdio.c >>> index 64347e1275..19fa5a01e9 100644 >>> --- a/test/dm/mdio.c >>> +++ b/test/dm/mdio.c >>> @@ -25,8 +25,9 @@ >>> static int dm_test_mdio(struct unit_test_state *uts) >>> { >>> struct uclass *uc; >>> - struct udevice *dev; >>> + struct udevice *dev, *eth_dev; >>> struct mdio_ops *ops; >>> + struct phy_device *phy; >>> u16 reg; >>> >>> ut_assertok(uclass_get(UCLASS_MDIO, &uc)); >>> @@ -52,6 +53,10 @@ static int dm_test_mdio(struct unit_test_state *uts) >>> SANDBOX_PHY_REG); >>> ut_asserteq(reg, 0); >>> >>> + ut_assertok(uclass_first_device(UCLASS_ETH, ð_dev)); >>> + phy = dm_mdio_phy_connect(dev, 0, eth_dev, PHY_INTERFACE_MODE_MII); >>> + ut_assertnonnull(phy); >>> + >>> return 0; >>> } >>> >>> So this means the original fdtdec_ version codes also have an issue, >>> but I have no idea why you did not encounter such an issue before. >> >> I think it is related that zynq gem driver has all the time pointing to >> bridge directly not generic mdio node with several subnodes. >> And when you point directly to one node without others you will never >> end up in that loop. >> >> And for that changes you have done it looks like they behaves >> differently in sense of accepting 0 as valid node which is causing that >> issue. >> >> Maybe because of direct pointing to phy we shouldn't really change any >> other node and remove that loop completely because it is not needed in >> our scenario. > > I might know what is wrong. > > In original codes, fdt_node_offset_by_compatible() is used, and this > API will go through each DT node one by one, so this API will always > return OK and break the while() loop. > > if (off == -FDT_ERR_NOTFOUND) > sn = fdt_first_subnode(gd->fdt_blob, sn); > else > printf("%s: Error finding compat string:%d\n", > __func__, off); > > So above codes to call fdt_first_subnode() is never reachable. These > are dead codes. > > With the new change, ofnode_by_compatible() only checks one node, and > does not go through every DT node, so calls to ofnode_first_subnode() > is now reachable and is causing issue if the first subnode is not the > bridge. I will see if I can create a complete test case for this. great. > > For this patch, > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Applied. Thanks, Michal
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index dcdef9e661d6..ed197fa46d73 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -948,9 +948,9 @@ static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus, phy_interface_t interface) { struct phy_device *phydev = NULL; - ofnode node = dev_ofnode(dev); + ofnode node; - while (ofnode_valid(node)) { + ofnode_for_each_subnode(node, dev_ofnode(dev)) { node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0"); if (ofnode_valid(node)) { phydev = phy_device_create(bus, 0,
The commit 6c993815bbea ("net: phy: xilinx: Be compatible with live OF tree") change driver behavior to while loop which wasn't correct because the driver was looping over again and again. The reason was that ofnode_valid() is taking 0 as correct value. Fix it by changing while loop to ofnode_for_each_subnode() which is only loop over available nodes. Fixes: 6c993815bbea ("net: phy: xilinx: Be compatible with live OF tree") Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- drivers/net/phy/phy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)