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

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

[Alex Marginean]

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

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

[Vladimir Oltean]

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

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

[Alex Marginean]

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.

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

[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