[PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Rasmus Villemoes]

The current spi-imx driver completely fails when used with more than
four (gpio) chip-selects, since the chip select number is used
unconditionally as shift amount when updating the control and
configuration registers, so the code ends up modifying random bits
outside the intended fields.

This fixes it by making use of the unused_native_cs variable filled in
by the spi core, and use that as the "channel number" for all gpiod
chip selects.

In the presumably common case where all chip selects are gpios, this
means we end up using channel 0 exclusively, so the optimization where
the config register is left alone if it is unchanged (see
184434fcd617) might become less effective, if the workload consists of
different slaves with differing spi modes being accessed one after the
other. It would be nice if one could make use of the unused native
chip selects in a round-robin manner, but for that the core would have
to tell us not just unused_native_cs, but the whole ~native_cs_mask
from spi_get_gpio_descs(). Maybe a simpler fix, if there is anything
to fix, is to make the new mx51_ecspi_channel() do

	if (!spi->cs_gpiod || spi->controller->num_chipselect <= 4)


Rasmus Villemoes (3):
  spi: spi-imx: use "controller" variable consistently in
    spi_imx_probe()
  spi: spi-imx: set max_native_cs for imx51/imx53/imx6 variants
  spi: spi-imx: fix use of more than four chipselects

 drivers/spi/spi-imx.c | 56 +++++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

-- 
2.37.2

[PATCH 1/3] spi: spi-imx: use "controller" variable consistently in spi_imx_probe()

[Rasmus Villemoes]

Near the top of the function, spi_imx->controller is set to
controller (and is of course never modified again). The rest of the
function uses a mix of the two expressions.

For consistency, readability and better code generation, drop all the
spi_imx-> indirections.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/spi/spi-imx.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index e4ccd0c329d0..6fa53a82674a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1725,20 +1725,20 @@ static int spi_imx_probe(struct platform_device *pdev)
 	else
 		controller->num_chipselect = 3;
 
-	spi_imx->controller->transfer_one = spi_imx_transfer_one;
-	spi_imx->controller->setup = spi_imx_setup;
-	spi_imx->controller->cleanup = spi_imx_cleanup;
-	spi_imx->controller->prepare_message = spi_imx_prepare_message;
-	spi_imx->controller->unprepare_message = spi_imx_unprepare_message;
-	spi_imx->controller->slave_abort = spi_imx_slave_abort;
-	spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_NO_CS;
+	controller->transfer_one = spi_imx_transfer_one;
+	controller->setup = spi_imx_setup;
+	controller->cleanup = spi_imx_cleanup;
+	controller->prepare_message = spi_imx_prepare_message;
+	controller->unprepare_message = spi_imx_unprepare_message;
+	controller->slave_abort = spi_imx_slave_abort;
+	controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_NO_CS;
 
 	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
 	    is_imx53_ecspi(spi_imx))
-		spi_imx->controller->mode_bits |= SPI_LOOP | SPI_READY;
+		controller->mode_bits |= SPI_LOOP | SPI_READY;
 
 	if (is_imx51_ecspi(spi_imx) || is_imx53_ecspi(spi_imx))
-		spi_imx->controller->mode_bits |= SPI_RX_CPHA_FLIP;
+		controller->mode_bits |= SPI_RX_CPHA_FLIP;
 
 	if (is_imx51_ecspi(spi_imx) &&
 	    device_property_read_u32(&pdev->dev, "cs-gpios", NULL))
@@ -1747,7 +1747,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 		 * setting the burst length to the word size. This is
 		 * considerably faster than manually controlling the CS.
 		 */
-		spi_imx->controller->mode_bits |= SPI_CS_WORD;
+		controller->mode_bits |= SPI_CS_WORD;
 
 	spi_imx->spi_drctl = spi_drctl;
 
-- 
2.37.2

[PATCH 2/3] spi: spi-imx: set max_native_cs for imx51/imx53/imx6 variants

[Rasmus Villemoes]

