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

[Fidelio LAWSON]

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

Hi Vladimir,
Thanks for the review and for the detailed feedback!
I’ll apply your suggestions in the next version of the patch.
Thanks again for the guidance.

Best regards,
Fidelio