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

[Bastien Curutchet]

Hi Vladimir, Hi Fidelio

On 3/26/26 10:42 AM, Vladimir Oltean wrote:
> 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

Indeed, I'm currently polishing a first iteration for it. I'll send it 
ASAP.


Best regards,
Bastien