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

[Jon Hunter]

On 18/07/2024 20:05, Bartosz Golaszewski wrote:
> 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. :(


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