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

Jon Hunter
2024-07-18T17:42:11Z

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