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

BUG: Fix STM32 SPI CR1 #5250

Closed
wants to merge 1 commit into from

Conversation

bigtreetech
Copy link
Contributor

STM32 SPI_CR1 should not be changed when communication is ongoing.

fix #5198

@cbc02009
Copy link
Contributor

cbc02009 commented Feb 9, 2022

This does not work for me. With the patch and newly flashed firmware I still get the same ADC out of range error I was getting last time I tried using hardware SPI.

@cbc02009
Copy link
Contributor

cbc02009 commented Feb 9, 2022

what's interesting is using spi->CR1 = 0x00; works for me, so there must be some other bits in the register that aren't being handled properly.

@bigtreetech bigtreetech marked this pull request as draft February 10, 2022 02:22
@KevinOConnor
Copy link
Collaborator

Thanks. I think there are two separate issues occurring.

  1. The spi_transfer() function should not be exiting until after all SCLK pin updates are complete. Otherwise, the CS pin may rise before the final SCLK pin updates, and subsequent writes to CR1 might occur while a transmission is still in progress (in spi mode 0 and 1).
  2. It looks like the specs do state that the SPE bit needs to be cleared before changing CPOL or CPHA bits.

Based on this, something like the following should fix it:

--- a/src/stm32/spi.c
+++ b/src/stm32/spi.c
@@ -100,6 +100,11 @@ void
 spi_prepare(struct spi_config config)
 {
     SPI_TypeDef *spi = config.spi;
+    uint32_t cr1 = spi->CR1;
+    if (cr1 == config.spi_cr1)
+        return;
+    // The SPE bit must be disabled before changing CPOL/CPHA bits
+    spi->CR1 = cr1 & ~SPI_CR1_SPE;
     spi->CR1 = config.spi_cr1;
 }
 
@@ -117,4 +122,7 @@ spi_transfer(struct spi_config config, uint8_t receive_data,
             *data = rdata;
         data++;
     }
+    // Wait for any remaining SCLK updates before returning
+    while ((spi->SR & (SPI_SR_TXE|SPI_SR_BSY)) != SPI_SR_TXE)
+        ;
 }

-Kevin

@cbc02009
Copy link
Contributor

cbc02009 commented Feb 10, 2022

--- a/src/stm32/spi.c
+++ b/src/stm32/spi.c
@@ -100,6 +100,12 @@ void
 spi_prepare(struct spi_config config)
 {
     SPI_TypeDef *spi = config.spi;
+    uint32_t cr1 = spi->CR1;
+    if (cr1 == config.spi_cr1)
+        return;
+    // The SPE bit must be disabled before changing CPOL/CPHA bits
+    spi->CR1 = cr1 & ~SPI_CR1_SPE;
+    spi->CR1; // Force flush of previous write
     spi->CR1 = config.spi_cr1;
 }
 
@@ -117,4 +123,7 @@ spi_transfer(struct spi_config config, uint8_t receive_data,
             *data = rdata;
         data++;
     }
+    // Wait for any remaining SCLK updates before returning
+    while ((spi->SR & (SPI_SR_TXE|SPI_SR_BSY)) != SPI_SR_TXE)
+        ;
 }

appears to work for me

@KevinOConnor
Copy link
Collaborator

Okay, thanks. I went ahead and made these changes (commit 99d5518 and e3cbe7e).

-Kevin

@KevinOConnor KevinOConnor added the resolved Issue is thought to now be fixed label Feb 10, 2022
@bigtreetech bigtreetech deleted the stm32_spi_bugfix branch March 8, 2022 11:57
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
resolved Issue is thought to now be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAX31865 and TMC5160 on shared SPI bus doesn't work (stm32f429)
3 participants