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.si...@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 m...@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
