On Thu, Jul 18, 2024 at 7:42 PM Jon Hunter <jonathanh@xxxxxxxxxx> 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. :( Bartosz > Cheers > Jon > > [0] > https://lore.kernel.org/linux-tegra/20230628124326.55732-3-ruppala@xxxxxxxxxx/#t > > -- > nvpublic
