Message-ID:
 <DM3PR11MB87360DB5CDD47DF4A64FC33BEC59A@DM3PR11MB8736.namprd11.prod.outlook.com>
Date: Fri, 25 Jul 2025 00:17:26 +0000
From: <Tristram.Ha@...rochip.com>
To: <horms@...nel.org>
CC: <Woojung.Huh@...rochip.com>, <andrew@...n.ch>, <olteanv@...il.com>,
	<kuba@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
	<conor+dt@...nel.org>, <maxime.chevallier@...tlin.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<marex@...x.de>, <UNGLinuxDriver@...rochip.com>,
	<devicetree@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next v4 4/7] net: dsa: microchip: Use different
 registers for KSZ8463

> On Thu, Jul 24, 2025 at 02:28:56AM +0000, Tristram.Ha@...rochip.com wrote:
> > > On Wed, Jul 23, 2025 at 02:25:27AM +0000, Tristram.Ha@...rochip.com wrote:
> > > > > 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@...rochip.com
> wrote:
> > > > > > > From: Tristram Ha <tristram.ha@...rochip.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@...rochip.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.
> > > >
> > > > The broadcast storm value 0x7ff is stored in registers 6 and 7 in KSZ8863
> > > > where register 6 holds the 0x7 part while register 7 holds the 0xff part.
> > > > In KSZ8463 register 6 is defined as 16-bit where the 0x7 part is held in
> > > > lower byte and the 0xff part is held in higher byte.  It is necessary to
> > > > swap the bytes when the value is passed to the 16-bit write function.
> > >
> > > Perhaps naively, I would have expected
> > >
> > >         .val_format_endian = REGMAP_ENDIAN_LITTLE
> > >
> > > to handle writing the 16-bit value 0x7ff such that 0x7 is in
> > > the lower byte, while 0xff is in the upper byte. Is that not the case?
> > >
> > > If not, do you get the desired result by removing the swab16() calls
> > > and using
> > >
> > >         .val_format_endian = REGMAP_ENDIAN_BIG
> > >
> > > But perhaps I misunderstand how .val_format_endian works.
> > >
> > > >
> > > > All other KSZ switches use 8-bit access with automatic address increase
> > > > so a write to register 0 with value 0x12345678 means 0=0x12, 1=0x34,
> > > > 2=0x56, and 3=0x78.
> >
> > It is not about big-endian or little-endian.  It is just the presentation
> > of this register is different between KSZ8863 and KSZ8463.  KSZ8863 uses
> > big-endian for register value as the access is 8-bit and the address is
> > automatically increased by 1.  Writing a value 0x03ff to register 6 means
> > 6=0x03 and 7=0xff.  The actual SPI transfer commands are "02 06 03 ff."
> > KSZ8463 uses little-endian for register value as the access is fixed at
> > 8-bit, 16-bit, or 32-bit.  Writing 0x03ff results in the actual SPI
> > transfer commands "80 70 ff 03" where the correct commands are
> > "80 70 03 ff."
> 
> The difference between expressing a 16-bit value as "ff 03" and "03 ff"
> sounds a lot like endianness to me.
> 
> "ff 03" is the little endian representation of 0x3ff.
> "03 ff" is the big endian representation of 0x3ff.
> 
> I am very confused as to why you say "KSZ8463 uses little-endian for
> register value". And then go on to say that the correct transfer command is
> "02 06 03 ff", where the value in that command is "03 ff." That looks like
> a big endian value to me.
> 
> 
> In my reading of your code, it takes a host byte order value, and swapping
> it's byte order. It is then passing it to an API that expects a host byte
> order value. I think it would be much better to avoid doing that. This is
> my main point.
> 
> Let us consider the (likely) case that the host is little endian.  The
> value (and mask) are byte swapped, becoming big endian.  Thisbig endian
> value (and mask) is passed to regmap_update_bits().
> 
> Now let us assume that, because REGMAP_ENDIAN_LITTLE is used,
> they then pass through something like cpu_to_le16().
> That's a noop on a little endian system. So the value remains big endian.
> 
> Next, let us consider a big endian host.
> The value (and mask) are byte swapped, becoming little endian.
> This little endian value (and mask) is passed to regmap_update_bits().
> 
> Then, let us assume that, because REGMAP_ENDIAN_LITTLE is used,
> they then pass through something like cpu_to_le16().
> This is a byte-swap on big endian hosts.
> So the value (and mask) become big endian.
> 
> The result turns out to be the same for little endian and big endian hosts,
> which is nice. But now let us assume that instead of passing byte-swapped
> values to APIs that expect host byte order values, we instead pass host
> byte order values and use REGMAP_ENDIAN_BIG.
> 
> In this case the host byte order values are passed to regmap_update_bits().
> Then, as per our earlier assumptions, because REGMAP_ENDIAN_BIG is used,
> the value (and mask) pass through cpu_to_be16 or similar. After which they
> are big endian. The same result as above. But without passing byte-swapped
> values to APIs that expect host byte order values.
> 
> Is my understanding of the effect of REGMAP_ENDIAN_LITTLE and
> REGMAP_ENDIAN_BIG incorrect? Is some other part of my reasoning faulty?
> 
> 
> I feel that we are talking past each other.
> Let's try to find a common understanding.

It is really about the register definition of this specific register.
In KSZ8863 when presenting in 16-bit the value is 0x07ff, but in KSZ8463
it is 0xff07.  It is the fault of the hardware to define such value.

Note that in the new patch KSZ8463 SPI driver implements its own access
functions, so native mode is used instead and there is no automatic
swapping depending on the big-endian or little-endian format.  Still this
code is needed to program the register correctly.

Powered by blists - more mailing lists