[PATCH 4/6] net: fec: add support for DM_MDIO

[Vladimir Oltean]

On Thu, Mar 31, 2022 at 10:48:55AM -0700, Tim Harvey wrote:
> > On which branch does this apply? The context above fecmxc_read_rom_hwaddr()
> > is different in the branches I've checked:
> > https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/fec_mxc.c#L1276
> > https://source.denx.de/u-boot/custodians/u-boot-net/-/blob/next/drivers/net/fec_mxc.c#L1276
> >
> 
> Sorry, I should have specified in my cover letter that this is on top
> of next which removes the non dm-eth support from the driver which is
> quite the cleanup.

Ok, but now the next patch fails to apply to the 'next' branch:

Applying: net: dsa: move cpu port probe to dsa_post_probe
Applying: net: mdio-uclass: add wrappers for read/write/reset operations
Applying: net: fec: add support for DM_MDIO
Applying: net: add MV88E61xx DSA driver
error: patch failed: drivers/net/Kconfig:428
error: drivers/net/Kconfig: patch does not apply
error: patch failed: drivers/net/Makefile:66
error: drivers/net/Makefile: patch does not apply
Patch failed at 0005 net: add MV88E61xx DSA driver

> Also, after more testing I believe the dm-mdio driver code 'above' is
> correct but the business of trying to use it and fallback 'below' is
> wrong and needs work. It doesn't break current users of fec_mxc from
> what I can tell but it also doesn't properly connect the dm_mdio
> driver.

Can you spell out what is wrong about the fallback logic?

The whole thing with eth_phy_get_mdio_bus()/eth_phy_set_mdio_bus() has
me so confused that I am not really following along anymore.

> > >  static int fecmxc_read_rom_hwaddr(struct udevice *dev)
> > >  {
> > >       struct fec_priv *priv = dev_get_priv(dev);
> > > @@ -1088,7 +1164,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev)
> > >
> > >  static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
> > >  {
> > > -     struct phy_device *phydev;
> > > +     struct phy_device *phydev = NULL;
> > >       int addr;
> > >
> > >       addr = device_get_phy_addr(priv, dev);
> > > @@ -1096,7 +1172,12 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev)
> > >       addr = CONFIG_FEC_MXC_PHYADDR;
> > >  #endif
> > >
> > > -     phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> > > +#ifdef CONFIG_DM_MDIO
> > > +     if (priv->dm_mdio)
> > > +             phydev = dm_eth_phy_connect(dev);
> > > +#endif
> > > +     if (!phydev)
> > > +             phydev = phy_connect(priv->bus, addr, dev, priv->interface);
> > >       if (!phydev)
> > >               return -ENODEV;
> > >
> > > @@ -1227,11 +1308,19 @@ static int fecmxc_probe(struct udevice *dev)
> > >
> > >       priv->dev_id = dev_seq(dev);
> > >
> > > +#ifdef CONFIG_DM_MDIO
> > > +     ret = dm_fec_bind_mdio(dev);
> > > +     if (!ret) {
> > > +             ret = fec_phy_init(priv, dev);
> > > +             if (!ret)
> > > +                     priv->dm_mdio = true;
> > > +     }
> > > +#endif
> > >  #ifdef CONFIG_DM_ETH_PHY
> > >       bus = eth_phy_get_mdio_bus(dev);
> > >  #endif
> > >
> > > -     if (!bus) {
> > > +     if (!bus && !priv->dm_mdio) {
> > >               dm_mii_bus = false;
> > >  #ifdef CONFIG_FEC_MXC_MDIO_BASE
> > >               bus = fec_get_miibus((ulong)CONFIG_FEC_MXC_MDIO_BASE,
> > > @@ -1240,7 +1329,7 @@ static int fecmxc_probe(struct udevice *dev)
> > >               bus = fec_get_miibus((ulong)priv->eth, dev_seq(dev));
> > >  #endif
> > >       }
> > > -     if (!bus) {
> > > +     if (!bus && !priv->dm_mdio) {
> > >               ret = -ENOMEM;
> > >               goto err_mii;
> > >       }
> > > @@ -1271,14 +1360,16 @@ static int fecmxc_probe(struct udevice *dev)
> > >               break;
> > >       }
> > >
> > > -     ret = fec_phy_init(priv, dev);
> > > -     if (ret)
> > > -             goto err_phy;
> > > +     if (!priv->dm_mdio) {
> > > +             ret = fec_phy_init(priv, dev);
> > > +             if (ret)
> > > +                     goto err_phy;
> > > +     }
> > >
> > >       return 0;
> > >
> > >  err_phy:
> > > -     if (!dm_mii_bus) {
> > > +     if (!dm_mii_bus && !priv->dm_mdio) {
> > >               mdio_unregister(bus);
> > >               free(bus);
> > >       }
> > > @@ -1294,8 +1385,10 @@ static int fecmxc_remove(struct udevice *dev)
> > >
> > >       free(priv->phydev);
> > >       fec_free_descs(priv);
> > > -     mdio_unregister(priv->bus);
> > > -     mdio_free(priv->bus);
> > > +     if (priv->bus) {
> > > +             mdio_unregister(priv->bus);
> > > +             mdio_free(priv->bus);
> > > +     }
> > >
> > >  #ifdef CONFIG_DM_REGULATOR
> > >       if (priv->phy_supply)
> > > diff --git a/drivers/net/fec_mxc.h b/drivers/net/fec_mxc.h
> > > index 48faa33d66ec..c297880ccc54 100644
> > > --- a/drivers/net/fec_mxc.h
> > > +++ b/drivers/net/fec_mxc.h
> > > @@ -248,6 +248,7 @@ struct fec_priv {
> > >       uint8_t *tdb_ptr;
> > >       int dev_id;
> > >       struct mii_dev *bus;
> > > +     bool dm_mdio;
> > >  #ifdef CONFIG_PHYLIB
> > >       struct phy_device *phydev;
> > >       ofnode phy_of_node;
> > > --
> > > 2.17.1
> > >
> 
> What I'm trying to accomplish here vs just simply using
> dm_fec_bind_mdio(dev) and dm_eth_phy_connect(dev) is to provide a
> fallback for the many users of fec_mxc that do not have a dt that
> works with dm-mdio yet still have defconfig's that enable DM_MDIO.
> Most of the users would not currently have an mdio subnode in the fec
> node, nor have the required phy-mode 'and' phy-handle prop or
> fixed-link subnode which would cause the dm_eth_phy_connect to fail.
> 
> Best Regards,
> 
> Tim