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

[Bin Meng]

Hi Michal,

On Tue, Apr 27, 2021 at 7:22 PM Michal Simek <michal.simek at 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 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

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