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

[Bin Meng]

Hi Vladimir,

On Tue, Jul 16, 2019 at 6:02 PM Vladimir Oltean <olteanv at gmail.com> wrote:
>
> 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.

I really don't remember now. But I think I should have tested it
before submitting any patches :)

Regards,
Bin