[PATCH v4 06/22] net: phy: xilinx: Be compatible with live OF tree

Bin Meng bmeng.cn at gmail.com
Fri Apr 23 11:23:18 CEST 2021


On Fri, Apr 23, 2021 at 5:14 PM Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Michal,
>
> On Fri, Apr 23, 2021 at 3:35 PM Michal Simek <monstr at monstr.eu> wrote:
> >
> > Hi Bin,
> >
> > ne 14. 3. 2021 v 13:17 odesílatel Bin Meng <bmeng.cn at gmail.com> napsal:
> > >
> > > Following the same updates that were done to the fixed phy driver,
> > > use ofnode_ APIs instead of fdt_ APIs so that the Xilinx PHY driver
> > > can support live DT.
> > >
> > > Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
> > >
> > > ---
> > >
> > > (no changes since v2)
> > >
> > > Changes in v2:
> > > - move device tree parsing from xilinxgmiitorgmii_probe() to
> > >   xilinxgmiitorgmii_config() and use OF APIs
> > >
> > >  drivers/net/phy/phy.c               | 23 +++++------
> > >  drivers/net/phy/xilinx_gmii2rgmii.c | 61 ++++++++++++++---------------
> > >  2 files changed, 40 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > > index eae40cc0d6..d464379121 100644
> > > --- a/drivers/net/phy/phy.c
> > > +++ b/drivers/net/phy/phy.c
> > > @@ -953,23 +953,20 @@ static struct phy_device *phy_connect_gmii2rgmii(struct mii_dev *bus,
> > >  #endif
> > >  {
> > >         struct phy_device *phydev = NULL;
> > > -       int sn = dev_of_offset(dev);
> > > -       int off;
> > > -
> > > -       while (sn > 0) {
> > > -               off = fdt_node_offset_by_compatible(gd->fdt_blob, sn,
> > > -                                                   "xlnx,gmii-to-rgmii-1.0");
> > > -               if (off > 0) {
> > > -                       phydev = phy_device_create(bus, off,
> > > +       ofnode node = dev_ofnode(dev);
> > > +
> > > +       while (ofnode_valid(node)) {
> > > +               node = ofnode_by_compatible(node, "xlnx,gmii-to-rgmii-1.0");
> > > +               if (ofnode_valid(node)) {
> > > +                       phydev = phy_device_create(bus, 0,
> > >                                                    PHY_GMII2RGMII_ID, false,
> > >                                                    interface);
> > > +                       if (phydev)
> > > +                               phydev->node = node;
> > >                         break;
> > >                 }
> > > -               if (off == -FDT_ERR_NOTFOUND)
> > > -                       sn = fdt_first_subnode(gd->fdt_blob, sn);
> > > -               else
> > > -                       printf("%s: Error finding compat string:%d\n",
> > > -                              __func__, off);
> > > +
> > > +               node = ofnode_first_subnode(node);
> > >         }
> > >
> > >         return phydev;
> > > diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > index 74105c0b7d..635c0570ef 100644
> > > --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> > > +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> > > @@ -18,9 +18,38 @@ DECLARE_GLOBAL_DATA_PTR;
> > >
> > >  static int xilinxgmiitorgmii_config(struct phy_device *phydev)
> > >  {
> > > -       struct phy_device *ext_phydev = phydev->priv;
> > > +       ofnode node = phy_get_ofnode(phydev);
> > > +       struct phy_device *ext_phydev;
> > > +       struct ofnode_phandle_args phandle;
> > > +       int ext_phyaddr = -1;
> > > +       int ret;
> > >
> > >         debug("%s\n", __func__);
> > > +
> > > +       if (!ofnode_valid(node))
> > > +               return -EINVAL;
> > > +
> > > +       phydev->addr = ofnode_read_u32_default(node, "reg", -1);
> > > +       ret = ofnode_parse_phandle_with_args(node, "phy-handle",
> > > +                                            NULL, 0, 0, &phandle);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       ext_phyaddr = ofnode_read_u32_default(phandle.node, "reg", -1);
> > > +       ext_phydev = phy_find_by_mask(phydev->bus,
> > > +                                     1 << ext_phyaddr,
> > > +                                     PHY_INTERFACE_MODE_RGMII);
> > > +       if (!ext_phydev) {
> > > +               printf("%s, No external phy device found\n", __func__);
> > > +               return -EINVAL;
> > > +       }
> > > +
> > > +       ext_phydev->node = phandle.node;
> > > +       phydev->priv = ext_phydev;
> > > +
> > > +       debug("%s, gmii2rgmmi:0x%x, extphy:0x%x\n", __func__, phydev->addr,
> > > +             ext_phyaddr);
> > > +
> > >         if (ext_phydev->drv->config)
> > >                 ext_phydev->drv->config(ext_phydev);
> > >
> > > @@ -83,11 +112,6 @@ static int xilinxgmiitorgmii_startup(struct phy_device *phydev)
> > >
> > >  static int xilinxgmiitorgmii_probe(struct phy_device *phydev)
> > >  {
> > > -       int ofnode = phydev->addr;
> > > -       u32 phy_of_handle;
> > > -       int ext_phyaddr = -1;
> > > -       struct phy_device *ext_phydev;
> > > -
> > >         debug("%s\n", __func__);
> > >
> > >         if (phydev->interface != PHY_INTERFACE_MODE_GMII) {
> > > @@ -95,31 +119,6 @@ static int xilinxgmiitorgmii_probe(struct phy_device *phydev)
> > >                 return -EINVAL;
> > >         }
> > >
> > > -       /*
> > > -        * Read the phy address again as the one we read in ethernet driver
> > > -        * was overwritten for the purpose of storing the ofnode
> > > -        */
> > > -       phydev->addr = fdtdec_get_int(gd->fdt_blob, ofnode, "reg", -1);
> > > -       phy_of_handle = fdtdec_lookup_phandle(gd->fdt_blob, ofnode,
> > > -                                             "phy-handle");
> > > -       if (phy_of_handle > 0)
> > > -               ext_phyaddr = fdtdec_get_int(gd->fdt_blob,
> > > -                                            phy_of_handle,
> > > -                                            "reg", -1);
> > > -       ext_phydev = phy_find_by_mask(phydev->bus,
> > > -                                     1 << ext_phyaddr,
> > > -                                     PHY_INTERFACE_MODE_RGMII);
> > > -       if (!ext_phydev) {
> > > -               printf("%s, No external phy device found\n", __func__);
> > > -               return -EINVAL;
> > > -       }
> > > -
> > > -       ext_phydev->node = offset_to_ofnode(phy_of_handle);
> > > -       phydev->priv = ext_phydev;
> > > -
> > > -       debug("%s, gmii2rgmmi:0x%x, extphy:0x%x\n", __func__, phydev->addr,
> > > -             ext_phyaddr);
> > > -
> > >         phydev->flags |= PHY_FLAG_BROKEN_RESET;
> > >
> > >         return 0;
> > > --
> > > 2.25.1
> > >
> >
> > This patch is breaking u-boot on ZynqMP. In phy_connect_gmii2rgmii you
> > get a valid node from dev_ofnode(dev)
> > and you enter the while loop because ofnode_valid(node) returns true.
> > ofnode_by_compatible() will fail because it is not normally
> > gmii-to-rgmii bridge and will go to ofnode_first_subnode() which
> > returns 0

A second thought, I am curious why ofnode_first_subnode() returns a
valid ofnode if this fails? Is there a DT example?

As ofnode_for_each_subnode() also uses ofnode_valid() to check.

> > which goes back to ofnode_valid() which pass again because 0 is valid
> > and it keeps going in this while loop.
> > This is with xilinx_zynqmp_virt_defconfig configuration with OF_LIVE
> > off but with OF_LIVE on behavior is the same.
> >
> > It means that the while loop should be replaced by something more deterministic.
> > What about?
> > ofnode_for_each_subnode(node, dev_ofnode(dev)) {
>
> This sounds good to me.
>
> I cannot find an in-tree DTS that contains the compatible string
> "xlnx,gmii-to-rgmii-1.0". Is this an out-of-tree DT?

Regards,
Bin


More information about the U-Boot mailing list