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

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

[Bastien Curutchet]

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@bootlin.com>
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..dcc0dbddf7b9d70fbfb31d4b260b80ca78a65975 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

Best regards,
-- 
Bastien Curutchet <bastien.curutchet@bootlin.com>

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

[Tristram.Ha]

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

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.

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@microchip.com 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@bootlin.com>
>> 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

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

[Tristram.Ha]

> On 9/11/25 12:10 AM, Tristram.Ha@microchip.com 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@bootlin.com>
> >> 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.

It seems you may have the same issue I faced before.  A workaround in the
SPI bus driver may avoid the problem, but the change happens in spi.c, so
it may not be a good solution.

Your best bet is to remove the spi_setup call and use mode 0 in the
device tree.

Another quick fix is to do a dummy write in the DSA driver before reading
and checking the chip id.