[U-Boot] [PATCH 2/8] net: tsec: Fix offset of MDIO registers for DM_ETH

[Vladimir Oltean]

Hi Bin,

On Tue, 16 Jul 2019 at 04:19, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Vladimir,
>
> On Tue, Jul 16, 2019 at 6:00 AM Vladimir Oltean <olteanv at gmail.com> wrote:
> >
> > Hi Joe,
> >
> > On Mon, 15 Jul 2019 at 21:00, Joe Hershberger <joe.hershberger at ni.com> wrote:
> > >
> > > On Sun, Jun 23, 2019 at 12:51 PM Vladimir Oltean <olteanv at gmail.com> wrote:
> > > >
> > > > By convention, the eTSEC MDIO controller nodes are defined in DT at
> > > > 0x2d24000 and 0x2d50000, but actually U-boot does not touch the
> > > > interrupt portion of the register map (MDIO_IEVENTM, MDIO_IMASKM,
> > > > MDIO_EMAPM).
> > > >
> > > > That leaves only the MDIO bus registers (MDIO_MIIMCFG, MDIO_MIIMCOM,
> > > > MDIO_MIIMADD, MDIO_MIIMADD, MDIO_MIIMCON, MDIO_MIIMSTAT) which start at
> > > > the 0x520 offset.
> > > >
> > > > So shift the DT-defined register map by the offset of MDIO_MIIMCFG when
> > > > mapping the MDIO bus registers.
> > > >
> > > > Signed-off-by: Vladimir Oltean <olteanv at gmail.com>
> > >
> > > Acked-by: Joe Hershberger <joe.hershberger at ni.com>
> > >
> > > > ---
> > > >  drivers/net/tsec.c | 13 +++++++------
> > > >  include/tsec.h     |  4 +++-
> > > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> > > > index 53eb5470f4c8..576398676af7 100644
> > > > --- a/drivers/net/tsec.c
> > > > +++ b/drivers/net/tsec.c
> > > > @@ -801,6 +801,7 @@ int tsec_probe(struct udevice *dev)
> > > >         u32 tbiaddr = CONFIG_SYS_TBIPA_VALUE;
> > > >         ofnode parent;
> > > >         const char *phy_mode;
> > > > +       fdt_addr_t reg;
> > > >         int ret;
> > > >
> > > >         pdata->iobase = (phys_addr_t)dev_read_addr(dev);
> > > > @@ -817,15 +818,15 @@ int tsec_probe(struct udevice *dev)
> > > >         }
> > > >
> > > >         parent = ofnode_get_parent(phandle_args.node);
> > > > -       if (ofnode_valid(parent)) {
> > > > -               int reg = ofnode_get_addr_index(parent, 0);
> > > > -
> > > > -               priv->phyregs_sgmii = (struct tsec_mii_mng *)reg;
> > > > -       } else {
> > > > -               debug("No parent node for PHY?\n");
> > > > +       if (!ofnode_valid(parent)) {
> > > > +               printf("No parent node for PHY?\n");
> > > >                 return -ENOENT;
> > > >         }
> > > >
> > > > +       reg = ofnode_get_addr_index(parent, 0);
> > > > +       priv->phyregs_sgmii = (struct tsec_mii_mng *)
> > > > +                       (reg + TSEC_MDIO_REGS_OFFSET);
> > >
> > > I'm surprised not to see a .dts change in this patch as well or some
> > > other consumer of this phyregs_sgmii member.
> > >
> >
> > This surprises me as well, to be honest.
> > Actually Bin Meng's patchset to convert the TSEC driver to DM never
> > got completely merged. I suppose the LS1021A-TWR conversion was sort
> > of mechanical and probably not tested on hardware, otherwise I can't
>
> Actually I did test the LS1021A-TWR TSEC DM conversion patch when I
> submitted it before. I vaguely remember the reason why it was not
> merged was because DM ETH depended on DM PCI conversion and at that
> time, DM PCI conversion on LS1021A was not ready.
>

But did you actually test with traffic? There's no way this could have
worked back then - with improper MDIO bus register mapping, it would
have said "Waiting for PHY auto-negotiation to complete" ad infinitum.

> > explain. There is no DT patch because this portion of the code never
> > worked. The DM version of the TSEC driver had no (upstream?) users.
>
> Regards,
> Bin

Thanks,
-Vladimir