Re: [PATCH net-next v4 4/7] net: dsa: microchip: Use different registers for KSZ8463

[Simon Horman <horms@kernel.org>]

On Sun, Jul 20, 2025 at 11:17:03AM +0100, Simon Horman wrote:
> On Fri, Jul 18, 2025 at 06:21:03PM -0700, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.com>
> > 
> > KSZ8463 does not use same set of registers as KSZ8863 so it is necessary
> > to change some registers when using KSZ8463.
> > 
> > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> > ---
> > v3
> > - Replace cpu_to_be16() with swab16() to avoid compiler warning
> 
> ...
> 
> > diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> 
> ...
> 
> > @@ -2980,10 +2981,15 @@ static int ksz_setup(struct dsa_switch *ds)
> >  	}
> >  
> >  	/* set broadcast storm protection 10% rate */
> > -	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
> > -			   BROADCAST_STORM_RATE,
> > -			   (BROADCAST_STORM_VALUE *
> > -			   BROADCAST_STORM_PROT_RATE) / 100);
> > +	storm_mask = BROADCAST_STORM_RATE;
> > +	storm_rate = (BROADCAST_STORM_VALUE * BROADCAST_STORM_PROT_RATE) / 100;
> > +	if (ksz_is_ksz8463(dev)) {
> > +		storm_mask = swab16(storm_mask);
> > +		storm_rate = swab16(storm_rate);
> > +	}
> > +	regmap_update_bits(ksz_regmap_16(dev),
> > +			   reg16(dev, regs[S_BROADCAST_CTRL]),
> > +			   storm_mask, storm_rate);
> 
> Hi Tristram,
> 
> I am confused by the use of swab16() here.
> 
> Let us say that we are running on a little endian host (likely).
> Then the effect of this is to pass big endian values to regmap_update_bits().
> 
> But if we are running on a big endian host, the opposite will be true:
> little endian values will be passed to regmap_update_bits().
> 
> 
> Looking at KSZ_REGMAP_ENTRY() I see:
> 
> #define KSZ_REGMAP_ENTRY(width, swp, regbits, regpad, regalign)         \
>         {                                                               \
> 		...
>                 .reg_format_endian = REGMAP_ENDIAN_BIG,                 \
>                 .val_format_endian = REGMAP_ENDIAN_BIG                  \
>         }

Update; I now see this in another patch of the series:

+#define KSZ8463_REGMAP_ENTRY(width, swp, regbits, regpad, regalign)    \
+       {                                                               \
		...
+               .reg_format_endian = REGMAP_ENDIAN_BIG,                 \
+               .val_format_endian = REGMAP_ENDIAN_LITTLE               \
+       }

Which I understand to mean that the hardware is expecting little endian
values. But still, my concerns raised in my previous email of this
thread remain.

And I have a question: does this chip use little endian register values
whereas other chips used big endian register values?

> 
> Which based on a skimming the regmap code implies to me that
> regmap_update_bits() should be passed host byte order values
> which regmap will convert to big endian when writing out
> these values.
> 
> It is unclear to me why changing the byte order of storm_mask
> and storm_rate is needed here. But it does seem clear that
> it will lead to inconsistent results on big endian and little
> endian hosts.
> 
> ...
>