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

[Marek Vasut]

On 5/5/26 1:42 PM, Fidelio Lawson wrote:

[...]

> @@ -2096,11 +2141,39 @@ int ksz8463_w_phy(struct ksz_device *dev, u16 phy, u16 reg, u16 val)
>   	return 0;
>   }

Nitpick, either turn the "Apply the Microchip..." code comment into a 
kerneldoc for this function, or use netdev style multi-line comment.

> +int ksz87xx_apply_low_loss_preset(struct ksz_device *dev, bool enable)
> +{
> +	/* Apply the Microchip erratum short-cable preset (LPF 62 MHz, EQ init 0) */
> +	/* providing a conservative configuration for short or low-loss cables. */
> +	u8 lpf_bw, eq_init;
> +	int ret;
> +
> +	lpf_bw = KSZ87XX_PHY_LPF_62MHZ;
> +	eq_init = KSZ87XX_DSP_EQ_INIT_LOW_LOSS;
> +
> +	if (!ksz_is_ksz87xx(dev))
> +		return -EOPNOTSUPP;

Please add newline here.

> +	if (!enable)
> +		return 0;

Please add newline here.

> +	ret = ksz8_ind_write8(dev, TABLE_LINK_MD, KSZ87XX_REG_PHY_LPF, lpf_bw);
> +	if (ret)
> +		return ret;

Please add newline here.

> +	dev->lpf_bw = lpf_bw;
> +	ret = ksz8_ind_write8(dev, TABLE_LINK_MD, KSZ87XX_REG_DSP_EQ, eq_init);
> +	if (ret)
> +		return ret;

Please add newline here.

> +	dev->eq_init = eq_init;
> +
> +	return ret;
> +}
> +

[...]

> diff --git a/drivers/net/dsa/microchip/ksz8_reg.h b/drivers/net/dsa/microchip/ksz8_reg.h
> index 332408567b47..cd41214f874e 100644
> --- a/drivers/net/dsa/microchip/ksz8_reg.h
> +++ b/drivers/net/dsa/microchip/ksz8_reg.h

[...]

> @@ -729,6 +736,21 @@
>   #define PHY_POWER_SAVING_ENABLE		BIT(2)
>   #define PHY_REMOTE_LOOPBACK		BIT(1)
>   
> +/* Vendor-specific Clause 22 PHY registers (virtualized) */
> +#define PHY_REG_KSZ87XX_SHORT_CABLE		0x1A
> +#define PHY_REG_KSZ87XX_LPF_BW			0x1B
> +#define PHY_REG_KSZ87XX_EQ_INIT			0x1C
> +
> +/* LPF bandwidth bits [7:6]: 00 = 90MHz (default), 01 = 62MHz, 10 = 55MHz, 11 = 44MHz  */
> +#define KSZ87XX_PHY_LPF_90MHZ          0x00
> +#define KSZ87XX_PHY_LPF_62MHZ          0x40
> +#define KSZ87XX_PHY_LPF_55MHZ          0x80
> +#define KSZ87XX_PHY_LPF_44MHZ          0xC0

You could use GENMASK_U32() and FIELD_PREP() for these bitfields:

#define KSZ87XX_PHY_LPF_MASK     GENMASK(7, 6)
#define KSZ87XX_PHY_LPF_90MHZ    FIELD_PREP(KSZ87XX_PHY_LPF_MASK, 0)
#define KSZ87XX_PHY_LPF_62MHZ    FIELD_PREP(KSZ87XX_PHY_LPF_MASK, 1)
...

[...]