[PATCH 3/3] spi: spi-imx: fix use of more than four chipselects
Mahapatra, Amit Kumar
amit.kumar-mahapatra at amd.com
Tue May 16 05:07:22 PDT 2023
Hello,
> -----Original Message-----
> From: Rasmus Villemoes <linux at rasmusvillemoes.dk>
> Sent: Tuesday, April 25, 2023 7:15 PM
> To: Mark Brown <broonie at kernel.org>; Shawn Guo <shawnguo at kernel.org>;
> Sascha Hauer <s.hauer at pengutronix.de>; Pengutronix Kernel Team
> <kernel at pengutronix.de>; Fabio Estevam <festevam at gmail.com>; NXP Linux
> Team <linux-imx at nxp.com>
> Cc: Marc Kleine-Budde <mkl at pengutronix.de>; Rasmus Villemoes
> <linux at rasmusvillemoes.dk>; linux-spi at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> 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 at rasmusvillemoes.dk>
> ---
> 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
More information about the linux-arm-kernel
mailing list