carlescufi
commented
1 year ago Architecture WG:
- CMSIS is an incomplete set of headers: Vendors need to supply their own headers
- One of the reasons we have the code we have in https://github.com/zephyrproject-rtos/zephyr/blob/7b8170dd278ed40913d5df0944f34d81975ae8f0/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h#L25-L80 is qemu_cortex_m3, which is a "native port" that does not use CMSIS at all
- @stephanosio suggests moving the macros required by CMSIS to a new
soc-cmsis.hspecific to each core. The type itself,IRQn_Type, would have to be redefined in our CMSIS fork - @nashif states that a problem is that we require
soc.hin the drivers everywhere - No support for moving away from CMSIS
- @galak suggests sending a patch upstream to CMSIS in order to be able to selectively disable the use of
IRQn_Type - @galak suggests investigating having our own copy of CMSIS in-tree to avoid these issues
zephyrbot
Introduction
This PR is about detailing the problems with CMSIS and soc.h, and proposing a path forward.
Problem description
<soc.h>has traditionally been used as a proxy to HAL headers, register definitions, etc. Nowadays,<soc.h>is anarchy, specially on ARM platforms. It serves a different purpose depending on the SoC. In most cases, it includes HALs, in some others, it works as a header sink/proxy (for no good reason), as a register definition when there's no HAL (bypassing the purpose of DT in some cases…).Some platforms like ARC/ARM64 no longer require it after https://github.com/zephyrproject-rtos/zephyr/pull/46585 and https://github.com/zephyrproject-rtos/zephyr/pull/46147.
But... let's talk about ARM. The ARM port uses CMSIS APIs in many places, e.g. IRQ handling calls. For CMSIS we actually have a module in Zephyr: https://github.com/zephyrproject-rtos/cmsis. In Zephyr, CMSIS is typically used by including https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h. And here's where the problem starts:
https://github.com/zephyrproject-rtos/zephyr/blob/7b8170dd278ed40913d5df0944f34d81975ae8f0/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h#L19
That is, cmsis.h including soc.h. The main reason is that CMSIS headers are incomplete. For example, they rely on someone else defining
IRQn_Typetype, or on__NVIC_PRIO_BITS. This is typically done by HALs. This means that we need to include HAL headers to use CMSIS. HAL headers are typically bloated with lots of unnecessary includes. And because we have many code in headers (due to inlining), we end up propagating soc.h (and so bloated HAL headers) almost everywhere. Actually, we support non-CMSIS compliant HALs:https://github.com/zephyrproject-rtos/zephyr/blob/7b8170dd278ed40913d5df0944f34d81975ae8f0/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h#L25-L80
So one could think that instead of relying on HALs to do this work, we could just do that for all cases. But the caveat is that it is then not possible to use HALs in drivers that also include Zephyr's headers (i.e., all drivers).
Let's take a quick example that illustrate what is explained above:
https://github.com/zephyrproject-rtos/zephyr/blob/7b8170dd278ed40913d5df0944f34d81975ae8f0/samples/hello_world/src/main.c#L7-L13
Looks clean, just
<zephyr/kernel.h>. But... If we modify$ZEPHYR_BASE/../modules/hal/gigadevice/gd32f4xx/standard_peripheral/include/gd32f4xx_dac.h(HAL DAC driver) with a#warning, and compile the sample with e.g.west build -b gd32f450v_start samples/hello_world -p, you'll soon see tons of warnings:The main reason, as said, is because we end up including HAL headers almost everywhere, without even noticing. This is also causing other issues, see for example: https://github.com/zephyrproject-rtos/hal_nxp/pull/230. So in practice, it is also preventing us from applying some Zephyr coding rules properly.
Proposed change
There's not much we can do about CMSIS architecture. The same applies to existing HALs. Ideally, HALs should provide a minimal header that fulfills CMSIS requirements (e.g. necessary types or definitions). Then, each soc could provide a
soc_cmsis.hthat can be used without including too much and so polluting almost every source file in Zephyr. This is still a fragile approach, as we are at the mercy of vendors doing the right job.Another approach is to not use CMSIS API within Zephyr code, at least code that lives in header files. While this may seem like a lot of work, other platforms have managed to implement the same or equivalent functionality without the need of CMSIS, see e.g. https://github.com/apache/nuttx/tree/master/arch/arm/src/armv7-m.
Detailed RFC
Pending discussion before proposal.
Proposed change (Detailed)
Dependencies
N/A
Concerns and Unresolved Questions
N/A
Alternatives
Do nothing.