[net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

The KSZ9477 errata points out (in 'Module 4') the link up/down problems
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.

To avoid regressions with other switch ICs the new MICREL_NO_EEE flag
has been introduced.

Moreover, the in-register disablement of MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV
MMD register is removed, as this code is both; now executed too late
(after previous rework of the PHY and DSA for KSZ switches) and not
required as setting all members of eee_broken_modes bit field prevents
the KSZ9477 from advertising EEE.

Fixes: 69d3b36ca045 ("net: dsa: microchip: enable EEE support") (for KSZ9477).

Signed-off-by: Lukasz Majewski <lukma@denx.de>
Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
Confirmed disabled EEE with oscilloscope.
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

---
Changes for v2:
- Move this fix from clearing directly
  MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV MMD register in KSZ DSA switch
  initialization to more generic solution in the micrel.c PHY driver

Changes for v3:
- Drop the rename patch
- Use MICREL_NO_EEE flag as suggested by Oleksij
- Extend the ksz9477_config_init to avoid regression
- Do not use the direct write to MMD 7.60 register - instead the
  phydev->eee_broken_modes is used

Changes for v4:
- Rebase on top of net-next with Oleksij Rempel patch (adding BIT()
  macros on to Micrel flags) applied
- Add SoB and Tested-by tags
---
 drivers/net/dsa/microchip/ksz_common.c | 16 +++++++++++++++-
 drivers/net/phy/micrel.c               |  9 ++++++---
 include/linux/micrel_phy.h             |  1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 6673122266b7..42db7679c360 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2335,13 +2335,27 @@ static u32 ksz_get_phy_flags(struct dsa_switch *ds, int port)
 {
 	struct ksz_device *dev = ds->priv;
 
-	if (dev->chip_id == KSZ8830_CHIP_ID) {
+	switch (dev->chip_id) {
+	case KSZ8830_CHIP_ID:
 		/* Silicon Errata Sheet (DS80000830A):
 		 * Port 1 does not work with LinkMD Cable-Testing.
 		 * Port 1 does not respond to received PAUSE control frames.
 		 */
 		if (!port)
 			return MICREL_KSZ8_P1_ERRATA;
+		break;
+	case KSZ9477_CHIP_ID:
+		/* 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.
+		 */
+		return MICREL_NO_EEE;
 	}
 
 	return 0;
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index b6d7981b2d1e..927d3d54658e 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -1800,9 +1800,6 @@ static const struct ksz9477_errata_write ksz9477_errata_writes[] = {
 	/* Transmit waveform amplitude can be improved (1000BASE-T, 100BASE-TX, 10BASE-Te) */
 	{0x1c, 0x04, 0x00d0},
 
-	/* Energy Efficient Ethernet (EEE) feature select must be manually disabled */
-	{0x07, 0x3c, 0x0000},
-
 	/* Register settings are required to meet data sheet supply current specifications */
 	{0x1c, 0x13, 0x6eff},
 	{0x1c, 0x14, 0xe6ff},
@@ -1847,6 +1844,12 @@ 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)
+		phydev->eee_broken_modes = -1;
+
 	err = genphy_restart_aneg(phydev);
 	if (err)
 		return err;
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 322d87255984..4e27ca7c49de 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -44,6 +44,7 @@
 #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.20.1

Re: [net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Jakub Kicinski]

On Tue,  5 Sep 2023 11:33:15 +0200 Lukasz Majewski wrote:
> Fixes: 69d3b36ca045 ("net: dsa: microchip: enable EEE support") (for KSZ9477).
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

No need to repost for just this, but if there is a v5:
 - no empty line between Fixes and other tags
 - add the comment after '#' i.e.:

Fixes: 69d3b36ca045 ("net: dsa: microchip: enable EEE support") # for KSZ9477

Re: [net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Florian Fainelli]

On 9/5/23 02:33, Lukasz Majewski wrote:
> The KSZ9477 errata points out (in 'Module 4') the link up/down problems
> 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.
> 
> To avoid regressions with other switch ICs the new MICREL_NO_EEE flag
> has been introduced.
> 
> Moreover, the in-register disablement of MMD_DEVICE_ID_EEE_ADV.MMD_EEE_ADV
> MMD register is removed, as this code is both; now executed too late
> (after previous rework of the PHY and DSA for KSZ switches) and not
> required as setting all members of eee_broken_modes bit field prevents
> the KSZ9477 from advertising EEE.
> 
> Fixes: 69d3b36ca045 ("net: dsa: microchip: enable EEE support") (for KSZ9477).
> 
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Confirmed disabled EEE with oscilloscope.
> Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Vladimir Oltean]

On Tue, Sep 05, 2023 at 11:33:15AM +0200, Lukasz Majewski wrote:
> @@ -1847,6 +1844,12 @@ 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)
> +		phydev->eee_broken_modes = -1;

I know this is just another quick'n'dirty code snippet exchanged over
email which turned into a proper patch, but wouldn't it be more
civilized to use "MDIO_EEE_100TX | MDIO_EEE_1000T" here, than to declare
EEE broken for link modes which aren't even supported by the internal
switch PHYs?

It's probably not causing practical harm, but currently, we tell
genphy_config_eee_advert() to include reserved bits of the MMD EEE
Advertising Register (MMD 0x07 : 0x3C) into its modification mask.

Re: [net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Russell King (Oracle)]

On Wed, Sep 06, 2023 at 01:29:45AM +0300, Vladimir Oltean wrote:
> It's probably not causing practical harm, but currently, we tell
> genphy_config_eee_advert() to include reserved bits of the MMD EEE
> Advertising Register (MMD 0x07 : 0x3C) into its modification mask.

Ah, you've fallen into that trap. genphy_config_eee_advert() is
obsolete and redundant. Nothing calls it, and it should be removed.

What's actually used is genphy_c45_an_config_eee_aneg() and
genphy_c45_write_eee_adv(). The latter will not modify any reserved
bits in the register.

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

Re: [net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[Lukasz Majewski]

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

Hi Jakub,

> On Tue,  5 Sep 2023 11:33:15 +0200 Lukasz Majewski wrote:
> > Fixes: 69d3b36ca045 ("net: dsa: microchip: enable EEE support")
> > (for KSZ9477).
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>  
> 
> No need to repost for just this, but if there is a v5:

I hope that this is the last version of this errata patch ... :-)

>  - no empty line between Fixes and other tags
>  - add the comment after '#' i.e.:
> 
> Fixes: 69d3b36ca045 ("net: dsa: microchip: enable EEE support") # for
> KSZ9477

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: [net v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  5 Sep 2023 11:33:15 +0200 you wrote:
> The KSZ9477 errata points out (in 'Module 4') the link up/down problems
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v4] net: phy: Provide Module 4 KSZ9477 errata (DS80000754C)
    https://git.kernel.org/netdev/net/c/08c6d8bae48c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html