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

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On 2/5/19 1:28 AM, Joe Hershberger wrote:
> On Fri, Jan 25, 2019 at 7:12 AM Vladimir Oltean <vladimir.oltean@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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fnet%2Fphy%2Fphy-core.c%23L296&amp;data=02%7C01%7Cvladimir.oltean%40nxp.com%7C826fd741578446f6f36908d68af87b27%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636849197162094704&amp;sdata=r9AlGZbzGtLC2z7u2HgKKZt17Cl1OcHncjeY00xlVWE%3D&amp;reserved=0
>>>
>>
>> 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.
> 

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?

-Vladimir