RE: [PATCH 3/3] spi: spi-imx: fix use of more than four chipselects

["Mahapatra, Amit Kumar" <amit.kumar-mahapatra@xxxxxxx>]

Hello,

> -----Original Message-----
> From: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, April 25, 2023 7:15 PM
> To: Mark Brown <broonie@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>;
> Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; NXP Linux
> Team <linux-imx@xxxxxxx>
> Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>; Rasmus Villemoes
> <linux@xxxxxxxxxxxxxxxxxx>; linux-spi@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: [PATCH 3/3] spi: spi-imx: fix use of more than four chipselects
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> Currently, the spi->chip_select is used unconditionally in code such as
> 
>         /* set chip select to use */
>         ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
> 
> and
> 
>         if (spi->mode & SPI_CPHA)
>                 cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
>         else
>                 cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> 
> with these macros being
> 
>         #define MX51_ECSPI_CTRL_CS(cs)          ((cs) << 18)
>         #define MX51_ECSPI_CONFIG_SCLKPHA(cs)   (1 << ((cs) +  0))
> 
> However, the CHANNEL_SELECT field in the control register is only two bits
> wide, so when spi->chip_select >= 4, we end up writing garbage into the
> BURST_LENGTH field. Similarly, there are only four bits in the SCLK_PHA field,
> so the code above ends up actually modifying bits in the SCLK_POL (or higher)
> field.
> 
> The scrambling of the BURST_LENGTH field itself is probably benign, since
> that is explicitly completely initialized later, in
> ->prepare_transfer.
> 
> But, since we effectively write (spi->chip_select & 3) into the
> CHANNEL_SELECT field, that value is what the IP block then uses to determine
> which bits of the configuration register control phase, polarity etc., and those
> bits are not properly initialized, so communication with the spi device
> completely fails.
> 
> Fix this by using the ->unused_native_cs value as channel number for any spi
> device which uses a gpio as chip select.
> 
> Signed-off-by: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/spi/spi-imx.c | 31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> e8f7afbd9847..569a5132f324 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -504,6 +504,13 @@ static void mx51_ecspi_disable(struct spi_imx_data
> *spi_imx)
>         writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);  }
> 
> +static int mx51_ecspi_channel(const struct spi_device *spi) {
> +       if (!spi->cs_gpiod)
> +               return spi->chip_select;

New set/get APIs for accessing spi->chip_select and spi->cs_gpiod were introduced by
https://github.com/torvalds/linux/commit/303feb3cc06ac0665d0ee9c1414941200e60e8a3
please use these APIs instead of accessing spi->chip_select & spi->cs_gpiod directly.

Regards,
Amit

> +       return spi->controller->unused_native_cs;
> +}
> +
>  static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
>                                       struct spi_message *msg)  { @@ -514,6 +521,7 @@
> static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
>         u32 testreg, delay;
>         u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
>         u32 current_cfg = cfg;
> +       int channel = mx51_ecspi_channel(spi);
> 
>         /* set Master or Slave mode */
>         if (spi_imx->slave_mode)
> @@ -528,7 +536,7 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
>                 ctrl |= MX51_ECSPI_CTRL_DRCTL(spi_imx->spi_drctl);
> 
>         /* set chip select to use */
> -       ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select);
> +       ctrl |= MX51_ECSPI_CTRL_CS(channel);
> 
>         /*
>          * The ctrl register must be written first, with the EN bit set other @@ -
> 549,22 +557,22 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
>          * BURST_LENGTH + 1 bits are received
>          */
>         if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
> -               cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
> +               cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(channel);
>         else
> -               cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select);
> +               cfg |= MX51_ECSPI_CONFIG_SBBCTRL(channel);
> 
>         if (spi->mode & SPI_CPOL) {
> -               cfg |= MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
> -               cfg |= MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
> +               cfg |= MX51_ECSPI_CONFIG_SCLKPOL(channel);
> +               cfg |= MX51_ECSPI_CONFIG_SCLKCTL(channel);
>         } else {
> -               cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select);
> -               cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select);
> +               cfg &= ~MX51_ECSPI_CONFIG_SCLKPOL(channel);
> +               cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(channel);
>         }
> 
>         if (spi->mode & SPI_CS_HIGH)
> -               cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
> +               cfg |= MX51_ECSPI_CONFIG_SSBPOL(channel);
>         else
> -               cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select);
> +               cfg &= ~MX51_ECSPI_CONFIG_SSBPOL(channel);
> 
>         if (cfg == current_cfg)
>                 return 0;
> @@ -609,14 +617,15 @@ static void mx51_configure_cpha(struct
> spi_imx_data *spi_imx,
>         bool cpha = (spi->mode & SPI_CPHA);
>         bool flip_cpha = (spi->mode & SPI_RX_CPHA_FLIP) && spi_imx->rx_only;
>         u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> +       int channel = mx51_ecspi_channel(spi);
> 
>         /* Flip cpha logical value iff flip_cpha */
>         cpha ^= flip_cpha;
> 
>         if (cpha)
> -               cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> +               cfg |= MX51_ECSPI_CONFIG_SCLKPHA(channel);
>         else
> -               cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select);
> +               cfg &= ~MX51_ECSPI_CONFIG_SCLKPHA(channel);
> 
>         writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);  }
> --
> 2.37.2