STM32H7 SPI implementation is not as per the reference manual

[donotnoot]

Recently, I got my hands on a STM32H735G-DK board. Obviously there's no way I'm using the abomination that CubeMX is so I'm using libopencm3 as much as I can, writing directly to registers and such when things are not covered by this fantastic lib.

I was trying to get SPI up and running since it seemed like libopencm3 supports it, but the implementation seems to be using the "classic" SPI peripheral that other chips are using, and it is not compatible with the fancier SPI peripheral ST put on this line.

I checked the reference manuals for both the h7[23] and h7[45] lines and they both have this new peripheral:

Page 2261 and 2236 respectively. https://www.st.com/resource/en/reference_manual/dm00603761-stm32h723733-stm32h725735-and-stm32h730-value-line-advanced-armbased-32bit-mcus-stmicroelectronics.pdf https://www.st.com/resource/en/reference_manual/dm00314099-stm32h742-stm32h743-753-and-stm32h750-value-line-advanced-arm-based-32-bit-mcus-stmicroelectronics.pdf

As you can see, there are quite a few new registers and the way the peripheral works is considerably different than the rest of the lines. I've not combed through them both to make sure they're 100% the same but it looks like they are.

I'd like to know whether I'm missing something and whether a contribution for this new SPI peripheral would be useful. I'm porting some software that runs on F4 to H7 and need the SPI working, so at some point I might stop writing to the registers raw and move it to opencm3, but only if nobody is already working on that.

There's another thing that I noticed too, and it is that the current RCC implementation doesn't allow to use the SMPS because initialising the LDO is hardcoded into the RCC PLL config function. Should I open a separate issue for that? It's fairly simple to fix, one just needs to separate the PWR stuff out of the RCC function and set up the SMPS before setting up the PLL.

[karlp]

yes, looks like spi for h7, is actually "v3" or... well, according to ST's AN5543

libopencm3 spi-v1 == ST spi version 1.2 libopencm3 spi-v2 == ST spi version 1.3 libopencm3 not-implemented (-v3) == ST spi version 2 libopencm3 not implemented (-v4) == ST spi version 3

So yeah, h7 should get "more" at least. I've not looked at how compatible it is, whether it's an extension or a replacement.

I'm going to close this, as we don't keep issues tracking unimplemented code, but feel free to work on that.

For the RCC pwr options, open that separately, it's too hard to track different things in the same issue like that. Don't "just take it out" though, as that would be very unlike all teh other platforms. you might want extra/different options in the config struct instead.

[donotnoot]

As I said I'm kinda more or less porting it already. As for compatibility, it's very different. The CR1/2 registers have the same names but very different configs and there are two entirely new registers to configure it further. Also there's no DR, there are separate registers for TX/RX. Anyway, if/when I do port it properly I'll try to make the API similar, but the hardware is rather different so it might not be totally the same.

Fair enough on closing it. I'll keep playing with the dev board that I got and see what the best solution for the SMPS would be in terms of the API, but yeah as you said adding a config to the struct seems like a sensible idea.

[karlp]

according to AN4936, the h7 spi stuff should be mostly additional vs f7 (which is -v2 in locm3) so looks like just extra functionality, and just h7 includes the -v2 and the "new" -v3 spi file.

[karlp]

and yes, f4 is -v1, you don't want to really even look at that.

[donotnoot]

I'm not too sure how including the v2 and adding stuff on top of that would work... Maybe I'm missing something, but many #defines from the F7 are not applicable/incorrect if used for H7. To give one example, CR2 has nothing to do with each other between versions, in F7 it contains many configs for the SPI peripheral, in H7 it's just two 16b numbers. We could just keep the defines for both I guess, but that's not all that's very different, worse conflicts would be that spi_common_all.h, included by the SPI v2 header has SPI_CR1_SPE as 1<<6, but for the h7 SPI_CR1_SPE needs to be 1<<0 and spi_common_all.h also has SPI_SR at + 0x8, but it's at + 0x14 in H7. Since I'm likely implementing this, what do you think the best way to handle this should be?

[karlp]

no problem, if they're truly different, I was just going on st's app note, and the fact that someone added it as -v2 already, so it worked enough for them that way already. if't wrong, and it's not at all compat, no problems, just make a -v3 and use that for h7

[karlp]

@vielster hey, you added spi-v2 to the h7, what did you test that with? I don't see how that could have worked?

