RIOT icon indicating copy to clipboard operation
RIOT copied to clipboard

Stm32 u5 spi no dma

Open shimunn opened this issue 1 year ago • 8 comments

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.

shimunn avatar Apr 22 '24 14:04 shimunn

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?

Teufelchen1 avatar Apr 23 '24 12:04 Teufelchen1

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.

shimunn avatar Apr 23 '24 12:04 shimunn

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 avatar Apr 29 '24 18:04 Enoch247

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?

Enoch247 avatar Apr 29 '24 18:04 Enoch247

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.

shimunn avatar Apr 30 '24 08:04 shimunn

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 avatar May 01 '24 14:05 Enoch247

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.

Enoch247 avatar May 01 '24 19:05 Enoch247

I spotted some commented out code and C++ style comments. I believe the codding conventions does not allow this.

Also, I still believe the SPI is not different enough to merit splitting it out from the others Or, at least if it should be split out, I think the commonalities should go into a 3rd shared file.

Enoch247 avatar Jun 22 '24 11:06 Enoch247