Re: [PATCH net] net: dsa: microchip: Select SPI_MODE 0 for KSZ8463

[Bastien Curutchet]

Hi Tristram

On 9/11/25 12:10 AM, Tristram.Ha@xxxxxxxxxxxxx wrote:
> > KSZ8463 expects the SPI clock to be low on idle and samples data on
> > rising edges. This fits SPI mode 0 (CPOL = 0 / CPHA = 0) but the SPI
> > mode is set to 3 for all the switches supported by the driver. This
> > can lead to invalid read/write on the SPI bus.
> >
> > Set SPI mode to 0 for the KSZ8463.
> > Leave SPI mode 3 as default for the other switches.
> >
> > Signed-off-by: Bastien Curutchet (Schneider Electric)
> > <bastien.curutchet@xxxxxxxxxxx>
> > Fixes: 84c47bfc5b3b ("net: dsa: microchip: Add KSZ8463 switch support to KSZ DSA
> > driver")
> > ---
> >   drivers/net/dsa/microchip/ksz_spi.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/dsa/microchip/ksz_spi.c
> > b/drivers/net/dsa/microchip/ksz_spi.c
> > index
> > d8001734b05741446fa78a1e88c2f82e894835ce..dcc0dbddf7b9d70fbfb31d4b260b80
> > ca78a65975 100644
> > --- a/drivers/net/dsa/microchip/ksz_spi.c
> > +++ b/drivers/net/dsa/microchip/ksz_spi.c
> > @@ -139,6 +139,7 @@ static int ksz_spi_probe(struct spi_device *spi)
> >          const struct regmap_config *regmap_config;
> >          const struct ksz_chip_data *chip;
> >          struct device *ddev = &spi->dev;
> > +       u32 spi_mode = SPI_MODE_3;
> >          struct regmap_config rc;
> >          struct ksz_device *dev;
> >          int i, ret = 0;
> > @@ -155,8 +156,10 @@ static int ksz_spi_probe(struct spi_device *spi)
> >          dev->chip_id = chip->chip_id;
> >          if (chip->chip_id == KSZ88X3_CHIP_ID)
> >                  regmap_config = ksz8863_regmap_config;
> > -       else if (chip->chip_id == KSZ8463_CHIP_ID)
> > +       else if (chip->chip_id == KSZ8463_CHIP_ID) {
> >                  regmap_config = ksz8463_regmap_config;
> > +               spi_mode = SPI_MODE_0;
> > +       }
> >          else if (chip->chip_id == KSZ8795_CHIP_ID ||
> >                   chip->chip_id == KSZ8794_CHIP_ID ||
> >                   chip->chip_id == KSZ8765_CHIP_ID)
> > @@ -185,7 +188,7 @@ static int ksz_spi_probe(struct spi_device *spi)
> >                  dev->pdata = spi->dev.platform_data;
> >
> >          /* setup spi */
> > -       spi->mode = SPI_MODE_3;
> > +       spi->mode = spi_mode;
> >          ret = spi_setup(spi);
> >          if (ret)
> >                  return ret;
> >
> > ---
> > base-commit: c65e2aee8971eb9d4bc2b8edc3a3a62dc98f0410
> > change-id: 20250910-fix-omap-spi-d7c64f2416df
>
> Actually it is best to completely remove the code.  The SPI mode should
> be dictated by spi-cpol and spi-cpha settings in the device tree.  I do
> not know why that code was there from the beginning.

Ok, I didn't know these settings were available on the device-tree, I 
can remove the spi->mode setting in a new patch.

> All KSZ switches can use SPI mode 0 and 3, and 3 is recommended for high
> SPI frequency.  Sometimes a bug/quirk in the SPI bus driver prevents the
> very first SPI transfer to be successful in mode 3 because of a missed
> rising edge clock signal, so it is forced to use mode 0.  (The Atmel SPI
> bus driver has this issue in some old kernel versions.)
>
> As for KSZ8463 I have always used mode 3 and do not know of any issue of
> using that mode.

I have issues on the first transfer with the AM335x's spi-omap2-mcspi 
driver. I first tried to fix this driver but since the KSZ8463's 
datasheet explicitly mentions that it expects the CLK to be low at idle, 
I thought this was the right fix.

But I'll fix the SPI driver then, thanks.


Best regards,
--
Bastien Curutchet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com