[PATCH v2] spi: stm32: enable controller before asserting CS

[Leonard Göhrs]

Hi,

I am in the process of updating an STM32MP157 based device from 6.8 to 6.9
and have noticed SPI related issues that may be caused by this change.

I am testing on an LXA TAC Generation 2 (arch/arm/boot/dts/st/stm32mp157c-lxa-tac-gen2.dts)
and the issues I see are SPI transfer timeouts:

   [   13.565081] panel-mipi-dbi-spi spi2.0: SPI transfer timed out
   [   13.565131] spi_master spi2: failed to transfer one message from queue
   [   13.565134] spi_stm32 44005000.spi: spurious IT (sr=0x00010002, ier=0x00000000)
   [   13.565145] spi_master spi2: noqueue transfer failed
   [   13.565183] panel-mipi-dbi-spi spi2.0: error -110 when sending command 0x2a
   [   13.769113] panel-mipi-dbi-spi spi2.0: SPI transfer timed out
   [   13.769163] spi_master spi2: failed to transfer one message from queue
   [   13.769164] spi_stm32 44005000.spi: spurious IT (sr=0x00010002, ier=0x00000000)
   [   13.769177] spi_master spi2: noqueue transfer failed
   [   13.769210] panel-mipi-dbi-spi spi2.0: error -110 when sending command 0x2b
   [   13.977028] panel-mipi-dbi-spi spi2.0: SPI transfer timed out
   [   13.977082] spi_master spi2: failed to transfer one message from queue
   [   13.977095] spi_master spi2: noqueue transfer failed
   [   14.460924] panel-mipi-dbi-spi spi2.0: SPI transfer timed out

Followed by workqueue lockups and the device becoming unresponsive later
in the boot, preventing me from logging in and investigating further that way:

   [   17.026263] spi_master spi2: noqueue transfer failed

   TAC OS - The LXA TAC operating system 24.04+dev lxatac-00011 ttySTM0

   lxatac-00011 login: root
   [   62.434326] BUG: workqueue lockup - pool cpus=0 node=0 flags=0x0 nice=0 stuck for 44s!
   [   62.441321] BUG: workqueue lockup - pool cpus=0 node=0 flags=0x0 nice=-20 stuck for 44s!
   …

Reverting this commit fixes the issue for me. It may be some time before
I get around to investigating the issue in detail, so I thought I should
ask if anyone else has already noticed this as well.

We are currently in the process of adding the device in question to
KernelCI [1], which may help in catching such problems earlier.

[1]: https://github.com/kernelci/kernelci-core/pull/2542


On 24.04.24 15:52, Ben Wolsieffer wrote:
> On the STM32F4/7, the MOSI and CLK pins float while the controller is
> disabled. CS is a regular GPIO, and therefore always driven. Currently,
> the controller is enabled in the transfer_one() callback, which runs
> after CS is asserted.  Therefore, there is a period where the SPI pins
> are floating while CS is asserted, making it possible for stray signals
> to disrupt communications. An analogous problem occurs at the end of the
> transfer when the controller is disabled before CS is released.
> 
> This problem can be reliably observed by enabling the pull-up (if
> CPOL=0) or pull-down (if CPOL=1) on the clock pin. This will cause two
> extra unintended clock edges per transfer, when the controller is
> enabled and disabled.
> 
> Note that this bug is likely not present on the STM32H7, because this
> driver sets the AFCNTR bit (not supported on F4/F7), which keeps the SPI
> pins driven even while the controller is disabled.
> 
> Enabling/disabling the controller as part of runtime PM was suggested as
> an alternative approach, but this breaks the driver on the STM32MP1 (see
> [1]). The following quote from the manual may explain this:
> 
>> To restart the internal state machine properly, SPI is strongly
>> suggested to be disabled and re-enabled before next transaction starts
>> despite its setting is not changed.
> 
> This patch has been tested on an STM32F746 with a MAX14830 UART
> expander.
> 
> [1] https://lore.kernel.org/lkml/ZXzRi_h2AMqEhMVw@dell-precision-5540/T/
> 
> Signed-off-by: Ben Wolsieffer <ben.wolsieffer at hefring.com>
> ---
> v2:
>   * Improve explanation of problem
>   * Discuss why not to use runtime PM instead
> 
>   drivers/spi/spi-stm32.c | 14 ++------------
>   1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c
> index e4e7ddb7524a..4a68abcdcc35 100644
> --- a/drivers/spi/spi-stm32.c
> +++ b/drivers/spi/spi-stm32.c
> @@ -1016,10 +1016,8 @@ static irqreturn_t stm32fx_spi_irq_event(int irq, void *dev_id)
>   static irqreturn_t stm32fx_spi_irq_thread(int irq, void *dev_id)
>   {
>   	struct spi_controller *ctrl = dev_id;
> -	struct stm32_spi *spi = spi_controller_get_devdata(ctrl);
>   
>   	spi_finalize_current_transfer(ctrl);
> -	stm32fx_spi_disable(spi);
>   
>   	return IRQ_HANDLED;
>   }
> @@ -1187,6 +1185,8 @@ static int stm32_spi_prepare_msg(struct spi_controller *ctrl,
>   			 ~clrb) | setb,
>   			spi->base + spi->cfg->regs->cpol.reg);
>   
> +	stm32_spi_enable(spi);
> +
>   	spin_unlock_irqrestore(&spi->lock, flags);
>   
>   	return 0;
> @@ -1204,7 +1204,6 @@ static void stm32fx_spi_dma_tx_cb(void *data)
>   
>   	if (spi->cur_comm == SPI_SIMPLEX_TX || spi->cur_comm == SPI_3WIRE_TX) {
>   		spi_finalize_current_transfer(spi->ctrl);
> -		stm32fx_spi_disable(spi);
>   	}
>   }
>   
> @@ -1219,7 +1218,6 @@ static void stm32_spi_dma_rx_cb(void *data)
>   	struct stm32_spi *spi = data;
>   
>   	spi_finalize_current_transfer(spi->ctrl);
> -	spi->cfg->disable(spi);
>   }
>   
>   /**
> @@ -1307,8 +1305,6 @@ static int stm32fx_spi_transfer_one_irq(struct stm32_spi *spi)
>   
>   	stm32_spi_set_bits(spi, STM32FX_SPI_CR2, cr2);
>   
> -	stm32_spi_enable(spi);
> -
>   	/* starting data transfer when buffer is loaded */
>   	if (spi->tx_buf)
>   		spi->cfg->write_tx(spi);
> @@ -1345,8 +1341,6 @@ static int stm32h7_spi_transfer_one_irq(struct stm32_spi *spi)
>   
>   	spin_lock_irqsave(&spi->lock, flags);
>   
> -	stm32_spi_enable(spi);
> -
>   	/* Be sure to have data in fifo before starting data transfer */
>   	if (spi->tx_buf)
>   		stm32h7_spi_write_txfifo(spi);
> @@ -1378,8 +1372,6 @@ static void stm32fx_spi_transfer_one_dma_start(struct stm32_spi *spi)
>   		 */
>   		stm32_spi_set_bits(spi, STM32FX_SPI_CR2, STM32FX_SPI_CR2_ERRIE);
>   	}
> -
> -	stm32_spi_enable(spi);
>   }
>   
>   /**
> @@ -1413,8 +1405,6 @@ static void stm32h7_spi_transfer_one_dma_start(struct stm32_spi *spi)
>   
>   	stm32_spi_set_bits(spi, STM32H7_SPI_IER, ier);
>   
> -	stm32_spi_enable(spi);
> -
>   	if (STM32_SPI_HOST_MODE(spi))
>   		stm32_spi_set_bits(spi, STM32H7_SPI_CR1, STM32H7_SPI_CR1_CSTART);
>   }