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

Stm32 u5 spi no dma #20608

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

shimunn
Copy link

@shimunn shimunn commented Apr 22, 2024

Contribution description

  • Implemented an SPI driver(without dma so far) for the STM32U5 series, defined the APB3 clock divider as a requisite for SPI.
  • The spc.c file was split into an spi_all.c for all non U5 series STM32 and an spi_u5.c to keep code readable.

Testing procedure

  • ran tests/periph/spi with MOSI connected to MISO, confirmed that input == output (SPI3)
  • connected an sx127x shield to SPI3 and verified correct operation
  • ran the tests above on an nucleo-f767zi board, confirming that the U5 related changes didn't break SPI for other series.

Issues/PRs references

SPI3: requires VddIO2 to be enabled using:

PWR->SVMCR |= PWR_SVMCR_IO2SV;

as well as an clock source:

RCC->CCIPR3 &= ~RCC_CCIPR3_SPI3SEL;
// Must be set to MSIK
RCC->CCIPR3 |= RCC_CCIPR3_SPI3SEL_1;

I'm not sure if those step should also be part of this PR or not.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Apr 22, 2024
@Teufelchen1
Copy link
Contributor

Hello! Thank you for bringing spi on the u5 to RIOT.

Could you explain in a few words what makes the U5 series's SPI so different from the others that it needed to be split into two files?

@shimunn
Copy link
Author

shimunn commented Apr 23, 2024

Hello! Thank you for bringing spi on the u5 to RIOT.

Could you explain in a few words what makes the U5 series's SPI so different from the others that it needed to be split into two files?

There are a lot more registers for the U5, instead of DR there are TXDR and RXDR for instance. I've tried to use #ifdef at first but ended up needing one for almost every register access, which made the code very verbose.

@Enoch247
Copy link
Contributor

I believe this implementation will also support the STMH7 (port in progress) and STM32MP1 (already in tree). They all seem to have similar/same SPI peripheral which is bit different from the implementation used in all other chips.

@Enoch247
Copy link
Contributor

Enoch247 commented Apr 29, 2024

Hello! Thank you for bringing spi on the u5 to RIOT.
Could you explain in a few words what makes the U5 series's SPI so different from the others that it needed to be split into two files?

There are a lot more registers for the U5, instead of DR there are TXDR and RXDR for instance. I've tried to use #ifdef at first but ended up needing one for almost every register access, which made the code very verbose.

I had started to work on a port for the H7. I had in-mind to wrap the register access with static inline functions that are CPU specific. You are closer to the problem than I, but I'll ask, is that a possibility instead?

@shimunn
Copy link
Author

shimunn commented Apr 30, 2024

Hello! Thank you for bringing spi on the u5 to RIOT.
Could you explain in a few words what makes the U5 series's SPI so different from the others that it needed to be split into two files?

There are a lot more registers for the U5, instead of DR there are TXDR and RXDR for instance. I've tried to use #ifdef at first but ended up needing one for almost every register access, which made the code very verbose.

I had started to work on a port for the H7. I had in-mind to wrap the register access with static inline functions that are CPU specific. You are closer to the problem than I, but I'll ask, is that a possibility instead?

Would that improve things? I reckon doing so would make things even harder to read than putting an #ifdef around every register access.

cpu/stm32/periph/spi_u5.c Outdated Show resolved Hide resolved
cpu/stm32/periph/spi_u5.c Outdated Show resolved Hide resolved
cpu/stm32/periph/spi_u5.c Outdated Show resolved Hide resolved
cpu/stm32/periph/spi_u5.c Outdated Show resolved Hide resolved
cpu/stm32/periph/spi_u5.c Outdated Show resolved Hide resolved
@Enoch247
Copy link
Contributor

Enoch247 commented May 1, 2024

I tried this on an H7 and it does work, with some of the modifications I've suggested. I did run into an issue I am still trying to fully understand. I believe I accidentally selected an SPI clock rate that was too high for my CPU to keep up with (I only had the CPU clocked at 64 MHz as I have yet to get the PLL working). I suspect the SPI periph then had an under run condition and the state of its status flags were such that the driver got stuck in the data transfer loop. This is probably not something most people will ever run into, but it would be nice if the condition was handled more gracefully. Even a failed assert would have been helpful to debug the situation.

@Enoch247
Copy link
Contributor

Enoch247 commented May 1, 2024

On my H7. It seems the driver is reading 1 byte shy of what is requested I have to transfer 3 bytes to get 2 bytes to be modified in the rx buffer. Still digging into why that is.

Comment on lines 347 to 348
if (dev(bus)->SR & SPI_SR_EOT) {
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

In my testing on an STM32H7, this break causes the last received bytes to be left in the RX FIFO. The transfer completes (ie the hardware has shifted in/out the requested number of bytes), and we break out of the loop, leaving rx_remainder != 0. Is there a need for this check or another way it could be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

This explains my comment above:

On my H7. It seems the driver is reading 1 byte shy of what is requested I have to transfer 3 bytes to get 2 bytes to be modified in the rx buffer. Still digging into why that is.

Copy link
Author

Choose a reason for hiding this comment

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

Does dev(bus)->SR indicate that there might be some bytes left?

Does this help?

Suggested change
if (dev(bus)->SR & SPI_SR_EOT) {
break;
if (dev(bus)->SR & SPI_SR_EOT && rx_remainder == 0) {
break;

Copy link
Author

Choose a reason for hiding this comment

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

I've just ran into that issue myself while using the the fully featured SPI1 interface, the errata mentions an possible truncation issue however I do not understand how the mentioned (below) issue would apply to my PR since the SPI is only disabled once all bytes are received.

After an EOT event signaling the end of a non-zero transfer size transaction (TSIZE > 0) upon sampling the last
data bit, the software may disable the SPI peripheral. As expected, disabling SPI deactivates the SPI outputs
(SCK, MOSI and SS when the SPI operates as a master, MISO when as a slave), by making them float or
statically output their by-default levels, according to the AFCNTR bit of the SPI_CFG2 register.
With fast software execution (high PCLK frequency) and slow SPI (low SCK frequency), the SPI disable occurring
too fast may result in truncating the SPI output signals. For example, the device operating as a master then
generates an asymmetric last SCK pulse (with CPHA = 0), which may prevent the correct last data bit reception
by the other node involved in the communication.
Workaround
Apply one of the following measures or their combination:
• Decrease the ratio between PCLK and SCK frequencies.
• Add a delay between the EOT event and SPI disable action

Copy link
Author

Choose a reason for hiding this comment

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

It seems like the SPI peripheral looses some rx bits only if TSIZE is set on an fully featured interface. Not setting TSIZE seems to fix the problem. Tested with an sd card i9n SPI mode.

shimunn and others added 6 commits May 2, 2024 09:55
Co-authored-by: Joshua DeWeese <josh.deweese@gmail.com>
Co-authored-by: Joshua DeWeese <josh.deweese@gmail.com>
Co-authored-by: Joshua DeWeese <josh.deweese@gmail.com>
Co-authored-by: Joshua DeWeese <josh.deweese@gmail.com>
Co-authored-by: Joshua DeWeese <josh.deweese@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants