| Message ID | 20230830092119.458330-2-lukma@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/2] net: phy: Rename KSZ9477 get features function (to ksz9131_get_features) | expand |
On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > The KSZ9477 errata points out (in 'Module 4') the link up/down problem > when EEE (Energy Efficient Ethernet) is enabled in the device to which > the KSZ9477 tries to auto negotiate. > > The suggested workaround is to clear advertisement of EEE for PHYs in > this chip driver. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> Hi, As the net is target you should add fixes tag which the commit that your changes is fixing (workarounding :) ) > --- > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 87b090ad2874..469dcd8a5711 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct phy_device *phydev) > return 0; > } > > +static int ksz9477_get_features(struct phy_device *phydev) > +{ > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; = { 0 }; > + int ret; > + > + ret = genphy_read_abilities(phydev); > + if (ret) > + return ret; > + > + /* KSZ9477 Errata DS80000754C > + * > + * Module 4: Energy Efficient Ethernet (EEE) feature select must be > + * manually disabled > + * The EEE feature is enabled by default, but it is not fully > + * operational. It must be manually disabled through register > + * controls. If not disabled, the PHY ports can auto-negotiate > + * to enable EEE, and this feature can cause link drops when linked > + * to another device supporting EEE. > + * > + * Although, the KSZ9477 MMD register > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that EEE is > + * operational one needs to manualy clear them to follow the chip > + * errata. > + */ > + linkmode_and(phydev->supported_eee, phydev->supported, zero); > + > + return 0; > +} > + > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { > .handle_interrupt = kszphy_handle_interrupt, > .suspend = genphy_suspend, > .resume = genphy_resume, > - .get_features = ksz9131_get_features, > + .get_features = ksz9477_get_features, > } }; > > module_phy_driver(ksphy_driver); > -- > 2.20.1 >
On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > The KSZ9477 errata points out (in 'Module 4') the link up/down problem > when EEE (Energy Efficient Ethernet) is enabled in the device to which > the KSZ9477 tries to auto negotiate. > > The suggested workaround is to clear advertisement of EEE for PHYs in > this chip driver. > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > --- > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > 1 file changed, 30 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > index 87b090ad2874..469dcd8a5711 100644 > --- a/drivers/net/phy/micrel.c > +++ b/drivers/net/phy/micrel.c > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct phy_device *phydev) > return 0; > } > > +static int ksz9477_get_features(struct phy_device *phydev) > +{ > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; > + int ret; > + > + ret = genphy_read_abilities(phydev); > + if (ret) > + return ret; > + > + /* KSZ9477 Errata DS80000754C > + * > + * Module 4: Energy Efficient Ethernet (EEE) feature select must be > + * manually disabled > + * The EEE feature is enabled by default, but it is not fully > + * operational. It must be manually disabled through register > + * controls. If not disabled, the PHY ports can auto-negotiate > + * to enable EEE, and this feature can cause link drops when linked > + * to another device supporting EEE. > + * > + * Although, the KSZ9477 MMD register > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that EEE is > + * operational one needs to manualy clear them to follow the chip > + * errata. > + */ > + linkmode_and(phydev->supported_eee, phydev->supported, zero); > + > + return 0; > +} > + > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { > .handle_interrupt = kszphy_handle_interrupt, > .suspend = genphy_suspend, > .resume = genphy_resume, > - .get_features = ksz9131_get_features, > + .get_features = ksz9477_get_features, Sorry, i didn't described all details how to implement it. This code will break EEE support for the KSZ8563R switch. Please search for MICREL_KSZ8_P1_ERRATA in the kernel source. Then add new flag, for example MICREL_NO_EEE and use it in a similar way how MICREL_KSZ8_P1_ERRATA was set and used. With this implementation, first patch is not needed. The code will be something like this: if (dev_flags & MICREL_NO_EEE) /* lots of comments */ linkmode_and(phydev->supported_eee, phydev->supported, zero); else /* lots of other comments */ linkmode_and(phydev->supported_eee, phydev->supported, PHY_EEE_CAP1_FEATURES); On the switch driver side i would expect something like this: ksz_get_phy_flags(struct dsa_switch *ds, int port) swtich (dev->chip_id) case KSZ8830_CHIP_ID: .... break; case KSZ9477_CHIP_ID: return MICREL_NO_EEE; Regards, Oleksij
Hi Michal, > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > > The KSZ9477 errata points out (in 'Module 4') the link up/down > > problem when EEE (Energy Efficient Ethernet) is enabled in the > > device to which the KSZ9477 tries to auto negotiate. > > > > The suggested workaround is to clear advertisement of EEE for PHYs > > in this chip driver. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > Hi, > > As the net is target you should add fixes tag which the commit that > your changes is fixing (workarounding :) ) > I'm applying code for vendor's errata. I will search when it has been modified. > > --- > > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 87b090ad2874..469dcd8a5711 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct > > phy_device *phydev) return 0; > > } > > > > +static int ksz9477_get_features(struct phy_device *phydev) > > +{ > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; > = { 0 }; Ok. > > > + int ret; > > + > > + ret = genphy_read_abilities(phydev); > > + if (ret) > > + return ret; > > + > > + /* KSZ9477 Errata DS80000754C > > + * > > + * Module 4: Energy Efficient Ethernet (EEE) feature > > select must be > > + * manually disabled > > + * The EEE feature is enabled by default, but it is not > > fully > > + * operational. It must be manually disabled through > > register > > + * controls. If not disabled, the PHY ports can > > auto-negotiate > > + * to enable EEE, and this feature can cause link drops > > when linked > > + * to another device supporting EEE. > > + * > > + * Although, the KSZ9477 MMD register > > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that > > EEE is > > + * operational one needs to manualy clear them to follow > > the chip > > + * errata. > > + */ > > + linkmode_and(phydev->supported_eee, phydev->supported, > > zero); + > > + return 0; > > +} > > + > > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { > > .handle_interrupt = kszphy_handle_interrupt, > > .suspend = genphy_suspend, > > .resume = genphy_resume, > > - .get_features = ksz9131_get_features, > > + .get_features = ksz9477_get_features, > > } }; > > > > module_phy_driver(ksphy_driver); > > -- > > 2.20.1 > > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Oleksij, > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > > The KSZ9477 errata points out (in 'Module 4') the link up/down > > problem when EEE (Energy Efficient Ethernet) is enabled in the > > device to which the KSZ9477 tries to auto negotiate. > > > > The suggested workaround is to clear advertisement of EEE for PHYs > > in this chip driver. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 87b090ad2874..469dcd8a5711 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct > > phy_device *phydev) return 0; > > } > > > > +static int ksz9477_get_features(struct phy_device *phydev) > > +{ > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; > > + int ret; > > + > > + ret = genphy_read_abilities(phydev); > > + if (ret) > > + return ret; > > + > > + /* KSZ9477 Errata DS80000754C > > + * > > + * Module 4: Energy Efficient Ethernet (EEE) feature > > select must be > > + * manually disabled > > + * The EEE feature is enabled by default, but it is not > > fully > > + * operational. It must be manually disabled through > > register > > + * controls. If not disabled, the PHY ports can > > auto-negotiate > > + * to enable EEE, and this feature can cause link drops > > when linked > > + * to another device supporting EEE. > > + * > > + * Although, the KSZ9477 MMD register > > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that > > EEE is > > + * operational one needs to manualy clear them to follow > > the chip > > + * errata. > > + */ > > + linkmode_and(phydev->supported_eee, phydev->supported, > > zero); + > > + return 0; > > +} > > + > > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { > > .handle_interrupt = kszphy_handle_interrupt, > > .suspend = genphy_suspend, > > .resume = genphy_resume, > > - .get_features = ksz9131_get_features, > > + .get_features = ksz9477_get_features, > > Sorry, i didn't described all details how to implement it. > > This code will break EEE support for the KSZ8563R switch. > And then another switch (KSZ8563R) pops up.... with regression.... In the micrel.c the ksz9477_get_features was only present in: https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4832 https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4874 so I only changed it there. Apparently the KSZ8563R re-uses the KSZ9477 code. > Please search for MICREL_KSZ8_P1_ERRATA in the kernel source. > Then add new flag, for example MICREL_NO_EEE and use it in a similar > way how MICREL_KSZ8_P1_ERRATA was set and used. With this > implementation, first patch is not needed. > > The code will be something like this: > if (dev_flags & MICREL_NO_EEE) > /* lots of comments */ > linkmode_and(phydev->supported_eee, phydev->supported, zero); > else > /* lots of other comments */ > linkmode_and(phydev->supported_eee, phydev->supported, > PHY_EEE_CAP1_FEATURES); > > On the switch driver side i would expect something like this: > ksz_get_phy_flags(struct dsa_switch *ds, int port) > > swtich (dev->chip_id) > case KSZ8830_CHIP_ID: > .... > break; > case KSZ9477_CHIP_ID: > return MICREL_NO_EEE; > Ok. > > Regards, > Oleksij Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Wed, Aug 30, 2023 at 12:52:24PM +0200, Lukasz Majewski wrote: > Hi Oleksij, > > > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > > > The KSZ9477 errata points out (in 'Module 4') the link up/down > > > problem when EEE (Energy Efficient Ethernet) is enabled in the > > > device to which the KSZ9477 tries to auto negotiate. > > > > > > The suggested workaround is to clear advertisement of EEE for PHYs > > > in this chip driver. > > > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > --- > > > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > > index 87b090ad2874..469dcd8a5711 100644 > > > --- a/drivers/net/phy/micrel.c > > > +++ b/drivers/net/phy/micrel.c > > > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct > > > phy_device *phydev) return 0; > > > } > > > > > > +static int ksz9477_get_features(struct phy_device *phydev) > > > +{ > > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; > > > + int ret; > > > + > > > + ret = genphy_read_abilities(phydev); > > > + if (ret) > > > + return ret; > > > + > > > + /* KSZ9477 Errata DS80000754C > > > + * > > > + * Module 4: Energy Efficient Ethernet (EEE) feature > > > select must be > > > + * manually disabled > > > + * The EEE feature is enabled by default, but it is not > > > fully > > > + * operational. It must be manually disabled through > > > register > > > + * controls. If not disabled, the PHY ports can > > > auto-negotiate > > > + * to enable EEE, and this feature can cause link drops > > > when linked > > > + * to another device supporting EEE. > > > + * > > > + * Although, the KSZ9477 MMD register > > > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that > > > EEE is > > > + * operational one needs to manualy clear them to follow > > > the chip > > > + * errata. > > > + */ > > > + linkmode_and(phydev->supported_eee, phydev->supported, > > > zero); + > > > + return 0; > > > +} > > > + > > > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > > > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > > > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > > > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { > > > .handle_interrupt = kszphy_handle_interrupt, > > > .suspend = genphy_suspend, > > > .resume = genphy_resume, > > > - .get_features = ksz9131_get_features, > > > + .get_features = ksz9477_get_features, > > > > Sorry, i didn't described all details how to implement it. > > > > This code will break EEE support for the KSZ8563R switch. > > > > And then another switch (KSZ8563R) pops up.... with regression.... Initially ksz9477_get_features() was introduced to fix KSZ8563R. > In the micrel.c the ksz9477_get_features was only present in: > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4832 > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4874 > so I only changed it there. > > Apparently the KSZ8563R re-uses the KSZ9477 code. Most (all?) switch variant support by the ksz9477 DSA driver share the same PHYid, so it is not possible to identify it by the micrel.c PHY driver. As far as a know, the only commonly accepted way to notify about some quirks between this both drivers is not user dev_flags. Regards, Oleskij
On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > + /* KSZ9477 Errata DS80000754C > + * > + * Module 4: Energy Efficient Ethernet (EEE) feature select must be > + * manually disabled > + * The EEE feature is enabled by default, but it is not fully > + * operational. It must be manually disabled through register > + * controls. If not disabled, the PHY ports can auto-negotiate > + * to enable EEE, and this feature can cause link drops when linked > + * to another device supporting EEE. > + * > + * Although, the KSZ9477 MMD register > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that EEE is > + * operational one needs to manualy clear them to follow the chip > + * errata. > + */ > + linkmode_and(phydev->supported_eee, phydev->supported, zero); Hi, I'm wondering whether you had a reason to write the above, rather than use the simpler: linkmode_zero(phydev->supported_eee); Thanks.
On Wed, Aug 30, 2023 at 12:18:13PM +0200, Oleksij Rempel wrote: > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > > The KSZ9477 errata points out (in 'Module 4') the link up/down problem > > when EEE (Energy Efficient Ethernet) is enabled in the device to which > > the KSZ9477 tries to auto negotiate. > > > > The suggested workaround is to clear advertisement of EEE for PHYs in > > this chip driver. > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > --- > > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > index 87b090ad2874..469dcd8a5711 100644 > > --- a/drivers/net/phy/micrel.c > > +++ b/drivers/net/phy/micrel.c > > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct phy_device *phydev) > > return 0; > > } > > > > +static int ksz9477_get_features(struct phy_device *phydev) > > +{ > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; > > + int ret; > > + > > + ret = genphy_read_abilities(phydev); > > + if (ret) > > + return ret; > > + > > + /* KSZ9477 Errata DS80000754C > > + * > > + * Module 4: Energy Efficient Ethernet (EEE) feature select must be > > + * manually disabled > > + * The EEE feature is enabled by default, but it is not fully > > + * operational. It must be manually disabled through register > > + * controls. If not disabled, the PHY ports can auto-negotiate > > + * to enable EEE, and this feature can cause link drops when linked > > + * to another device supporting EEE. > > + * > > + * Although, the KSZ9477 MMD register > > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that EEE is > > + * operational one needs to manualy clear them to follow the chip > > + * errata. > > + */ > > + linkmode_and(phydev->supported_eee, phydev->supported, zero); > > + > > + return 0; > > +} > > + > > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { > > .handle_interrupt = kszphy_handle_interrupt, > > .suspend = genphy_suspend, > > .resume = genphy_resume, > > - .get_features = ksz9131_get_features, > > + .get_features = ksz9477_get_features, > > Sorry, i didn't described all details how to implement it. > > This code will break EEE support for the KSZ8563R switch. > > Please search for MICREL_KSZ8_P1_ERRATA in the kernel source. > Then add new flag, for example MICREL_NO_EEE and use it in a similar > way how MICREL_KSZ8_P1_ERRATA was set and used. With this > implementation, first patch is not needed. > > The code will be something like this: > if (dev_flags & MICREL_NO_EEE) > /* lots of comments */ > linkmode_and(phydev->supported_eee, phydev->supported, zero); I can't believe two people are writing code like this... linkmode_zero(phydev->supported_eee); will work just as well.
On Wed, Aug 30, 2023 at 12:08:17PM +0100, Russell King (Oracle) wrote: > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > > + /* KSZ9477 Errata DS80000754C > > + * > > + * Module 4: Energy Efficient Ethernet (EEE) feature select must be > > + * manually disabled > > + * The EEE feature is enabled by default, but it is not fully > > + * operational. It must be manually disabled through register > > + * controls. If not disabled, the PHY ports can auto-negotiate > > + * to enable EEE, and this feature can cause link drops when linked > > + * to another device supporting EEE. > > + * > > + * Although, the KSZ9477 MMD register > > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that EEE is > > + * operational one needs to manualy clear them to follow the chip > > + * errata. > > + */ > > + linkmode_and(phydev->supported_eee, phydev->supported, zero); > > Hi, > > I'm wondering whether you had a reason to write the above, rather than > use the simpler: > > linkmode_zero(phydev->supported_eee); Ah, I wondered what was the proper name for this and was not able to found it. Thank you! Regards, Oleksij
Hi Oleksij, > On Wed, Aug 30, 2023 at 12:52:24PM +0200, Lukasz Majewski wrote: > > Hi Oleksij, > > > > > On Wed, Aug 30, 2023 at 11:21:19AM +0200, Lukasz Majewski wrote: > > > > The KSZ9477 errata points out (in 'Module 4') the link up/down > > > > problem when EEE (Energy Efficient Ethernet) is enabled in the > > > > device to which the KSZ9477 tries to auto negotiate. > > > > > > > > The suggested workaround is to clear advertisement of EEE for > > > > PHYs in this chip driver. > > > > > > > > Signed-off-by: Lukasz Majewski <lukma@denx.de> > > > > --- > > > > drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- > > > > 1 file changed, 30 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c > > > > index 87b090ad2874..469dcd8a5711 100644 > > > > --- a/drivers/net/phy/micrel.c > > > > +++ b/drivers/net/phy/micrel.c > > > > @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct > > > > phy_device *phydev) return 0; > > > > } > > > > > > > > +static int ksz9477_get_features(struct phy_device *phydev) > > > > +{ > > > > + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; > > > > + int ret; > > > > + > > > > + ret = genphy_read_abilities(phydev); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + /* KSZ9477 Errata DS80000754C > > > > + * > > > > + * Module 4: Energy Efficient Ethernet (EEE) feature > > > > select must be > > > > + * manually disabled > > > > + * The EEE feature is enabled by default, but it is > > > > not fully > > > > + * operational. It must be manually disabled through > > > > register > > > > + * controls. If not disabled, the PHY ports can > > > > auto-negotiate > > > > + * to enable EEE, and this feature can cause link > > > > drops when linked > > > > + * to another device supporting EEE. > > > > + * > > > > + * Although, the KSZ9477 MMD register > > > > + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that > > > > EEE is > > > > + * operational one needs to manualy clear them to > > > > follow the chip > > > > + * errata. > > > > + */ > > > > + linkmode_and(phydev->supported_eee, phydev->supported, > > > > zero); + > > > > + return 0; > > > > +} > > > > + > > > > #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 > > > > #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) > > > > #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) > > > > @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = > > > > { .handle_interrupt = kszphy_handle_interrupt, > > > > .suspend = genphy_suspend, > > > > .resume = genphy_resume, > > > > - .get_features = ksz9131_get_features, > > > > + .get_features = ksz9477_get_features, > > > > > > Sorry, i didn't described all details how to implement it. > > > > > > This code will break EEE support for the KSZ8563R switch. > > > > > > > And then another switch (KSZ8563R) pops up.... with regression.... > > Initially ksz9477_get_features() was introduced to fix KSZ8563R. > > > In the micrel.c the ksz9477_get_features was only present in: > > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4832 > > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/micrel.c#L4874 > > so I only changed it there. > > > > Apparently the KSZ8563R re-uses the KSZ9477 code. > > Most (all?) switch variant support by the ksz9477 DSA driver share > the same PHYid, so it is not possible to identify it by the micrel.c > PHY driver. As far as a know, the only commonly accepted way to > notify about some quirks between this both drivers is not user > dev_flags. > We would need another idea on fixing this problem as there is following order of function calls: -> ksz9477_setup -> ksz9477_get_features (here we are supposed to use the MICREL_NO_EEE flag) -> ksz_get_phy_flags (here we are supposed to set the MICREL_NO_EEE flag) The ksz9477_get_features is called early. It looks like the most optimal solution would be the one proposed by Tristam: https://www.spinics.net/lists/netdev/msg932044.html It would clear the 7.60 per-port register with two ksz_write16 functions. That exactly follows the recommendation for KSZ9477 Errata Module4. > Regards, > Oleskij Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski wrote: > Hi Oleksij, > It looks like the most optimal solution would be the one proposed by > Tristam: > https://www.spinics.net/lists/netdev/msg932044.html In this case, please add the reason why it would work on this HW and will not break by any changes in PHYlib or micrel.c driver. If I remember it correctly, in KSZ9477 variants, if you write to EEE advertisement register, it will affect the state of a EEE capability register. Which break IEEE 802.3 specification and the reason why ksz9477_get_features() actually exist. But can be used as workaround if it is written early enough before PHYlib tried to read EEE capability register. Please confirm my assumption by applying your workaround and testing it with ethtool --show-eee lanX. It should be commented in the code with all kind of warnings: Don't move!!! We use one bug to workaround another bug!!! If PHYlib start scanning PHYs before this code is executed, then thing may break!! ... it is broken as hell....
On Wed, Aug 30, 2023 at 02:17:38PM +0200, Oleksij Rempel wrote: > On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski wrote: > > Hi Oleksij, > > > It looks like the most optimal solution would be the one proposed by > > Tristam: > > https://www.spinics.net/lists/netdev/msg932044.html > > In this case, please add the reason why it would work on this HW and > will not break by any changes in PHYlib or micrel.c driver. > > If I remember it correctly, in KSZ9477 variants, if you write to EEE > advertisement register, it will affect the state of a EEE capability > register. Which break IEEE 802.3 specification and the reason why > ksz9477_get_features() actually exist. But can be used as workaround if > it is written early enough before PHYlib tried to read EEE capability > register. > > Please confirm my assumption by applying your workaround and testing it > with ethtool --show-eee lanX. > > It should be commented in the code with all kind of warnings: > Don't move!!! We use one bug to workaround another bug!!! If PHYlib > start scanning PHYs before this code is executed, then thing may break!! Why would phylib's scanning cause breakage? phylib's scanning for PHYs is about reading the ID registers etc. It doesn't do anything until the PHY has been found, and then the first thing that happens when the phy_device structure is created is an appropriate driver is located, and the driver's ->probe function is called. If that is successful, then the fewatures are read. If the PHY driver's ->features member is set, then that initialises the "supported" mask and we read the EEE abilities. If ->features is not set, then we look to see whether the driver provides a ->get_features method, and call that. Otherwise we use the generic genphy_c45_pma_read_abilities() or genphy_read_abilities() depending whether the PHY's is_c45 is set or not. So, if you want to do something very early before features are read, then either don't set .features, and do it early in .get_features before calling anything else, or do it in the ->probe function.
On Wed, Aug 30, 2023 at 01:35:18PM +0100, Russell King (Oracle) wrote: > On Wed, Aug 30, 2023 at 02:17:38PM +0200, Oleksij Rempel wrote: > > On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski wrote: > > > Hi Oleksij, > > > > > It looks like the most optimal solution would be the one proposed by > > > Tristam: > > > https://www.spinics.net/lists/netdev/msg932044.html > > > > In this case, please add the reason why it would work on this HW and > > will not break by any changes in PHYlib or micrel.c driver. > > > > If I remember it correctly, in KSZ9477 variants, if you write to EEE > > advertisement register, it will affect the state of a EEE capability > > register. Which break IEEE 802.3 specification and the reason why > > ksz9477_get_features() actually exist. But can be used as workaround if > > it is written early enough before PHYlib tried to read EEE capability > > register. > > > > Please confirm my assumption by applying your workaround and testing it > > with ethtool --show-eee lanX. > > > > It should be commented in the code with all kind of warnings: > > Don't move!!! We use one bug to workaround another bug!!! If PHYlib > > start scanning PHYs before this code is executed, then thing may break!! > > Why would phylib's scanning cause breakage? > > phylib's scanning for PHYs is about reading the ID registers etc. It > doesn't do anything until the PHY has been found, and then the first > thing that happens when the phy_device structure is created is an > appropriate driver is located, and the driver's ->probe function > is called. > > If that is successful, then the fewatures are read. If the PHY > driver's ->features member is set, then that initialises the > "supported" mask and we read the EEE abilities. > > If ->features is not set, then we look to see whether the driver > provides a ->get_features method, and call that. > > Otherwise we use the generic genphy_c45_pma_read_abilities() or > genphy_read_abilities() depending whether the PHY's is_c45 is set > or not. > > So, if you want to do something very early before features are read, > then either don't set .features, and do it early in .get_features > before calling anything else, or do it in the ->probe function. Let me summarize my view on the problem, so may be you can suggest a better way to solve it. - KSZ9477, KSZ8565, KSZ9893, KSZ9563, seems to have different quirks by the same PHYid. micrel.c driver do now know what exact HW is actually in use. - A set of PHY workarounds was moved from dsa/microchip/ksz9477.c to micrel.c, one of this workaround was clearing EEE advertisement register, which by accident was clearing EEE capability register. Since EEE cap was cleared by the dsa/microchip/ksz9477.c code before micrel.c was probed, PHYlib was assuming that his PHY do not supports EEE and dint tried to use it. After moving this code to micrel.c, it is now trying to change EEE advertisement state without letting PHYlib to know about it and PHYlib re enables it as actually excepted. - so far, only KSZ9477 seems to be broken beyond repair, so it is better to disable EEE without giving it as a choice for user configuration.
Hi Oleksij, > On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski wrote: > > Hi Oleksij, > > > It looks like the most optimal solution would be the one proposed by > > Tristam: > > https://www.spinics.net/lists/netdev/msg932044.html > > In this case, please add the reason why it would work on this HW and > will not break by any changes in PHYlib or micrel.c driver. > The ksz9477_config_cpu_port() is called from ksz_setup. In this function we would clear 7.60 MMD register for EEE advertisement. Only after the switch initialization, the phy code reads 7.60 register for each port and on that basis decides if the EEE is supported or not. (And only then ksz9477_get_features() is executed. Finally ksz_get_phy_flags() is called. > If I remember it correctly, in KSZ9477 variants, if you write to EEE > advertisement register, it will affect the state of a EEE capability > register. Which break IEEE 802.3 specification and the reason why > ksz9477_get_features() actually exist. But can be used as workaround > if it is written early enough before PHYlib tried to read EEE > capability register. > > Please confirm my assumption by applying your workaround and testing > it with ethtool --show-eee lanX. > # ethtool --show-eee lan1 EEE Settings for lan1: EEE status: disabled Tx LPI: 0 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: Not reported Link partner advertised EEE link modes: Not reported # # ethtool --show-eee lan2 EEE Settings for lan2: EEE status: disabled Tx LPI: 0 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: Not reported Link partner advertised EEE link modes: Not reported > It should be commented in the code with all kind of warnings: > Don't move!!! We use one bug to workaround another bug!!! As I've pointed out in the previous e-mail. This kind of bug cannot be easily fixed with modifying flags in ksz_get_phy_flags() as this function is called after ksz9477_get_features(). I'm open for ideas to do it right... > If PHYlib > start scanning PHYs before this code is executed, then thing may > break!! Is it possible that PHY lib will start scanning for phys before the DSA switch IC is probed with SPI? KSZ9477-EVB DTS is not using "mdio" node - i.e. the "old" scheme for DSA switch is used. > > ... it is broken as hell.... > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
On Wed, Aug 30, 2023 at 03:06:49PM +0200, Oleksij Rempel wrote: > On Wed, Aug 30, 2023 at 01:35:18PM +0100, Russell King (Oracle) wrote: > > On Wed, Aug 30, 2023 at 02:17:38PM +0200, Oleksij Rempel wrote: > > > On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski wrote: > > > > Hi Oleksij, > > > > > > > It looks like the most optimal solution would be the one proposed by > > > > Tristam: > > > > https://www.spinics.net/lists/netdev/msg932044.html > > > > > > In this case, please add the reason why it would work on this HW and > > > will not break by any changes in PHYlib or micrel.c driver. > > > > > > If I remember it correctly, in KSZ9477 variants, if you write to EEE > > > advertisement register, it will affect the state of a EEE capability > > > register. Which break IEEE 802.3 specification and the reason why > > > ksz9477_get_features() actually exist. But can be used as workaround if > > > it is written early enough before PHYlib tried to read EEE capability > > > register. > > > > > > Please confirm my assumption by applying your workaround and testing it > > > with ethtool --show-eee lanX. > > > > > > It should be commented in the code with all kind of warnings: > > > Don't move!!! We use one bug to workaround another bug!!! If PHYlib > > > start scanning PHYs before this code is executed, then thing may break!! > > > > Why would phylib's scanning cause breakage? > > > > phylib's scanning for PHYs is about reading the ID registers etc. It > > doesn't do anything until the PHY has been found, and then the first > > thing that happens when the phy_device structure is created is an > > appropriate driver is located, and the driver's ->probe function > > is called. > > > > If that is successful, then the fewatures are read. If the PHY > > driver's ->features member is set, then that initialises the > > "supported" mask and we read the EEE abilities. > > > > If ->features is not set, then we look to see whether the driver > > provides a ->get_features method, and call that. > > > > Otherwise we use the generic genphy_c45_pma_read_abilities() or > > genphy_read_abilities() depending whether the PHY's is_c45 is set > > or not. > > > > So, if you want to do something very early before features are read, > > then either don't set .features, and do it early in .get_features > > before calling anything else, or do it in the ->probe function. > > Let me summarize my view on the problem, so may be you can suggest a better > way to solve it. > - KSZ9477, KSZ8565, KSZ9893, KSZ9563, seems to have different quirks by > the same PHYid. micrel.c driver do now know what exact HW is actually > in use. > - A set of PHY workarounds was moved from dsa/microchip/ksz9477.c to > micrel.c, one of this workaround was clearing EEE advertisement > register, which by accident was clearing EEE capability register. > Since EEE cap was cleared by the dsa/microchip/ksz9477.c code before > micrel.c was probed, PHYlib was assuming that his PHY do not supports > EEE and dint tried to use it. > After moving this code to micrel.c, it is now trying to change EEE > advertisement state without letting PHYlib to know about it and PHYlib > re enables it as actually excepted. > - so far, only KSZ9477 seems to be broken beyond repair, so it is better > to disable EEE without giving it as a choice for user configuration. We do have support in phylib for "broken EEE modes" which DT could set for the broken PHYs, and as it is possible to describe the DSA PHYs in DT. This sets phydev->eee_broken_modes. phydev->eee_broken_modes gets looked at when genphy_config_aneg() or genphy_c45_an_config_aneg() gets called - which will happen when the PHY is being "started". So, you could add the DT properties as appropriate to disable all the EEE modes. Alternatively, in your .config_init function, you could detect your flag and force eee_broken_modes to all-ones. The problem with clearing ->supported_eee is that will stop genphy_c45_write_eee_adv() writing the advertisement register - which means if bits are set in the register, they won't be cleared because phylib thinks the registers aren't supported. So you won't actually be disabling anything by clearing ->supported_eee.
On Wed, Aug 30, 2023 at 02:30:55PM +0100, Russell King (Oracle) wrote: > On Wed, Aug 30, 2023 at 03:06:49PM +0200, Oleksij Rempel wrote: > > On Wed, Aug 30, 2023 at 01:35:18PM +0100, Russell King (Oracle) wrote: > > > On Wed, Aug 30, 2023 at 02:17:38PM +0200, Oleksij Rempel wrote: > > > > On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski wrote: > > > > > Hi Oleksij, > > > > > > > > > It looks like the most optimal solution would be the one proposed by > > > > > Tristam: > > > > > https://www.spinics.net/lists/netdev/msg932044.html > > > > > > > > In this case, please add the reason why it would work on this HW and > > > > will not break by any changes in PHYlib or micrel.c driver. > > > > > > > > If I remember it correctly, in KSZ9477 variants, if you write to EEE > > > > advertisement register, it will affect the state of a EEE capability > > > > register. Which break IEEE 802.3 specification and the reason why > > > > ksz9477_get_features() actually exist. But can be used as workaround if > > > > it is written early enough before PHYlib tried to read EEE capability > > > > register. > > > > > > > > Please confirm my assumption by applying your workaround and testing it > > > > with ethtool --show-eee lanX. > > > > > > > > It should be commented in the code with all kind of warnings: > > > > Don't move!!! We use one bug to workaround another bug!!! If PHYlib > > > > start scanning PHYs before this code is executed, then thing may break!! > > > > > > Why would phylib's scanning cause breakage? > > > > > > phylib's scanning for PHYs is about reading the ID registers etc. It > > > doesn't do anything until the PHY has been found, and then the first > > > thing that happens when the phy_device structure is created is an > > > appropriate driver is located, and the driver's ->probe function > > > is called. > > > > > > If that is successful, then the fewatures are read. If the PHY > > > driver's ->features member is set, then that initialises the > > > "supported" mask and we read the EEE abilities. > > > > > > If ->features is not set, then we look to see whether the driver > > > provides a ->get_features method, and call that. > > > > > > Otherwise we use the generic genphy_c45_pma_read_abilities() or > > > genphy_read_abilities() depending whether the PHY's is_c45 is set > > > or not. > > > > > > So, if you want to do something very early before features are read, > > > then either don't set .features, and do it early in .get_features > > > before calling anything else, or do it in the ->probe function. > > > > Let me summarize my view on the problem, so may be you can suggest a better > > way to solve it. > > - KSZ9477, KSZ8565, KSZ9893, KSZ9563, seems to have different quirks by > > the same PHYid. micrel.c driver do now know what exact HW is actually > > in use. > > - A set of PHY workarounds was moved from dsa/microchip/ksz9477.c to > > micrel.c, one of this workaround was clearing EEE advertisement > > register, which by accident was clearing EEE capability register. > > Since EEE cap was cleared by the dsa/microchip/ksz9477.c code before > > micrel.c was probed, PHYlib was assuming that his PHY do not supports > > EEE and dint tried to use it. > > After moving this code to micrel.c, it is now trying to change EEE > > advertisement state without letting PHYlib to know about it and PHYlib > > re enables it as actually excepted. > > - so far, only KSZ9477 seems to be broken beyond repair, so it is better > > to disable EEE without giving it as a choice for user configuration. > > We do have support in phylib for "broken EEE modes" which DT could set > for the broken PHYs, and as it is possible to describe the DSA PHYs in > DT. This sets phydev->eee_broken_modes. > > phydev->eee_broken_modes gets looked at when genphy_config_aneg() or > genphy_c45_an_config_aneg() gets called - which will happen when the > PHY is being "started". > > So, you could add the DT properties as appropriate to disable all the > EEE modes. > > Alternatively, in your .config_init function, you could detect your > flag and force eee_broken_modes to all-ones. @Lukasz, can you please try to set eee_broken_modes to all-ones. Somewhat like this: ksz9477_config_init() ... ...quirks... if (phydev->dev_flages & .. NO_EEE...) phydev->eee_broken_modes = -1; err = genphy_restart_aneg(phydev); ... @Russell, thx! Regards, Oleksij
On Wed, Aug 30, 2023 at 04:26:50PM +0200, Oleksij Rempel wrote: > @Lukasz, > > can you please try to set eee_broken_modes to all-ones. Somewhat like > this: > ksz9477_config_init() > ... > ...quirks... > > if (phydev->dev_flages & .. NO_EEE...) > phydev->eee_broken_modes = -1; That's fine in config_init(). > err = genphy_restart_aneg(phydev); That isn't necessary, and in any case, calling it will just cause the AN enable and AN restart bits in BMCR to be set, nothing will be reprogrammed. However, at a later point, when the PHY is started (by phy_start() being called) the state will be set to PHY_UP, and the state machine triggered. That sets needs_aneg which will then call phy_start_aneg(). That then goes on to call phy_config_aneg(), which will either call the driver specific config_aneg() function, or one of the two generic genphy.*config_aneg() functions. These will then program the EEE advertisement.
On Wed, Aug 30, 2023 at 03:38:55PM +0100, Russell King (Oracle) wrote: > On Wed, Aug 30, 2023 at 04:26:50PM +0200, Oleksij Rempel wrote: > > @Lukasz, > > > > can you please try to set eee_broken_modes to all-ones. Somewhat like > > this: > > ksz9477_config_init() > > ... > > ...quirks... > > > > if (phydev->dev_flages & .. NO_EEE...) > > phydev->eee_broken_modes = -1; > > That's fine in config_init(). > > > err = genphy_restart_aneg(phydev); > > That isn't necessary, and in any case, calling it will just cause the > AN enable and AN restart bits in BMCR to be set, nothing will be > reprogrammed. ack. It is already existing code, see: https://elixir.bootlin.com/linux/v6.5/source/drivers/net/phy/micrel.c#L1822 Setting eee_broken_modes probably can be done at any plaice in this function. Regards, Oleksij
Hi Oleksij, > On Wed, Aug 30, 2023 at 02:30:55PM +0100, Russell King (Oracle) wrote: > > On Wed, Aug 30, 2023 at 03:06:49PM +0200, Oleksij Rempel wrote: > > > On Wed, Aug 30, 2023 at 01:35:18PM +0100, Russell King (Oracle) > > > wrote: > > > > On Wed, Aug 30, 2023 at 02:17:38PM +0200, Oleksij Rempel wrote: > > > > > > > > > On Wed, Aug 30, 2023 at 01:51:51PM +0200, Lukasz Majewski > > > > > wrote: > > > > > > Hi Oleksij, > > > > > > > > > > > It looks like the most optimal solution would be the one > > > > > > proposed by Tristam: > > > > > > https://www.spinics.net/lists/netdev/msg932044.html > > > > > > > > > > In this case, please add the reason why it would work on this > > > > > HW and will not break by any changes in PHYlib or micrel.c > > > > > driver. > > > > > > > > > > If I remember it correctly, in KSZ9477 variants, if you write > > > > > to EEE advertisement register, it will affect the state of a > > > > > EEE capability register. Which break IEEE 802.3 specification > > > > > and the reason why ksz9477_get_features() actually exist. But > > > > > can be used as workaround if it is written early enough > > > > > before PHYlib tried to read EEE capability register. > > > > > > > > > > Please confirm my assumption by applying your workaround and > > > > > testing it with ethtool --show-eee lanX. > > > > > > > > > > It should be commented in the code with all kind of warnings: > > > > > Don't move!!! We use one bug to workaround another bug!!! If > > > > > PHYlib start scanning PHYs before this code is executed, then > > > > > thing may break!! > > > > > > > > Why would phylib's scanning cause breakage? > > > > > > > > phylib's scanning for PHYs is about reading the ID registers > > > > etc. It doesn't do anything until the PHY has been found, and > > > > then the first thing that happens when the phy_device structure > > > > is created is an appropriate driver is located, and the > > > > driver's ->probe function is called. > > > > > > > > If that is successful, then the fewatures are read. If the PHY > > > > driver's ->features member is set, then that initialises the > > > > "supported" mask and we read the EEE abilities. > > > > > > > > If ->features is not set, then we look to see whether the driver > > > > provides a ->get_features method, and call that. > > > > > > > > Otherwise we use the generic genphy_c45_pma_read_abilities() or > > > > genphy_read_abilities() depending whether the PHY's is_c45 is > > > > set or not. > > > > > > > > So, if you want to do something very early before features are > > > > read, then either don't set .features, and do it early in > > > > .get_features before calling anything else, or do it in the > > > > ->probe function. > > > > > > Let me summarize my view on the problem, so may be you can > > > suggest a better way to solve it. > > > - KSZ9477, KSZ8565, KSZ9893, KSZ9563, seems to have different > > > quirks by the same PHYid. micrel.c driver do now know what exact > > > HW is actually in use. > > > - A set of PHY workarounds was moved from dsa/microchip/ksz9477.c > > > to micrel.c, one of this workaround was clearing EEE advertisement > > > register, which by accident was clearing EEE capability > > > register. Since EEE cap was cleared by the > > > dsa/microchip/ksz9477.c code before micrel.c was probed, PHYlib > > > was assuming that his PHY do not supports EEE and dint tried to > > > use it. After moving this code to micrel.c, it is now trying to > > > change EEE advertisement state without letting PHYlib to know > > > about it and PHYlib re enables it as actually excepted. > > > - so far, only KSZ9477 seems to be broken beyond repair, so it is > > > better to disable EEE without giving it as a choice for user > > > configuration. > > > > We do have support in phylib for "broken EEE modes" which DT could > > set for the broken PHYs, and as it is possible to describe the DSA > > PHYs in DT. This sets phydev->eee_broken_modes. > > > > phydev->eee_broken_modes gets looked at when genphy_config_aneg() or > > genphy_c45_an_config_aneg() gets called - which will happen when the > > PHY is being "started". > > > > So, you could add the DT properties as appropriate to disable all > > the EEE modes. > > > > Alternatively, in your .config_init function, you could detect your > > flag and force eee_broken_modes to all-ones. > > @Lukasz, > > can you please try to set eee_broken_modes to all-ones. Somewhat like > this: > ksz9477_config_init() > ... > ...quirks... > > if (phydev->dev_flages & .. NO_EEE...) > phydev->eee_broken_modes = -1; > > err = genphy_restart_aneg(phydev); > ... > The implementation as you suggested seems to work :-) The ksz_get_phy_flags() - where the MICREL_NO_EEE is set is executed before ksz9477_config_init(). And then the eee_broken_modes are taken into account. # ethtool --show-eee lan1 EEE Settings for lan1: EEE status: disabled Tx LPI: 0 (us) Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: Not reported Link partner advertised EEE link modes: Not reported I will prepare tomorrow a proper patch. > @Russell, thx! > > Regards, > Oleksij Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Hi Lukasz, On Wed, Aug 30, 2023 at 06:38:18PM +0200, Lukasz Majewski wrote: > Hi Oleksij, > The implementation as you suggested seems to work :-) > > The ksz_get_phy_flags() - where the MICREL_NO_EEE is set is executed > before ksz9477_config_init(). > > And then the eee_broken_modes are taken into account. > > # ethtool --show-eee lan1 > EEE Settings for lan1: > EEE status: disabled > Tx LPI: 0 (us) > Supported EEE link modes: 100baseT/Full > 1000baseT/Full > Advertised EEE link modes: Not reported > Link partner advertised EEE link modes: Not reported > > I will prepare tomorrow a proper patch. can you please by the way remove this line: https://elixir.bootlin.com/linux/v6.5/source/drivers/net/phy/micrel.c#L1803 it is obsolet by eee_broken_modes. Regards, Oleksij
On Thu, Aug 31, 2023 at 06:40:04AM +0200, Oleksij Rempel wrote: > Hi Lukasz, > > On Wed, Aug 30, 2023 at 06:38:18PM +0200, Lukasz Majewski wrote: > > Hi Oleksij, > > > The implementation as you suggested seems to work :-) > > > > The ksz_get_phy_flags() - where the MICREL_NO_EEE is set is executed > > before ksz9477_config_init(). > > > > And then the eee_broken_modes are taken into account. > > > > # ethtool --show-eee lan1 > > EEE Settings for lan1: > > EEE status: disabled > > Tx LPI: 0 (us) > > Supported EEE link modes: 100baseT/Full > > 1000baseT/Full > > Advertised EEE link modes: Not reported > > Link partner advertised EEE link modes: Not reported > > > > I will prepare tomorrow a proper patch. > > can you please by the way remove this line: > https://elixir.bootlin.com/linux/v6.5/source/drivers/net/phy/micrel.c#L1803 > > it is obsolet by eee_broken_modes. ... and if possible verify on the link partner side that indeed no EEE modes are being advertised by the Micrel device.
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c index 87b090ad2874..469dcd8a5711 100644 --- a/drivers/net/phy/micrel.c +++ b/drivers/net/phy/micrel.c @@ -1418,6 +1418,35 @@ static int ksz9131_get_features(struct phy_device *phydev) return 0; } +static int ksz9477_get_features(struct phy_device *phydev) +{ + __ETHTOOL_DECLARE_LINK_MODE_MASK(zero) = { 0, }; + int ret; + + ret = genphy_read_abilities(phydev); + if (ret) + return ret; + + /* KSZ9477 Errata DS80000754C + * + * Module 4: Energy Efficient Ethernet (EEE) feature select must be + * manually disabled + * The EEE feature is enabled by default, but it is not fully + * operational. It must be manually disabled through register + * controls. If not disabled, the PHY ports can auto-negotiate + * to enable EEE, and this feature can cause link drops when linked + * to another device supporting EEE. + * + * Although, the KSZ9477 MMD register + * (MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV) advertise that EEE is + * operational one needs to manualy clear them to follow the chip + * errata. + */ + linkmode_and(phydev->supported_eee, phydev->supported, zero); + + return 0; +} + #define KSZ8873MLL_GLOBAL_CONTROL_4 0x06 #define KSZ8873MLL_GLOBAL_CONTROL_4_DUPLEX BIT(6) #define KSZ8873MLL_GLOBAL_CONTROL_4_SPEED BIT(4) @@ -4871,7 +4900,7 @@ static struct phy_driver ksphy_driver[] = { .handle_interrupt = kszphy_handle_interrupt, .suspend = genphy_suspend, .resume = genphy_resume, - .get_features = ksz9131_get_features, + .get_features = ksz9477_get_features, } }; module_phy_driver(ksphy_driver);
The KSZ9477 errata points out (in 'Module 4') the link up/down problem when EEE (Energy Efficient Ethernet) is enabled in the device to which the KSZ9477 tries to auto negotiate. The suggested workaround is to clear advertisement of EEE for PHYs in this chip driver. Signed-off-by: Lukasz Majewski <lukma@denx.de> --- drivers/net/phy/micrel.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-)