[RFC] soc.h and CMSIS: a difficult story

[gmarull]

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 #46585 and #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:

zephyr/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h

Line 19 in 7b8170d

#include <soc.h>

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_Type type, 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:

zephyr/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h

Lines 25 to 80 in 7b8170d

	/* Fill in CMSIS required values for non-CMSIS compliant SoCs.
	* Use __NVIC_PRIO_BITS as it is required and simple to check, but
	* ultimately all SoCs will define their own CMSIS types and constants.
	*/
	#ifndef __NVIC_PRIO_BITS
	typedef enum {
	Reset_IRQn = -15,
	NonMaskableInt_IRQn = -14,
	HardFault_IRQn = -13,
	#if defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE)
	MemoryManagement_IRQn = -12,
	BusFault_IRQn = -11,
	UsageFault_IRQn = -10,
	#if defined(CONFIG_ARM_SECURE_FIRMWARE)
	SecureFault_IRQn = -9,
	#endif /* CONFIG_ARM_SECURE_FIRMWARE */
	#endif /* CONFIG_ARMV7_M_ARMV8_M_MAINLINE */
	SVCall_IRQn = -5,
	DebugMonitor_IRQn = -4,
	PendSV_IRQn = -2,
	SysTick_IRQn = -1,
	Max_IRQn = CONFIG_NUM_IRQS,
	} IRQn_Type;
	#if defined(CONFIG_CPU_CORTEX_M0)
	#define __CM0_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M0PLUS)
	#define __CM0PLUS_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M1)
	#define __CM1_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M3)
	#define __CM3_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M4)
	#define __CM4_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M7)
	#define __CM7_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M23)
	#define __CM23_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M33)
	#define __CM33_REV 0
	#elif defined(CONFIG_CPU_CORTEX_M55)
	#define __CM55_REV 0
	#else
	#error "Unknown Cortex-M device"
	#endif
	#ifndef __MPU_PRESENT
	#define __MPU_PRESENT 0U
	#endif
	#define __NVIC_PRIO_BITS NUM_IRQ_PRIO_BITS
	#define __Vendor_SysTickConfig 0 /* Default to standard SysTick */
	#endif /* __NVIC_PRIO_BITS */
	#if __NVIC_PRIO_BITS != NUM_IRQ_PRIO_BITS
	#error "NUM_IRQ_PRIO_BITS and __NVIC_PRIO_BITS are not set to the same value"
	#endif

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:

zephyr/samples/hello_world/src/main.c

Lines 7 to 13 in 7b8170d

	#include <zephyr/kernel.h>
	int main(void)
	{
	printk("Hello World! %s\n", CONFIG_BOARD);
	return 0;
	}

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:

[141/153] Building C object zephyr/kernel/CMakeFiles/kernel.dir/sched.c.obj
In file included from /home/gmarull/ws/nordic/zephyrproject/modules/hal/gigadevice/gd32f4xx/standard_peripheral/include/gd32f4xx_libopt.h:47,
                 from /home/gmarull/ws/nordic/zephyrproject/modules/hal/gigadevice/gd32f4xx/cmsis/gd/gd32f4xx/include/gd32f4xx.h:364,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/soc/arm/gigadevice/gd32f4xx/soc.h:11,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h:19,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/mpu/arm_mpu_v7m.h:11,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/mpu/arm_mpu.h:14,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/arch.h:266,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/cpu.h:19,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/kernel_includes.h:33,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/kernel.h:17,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/kernel/sched.c:6:
/home/gmarull/ws/nordic/zephyrproject/modules/hal/gigadevice/gd32f4xx/standard_peripheral/include/gd32f4xx_dac.h:43:2: warning: #warning "Hey, I'm HAL DAC header!" [-Wcpp]
   43 | #warning "Hey, I'm HAL DAC header!"
      |  ^~~~~~~
[143/153] Linking C executable zephyr/zephyr_pre0.elf

[147/153] Linking C executable zephyr/zephyr_pre1.elf