The ecspi IP block on imx51/imx53/imx6 have four native chip
selects. Tell that to the spi core so that any non-gpio chip selects
get validated against that upper bound.

Also set the SPI_MASTER_GPIO_SS so that the core verifies that, in the
case where both native and gpio chip selects are in use, there is at
least one leftover native chip select (or "channel", in the ecspi
language) for use by the slaves sitting on gpio chip selects.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/spi/spi-imx.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 6fa53a82674a..e8f7afbd9847 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1749,6 +1749,11 @@ static int spi_imx_probe(struct platform_device *pdev)
 		 */
 		controller->mode_bits |= SPI_CS_WORD;
 
+	if (is_imx51_ecspi(spi_imx) || is_imx53_ecspi(spi_imx)) {
+		controller->max_native_cs = 4;
+		controller->flags |= SPI_MASTER_GPIO_SS;
+	}
+
 	spi_imx->spi_drctl = spi_drctl;
 
 	init_completion(&spi_imx->xfer_done);
-- 
2.37.2

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

[Rasmus Villemoes]

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;
+	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

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

[Mahapatra, Amit Kumar]

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

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Rasmus Villemoes]

On 25/04/2023 15.45, Rasmus Villemoes wrote:
> The current spi-imx driver completely fails when used with more than
> four (gpio) chip-selects, since the chip select number is used
> unconditionally as shift amount when updating the control and
> configuration registers, so the code ends up modifying random bits
> outside the intended fields.
> 
> This fixes it by making use of the unused_native_cs variable filled in
> by the spi core, and use that as the "channel number" for all gpiod
> chip selects.

So I obviously hadn't seen
https://lore.kernel.org/lkml/20230318222132.3373-1-kgroeneveld@lenbrook.com/T/#u
when I sent this.

I did consider that approach, but rejected it because it wouldn't work
with mixing native and gpio chip selects. Say, somebody uses SS0
natively, but then also have four additional gpios. Then chipselect 4
would end up activating both the SS0 pin as well as the gpio, selecting
both devices.

I don't know if that's really a realistic scenario. But at least I think
the driver should then somehow have a way to indicate to the core that
one should either use native or gpio chip selects, but not a mix.

Thoughts?

Rasmus

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 961 bytes --]

On Wed, Apr 26, 2023 at 09:19:29AM +0200, Rasmus Villemoes wrote:

> I did consider that approach, but rejected it because it wouldn't work
> with mixing native and gpio chip selects. Say, somebody uses SS0
> natively, but then also have four additional gpios. Then chipselect 4
> would end up activating both the SS0 pin as well as the gpio, selecting
> both devices.

> I don't know if that's really a realistic scenario. But at least I think
> the driver should then somehow have a way to indicate to the core that
> one should either use native or gpio chip selects, but not a mix.

I'm not sure this is sensible, it'll be a fairly rare situation and we
don't want to preclude using the built in chip select functionality for
some of the chip selects.  In a situation like this we only need to have
a single chip select to be managed as a GPIO rather than all of them,
which I'd expect to end up handled in the DT by not allocating that chip
select number.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Rasmus Villemoes]

On 26/04/2023 14.25, Mark Brown wrote:
> On Wed, Apr 26, 2023 at 09:19:29AM +0200, Rasmus Villemoes wrote:
> 
>> I did consider that approach, but rejected it because it wouldn't work
>> with mixing native and gpio chip selects. Say, somebody uses SS0
>> natively, but then also have four additional gpios. Then chipselect 4
>> would end up activating both the SS0 pin as well as the gpio, selecting
>> both devices.
> 
>> I don't know if that's really a realistic scenario. But at least I think
>> the driver should then somehow have a way to indicate to the core that
>> one should either use native or gpio chip selects, but not a mix.
> 
> I'm not sure this is sensible, it'll be a fairly rare situation and we
> don't want to preclude using the built in chip select functionality for
> some of the chip selects.  In a situation like this we only need to have
> a single chip select to be managed as a GPIO rather than all of them,
> which I'd expect to end up handled in the DT by not allocating that chip
> select number.

