Re: [PATCH 3/3] net: dsa: microchip: implement KSZ87xx Module 3 low-loss cable errata

[Vladimir Oltean]

On Thu, Mar 26, 2026 at 10:10:23AM +0100, Fidelio Lawson wrote:
> Implement the "Module 3: Equalizer fix for short cables" erratum from
> Microchip document DS80000687C for KSZ87xx switches.
> 
> The issue affects short or low-loss cable links (e.g. CAT5e/CAT6),
> where the PHY receiver equalizer may amplify high-amplitude signals
> excessively, resulting in internal distortion and link establishment
> failures.
> 
> Depending on the selected workaround (1 or 2), the driver writes a
> specific value to the indirect PHY register
> using the 6E/6F/A0 indirect access mechanism.
> 
> The errata fix is applied during global switch initialization when
> enabled via device tree.
> 
> Signed-off-by: Fidelio Lawson <fidelio.lawson@xxxxxxxxxx>
> ---
>  drivers/net/dsa/microchip/ksz8.c | 46 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 78b42cf50ce2..b6f3a1ce85fc 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -1901,6 +1901,41 @@ void ksz8_phylink_mac_link_up(struct phylink_config *config,
>  		ksz8_phy_port_link_up(dev, port, duplex, tx_pause, rx_pause);
>  }
>  
> +static int ksz8_handle_module3_errata(struct ksz_device *dev)
> +{
> +	int ret = 0;

"ret" does not need to be zero-initialized. It is unconditionally
overwritten by ksz_write8().

And please sort lines with variable declarations in decreasing line
length order. Netdev calls this "reverse Christmas tree" ordering and is
the preferred coding style.

> +	const u16 *regs = dev->info->regs;
> +	u16 indir_reg = 0x0000;
> +	u8 indir_val = 0x00;
> +
> +	switch (dev->low_loss_wa_mode) {
> +	case KSZ_LOW_LOSS_WA_1:
> +		indir_reg = 0x3C;
> +		indir_val = 0x15;
> +		break;
> +	case KSZ_LOW_LOSS_WA_2:
> +		indir_reg = 0x4C;
> +		indir_val = 0x40;

Do the 3c and 4c registers have any associated documentation? Do we know
what they are or what they do? We should have some macros for them,
instead of magic numbers.

> +		break;
> +	default:
> +		break;

Is it expected that in the default case (no workaround), the code flow
writes indir_val = 0x00 to indir_reg = 0x0000? Or would it be better to
just exit early without making any change?

> +	}
> +
> +	mutex_lock(&dev->alu_mutex);
> +
> +	ret = ksz_write8(dev, regs[REG_IND_CTRL_0], 0xA0);
> +
> +	if (!ret)
> +		ret = ksz_write8(dev, 0x6F, indir_reg);
> +
> +	if (!ret)
> +		ret = ksz_write8(dev, regs[REG_IND_BYTE], indir_val);

Is this sequence better suited for ksz8_ind_write8()? Perhaps wrapped in
another layer similar to ksz8_pme_write8(), once we know what the magic
numbers represent in terms of indirect table?

> +
> +	mutex_unlock(&dev->alu_mutex);
> +
> +	return ret;
> +}
> +
>  static int ksz8_handle_global_errata(struct dsa_switch *ds)
>  {
>  	struct ksz_device *dev = ds->priv;
> @@ -1915,6 +1950,17 @@ static int ksz8_handle_global_errata(struct dsa_switch *ds)
>  	if (dev->info->ksz87xx_eee_link_erratum)
>  		ret = ksz8_ind_write8(dev, TABLE_EEE, REG_IND_EEE_GLOB2_HI, 0);
>  
> +	/* KSZ87xx Errata DS80000687C.
> +	 * Module 3: Equalizer fix for short cables
> +	 * The receiver of the embedded PHYs is tuned by default
> +	 * to support long cable length applications.
> +	 * Because of this, the equalizer in the PHY may amplify
> +	 * high amplitude receiver signals to the point that
> +	 * the signal is distorted internally
> +	 */
> +	if (!ret && dev->low_loss_wa_enable && ksz_is_ksz87xx(dev))
> +		ret = ksz8_handle_module3_errata(dev);
> +
>  	return ret;
>  }
>  
> 
> -- 
> 2.53.0
> 

FYI, the driver is in a restructuring process. The ksz88xx_switch_ops
will be split out of the common ksz_switch_ops. This will conflict with
your series, so you should rebase on top of that much larger set.
You can coordinate with Bastien Curutchet to see what is the status:
https://lore.kernel.org/netdev/20260313153849.qkfzv5c2u6fepjku@skbuf