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