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

[Joe Hershberger]

On Tue, Feb 5, 2019 at 7:37 PM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> On 2/6/19 12:10 AM, Joe Hershberger wrote:
> > On Tue, Feb 5, 2019 at 9:20 AM Carlo Caione <ccaione at baylibre.com> wrote:
> >>
> >> On 05/02/2019 00:15, Joe Hershberger wrote:
> >>> On Mon, Feb 4, 2019 at 5:39 PM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
> >>
> >> /cut
> >>>> Which brings me to my next point.
> >>>> If we can't properly make the distinction between an indirect C22 MMD
> >>>> access and a proper C45 MMD access, and hence not keeping proper API
> >>>> compatibility with Linux kernel, aren't we better off going back to
> >>>> square 1 and using phy_read_mmd_indirect and phy_write_mmd_indirect?
> >>>
> >>> I think we can and should make the new wrapper functions remain named
> >>> phy_*_mmd_indirect and the names of the override functions in the phy
> >>> driver ops should be *_mmd_indirect. The override is still for an
> >>> indirect access of c45 registers, just an apparently non-standard one.
> >>> It is this way in Linux as well.
>
> I guess it is just me who is still unclear on this?
> Since Russell King's patch "3b85d8d net: phy: remove the indirect MMD
> read/write methods", the Linux API is no longer like that (the
> phy_driver pointers phy_read_mmd and phy_read_mmd_indirect were merged
> into one).
> Just want to make sure that everybody is on the same page and we agreed
> on API compatibility with pre-3b85d8d Linux.

Argh. I was looking at the patch that Carlo referenced and did not
look to see that it further changed.

But looking at 3b85d8d I don't see how the concerns that you and Carlo
discussed about determining what to do if c45 is also supported. Do
you know how Linux handles this or should I do some research?

> >>
> >> Alright then. I'll prepare a V5.
> >>
> >> A couple on notes:
> >>
> >> 1. I'd prefer the parameters of the "mdio" command to be name "rimmd"
> >> and "wimmd" for "r/w indirect MMD" to keep the (twisted) logic of the
> >> mdio command code of differentiating the parameters according to
> >> argv[1][1] and r/w according to argv[1][0]
> >
> > Is there a reason you want to keep the mmd in there? It seems implied
> > by doing any access using the mdio command.
> >
> > Maybe wi and ri or windirect and rindirect or wind and rind?
> >
>
> What about exposing the indirect read as
> "mii read   <addr> [<indirect_devad>.]<reg>"?
>
> It should be clear to most people (and if it isn't, it should be
> clarified) that the legacy "mii" command is clause 22 only, therefore
> the "<mmd>.<reg>" syntactic sugar must logically mean that indirect
> access is what's going on when applied to "mii". The implementation can
> freely call phy_read_mmd_indirect if it parses such syntax.

While it is clear, I would prefer to not encourage further use of the
mii command. I would rather add the ability to explicitly specify the
clause in the mdio command.

Perhaps the default can be to attempt to auto select, but if it is
ambiguous, require the explicit specification. It could follow a
similar approach to the "md" command.  We can add the ability to add
".22" and ".45" to the mdio command to explicitly select.

> Just my 2c. Either way, exposing an explicit command for indirect access
> means that U-boot commits long-term to not trying to implicitly know
> about, and populate, phydev->is_c45.

While using either mdio rindirect or mii / mdio.22 read they are
effectively explicit commands to select "indirect", so I'm not sure
what point you are making here.

-Joe

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