lkml.org 
[lkml]   [2023]   [May]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH 3/3] spi: spi-imx: fix use of more than four chipselects
Date
Hello,

> -----Original Message-----
> From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Sent: Tuesday, April 25, 2023 7:15 PM
> To: Mark Brown <broonie@kernel.org>; Shawn Guo <shawnguo@kernel.org>;
> Sascha Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; NXP Linux
> Team <linux-imx@nxp.com>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>; Rasmus Villemoes
> <linux@rasmusvillemoes.dk>; linux-spi@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@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@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

\
 
 \ /
  Last update: 2023-05-16 14:08    [W:0.138 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site