[152/153] Building C object zephyr/CMakeFiles/zephyr_final.dir/isr_tables.c.obj
In file included from /home/gmarull/ws/nordic/zephyrproject/modules/hal/gigadevice/gd32f4xx/standard_peripheral/include/gd32f4xx_libopt.h:47,
                 from /home/gmarull/ws/nordic/zephyrproject/modules/hal/gigadevice/gd32f4xx/cmsis/gd/gd32f4xx/include/gd32f4xx.h:364,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/soc/arm/gigadevice/gd32f4xx/soc.h:11,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h:19,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/mpu/arm_mpu_v7m.h:11,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/mpu/arm_mpu.h:14,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/arm/aarch32/arch.h:266,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/include/zephyr/arch/cpu.h:19,
                 from /home/gmarull/ws/nordic/zephyrproject/zephyr/build/zephyr/isr_tables.c:7:
/home/gmarull/ws/nordic/zephyrproject/modules/hal/gigadevice/gd32f4xx/standard_peripheral/include/gd32f4xx_dac.h:43:2: warning: #warning "Hey, I'm HAL DAC header!" [-Wcpp

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: zephyrproject-rtos/hal_nxp#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.h that 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)

• Remove all CMSIS dependencies from header files. Then, cleanup soc.h.
• Check if possible to better isolate some internals?

Dependencies

N/A

Concerns and Unresolved Questions

N/A

Alternatives

Do nothing.

[carlescufi]

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

  zephyr/include/zephyr/arch/arm/aarch32/cortex_m/cmsis.h

  Lines 25 to 80 in 7b8170d

  	/* Fill in CMSIS required values for non-CMSIS compliant SoCs.
  	* Use __NVIC_PRIO_BITS as it is required and simple to check, but
  	* ultimately all SoCs will define their own CMSIS types and constants.
  	*/
  	#ifndef __NVIC_PRIO_BITS
  	typedef enum {
  	Reset_IRQn = -15,
  	NonMaskableInt_IRQn = -14,
  	HardFault_IRQn = -13,
  	#if defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE)
  	MemoryManagement_IRQn = -12,
  	BusFault_IRQn = -11,
  	UsageFault_IRQn = -10,
  	#if defined(CONFIG_ARM_SECURE_FIRMWARE)
  	SecureFault_IRQn = -9,
  	#endif /* CONFIG_ARM_SECURE_FIRMWARE */
  	#endif /* CONFIG_ARMV7_M_ARMV8_M_MAINLINE */
  	SVCall_IRQn = -5,
  	DebugMonitor_IRQn = -4,
  	PendSV_IRQn = -2,
  	SysTick_IRQn = -1,
  	Max_IRQn = CONFIG_NUM_IRQS,
  	} IRQn_Type;
  	#if defined(CONFIG_CPU_CORTEX_M0)
  	#define __CM0_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M0PLUS)
  	#define __CM0PLUS_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M1)
  	#define __CM1_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M3)
  	#define __CM3_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M4)
  	#define __CM4_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M7)
  	#define __CM7_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M23)
  	#define __CM23_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M33)
  	#define __CM33_REV 0
  	#elif defined(CONFIG_CPU_CORTEX_M55)
  	#define __CM55_REV 0
  	#else
  	#error "Unknown Cortex-M device"
  	#endif
  	#ifndef __MPU_PRESENT
  	#define __MPU_PRESENT 0U
  	#endif
  	#define __NVIC_PRIO_BITS NUM_IRQ_PRIO_BITS
  	#define __Vendor_SysTickConfig 0 /* Default to standard SysTick */
  	#endif /* __NVIC_PRIO_BITS */
  	#if __NVIC_PRIO_BITS != NUM_IRQ_PRIO_BITS
  	#error "NUM_IRQ_PRIO_BITS and __NVIC_PRIO_BITS are not set to the same value"
  	#endif

  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.h specific 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.h in 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]

Hi @XenuIsWatching, @microbuilder, @stephanosio, @kristofer-jonsson-arm,

This issue, marked as an RFC, was opened a while ago and did not get any traction. It was just assigned to you based on the labels. If you don't consider yourself the right person to address this issue, please re-assing it to the right person.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

@gmarull you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you.

Thanks!