Sorry, I don't understand what you're saying. What exactly is not
sensible? And what is "a situation like this"?

I described a problem with what is now 87c614175bbf in linux-next: If
one has five spi devices, the first four of which use the four native
chip selects, there is no way to use a gpio for the fifth, because
whichever "channel" you choose in the CHANNEL_SELECT field will cause
the ecspi IP block to drive some SSx pin low, while the spi core is also
driving the gpio low, so two different devices would be selected.

It's not exactly a regression, because any chip_select >= 4 never
actually worked, but what I'm saying is that 87c614175bbf also isn't a
complete fix if one wants to support mixing native and gpio chip
selects. For that, one really needs the unused_native_cs to be used for
all gpio chip selects; in particular, one needs some unused native cs to
exist. IOW, what my series tries to do.

[OK, now that I re-read what I wrote, I didn't exactly describe "four
native CS, one gpio", but "one native CS, four gpios". That scenario
_could_ of course work with my series, but with 87c614175bbf just
masking the chip-select number, we do get the problem that two devices
would be selected at the same time. And I don't think expecting the DT
author to know to use regs 1, 2, 3, 5 for those four gpio chip selects
is reasonable; nor do I think it would actually work, since the missing
gpio phandle at index 4 in cs-gpios would be treated by the spi core as
a "that device, if any, uses native chip select 4", and that
would/should fail.]

Rasmus

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2028 bytes --]

On Wed, Apr 26, 2023 at 02:47:44PM +0200, Rasmus Villemoes wrote:
> On 26/04/2023 14.25, Mark Brown wrote:

> > I'm not sure this is sensible, it'll be a fairly rare situation and we
> > don't want to preclude using the built in chip select functionality for
> > some of the chip selects.  In a situation like this we only need to have
> > a single chip select to be managed as a GPIO rather than all of them,
> > which I'd expect to end up handled in the DT by not allocating that chip
> > select number.

> Sorry, I don't understand what you're saying. What exactly is not
> sensible? And what is "a situation like this"?

Building hardware which uses all the native chip selects and also GPIO
ones and then describing it in DT as using native chip selects.

> I described a problem with what is now 87c614175bbf in linux-next: If
> one has five spi devices, the first four of which use the four native
> chip selects, there is no way to use a gpio for the fifth, because
> whichever "channel" you choose in the CHANNEL_SELECT field will cause
> the ecspi IP block to drive some SSx pin low, while the spi core is also
> driving the gpio low, so two different devices would be selected.

Sure, and therefore I'd not expect anyone to actually describe the
hardware like that but to instead describe the hardware as using three
or fewer of the native chip selects with the remaining chip selects
described as GPIOs.  If the device requires that a native chip select be
controlled the hardware simply won't work without at least one native
chip select being unallocated.

> It's not exactly a regression, because any chip_select >= 4 never
> actually worked, but what I'm saying is that 87c614175bbf also isn't a
> complete fix if one wants to support mixing native and gpio chip
> selects. For that, one really needs the unused_native_cs to be used for
> all gpio chip selects; in particular, one needs some unused native cs to
> exist. IOW, what my series tries to do.

No, we only need one unused chip select to be available.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Rasmus Villemoes]

On 26/04/2023 15.10, Mark Brown wrote:
> On Wed, Apr 26, 2023 at 02:47:44PM +0200, Rasmus Villemoes wrote:
>> On 26/04/2023 14.25, Mark Brown wrote:

>> I described a problem with what is now 87c614175bbf in linux-next: If
>> one has five spi devices, the first four of which use the four native
>> chip selects, there is no way to use a gpio for the fifth, because
>> whichever "channel" you choose in the CHANNEL_SELECT field will cause
>> the ecspi IP block to drive some SSx pin low, while the spi core is also
>> driving the gpio low, so two different devices would be selected.
> 
> Sure, and therefore I'd not expect anyone to actually describe the
> hardware like that but to instead describe the hardware as using three
> or fewer of the native chip selects with the remaining chip selects
> described as GPIOs.  If the device requires that a native chip select be
> controlled the hardware simply won't work without at least one native
> chip select being unallocated.

