[U-Boot] [PATCH v4 3/3] cmd: mdio: Add new parameter to access MMD PHY registers

[Joe Hershberger]

On Fri, Jan 25, 2019 at 7:12 AM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> On 25.01.2019 12:12, Carlo Caione wrote:
> > On 24/01/2019 20:48, Vladimir Oltean wrote:
> >> On 1/24/19 10:19 PM, Carlo Caione wrote:
> >>> On 24/01/2019 20:12, Vladimir Oltean wrote:
> >>>>
> >>
> >> I can't completely answer that, TBH I don't even know who is supposed to
> >> make that distinction.
> >
> > In the kernel that distinction is made by the driver itself, hence my
> > question. See [0].
> >
> >> For Freescale parts that is a call for the MDIO bus driver to make, for
> >> good or bad (see drivers/net/fm/memac_phy.c where dev_addr is compared
> >> to MDIO_DEVAD_NONE).
> >
> >> And in your patch, phy_write_mmd is only a wrapper over bus->write in
> >> the end, with some more logic to handle C22 indirection.
> >> So my question of unifying "mdio rmmd" with "mdio read" translates into:
> >
> >> Does it make sense to also handle the check with MDIO_DEVAD_NONE in
> >> phy_write_mmd, instead of jumping straight ahead to perform indirection?
> >
> > Honestly I'm not quite sure of all the possible implications here IMO
> > the safest bet here is just to follow what's done by the kernel. Maybe
> > Joe can step in about this.
> >
> > In general we have 3 possible cases:
> >
> > 1) your driver is doing something non-standard when accessing the MMDs
> > and we deal with that using the PHY driver hooks
> > 2) your PHY is C22 and you have to use the indirect method
> > 3) your PHY is C45 and you can use the direct register reading (mangling
> > a bit the address apparently)
> >
> > The kernel is dealing with all the cases, U-Boot is only dealing with
> > C22 PHYs (cases 1 and 2) because AFAICT there isn't yet a generic way to
> > detect if the PHY is C22 or C45.
> >
> > I'm not sure if the indirect method works also for C45 PHYs.
> >
> >> The goal would then be to just call phy_write_mmd from cmd/mdio.c
> >> regardless of the target PHY's clause.
> >
> > Again I wrote that patch only assuming that we were going to deal with
> > C22 PHYs. At this point I wonder if the C22 indirect method works also
> > for C45 PHYs. If that's the case than the phy_write_mmd should already
> > work regardless of the target PHY clause.
> >
> > Cheers.
> >
> > [0]
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L296
> >
>
> I'm not suggesting to use C22 indirection if the PHY already supports
> native C45 addressing. Even if that worked, it would be a pointless
> exercise in all but a few cases (like the MDIO controller does not
> support C22, but the PHY does support both C22 and C45).
> I was just wondering out loud whether the introduction of the "mdio
> rmmd" command is justified or not. I now understand that using e.g.
> "mdio read 1.3" will confuse the command for clause 45 PHY's because it
> won't know whether it should access the PHY via native C45 or via
> indirect C22 (obviously it shouldn't do the latter). So in lack of a
> clear distinction mechanism, I now think that a new command truly is
> necessary for performing indirect C45 access on C22.
> What I am still not convinced of, however, is whether those commands
> should be called "rmmd" and "wmmd". It is not immediately obvious from
> the command description that this is what they are for, and a user may
> attempt to use them for C45 PHY's as well, which will probably not yield
> the intended result.

I agree. The MMD in the register name is simply "MDIO Manageable
Devices"... i.e. the phys.

I think the commands should be "iread" and "iwrite" to denote the
indirect access in use.

Thanks,
-Joe

> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot