[PATCH net-next v2 0/2] microchip/micrel switch: replace MICREL_NO_EEE workaround

[PATCH net-next v2 0/2] microchip/micrel switch: replace MICREL_NO_EEE workaround

[Heiner Kallweit]

On several supported switches the integrated PHY's have buggy EEE.
On the GBit-capable ones it's always the same type of PHY with PHY ID
0x00221631, so I think we can assume that all PHY's with this id
have this EEE issue. Let's simplify erratum handling by calling
phy_disable_eee() for this PHY type.

v2:
- Use phy_disable_eee() instead of clearing supported_eee in patch 1

Heiner Kallweit (2):
  net: phy: micrel: disable EEE on KSZ9477-type PHY
  net: dsa: microchip: remove MICREL_NO_EEE workaround

 drivers/net/dsa/microchip/ksz_common.c | 25 -------------------------
 drivers/net/phy/micrel.c               | 12 ++++++------
 include/linux/micrel_phy.h             |  1 -
 3 files changed, 6 insertions(+), 32 deletions(-)

-- 
2.47.1

[PATCH net-next v2 1/2] net: phy: micrel: disable EEE on KSZ9477-type PHY

[Heiner Kallweit]

On several supported switches the integrated PHY's have buggy EEE.
On the GBit-capable ones it's always the same type of PHY with PHY ID
0x00221631. So we can simplify erratum handling by calling
phy_disable_eee() for this PHY type.

Note: The KSZ9477 PHY driver also covers e.g. the internal PHY of
      KSZ9563 (ID: 0x00221637), which is unaffected by the EEE issue.
      Therefore check for the exact PHY ID.

Reported-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- call phy_disable_eee() instead of clearing supported_eee
---
 drivers/net/phy/micrel.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index eeb33eb18..e7eaa1264 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1524,6 +1524,12 @@ static int ksz9477_get_features(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
+	/* See KSZ9477 Errata DS80000754C Module 4 */
+	if (phydev->phy_id == PHY_ID_KSZ9477) {
+		phy_disable_eee(phydev);
+		return 0;
+	}
+
 	/* The "EEE control and capability 1" (Register 3.20) seems to be
 	 * influenced by the "EEE advertisement 1" (Register 7.60). Changes
 	 * on the 7.60 will affect 3.20. So, we need to construct our own list
@@ -2002,12 +2008,6 @@ static int ksz9477_config_init(struct phy_device *phydev)
 			return err;
 	}
 
-	/* According to KSZ9477 Errata DS80000754C (Module 4) all EEE modes
-	 * in this switch shall be regarded as broken.
-	 */
-	if (phydev->dev_flags & MICREL_NO_EEE)
-		linkmode_fill(phydev->eee_broken_modes);
-
 	return kszphy_config_init(phydev);
 }
 
-- 
2.47.1

[PATCH net-next v2 2/2] net: dsa: microchip: remove MICREL_NO_EEE workaround

[Heiner Kallweit]

The integrated PHY's on all these switch types have the same PHY ID.
So we can assume that the issue is related to the PHY type, not the
switch type. After having disabled EEE for this PHY type, we can remove
the workaround code here.

Note: On the fast ethernet models listed here the integrated PHY has
      PHY ID 0x00221550, which is handled by PHY driver
      "Micrel KSZ87XX Switch". This PHY driver doesn't handle flag
      MICREL_NO_EEE, therefore setting the flag for these models
      results in a no-op.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 25 -------------------------
 include/linux/micrel_phy.h             |  1 -
 2 files changed, 26 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index e3512e324..4871bb1fc 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3008,31 +3008,6 @@ static u32 ksz_get_phy_flags(struct dsa_switch *ds, int port)
 		if (!port)
 			return MICREL_KSZ8_P1_ERRATA;
 		break;
-	case KSZ8567_CHIP_ID:
-		/* KSZ8567R Errata DS80000752C Module 4 */
-	case KSZ8765_CHIP_ID:
-	case KSZ8794_CHIP_ID:
-	case KSZ8795_CHIP_ID:
-		/* KSZ879x/KSZ877x/KSZ876x Errata DS80000687C Module 2 */
-	case KSZ9477_CHIP_ID:
-		/* KSZ9477S Errata DS80000754A Module 4 */
-	case KSZ9567_CHIP_ID:
-		/* KSZ9567S Errata DS80000756A Module 4 */
-	case KSZ9896_CHIP_ID:
-		/* KSZ9896C Errata DS80000757A Module 3 */
-	case KSZ9897_CHIP_ID:
-	case LAN9646_CHIP_ID:
-		/* KSZ9897R Errata DS80000758C 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.
-		 *
-		 * The same item appears in the errata for all switches above.
-		 */
-		return MICREL_NO_EEE;
 	}
 
 	return 0;
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 591bf5b5e..9af01bdd8 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -44,7 +44,6 @@
 #define MICREL_PHY_50MHZ_CLK	BIT(0)
 #define MICREL_PHY_FXEN		BIT(1)
 #define MICREL_KSZ8_P1_ERRATA	BIT(2)
-#define MICREL_NO_EEE		BIT(3)
 
 #define MICREL_KSZ9021_EXTREG_CTRL	0xB
 #define MICREL_KSZ9021_EXTREG_DATA_WRITE	0xC
-- 
2.47.1

Re: [PATCH net-next v2 2/2] net: dsa: microchip: remove MICREL_NO_EEE workaround

[Andrew Lunn]

On Mon, Jan 06, 2025 at 02:23:36PM +0100, Heiner Kallweit wrote:
> The integrated PHY's on all these switch types have the same PHY ID.
> So we can assume that the issue is related to the PHY type, not the
> switch type. After having disabled EEE for this PHY type, we can remove
> the workaround code here.
> 
> Note: On the fast ethernet models listed here the integrated PHY has
>       PHY ID 0x00221550, which is handled by PHY driver
>       "Micrel KSZ87XX Switch". This PHY driver doesn't handle flag
>       MICREL_NO_EEE, therefore setting the flag for these models
>       results in a no-op.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2 2/2] net: dsa: microchip: remove MICREL_NO_EEE workaround

[Oleksij Rempel]

On Mon, Jan 06, 2025 at 02:23:36PM +0100, Heiner Kallweit wrote:
> The integrated PHY's on all these switch types have the same PHY ID.
> So we can assume that the issue is related to the PHY type, not the
> switch type. After having disabled EEE for this PHY type, we can remove
> the workaround code here.
> 
> Note: On the fast ethernet models listed here the integrated PHY has
>       PHY ID 0x00221550, which is handled by PHY driver
>       "Micrel KSZ87XX Switch". This PHY driver doesn't handle flag
>       MICREL_NO_EEE, therefore setting the flag for these models
>       results in a no-op.

Yes, it feels like no one is using KSZ87XX switches with the kernel DSA
driver.

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 25 -------------------------
>  include/linux/micrel_phy.h             |  1 -
>  2 files changed, 26 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index e3512e324..4871bb1fc 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -3008,31 +3008,6 @@ static u32 ksz_get_phy_flags(struct dsa_switch *ds, int port)
>  		if (!port)
>  			return MICREL_KSZ8_P1_ERRATA;
>  		break;
> -	case KSZ8567_CHIP_ID:
> -		/* KSZ8567R Errata DS80000752C Module 4 */
> -	case KSZ8765_CHIP_ID:
> -	case KSZ8794_CHIP_ID:
> -	case KSZ8795_CHIP_ID:
> -		/* KSZ879x/KSZ877x/KSZ876x Errata DS80000687C Module 2 */
> -	case KSZ9477_CHIP_ID:
> -		/* KSZ9477S Errata DS80000754A Module 4 */
> -	case KSZ9567_CHIP_ID:
> -		/* KSZ9567S Errata DS80000756A Module 4 */
> -	case KSZ9896_CHIP_ID:
> -		/* KSZ9896C Errata DS80000757A Module 3 */
> -	case KSZ9897_CHIP_ID:
> -	case LAN9646_CHIP_ID:
> -		/* KSZ9897R Errata DS80000758C 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.
> -		 *
> -		 * The same item appears in the errata for all switches above.
> -		 */

I have two problems with current patch set:
- dropped documentation, not all switches are officially broken, so
  keeping it documented is important.
- not all KSZ9xxx based switches are officially broken. All 3 port
  switches are not broken but still match against the KSZ9477 PHY
  driver:
  KSZ8563_CHIP_ID - 0x00221631
  KSZ9563_CHIP_ID - 0x00221631
  KSZ9893_CHIP_ID - 0x00221631

Best 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 net-next v2 2/2] net: dsa: microchip: remove MICREL_NO_EEE workaround

[Andrew Lunn]

> I have two problems with current patch set:
> - dropped documentation, not all switches are officially broken, so
>   keeping it documented is important.
> - not all KSZ9xxx based switches are officially broken. All 3 port
>   switches are not broken but still match against the KSZ9477 PHY
>   driver:
>   KSZ8563_CHIP_ID - 0x00221631
>   KSZ9563_CHIP_ID - 0x00221631
>   KSZ9893_CHIP_ID - 0x00221631

When you say "not broken", do you mean there is text in the errata
which says they do really, truly, work, or there is simply no errata
which says they are broken? Do you have these 3 ports switches and
have tested them?

It seems odd to me that the 3 port version should work. Why is it
special?

	Andrew

Re: [PATCH net-next v2 2/2] net: dsa: microchip: remove MICREL_NO_EEE workaround

[Oleksij Rempel]

On Tue, Jan 07, 2025 at 02:03:27PM +0100, Andrew Lunn wrote:
> > I have two problems with current patch set:
> > - dropped documentation, not all switches are officially broken, so
> >   keeping it documented is important.
> > - not all KSZ9xxx based switches are officially broken. All 3 port
> >   switches are not broken but still match against the KSZ9477 PHY
> >   driver:
> >   KSZ8563_CHIP_ID - 0x00221631
> >   KSZ9563_CHIP_ID - 0x00221631
> >   KSZ9893_CHIP_ID - 0x00221631
> 
> When you say "not broken", do you mean there is text in the errata
> which says they do really, truly, work, or there is simply no errata
> which says they are broken? Do you have these 3 ports switches and
> have tested them?

There are multiple true conditions in this case:
- Documentation claims EEE is supported
- Errata documentation do not recommend to disable it
- I have access to this variants, they are relatively common in
  industrial products. So far it seems to work without issues.

> It seems odd to me that the 3 port version should work. Why is it
> special?

In my case, on KSZ9477 the EEE starts to have issues as soon as i start to use
more then 2 ports. I assume, it is switch internal power domain
integration issues and not directly related to actual PHYs. May be it is
possible to handle it some how or disable EEE only for some ports, or
not enable for more then 2 ports, but if vendor removes EEE from
documentation I see it as officially not supported.

-- 
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 net-next v2 2/2] net: dsa: microchip: remove MICREL_NO_EEE workaround

[Heiner Kallweit]

On 07.01.2025 08:42, Oleksij Rempel wrote:
> On Mon, Jan 06, 2025 at 02:23:36PM +0100, Heiner Kallweit wrote:
>> The integrated PHY's on all these switch types have the same PHY ID.
>> So we can assume that the issue is related to the PHY type, not the
>> switch type. After having disabled EEE for this PHY type, we can remove
>> the workaround code here.
>>
>> Note: On the fast ethernet models listed here the integrated PHY has
>>       PHY ID 0x00221550, which is handled by PHY driver
>>       "Micrel KSZ87XX Switch". This PHY driver doesn't handle flag
>>       MICREL_NO_EEE, therefore setting the flag for these models
>>       results in a no-op.
> 
> Yes, it feels like no one is using KSZ87XX switches with the kernel DSA
> driver.
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/dsa/microchip/ksz_common.c | 25 -------------------------
>>  include/linux/micrel_phy.h             |  1 -
>>  2 files changed, 26 deletions(-)
>>
>> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
>> index e3512e324..4871bb1fc 100644
>> --- a/drivers/net/dsa/microchip/ksz_common.c
>> +++ b/drivers/net/dsa/microchip/ksz_common.c
>> @@ -3008,31 +3008,6 @@ static u32 ksz_get_phy_flags(struct dsa_switch *ds, int port)
>>  		if (!port)
>>  			return MICREL_KSZ8_P1_ERRATA;
>>  		break;
>> -	case KSZ8567_CHIP_ID:
>> -		/* KSZ8567R Errata DS80000752C Module 4 */
>> -	case KSZ8765_CHIP_ID:
>> -	case KSZ8794_CHIP_ID:
>> -	case KSZ8795_CHIP_ID:
>> -		/* KSZ879x/KSZ877x/KSZ876x Errata DS80000687C Module 2 */
>> -	case KSZ9477_CHIP_ID:
>> -		/* KSZ9477S Errata DS80000754A Module 4 */
>> -	case KSZ9567_CHIP_ID:
>> -		/* KSZ9567S Errata DS80000756A Module 4 */
>> -	case KSZ9896_CHIP_ID:
>> -		/* KSZ9896C Errata DS80000757A Module 3 */
>> -	case KSZ9897_CHIP_ID:
>> -	case LAN9646_CHIP_ID:
>> -		/* KSZ9897R Errata DS80000758C 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.
>> -		 *
>> -		 * The same item appears in the errata for all switches above.
>> -		 */
> 
> I have two problems with current patch set:
> - dropped documentation, not all switches are officially broken, so
>   keeping it documented is important.
> - not all KSZ9xxx based switches are officially broken. All 3 port
>   switches are not broken but still match against the KSZ9477 PHY
>   driver:
>   KSZ8563_CHIP_ID - 0x00221631
>   KSZ9563_CHIP_ID - 0x00221631
>   KSZ9893_CHIP_ID - 0x00221631
> 
OK, thanks for the clarification. Then we have to go another way.

> Best Regards,
> Oleksij