Refactor CMSIS

[stephanosio]

At the time of writing, the Zephyr repository only includes CMSIS-Core(M) variant under ext/hal/cmsis and assumes that the only form of "CMSIS" available is CMSIS-Core(M).

In order to facilitate the development of Cortex-A and Cortex-R ports, the CMSIS-to-Zephyr RTOS integration structure should be refactored to support multiple CMSIS-Core variants as well as other coprocessor and extension variants.

For more details, see the issue #19717.

[zephyrbot]

All checks passed.

checkpatch (informational only, not a failure)

-:289: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#289: FILE: modules/Kconfig.stm32:8:
+    select HAS_CMSIS_CORE$

- total: 0 errors, 1 warnings, 94 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[ioannisg]

It looks ok to me.
I added a few comments for additional commits that are actually unrelated to the patch. So @stephanosio I am fine if you don't want to do the changes in this PR.

[ioannisg]

@galak could you take a look as well?

[galak]

Hmm, wonder if this is better as 'default if CPU_CORTEX_M' instead of a select.

@ulfalizer thoughts?

[stephanosio]

To elaborate, the idea here is that HAS_CMSIS_CORE should automatically select an adequate CMSIS-Core type based on the CPU type.

For example, when CMSIS-Core(R) is added in the next PR, it would look something like this:

config HAS_CMSIS_CORE
	bool
	select HAS_CMSIS_CORE_M if CPU_CORTEX_M
	select HAS_CMSIS_CORE_R if CPU_CORTEX_R

The rationale for using select instead of default is as follows:

1. Since CMSIS-Core is a "driver" for CPU core, it makes very little sense to select a type that does not match the CPU type. For instance, if you have CPU_CORTEX_M selected, you would always want to select HAS_CMSIS_CORE_M which enables CMSIS-Core(M). For this reason, other config files should never need to override this.

2. Once HAS_CMSIS_CORE is selected, the right type of CMSIS-Core must be enabled. For example, it makes no sense to select HAS_CMSIS_CORE and then do HAS_CMSIS_CORE_M=n in another config file.

[ioannisg]

This design solves another problem. Assume a vendor with all Cortex-A, Cortex-M and Cortex-R support. The vendor module will select HAS_CMSIS in the root module Kconfig file, without having to care about the different ARM variants. The right type will be selected based on the actual CPU variant.

So I like the solution by proposed here by @stephanosio .

Of course, these options must be used somewhere.

[galak]

Generally looks good, one question about how to structure the Kconfig.

[ioannisg]

Generally looks good, one question about how to structure the Kconfig.

IMHO, HAS_CMSIS_CORE should not be selected by CPU_CORTEX_M. The option should signify that the vendor headers include the CMSIS headers.

In theory, there could be ARM vendor HAL headers that do not include CMSIS and define own custom ARM headers.

[stephanosio]

IMHO, HAS_CMSIS_CORE should not be selected by CPU_CORTEX_M. The option should signify that the vendor headers include the CMSIS headers.

The problem with not selecting HAS_CMSIS_CORE for CPU_CORTEX_M is that arch/arm/core directly makes use of CMSIS-Core in places like the following:

zephyr/arch/arm/core/prep_c.c

Lines 91 to 97 in f79fbac

	#if defined(CONFIG_USERSPACE)
	/* Full access */
	SCB->CPACR |= CPACR_CP10_FULL_ACCESS | CPACR_CP11_FULL_ACCESS;
	#else
	/* Privileged access only */
	SCB->CPACR |= CPACR_CP10_PRIV_ACCESS | CPACR_CP11_PRIV_ACCESS;
	#endif /* CONFIG_USERSPACE */

Since CPU_CORTEX_M signifies that ARM Cortex-M arch is enabled and this arch implementation depends on CMSIS-Core, HAS_CMSIS_CORE should be selected by it.

[ioannisg]

IMHO, HAS_CMSIS_CORE should not be selected by CPU_CORTEX_M. The option should signify that the vendor headers include the CMSIS headers.

The problem with not selecting HAS_CMSIS_CORE for CPU_CORTEX_M is that arch/arm/core directly makes use of CMSIS-Core in places like the following:

zephyr/arch/arm/core/prep_c.c

Lines 91 to 97 in f79fbac

	#if defined(CONFIG_USERSPACE)
	/* Full access */
	SCB->CPACR |= CPACR_CP10_FULL_ACCESS | CPACR_CP11_FULL_ACCESS;
	#else
	/* Privileged access only */
	SCB->CPACR |= CPACR_CP10_PRIV_ACCESS | CPACR_CP11_PRIV_ACCESS;
	#endif /* CONFIG_USERSPACE */

Since CPU_CORTEX_M signifies that ARM Cortex-M arch is enabled and this arch implementation depends on CMSIS-Core, HAS_CMSIS_CORE should be selected by it.

But that's my point @stephanosio : arch/arm/ code requires some sort of CMSIS compilant headers/defines, but not necessarily the ARM-provided CMSIS; maybe we could have custom-defined. I thought that is the point of this Kconfig option (HAS_CMSIS): to signify that the vendor headers pull-in CMSIS headers and not own-defined.

This is just my thought, however; it seems the option has not been actually used like this.

@galak perhaps we could actually do as i propose, for the ARM SoCs that don't pull-in CMSIS:
We could use this option in include/arch/arm/cortex-m/cmsis.h and use custom defined CMSIS if the option is not enabled by the vendor module.Now we have a hackish solution with the NVIC priority bits ;)

Now: if we accept that, by default, all ARM vendor headers must pull-in the right ARM CMSIS, we could even depreciate the option.

[stephanosio]

Now: if we accept that, by default, all ARM vendor headers must pull-in the right ARM CMSIS, we could even depreciate the option.

I think that would be the safest approach (i.e. require all vendor HALs to use the CMSIS-Core that is included in the Zephyr repository). If the original HAL provided by a vendor includes a subset of ARM CMSIS-Core, just remove that portion in the imported HAL that and make the necessary changes to use the official CMSIS-Core instead.

The rationale is as follows:

1. If any, the official ARM CMSIS-Core should be more complete and up-to-date than the one that is provided by the vendor.
2. Making a vendor HAL use the ARM CMSIS-Core should be very easy (after all, that is the whole point of having "Cortex Microcontroller Software Interface Standard").
3. Having different versions of CMSIS that can be used, given that two CMSISes cannot coexist for the same project, will likely introduce compatibility issues in the future (e.g. arch/arm expects a certain flavour of CMSIS but HAL provides a different flavour).
4. When ARM CMSIS-DSP is added in the future, since it makes heavy use of ARM CMSIS-Core, vendor HAL's CMSIS-Core probably won't be complete enough to support ARM CMSIS-DSP.

[ioannisg]

So, now, this selects CMSIS_CORE. But CMSIS_CORE is also selected at the module Kconfig.
Can we change this to "implies"? @ulfalizer @galak ?

[galak]

Let's make a slight tweak to the Kconfig