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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

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