Instead of disabling the SPI controller between each message, do it
as part of runtime PM.
The primary motivation for this change is that, 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 it
supports the AFCNTR bit (and this driver sets it), which keeps the SPI
pins driven even while the controller is disabled.
Additionally, not constantly enabling/disabling the controller should
slightly improve performance.
With this change, spi->cfg->config(spi) will be called as part of
runtime PM resume, rather than just during system resume. This was done
because this function must be called with the controller disabled, which
is no longer the case after runtime resume. The overhead of these extra
calls should be minimal.
This patch has been tested on an STM32F746 with a MAX14830 UART
expander.
Signed-off-by: Ben Wolsieffer <[email protected]>
---
Discussion of my original proposed fix for this bug:
https://lore.kernel.org/lkml/ZWpoKEcM0ZeYAsBa@dell-precision-5540/T/
drivers/spi/spi-stm32.c | 60 +++++++++--------------------------------
1 file changed, 12 insertions(+), 48 deletions(-)
diff --git a/drivers/spi/spi-stm32.c b/drivers/spi/spi-stm32.c
index c831f992c8a0..25d0757278fa 100644
--- a/drivers/spi/spi-stm32.c
+++ b/drivers/spi/spi-stm32.c
@@ -947,10 +947,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;
}
@@ -1134,7 +1132,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);
}
}
@@ -1149,7 +1146,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);
}
/**
@@ -1234,8 +1230,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);
@@ -1272,8 +1266,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);
@@ -1305,8 +1297,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);
}
/**
@@ -1340,8 +1330,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);
}
@@ -1786,21 +1774,6 @@ static int stm32_spi_transfer_one(struct spi_controller *ctrl,
return spi->cfg->transfer_one_irq(spi);
}
-/**
- * stm32_spi_unprepare_msg - relax the hardware
- * @ctrl: controller interface
- * @msg: pointer to the spi message
- */
-static int stm32_spi_unprepare_msg(struct spi_controller *ctrl,
- struct spi_message *msg)
-{
- struct stm32_spi *spi = spi_controller_get_devdata(ctrl);
-
- spi->cfg->disable(spi);
-
- return 0;
-}
-
/**
* stm32fx_spi_config - Configure SPI controller as SPI master
* @spi: pointer to the spi controller data structure
@@ -1827,6 +1800,8 @@ static int stm32fx_spi_config(struct stm32_spi *spi)
STM32FX_SPI_CR1_MSTR |
STM32FX_SPI_CR1_SSM);
+ stm32_spi_enable(spi);
+
spin_unlock_irqrestore(&spi->lock, flags);
return 0;
@@ -1870,6 +1845,8 @@ static int stm32h7_spi_config(struct stm32_spi *spi)
stm32_spi_set_bits(spi, STM32H7_SPI_CR1, cr1);
stm32_spi_set_bits(spi, STM32H7_SPI_CFG2, cfg2);
+ stm32_spi_enable(spi);
+
spin_unlock_irqrestore(&spi->lock, flags);
return 0;
@@ -2066,7 +2043,6 @@ static int stm32_spi_probe(struct platform_device *pdev)
ctrl->use_gpio_descriptors = true;
ctrl->prepare_message = stm32_spi_prepare_msg;
ctrl->transfer_one = stm32_spi_transfer_one;
- ctrl->unprepare_message = stm32_spi_unprepare_msg;
ctrl->flags = spi->cfg->flags;
if (STM32_SPI_DEVICE_MODE(spi))
ctrl->slave_abort = stm32h7_spi_device_abort;
@@ -2167,6 +2143,8 @@ static int __maybe_unused stm32_spi_runtime_suspend(struct device *dev)
struct spi_controller *ctrl = dev_get_drvdata(dev);
struct stm32_spi *spi = spi_controller_get_devdata(ctrl);
+ spi->cfg->disable(spi);
+
clk_disable_unprepare(spi->clk);
return pinctrl_pm_select_sleep_state(dev);
@@ -2182,7 +2160,11 @@ static int __maybe_unused stm32_spi_runtime_resume(struct device *dev)
if (ret)
return ret;
- return clk_prepare_enable(spi->clk);
+ ret = clk_prepare_enable(spi->clk);
+ if (ret)
+ return ret;
+
+ return spi->cfg->config(spi);
}
static int __maybe_unused stm32_spi_suspend(struct device *dev)
@@ -2200,31 +2182,13 @@ static int __maybe_unused stm32_spi_suspend(struct device *dev)
static int __maybe_unused stm32_spi_resume(struct device *dev)
{
struct spi_controller *ctrl = dev_get_drvdata(dev);
- struct stm32_spi *spi = spi_controller_get_devdata(ctrl);
int ret;
ret = pm_runtime_force_resume(dev);
if (ret)
return ret;
- ret = spi_controller_resume(ctrl);
- if (ret) {
- clk_disable_unprepare(spi->clk);
- return ret;
- }
-
- ret = pm_runtime_resume_and_get(dev);
- if (ret < 0) {
- dev_err(dev, "Unable to power device:%d\n", ret);
- return ret;
- }
-
- spi->cfg->config(spi);
-
- pm_runtime_mark_last_busy(dev);
- pm_runtime_put_autosuspend(dev);
-
- return 0;
+ return spi_controller_resume(ctrl);
}
static const struct dev_pm_ops stm32_spi_pm_ops = {
--
2.42.1
On Mon, Dec 04, 2023 at 03:20:55PM -0500, Ben Wolsieffer wrote:
> Instead of disabling the SPI controller between each message, do it
> as part of runtime PM.
This doesn't apply against current code, please check and resend.
Hi,
sorry for the delay.
On Thu, Dec 14, 2023 at 10:58:54AM +0000, Mark Brown wrote:
> On Mon, Dec 04, 2023 at 03:20:55PM -0500, Ben Wolsieffer wrote:
> > Instead of disabling the SPI controller between each message, do it
> > as part of runtime PM.
>
> This doesn't apply against current code, please check and resend.
I rapidly gave a try on this patch on top of the spi/for-next branch
(manually fixing the conflict due to the MASTER->HOST renaming).
It turns out that with that applied, transfers on the MP13
(compatible: st,stm32h7-spi) are not working anymore while simply
removing it back it works again.
(test is simply doing loopback spidev_test)
spi mode: 0x0
bits per word: 8
max speed: 500000 Hz (500 kHz)
TX | 8D D6 73 8B 9D 8B 1C 7D 8D 80 EC 32 F9 0D BA AD 9F 88 A5 9B 3F AA 48 8C 21 35 0D C1 C8 E5 6A 81 |..s....}...2........?.H.!5....j.|
RX | 8D 00 00 00 D6 00 73 00 8B 00 00 00 9D 00 00 8B 1C 00 00 00 7D 00 00 8D F9 00 00 00 BA 00 00 00 |......s.............}...........|
The RX data contains lots of 00 between each byte. Moreover it seems
that with this patch applied non-dma transfer (when there is no dmas
properties within the node) are now failing.
I'll check that and give more details but could you avoid applying this
patch for the time being ?
Thanks.
Alain
On Fri, Dec 15, 2023 at 07:27:39PM +0100, Alain Volmat wrote:
> Hi,
>
> sorry for the delay.
>
> On Thu, Dec 14, 2023 at 10:58:54AM +0000, Mark Brown wrote:
> > On Mon, Dec 04, 2023 at 03:20:55PM -0500, Ben Wolsieffer wrote:
> > > Instead of disabling the SPI controller between each message, do it
> > > as part of runtime PM.
> >
> > This doesn't apply against current code, please check and resend.
>
> I rapidly gave a try on this patch on top of the spi/for-next branch
> (manually fixing the conflict due to the MASTER->HOST renaming).
> It turns out that with that applied, transfers on the MP13
> (compatible: st,stm32h7-spi) are not working anymore while simply
> removing it back it works again.
> (test is simply doing loopback spidev_test)
That's unfortunate; I was worried about something like this because I
only have an STM32F7 to test. If you can't easily determine what's going
wrong, it would be interesting to know if the original version of this
patch has the same problem:
https://lore.kernel.org/lkml/ZWpoKEcM0ZeYAsBa@dell-precision-5540/T/
>
> spi mode: 0x0
> bits per word: 8
> max speed: 500000 Hz (500 kHz)
> TX | 8D D6 73 8B 9D 8B 1C 7D 8D 80 EC 32 F9 0D BA AD 9F 88 A5 9B 3F AA 48 8C 21 35 0D C1 C8 E5 6A 81 |..s....}...2........?.H.!5....j.|
> RX | 8D 00 00 00 D6 00 73 00 8B 00 00 00 9D 00 00 8B 1C 00 00 00 7D 00 00 8D F9 00 00 00 BA 00 00 00 |......s.............}...........|
>
> The RX data contains lots of 00 between each byte. Moreover it seems
> that with this patch applied non-dma transfer (when there is no dmas
> properties within the node) are now failing.
>
> I'll check that and give more details but could you avoid applying this
> patch for the time being ?
>
> Thanks.
> Alain