On Thu, Jul 18, 2024 at 2:23 PM Jon Hunter <jonathanh@xxxxxxxxxx> wrote: > > Hi Bartosz, > > On 08/07/2024 08:50, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > > > When the PHY is first coming up (or resuming from suspend), it's > > possible that although the FW status shows as running, we still see > > zeroes in the GLOBAL_CFG set of registers and cannot determine available > > modes. Since all models support 10M, add a poll and wait the config to > > become available. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > --- > > drivers/net/phy/aquantia/aquantia_main.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c > > index 974795bd0860..2c8ba2725a91 100644 > > --- a/drivers/net/phy/aquantia/aquantia_main.c > > +++ b/drivers/net/phy/aquantia/aquantia_main.c > > @@ -652,7 +652,13 @@ static int aqr107_fill_interface_modes(struct phy_device *phydev) > > unsigned long *possible = phydev->possible_interfaces; > > unsigned int serdes_mode, rate_adapt; > > phy_interface_t interface; > > - int i, val; > > + int i, val, ret; > > + > > + ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, > > + VEND1_GLOBAL_CFG_10M, val, val != 0, > > + 1000, 100000, false); > > + if (ret) > > + return ret; > > > > /* Walk the media-speed configuration registers to determine which > > * host-side serdes modes may be used by the PHY depending on the > > > With the current -next and mainline we are seeing the following issue on > our Tegra234 Jetson AGX Orin platform ... > > Aquantia AQR113C stmmac-0:00: aqr107_fill_interface_modes failed: -110 > tegra-mgbe 6800000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -110) > > > We have tracked it down to this change and looks like our PHY does not > support 10M ... > > $ ethtool eth0 > Settings for eth0: > Supported ports: [ ] > Supported link modes: 100baseT/Full > 1000baseT/Full > 10000baseT/Full > 1000baseKX/Full > 10000baseKX4/Full > 10000baseKR/Full > 2500baseT/Full > 5000baseT/Full > > The following fixes this for this platform ... > > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c > index d12e35374231..0b2db486d8bd 100644 > --- a/drivers/net/phy/aquantia/aquantia_main.c > +++ b/drivers/net/phy/aquantia/aquantia_main.c > @@ -656,7 +656,7 @@ static int aqr107_fill_interface_modes(struct phy_device *phydev) > int i, val, ret; > > ret = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, > - VEND1_GLOBAL_CFG_10M, val, val != 0, > + VEND1_GLOBAL_CFG_100M, val, val != 0, > 1000, 100000, false); > if (ret) > return ret; > > > However, I am not sure if this is guaranteed to work for all? Ah cr*p. No, I don't think it is. We should take the first supported mode for a given PHY I think. > > On a related note, we had also found an issue with this PHY where > the PHY reset is not working quite correctly. What we see is that > when polling for the firmware ID in aqr107_wait_reset_complete() > is that value in the firware ID registers transitions from 0 to > 0xffff and then to the firmware ID. We have been testing the > following change to fix this ... > > diff --git a/drivers/net/phy/aquantia/aquantia.h b/drivers/net/phy/aquantia/aquantia.h > index 2465345081f8..278e3b167c58 100644 > --- a/drivers/net/phy/aquantia/aquantia.h > +++ b/drivers/net/phy/aquantia/aquantia.h > @@ -20,6 +20,7 @@ > #define VEND1_GLOBAL_FW_ID 0x0020 > #define VEND1_GLOBAL_FW_ID_MAJOR GENMASK(15, 8) > #define VEND1_GLOBAL_FW_ID_MINOR GENMASK(7, 0) > +#define VEND1_GLOBAL_FW_ID_MASK GENMASK(15, 0) > > #define VEND1_GLOBAL_MAILBOX_INTERFACE1 0x0200 > #define VEND1_GLOBAL_MAILBOX_INTERFACE1_EXECUTE BIT(15) > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c > index 0b2db486d8bd..5023fd70050d 100644 > --- a/drivers/net/phy/aquantia/aquantia_main.c > +++ b/drivers/net/phy/aquantia/aquantia_main.c > @@ -447,7 +447,9 @@ int aqr_wait_reset_complete(struct phy_device *phydev) > int val; > > return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, > - VEND1_GLOBAL_FW_ID, val, val != 0, > + VEND1_GLOBAL_FW_ID, val, > + ((val & VEND1_GLOBAL_FW_ID_MASK) != 0 && > + (val & VEND1_GLOBAL_FW_ID_MASK) != VEND1_GLOBAL_FW_ID_MASK), > 20000, 2000000, false); > } > > I have not tried the resume use-case, but curious if this may > also help here? > Interesting. Unfortunately this doesn't help with the suspend/resume use-case if I revert my offending patch. Bart > Cheers > Jon > > -- > nvpublic
