libopencm3
STM32L5 Support, TrustZone Discussion
I've been working on adding some of the new peripherals to target the new STM32L5 line that has come out recently, with some success. Given this isn't ready for a pull request, I wanted to create an issue in case anyone else is working on this as well.
The majority of the microcontroller's peripherals are functional, save for the trust zone execution (the peripheral addresses change depending upon execution within the TrustZone or not). The new PKA (Public Key Accelerator) peripheral driver is nearly complete, and I have verified the more complex actions. The new additions to the hashing peripheral have been mostly verified as well (SHA256, HMAC-SHA256).
However the biggest decision is about the TrustZone execution, and I feel it should be discussed to ensure that the code base won't become unmaintainable. This is also important given the newer cortex m23 and m33 cores, and how the library will support them in the future.
Given the TrustZone execution creates some strange situations, I have thought of the two following solutions to integrate it into libopencm3:
- Create two separate targets being the normal STM32L5, and STM32L5 Secure.
- Add additional definitions within the driver files under the new STM32L5 library folder that are copies of the normal peripheral functions, with the changed addresses.
I feel that the second option is better, as then any additional changes done to the microcontroller series will be consistent, but of course this could prove difficult for maintainability with all the different peripherals. To potentially alleviate this, a script could run at compile time to copy over the specific peripherals, add in checks for valid access under the TrustZone execution, and then create the secure peripheral library files.
The major drawback to the separate targets, is that if you wish to create a binary with both TrustZone and non-TrustZone code, you wouldn't be able to write them both in the same file. It would only be possible at link time, to my knowledge at least. This is definitely the simpler to implement option.
Hi,
While I have yet to use the TrustZone functionality of the M33, I'm interested in using the STM32L5 peripherals. I'm happy to do some testing / debugging of any beta level support you have.
Option 2 seems like the easier way to use the system, but the opposite for maintenance. Could a middle ground be to supply an enum {M33_INSECURE, M33_SECURE} that could be passed as a parameter to each of the functions, to switch behavior between the two modes, without having to maintain two sets of functions?
I agree completely with the TrustZone and how it messes things up. My feeling is probably that we should add a dummy TrustZone support to all chips, but make it non-functional for chips without TrustZone. This is seen already in some of the peripherals because certain families implement only a subset of the peripheral that libopencm3 implements for that family.
Also, I am working on a PR, I posted it today although it's not ready for merge. We should definitely work on it together. I don't insist on using my one as the base, if more appropriate I could merge my work into yours, it all depends what is more complete.
See: https://github.com/libopencm3/libopencm3/pull/1351
Wow completely forgot about this; been a while since I've touched it, I'll have to double check. Quick look makes it seem that I may have a couple additional parts that you don't yet, but again I don't recall much since it has been about a year. I'll make another post with what I find
Ok went through it all, and there were a couple differences but I managed to put all your changes into my branch. I'll make a PR so we could continue to work on it from there.
I have a couple of the new peripherals added in, which makes it easier this way. Also, in the end I think it would be fine to have the separate drivers that just are copies of the original, with slightly different function names. It would be easier to read and manage as everything should be identical in the end. Two targets would be a nightmare for the same project. Though I'm not worried about the trustzone yet, that should just come later on. Though it may also be possible to not have to change much, as the offsets would be the same. Just have the TZ_SERCOM1 or something be passed to a usart_tx function, and we're golden. Probably the best way to go about it.
That's brilliant! It really shows the power of open source and community working.
Also, in the end I think it would be fine to have the separate drivers that just are copies of the original, with slightly different function names.
Yes, that's the approach I took with the RTC and OctoSPI peripherals, at the end of the day there is a fair bit of duplication in libopencm3 already, so a bit more isn't going to hurt.
Though I'm not worried about the trustzone yet, that should just come later on. Though it may also be possible to not have to change much, as the offsets would be the same.
I probably won't use TrustZone (I will use crypto as I mentioned), so my main concern is to get things to work normally in non-secure mode. If we do want to support it, I suggest we could add a couple of new functions to manipulate the secure and privilege control registers, and where TZ results in duplication of interrupt logic, we can simply duplicate the corresponding libopencm3 functions with a _s or _secure suffix. I was leaning towards putting this in the common code, conditioned by #ifdef of whether the relevant registers are defined (and this has been done in the EXTI peripheral already at least), but as you say there is no real problem in duplicating the code. I'll be interested to see if ST begins to standardize on TZ support for newer families (STM32L6??). Because if so, the TZ support should definitely move to common code. Otherwise, STM32L5 will just remain a bit of an oddball case. Either way works for me.
I probably won't use TrustZone (I will use crypto as I mentioned), so my main concern is to get things to work normally in non-secure mode. If we do want to support it, I suggest we could add a couple of new functions to manipulate the secure and privilege control registers, and where TZ results in duplication of interrupt logic, we can simply duplicate the corresponding libopencm3 functions with a
_sor_securesuffix. I was leaning towards putting this in the common code, conditioned by#ifdefof whether the relevant registers are defined (and this has been done in the EXTI peripheral already at least), but as you say there is no real problem in duplicating the code. I'll be interested to see if ST begins to standardize on TZ support for newer families (STM32L6??). Because if so, the TZ support should definitely move to common code. Otherwise, STM32L5 will just remain a bit of an oddball case. Either way works for me.
Yeah, I was thinking of something along those lines. Given nothing else has the trustzone implemented in the library yet, this is pretty much testing grounds for it. Plus having an example of how to use the trustzone I think will be nice since others will most likely end up copying it for other devices. So also getting it right is kinda important for future development
Also, in the end I think it would be fine to have the separate drivers that just are copies of the original, with slightly different function names.
Yes, that's the approach I took with the RTC and OctoSPI peripherals, at the end of the day there is a fair bit of duplication in libopencm3 already, so a bit more isn't going to hurt.
That's what everyone says when they start duplicating things, and then you end up with "a fair bit of duplication" and lots of files with almost/notquite/reallythesame definitions that didn't quite get updated after refactorings. really. Please. don't. It just makes work for someone else later, and traditionally at least, that person is not the person who introduced the duplicates.
as for trust zone, I fully expect ~all new mcus to have it, and any idea that involves duplicating the library with different addresses is never going to be maintainable. I don't know what the right path is, but duping it all is not it.
Also, in the end I think it would be fine to have the separate drivers that just are copies of the original, with slightly different function names.
Yes, that's the approach I took with the RTC and OctoSPI peripherals, at the end of the day there is a fair bit of duplication in libopencm3 already, so a bit more isn't going to hurt.
That's what everyone says when they start duplicating things, and then you end up with "a fair bit of duplication" and lots of files with almost/notquite/reallythesame definitions that didn't quite get updated after refactorings. really. Please. don't. It just makes work for someone else later, and traditionally at least, that person is not the person who introduced the duplicates.
hi Karl, I saw that you had reviewed bit0fun's PR and were not happy with the duplication (my PR is stated as not being ready for merge yet, so that's probably why you haven't looked at mine as well). Yes, I appreciate what you mean and it's good to have some firm policies laid out.
Getting the STM32L5 stuff merged is not really a priority for me right now as I am not working on that project actively at the moment. But I do use libopencm3. So from what I gather you would like to reduce duplication in libopencm3.
I mentioned above that there's a fair bit of duplication already, in my observation at least. So what I propose is that next time I'm working on libopencm3 I'll focus on trying to find ways to merge similar code using #ifdef and similar defines?
In that case, what would be your position on breakage of existing uses of the library? I mention this because there are plenty of places where similar functionality is implemented but with different function names and parameters.
An example of this is in setting up the oscillators, some of the ports (cannot remember which ones as I haven't looked at it in a while) use a general function that sets up everything using a data block, other ports use individual setting functions.
Another example is in the naming of the registers, in some cases there are inconsistencies in the published documentation between families (abbreviations used in one RM but not another, etc) and in other cases the inconsistency seems to arise from libopencm3, e.g. by copying an earlier family that had a singleton peripheral to a new family where there are multiple instances of that peripheral -- we'd need to rename the registers to include the number "0" or similar and this has not always been done.
What I'd probably propose is to unify the APIs as far as possible so that the parameters are generic and can be interpreted in different ways for different ports that have different underlying registers, i.e. provide the user a slightly higher level interface.
And then I'd propose to put shims in place, i.e. deprecated functions that can translate old style calls to new style calls. What do you think? I didn't attempt to do anything like this in the past because it looked like radical change for the sake of change, and it honestly didn't bother me to have differing APIs or duplication in the library. But if that's a focus then do you accept my ideas?
And for the registers I think we need to strictly name them according to the RM names but possibly add friendly names as well.
it's unrelated to this topic, so don't make any further comments to it in this issue, but you can break anything you like right now, we tagged a 0.8, and the "idea" is to go and break stuff if needed on our way to a newer release. The goal is to not break stuff, but to mark things deprecated, but if breakage is unavoidable, it's ok. Single vs multiple peripherals is already handled for the most part, its really just ADC, DAC and CAN that have ever seen this, and in ~all cases so far, we have compat defines of DAC == DAC1, ADC == ADC1, CAN==CAN1, if there's things left, by all means fix them, but I'm not aware of any lingering within stm32 at least. ST's changing names is unavoidable, in some cases it may be appropriate to duplicate the define names, to make it easier to follow a refman. Don't bother with shims in general, that's just more code, and people will then never upgrade. Just make them deprecated outright, and include the instructions on what the replacement is. The oscillators is known, and the path forward is taking the clock struct. All newer ports do that. Only F0 and F1 are holdouts, iirc, and they should just get the same style eventually. The naming/location of "higher level" apis so that they can be told apart frrom lower level apis is however problematic, and always has been, there's no solution there so far.