| Message ID | 20231201214014.2539031-1-ben.wolsieffer@hefring.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: stm32: enable controller before asserting CS | expand |
On Fri, Dec 01, 2023 at 04:40:14PM -0500, Ben Wolsieffer wrote: > This patch fixes the bug by enabling the controller in prepare_message() > and disabling it in unprepare_message(), which are called while CS is > not asserted. This feels like it'd be a good fit for moving to runtime PM - that way we avoid bouncing the controller on and off between messages which is probably better anyway. The driver already does pinctrl management for the device there. > Note that bug is likely not present on the STM32H7, because it supports > the AFCNTR bit (and this driver sets it), which keeps the SPI pins > driven even while the controller is disabled. It also occurs to me that this isn't going to work for devices which chip select inverted - for them we can't stop driving chip select at all since they need it held high when idle. There aren't that many such devices and it'd loose us the PM which is rather awkward... I guess that's an incremental issue with a more invasive fix though.
On Fri, Dec 01, 2023 at 09:50:33PM +0000, Mark Brown wrote: > On Fri, Dec 01, 2023 at 04:40:14PM -0500, Ben Wolsieffer wrote: > > This feels like it'd be a good fit for moving to runtime PM - that way > we avoid bouncing the controller on and off between messages which is > probably better anyway. The driver already does pinctrl management for > the device there. Yes, that probably makes sense. There are a few bits that can only be configured while the controller is disabled, but it doesn't look like that applies to any of the ones set in stm32_spi_prepare_msg(). I'm a little hesitant to make big changes to the driver since I can only test them on an STM32F7 though. > It also occurs to me that this isn't going to work for devices which > chip select inverted - for them we can't stop driving chip select at all > since they need it held high when idle. There aren't that many such > devices and it'd loose us the PM which is rather awkward... I guess > that's an incremental issue with a more invasive fix though. The driver only supports GPIO chip select rather than native, so I don't think this is a problem. Also, I don't think there's any difference between inverted or uninverted here. They both either need to be driven all the time or have pull-up/downs. Ben
On Fri, Dec 01, 2023 at 06:11:36PM -0500, Ben Wolsieffer wrote: > On Fri, Dec 01, 2023 at 09:50:33PM +0000, Mark Brown wrote: > > On Fri, Dec 01, 2023 at 04:40:14PM -0500, Ben Wolsieffer wrote: > > This feels like it'd be a good fit for moving to runtime PM - that way > > we avoid bouncing the controller on and off between messages which is > > probably better anyway. The driver already does pinctrl management for > > the device there. > Yes, that probably makes sense. There are a few bits that can only be > configured while the controller is disabled, but it doesn't look like > that applies to any of the ones set in stm32_spi_prepare_msg(). > I'm a little hesitant to make big changes to the driver since I can only > test them on an STM32F7 though. It doesn't seem much more complex than what you're already proposing. > > It also occurs to me that this isn't going to work for devices which > > chip select inverted - for them we can't stop driving chip select at all > > since they need it held high when idle. There aren't that many such > > devices and it'd loose us the PM which is rather awkward... I guess > > that's an incremental issue with a more invasive fix though. > The driver only supports GPIO chip select rather than native, so I don't > think this is a problem. Also, I don't think there's any difference So mentioning the drive seems a bit confusing then? > between inverted or uninverted here. They both either need to be driven > all the time or have pull-up/downs. It's a lot more likely you'll get away with things one way or another for a missing pull down.
On Mon, Dec 04, 2023 at 12:43:42PM +0000, Mark Brown wrote: > On Fri, Dec 01, 2023 at 06:11:36PM -0500, Ben Wolsieffer wrote: > > On Fri, Dec 01, 2023 at 09:50:33PM +0000, Mark Brown wrote: > > > On Fri, Dec 01, 2023 at 04:40:14PM -0500, Ben Wolsieffer wrote: > > > > This feels like it'd be a good fit for moving to runtime PM - that way > > > we avoid bouncing the controller on and off between messages which is > > > probably better anyway. The driver already does pinctrl management for > > > the device there. > > > Yes, that probably makes sense. There are a few bits that can only be > > configured while the controller is disabled, but it doesn't look like > > that applies to any of the ones set in stm32_spi_prepare_msg(). > > > I'm a little hesitant to make big changes to the driver since I can only > > test them on an STM32F7 though. > > It doesn't seem much more complex than what you're already proposing. I'm working on a new patch that uses runtime PM and will submit it soon. > > > It also occurs to me that this isn't going to work for devices which > > > chip select inverted - for them we can't stop driving chip select at all > > > since they need it held high when idle. There aren't that many such > > > devices and it'd loose us the PM which is rather awkward... I guess > > > that's an incremental issue with a more invasive fix though. > > > The driver only supports GPIO chip select rather than native, so I don't > > think this is a problem. Also, I don't think there's any difference > > So mentioning the drive seems a bit confusing then? Yes, I should have been more specific in the patch that only MOSI and CLK float when the controller is disabled and that CS remains driven.
diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c index 94df3836834c..885f53a51441 100644 --- a/drivers/spi/spi-stm32.c +++ b/drivers/spi/spi-stm32.c @@ -948,10 +948,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; } @@ -1118,6 +1116,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; @@ -1135,7 +1135,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); } } @@ -1150,7 +1149,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); } /** @@ -1235,8 +1233,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); @@ -1273,8 +1269,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); @@ -1306,8 +1300,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); } /** @@ -1341,8 +1333,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_MASTER_MODE(spi)) stm32_spi_set_bits(spi, STM32H7_SPI_CR1, STM32H7_SPI_CR1_CSTART); }
On the STM32F4/7, the SPI pins float while the controller is disabled. 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. This patch fixes the bug by enabling the controller in prepare_message() and disabling it in unprepare_message(), which are called while CS is not asserted. Note that bug is likely not present on the STM32H7, because it supports the AFCNTR bit (and this driver sets it), which keeps the SPI pins driven even while the controller is disabled. This patch has been tested on an STM32F746 with a MAX14830 UART expander. Signed-off-by: Ben Wolsieffer <ben.wolsieffer@hefring.com> --- drivers/spi/spi-stm32.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)