[PATCH] net: phy: xilinx: Break while loop over ethernet phy

[Michal Simek]

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 at 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 at ff0e0000
Scanning disk mmc at 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 at 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 at 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