[PATCH 1/2] net: phy: Rename KSZ9477 get features function (to ksz9131_get_features)

[PATCH 1/2] net: phy: Rename KSZ9477 get features function (to ksz9131_get_features)

[Lukasz Majewski]

The KSZ9131 differs in supported features from KSZ9477 - namely in the
EEE (Energy Efficient Ethernet) support.

To prepare the code for upcoming changes the ksz9477_get_features
has been renamed to ksz9131_get_features.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
 drivers/net/phy/micrel.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index b6d7981b2d1e..87b090ad2874 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1398,7 +1398,7 @@ static int ksz9131_config_aneg(struct phy_device *phydev)
 	return genphy_config_aneg(phydev);
 }
 
-static int ksz9477_get_features(struct phy_device *phydev)
+static int ksz9131_get_features(struct phy_device *phydev)
 {
 	int ret;
 
@@ -4829,7 +4829,7 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= kszphy_resume,
 	.cable_test_start	= ksz9x31_cable_test_start,
 	.cable_test_get_status	= ksz9x31_cable_test_get_status,
-	.get_features	= ksz9477_get_features,
+	.get_features	= ksz9131_get_features,
 }, {
 	.phy_id		= PHY_ID_KSZ8873MLL,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
@@ -4871,7 +4871,7 @@ static struct phy_driver ksphy_driver[] = {
 	.handle_interrupt = kszphy_handle_interrupt,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
-	.get_features	= ksz9477_get_features,
+	.get_features	= ksz9131_get_features,
 } };
 
 module_phy_driver(ksphy_driver);
-- 
2.20.1

[PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

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,
 } };
 
 module_phy_driver(ksphy_driver);
-- 
2.20.1

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Michal Swiatkowski]

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
> 

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4119 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 4621 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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....

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 5676 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Oleksij Rempel]

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
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |