[U-Boot] Reading from an arbitrary address over MDIO no longer works

[Vladimir Oltean]

On 29.05.2019 18:00, Alex Marginean wrote:
> On 5/29/2019 5:19 PM, Vladimir Oltean wrote:
>> On 29.05.2019 15:27, Alex Marginean wrote:
>>>
>>> Hi Carlo, everyone,
>>>
>>> I'm doing some MDIO work on a freescale/NXP platform and I bumped into
>>> errors with this command:
>>> => mdio r emdio#3 5 3
>>> Reading from bus emdio#3
>>> "Synchronous Abort" handler, esr 0x8600000e
>>> elr: ffffffff862b8000 lr : 000000008200cce4 (reloc)
>>> ...
>>>
>>> mdio list does not list any PHYs currently because ethernet is using DM
>>> and the interfaces are not probed at this time.  The PHY does exist
>>> on the bus though.
>>> The above scenario works with this commit reverted:
>>> e55047ec51a662c12ed53ff543ec7cdf158b2137 cmd: mdio: Switch to generic
>>> helpers when accessing the registers
>>>
>>> The current code using generic helpers only works for PHYs that have
>>> been registered and show up in bus->phymap and crashes for arbitrary
>>> IDs.  I find it useful to allow reading from other addresses over MDIO
>>> too, certainly helpful for people debugging MDIO on various boards.
>>>
>>> What's your take on this?  Could we revert the above patch, or should I
>>> add some code handling the case when bus->phymap comes up empty and then
>>> go to bus->read?
>>>
>>> Thanks!
>>> Alex
>>>
>>
>> Hi Alex,
>>
>> Thanks for reporting this.
>> I think that breaking access to arbitrary PHY addresses was only an
>> unintentional side-effect here.  Even moreso since having MDIO access to
>> PHYs not defined in DT is desirable, and not only for debugging.  It
>> certainly seems easy to add it back by introducing something like the
>> following (not tested):
>>
>>> diff --git a/cmd/mdio.c b/cmd/mdio.c
>>> index efe8c9ef0954..5e219f699d8d 100644
>>> --- a/cmd/mdio.c
>>> +++ b/cmd/mdio.c
>>> @@ -54,7 +54,10 @@ static int mdio_write_ranges(struct mii_dev *bus,
>>>
>>>               for (devad = devadlo; devad <= devadhi; devad++) {
>>>                       for (reg = reglo; reg <= reghi; reg++) {
>>> -                            if (!extended)
>>> +                            if (!phydev)
>>> +                                    err = bus->write(bus, addr, devad,
>>> +                                                     reg, data);
>>> +                            else if (!extended)
>>>                                       err = phy_write_mmd(phydev, devad,
>>>                                                           reg, data);
>>>                               else
>>> @@ -88,7 +91,9 @@ static int mdio_read_ranges(struct mii_dev *bus,
>>>                       for (reg = reglo; reg <= reghi; reg++) {
>>>                               int val;
>>>
>>> -                            if (!extended)
>>> +                            if (!phydev)
>>> +                                    val = bus->read(bus, addr, devad, reg);
>>> +                            else if (!extended)
>>>                                       val = phy_read_mmd(phydev, devad, reg);
>>>                               else
>>>                                       val = phydev->drv->readext(phydev, addr,
>>
>> Of course in this case we're back to not having the indirect C45
>> convenience commands, but it's still much better than not having access
>> at all.
>>
>> If there are no objections I can volunteer to either test a patch or
>> even submit one later today.
>>
>> Regards,
>> -Vladimir
>>
> 
> Hey Vladimir,
> 
> The downside is that the original patch from Carlo replaced old API
> calls with the more generic API, and now we would switch to using
> both generic API and old API although this change doesn't actually
> add any functionality.  It may be simpler to just switch to the
> old code, it's simpler than the three branches (!phydev, !extended
> and else) and it does the same thing.
> 

Hi Alex,

I don't think that reverting is necessary.
And in fact, the generic API does bring some functionality (albeit only 
for user convenience, but that matters sometimes too).
Say a C22 PHY had a register at 7.1. To write a value it, previously you 
had to:
mdio write <dev> 0xd 0x7
mdio write <dev> 0xe 0x1
mdio write <dev> 0xd 0x4007
mdio write <dev> 0xe <value>
Now you can just:
mdio write <dev> 7.1 <value>

> Back to the change you suggested, I have the changes applied to my tree
> and it works:
> => mdio list
> emdio#3:
> => mdio r emdio#3 5 3
> Reading from bus emdio#3
> PHY at address 5:
> 3 - 0xd072
> 
> I can't test C45 right now on my set-up, I would expect it to work
> though.
> 
> Thanks!
> Alex
> 

Thanks for testing, good to know that it works. I'll send a patch later 
today then.

-Vladimir