[vielster]

Our SPI changes are probably somewhere in the long list of merge requests (?).

Here is the header in our staging branch (you'll find lots some other goodies in here too for other peripherals).

https://github.com/allocortech/libopencm3/blob/staging/include/libopencm3/stm32/h7/spi.h

[donotnoot]

@vielster that's awesome! wish I had found it about a week ago :) That's pretty much what I wrote myself except that's more polished. I suppose you're planning on merging that? I can't see a PR for it from you: https://github.com/libopencm3/libopencm3/pulls?q=is%3Apr+author%3Avielster+

Also, since it appears like you implemented the RCC stuff, what do you think regarding the hard coded LDO config that I mentioned? Is this something you're also planning on doing/have already done or should I think about how to allow configuring the SMPS?

[vielster]

I'm sorry...the queue of PRs got pretty backed up, so we got kinda bad about keeping things sync'd, as it required a lot of dual remote rebasing and such. There are multiple developers over here, so possible these changes came from another author. We welcome anyone to use our branch, and cherry pick changes into the main repo, and may do so ourselves when our tree stabilizes. I also don't mind private sharing some of our driver implementations (C++ object based) as a basis for adding functionality to opencm if needed...we have fought quite a bit with some of the periphs to get them right.

I could see the SMPS going a couple of different ways. To keep it backwards compatible, you could add a config to the rcc struct to enable/use SMPS instead of the LDO, but no matter how you do it, this should be a pretty trivial update to our RCC use, and we may very well use the SMPS on the H7B3 we plan to start working with here soon.

[vielster]

Hey @donotnoot if you do have something existing for the SMPS, it would come in handy...just turned on an STM32H7A3 (and of course it's the Q variant with SMPS), so will need to make this happen on my end to.

I'm thinking that we could just extend the RCC struct to have an appropriate .system_supply_config which maps to Figure 20 from the A3 manual...just make sure 0 maps to LDO only, and it should be backwards compatible for implementations that don't set it. I haven't dug through all the details on the SMPS, but the struct could be extended to include any of those configs as well. Obviously SMPS settings will be ignored if .system_supply_config == PWR_SUPPLY_CONFIG_LDO

Do you want to create an issue specifically for this RCC change for the H7 to track this against?

[vielster]

Actually...I need this like yesterday, so @donotnoot I'm working on this presently. Will add you as a reviewer if that's cool.

[vielster]

Added #1304

[vielster]

FYI @donotnoot (I cannot seem to reference you from outside of this issue) See #1305

[donotnoot]

Hi @vielster! Sorry that I didn't get back before, been super busy with other stuff lately. Electronics is (unfortunately) just a hobby for me so sometimes I don't get to spend as much time as I'd like on this. :(

The code that I had for the SMPS was just quick and dirty direct register manipulation in a static function in my main.c, so nothing close to the quality of what you've put together. Once again, many thanks for this and for sharing the other changes on you staging repo! As I mentioned in the PR it works great on my STM32H735G-DK, and I'll be testing it too on a board that I designed with a H723 (though that one doesn't have SMPS). I don't see why it wouldn't work on that too.

Regards

[donotnoot]

Oh, one thing I forgot to mention that I could make a PR for. I got slightly confused regarding the values for the .ppre{1,2,3} fields of the struct rcc_pll_config. I (naively I guess) thought that they would accept the divider as an integer and then the function would set the register values based on that, but only after I had proper look through the source I realised that one actually needs to pass in one of RCC_DxCFGR_DxPPRE_DIVn to those. I think it would be nice to see that reflected in the documentation for the struct, similar to how other fields have a comment like /**< RCC_PLLCKSELR_PLLSRC_xxx value. */ next to them. wdyt?

[vielster]

Yeah...I believe at one point, I actually had all of these as enum values so that it was self-documented that the enum needed to be used, but this was (a) not in line with the other rcc implementations and (b) meant duplicate definitions of the values (one for the define, one for the enum), so they were scrapped.

Feel free to poke references into the RCC header...I will not be offended :-) I may also do so when I get my next spurt of opencm mods going

[karlp]

(I'm fine with enums if they're named nicely. They're easy to document, easy to reference, and don't cost anything, so don't feel you have to use #defines just because they've been used a lot int he past)

[vielster]

Next time :-)

[donotnoot]

@vielster fyi :) https://github.com/libopencm3/libopencm3/pull/1308