Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers: spi: stm32h7: avoid unnecessary suspend #79050

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dgastonochoa
Copy link

The stm32 H7 driver always suspends the transmission at the end of a transceive operation. However, according to the H7 datasheet, this is only needed when SPI is configured as receive-only. The driver never does this, as LL_SPI_SetTransferDirection(spi, LL_SPI_FULL_DUPLEX); is always called in spi_stm32_configure.

Unless there is a reason to still suspend the transaction, it should not be done. Thus, remove the unnecessary suspension to follow the datasheet instructions and to potentially reduce the minimum time between SPI transactions. However, this is not causing problems at the moment as far as I know; this should be taken into consideration before approving this PR.

Here is the explanation about what receive-only mode is and how to set it:

receive-only-mode

receive-only-mode-bits

And here is the disabling procedure stated by the H7's datasheet:

spi-disabling-procedure

Test plan

Run all the spi_loppback tests, with and without DMA/IRQs enabled, and with and without enabling the FIFO usage. That is:

Connect an STM32H7 board to the PC with pins D11 and D12 connected.

Open a minicom or equivalent terminal to read the test report.

Execute the following commands:

cd zephyrproject/zephyr

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi tests/drivers/spi/spi_loopback
west flash

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi -T tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_16bits_frames.loopback
west flash

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi -T tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback
west flash

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi -T tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_16bits_frames_dma.loopback
west flash

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi -T tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma_dt_nocache_mem.loopback
west flash

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi -T tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_16bits_frames_dma_dt_nocache_mem.loopback
west flash

rm -rf zephyrproject/zephyr/build
west build -p auto -b nucleo_h753zi -T tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_interrupt.loopback
west flash

Execute the commands above, this time with the SPI fifo enabled. To do this, add fifo-enable; to the spi1 node in tests/drivers/spi/spi_loopback/overlay-stm32-spi-16bits.overlay and in tests/drivers/spi/spi_loopback/boards/nucleo_h753zi.overlay

Comment on lines 221 to 233
#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_spi)
if (LL_SPI_IsActiveMasterTransfer(spi)) {
LL_SPI_SuspendMasterTransfer(spi);
while (LL_SPI_IsActiveMasterTransfer(spi)) {
/* NOP */
}
/* Flush RX buffer */
while (LL_SPI_IsActiveFlag_RXP(spi)) {
(void)LL_SPI_ReceiveData8(spi);
}

LL_SPI_Disable(spi);
while (LL_SPI_IsEnabled(spi)) {
/* NOP */
}

/* Flush RX buffer */
while (LL_SPI_IsActiveFlag_RXP(spi)) {
(void)LL_SPI_ReceiveData8(spi);
}
LL_SPI_ClearFlag_SUSP(spi);
#else
LL_SPI_Disable(spi);
#endif /* st_stm32h7_spi */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely, this can then be simplified to:

Suggested change
#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_spi)
if (LL_SPI_IsActiveMasterTransfer(spi)) {
LL_SPI_SuspendMasterTransfer(spi);
while (LL_SPI_IsActiveMasterTransfer(spi)) {
/* NOP */
}
/* Flush RX buffer */
while (LL_SPI_IsActiveFlag_RXP(spi)) {
(void)LL_SPI_ReceiveData8(spi);
}
LL_SPI_Disable(spi);
while (LL_SPI_IsEnabled(spi)) {
/* NOP */
}
/* Flush RX buffer */
while (LL_SPI_IsActiveFlag_RXP(spi)) {
(void)LL_SPI_ReceiveData8(spi);
}
LL_SPI_ClearFlag_SUSP(spi);
#else
LL_SPI_Disable(spi);
#endif /* st_stm32h7_spi */
#if DT_HAS_COMPAT_STATUS_OKAY(st_stm32h7_spi)
/* Flush RX buffer */
while (LL_SPI_IsActiveFlag_RXP(spi)) {
(void)LL_SPI_ReceiveData8(spi);
}
#endif /* st_stm32h7_spi */
LL_SPI_Disable(spi);
while (LL_SPI_IsEnabled(spi)) {
/* NOP */
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it can be simplified that way, but that will affect non-H7 devices, which I cannot test. Are you happy with this? If so, I will go ahead and apply it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll test it, but I don't expect issues to be seen.

The SPI stm32 H7 driver always suspends the SPI transaction
when finishing the transceive operation. This, according to
the stm32 H7 datasheet, must only be done when the SPI master
is configured as receive-only, and the driver always
configures it as full-duplex.

Hence, remove this unnecessary operation for clarify and for
a potential reduction in the minimum time between SPI
transceive operations.

Signed-off-by: Daniel Gaston Ochoa <dgastonochoa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: SPI SPI bus platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants