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 20:05, Bartosz Golaszewski wrote:
> On Thu, Jul 18, 2024 at 7:42 PM Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>>
>> 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?
>>
> 
> The patch you linked does not fix the suspend/resume either. :(


Thanks for testing! I have verified that the patch you sent resolves the 
issue introduced by this patch for Tegra. And likewise this patch does 
not resolve the long-standing issue (not related to this change) that we 
have been observing.

Cheers
Jon

-- 
nvpublic