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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

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@exotec.com>
>> ---
>>   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