Exactly. But the current state (as of next-20230425) of the spi-imx
driver also doesn't work if one describes the hardware using between 1
and 3 native chip selects and the rest as gpios, because the naive
masking of ->chip_select could easily hit one of those native ones.

>> It's not exactly a regression, because any chip_select >= 4 never
>> actually worked, but what I'm saying is that 87c614175bbf also isn't a
>> complete fix if one wants to support mixing native and gpio chip
>> selects. For that, one really needs the unused_native_cs to be used for
>> all gpio chip selects; in particular, one needs some unused native cs to
>> exist. IOW, what my series tries to do.
> 
> No, we only need one unused chip select to be available.

Which is exactly what I'm saying, so I think we're in agreement.

I.e., something like this 3-patch series is needed to actually support
mixing native and gpio chip selects (having the core verify that there
is an unused chip select available, and provide that in the
->unused_native_cs field in the spi_controller). I don't think there's
any textual conflict with 87c614175bbf, and the masking done by
87c614175bbf doesn't hurt, but also becomes irrelevant if this series is
applied, since we'd never pass any value > 3 to those macros.

Rasmus

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Rasmus Villemoes]

On 26/04/2023 15.23, Rasmus Villemoes wrote:

>>> It's not exactly a regression, because any chip_select >= 4 never
>>> actually worked, but what I'm saying is that 87c614175bbf also isn't a
>>> complete fix if one wants to support mixing native and gpio chip
>>> selects. For that, one really needs the unused_native_cs to be used for
>>> all gpio chip selects; in particular, one needs some unused native cs to
>>> exist. IOW, what my series tries to do.
>>
>> No, we only need one unused chip select to be available.
> 
> Which is exactly what I'm saying, so I think we're in agreement.
> 
> I.e., something like this 3-patch series is needed to actually support
> mixing native and gpio chip selects (having the core verify that there
> is an unused chip select available, and provide that in the
> ->unused_native_cs field in the spi_controller). I don't think there's
> any textual conflict with 87c614175bbf, and the masking done by
> 87c614175bbf doesn't hurt, but also becomes irrelevant if this series is
> applied, since we'd never pass any value > 3 to those macros.

So, what's the conclusion here? Will these three patches be applied, or
will we just live with the status as of next-20230516, namely that

* for up to four slaves, any combination of native and gpio chip select
works

* with more then four slaves, CSn and CS(n&3) must be be gpios for all n
>= 4

?

Rasmus

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

On Tue, May 16, 2023 at 01:43:23PM +0200, Rasmus Villemoes wrote:

> So, what's the conclusion here? Will these three patches be applied, or
> will we just live with the status as of next-20230516, namely that

As pointed out by Amit this will need a resend against current code.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] spi: spi-imx: fix use of more than four chip selects

[Mark Brown]

On Tue, 25 Apr 2023 15:45:24 +0200, Rasmus Villemoes wrote:
> The current spi-imx driver completely fails when used with more than
> four (gpio) chip-selects, since the chip select number is used
> unconditionally as shift amount when updating the control and
> configuration registers, so the code ends up modifying random bits
> outside the intended fields.
> 
> This fixes it by making use of the unused_native_cs variable filled in
> by the spi core, and use that as the "channel number" for all gpiod
> chip selects.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: spi-imx: use "controller" variable consistently in spi_imx_probe()
      commit: d9032b304541e1f560349e461611f25d67f44a49
[2/3] spi: spi-imx: set max_native_cs for imx51/imx53/imx6 variants
      commit: 8ce1bb9a5935385e9ef65bda1e8ca923c7fbb887
[3/3] spi: spi-imx: fix use of more than four chipselects
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark