Re: [RESEND PATCH net-next v3 3/4] net: phy: aquantia: wait for the GLOBAL_CFG to start returning real values

[Jon Hunter <jonathanh@nvidia.com>]

On 18/07/2024 15:59, Bartosz Golaszewski wrote:

...

>>>>> TBH I only observed the issue on AQR115C. I don't have any other model
>>>>> to test with. Is it fine to fix it by implementing
>>>>> aqr115_fill_interface_modes() that would first wait for this register
>>>>> to return non-0 and then call aqr107_fill_interface_modes()?
>>>>
>>>> I am doing a bit more testing. We have seen a few issues with this PHY
>>>> driver and so I am wondering if we also need something similar for the
>>>> AQR113C variant too.
>>>>
>>>> Interestingly, the product brief for these PHYs [0] do show that both
>>>> the AQR113C and AQR115C both support 10M. So I wonder if it is our
>>>> ethernet controller that is not supporting 10M? I will check on this too.
>>>>
>>>
>>> Oh you have an 113c? I didn't get this. Yeah, weird, all docs say it
>>> should support 10M. In fact all AQR PHYs should hence my initial
>>> change.
>>
>>
>> Yes we have an AQR113C. I agree it should support this, but for whatever
>> reason this is not advertised. I do see that 10M is advertised as
>> supported by the network ...
>>
>>    Link partner advertised link modes:  10baseT/Half 10baseT/Full
>>                                         100baseT/Half 100baseT/Full
>>                                         1000baseT/Full
>>
>> My PC that is on the same network supports 10M, but just not this Tegra
>> device. I am checking to see if this is expected for this device.
>>
> 
> I sent a patch for you to test. I think that even if it doesn't fully
> fix the issue you're observing, it's worth picking it up as it reduces
> the impact of the workaround I introduced.


Thanks! I will test this tonight.

> I'll be off next week so I'm sending it quickly with the hope it will be useful.


OK thanks for letting me know.

Another thought I had, which is also quite timely, is that I have 
recently been testing a patch [0] as I found that this actually resolves 
an issue where we occasionally see our device fail to get an IP address.

This was sent out over a year ago and sadly we failed to follow up :-(

Russell was concerned if this would make the function that was being 
changed fail if it did not have the link (if I am understanding the 
comments correctly). However, looking at the code now, I see that the 
aqr107_read_status() function checks if '!phydev->link' before we poll 
the TX ready status, and so I am wondering if this change is OK? From my 
testing it does work. I would be interested to know if this may also 
resolve your issue?

With this change [0] I have been able to do 500 boots on our board and 
verify that the ethernet controller is able to get an IP address every 
time. Without this change it would fail to get an IP address anywhere 
from 1-100 boots typically.

I will test your patch in the same way, but I am wondering if both are 
trying to address the same sort of issue?

Cheers
Jon

[0] 
https://lore.kernel.org/linux-tegra/20230628124326.55732-3-ruppala@nvidia.com/#t

-- 
nvpublic