modules: cmsis, cmsis_6: only add the intended cmsis module

[fabiobaltieri]

The current code base is meant to use cmsis for Cortex A and R and cmsis_6 for Cortex M, but the build system is configured to include the path for both when Cortex M is selected. This leaves us exposed to PR using the old headers, that would not get caught in CI but would fail the build on a project using Cortex M that only has the cmsis_6 module.

Change the cmsis module setting to only include the module files in the intended case.

Deps

• mec_ecia: use the cmsis_6 register name hal_microchip#35
• hal: use the cmsis_6 define name hal_ambiq#56
• si91x: use the cmsis_6 defines hal_silabs#122

[fabiobaltieri]

This should fail until #93707 is merged -- it does

[fabiobaltieri]

@wearyzen can we delete https://github.com/zephyrproject-rtos/cmsis/tree/master/CMSIS/Core entirely or there's a reason to keep it? Would still do this change as well but I'm happy to open a PR to drop that code if we don't need it anymore.

I am not sure if this will be enough,
cmsis-dsp uses the old cmsis here :

zephyr/modules/cmsis-dsp/CMakeLists.txt

Line 7 in cba9dce

set(cmsis_glue_path ${ZEPHYR_CMSIS_MODULE_DIR})

cmsis-nn here:

zephyr/modules/cmsis-nn/CMakeLists.txt

Line 7 in cba9dce

set(cmsis_glue_path ${ZEPHYR_CMSIS_MODULE_DIR})

So even if the ci doesn't catch this, this could result in another bug report

[wearyzen]

@wearyzen can we delete https://github.com/zephyrproject-rtos/cmsis/tree/master/CMSIS/Core entirely or there's a reason to keep it? Would still do this change as well but I'm happy to open a PR to drop that code if we don't need it anymore.

There are modules still using it so I don't think we should delete it.

[fabiobaltieri]

ok but it looks like both cmsis-dsp and cmsis-nn have that include line behind a CONFIG entry, so CI should still catch it for tests that do not enable those modules (i.e. most of them, I did run this change before merging the revert and it did indeed fail)

[fabiobaltieri]

Also, looks like it's not that unused after all, even putting those modules aside :-)

[wearyzen]

Also, looks like it's not that unused after all, even putting those modules aside :-)

Maybe we introduce a new config USE_CMSIS_5_MODULE which cmsis-nn and cmsis-dsp (or the other current reported modules) enables and is enabled by default for Cortex-A and R, and then the include goes under this config?

[wearyzen]

Also, looks like it's not that unused after all, even putting those modules aside :-)

Maybe we introduce a new config USE_CMSIS_5_MODULE which cmsis-nn and cmsis-dsp (or the other current reported modules) enables and is enabled by default for Cortex-A and R, and then the include goes under this config?

Yeah I think this would be good to keep track of who is using the old cmsis as well.

[wearyzen]

This seems to fix things locally for me and covers all the current CI failures

git diff
diff --git a/modules/Kconfig.microchip b/modules/Kconfig.microchip
index 7497d99ead0..947d92e9cf1 100644
--- a/modules/Kconfig.microchip
+++ b/modules/Kconfig.microchip
@@ -10,3 +10,6 @@ config HAS_MPFS_HAL

 config HAS_MEC5_HAL
        bool "Microchip MEC5 HAL drivers support"
+
+config HAS_CMSIS_5_CORE_M
+       default y
diff --git a/modules/cmsis/CMakeLists.txt b/modules/cmsis/CMakeLists.txt
index 4044ae891b3..b1c2bc270fd 100644
--- a/modules/cmsis/CMakeLists.txt
+++ b/modules/cmsis/CMakeLists.txt
@@ -1,7 +1,10 @@
 # Copyright (c) 2023 Nordic Semiconductor ASA
 # SPDX-License-Identifier: Apache-2.0

-if(CONFIG_CPU_AARCH32_CORTEX_A OR CONFIG_CPU_AARCH32_CORTEX_R)
+if(CONFIG_HAS_CMSIS_5_CORE_M OR CONFIG_HAS_CMSIS_CORE_A OR CONFIG_HAS_CMSIS_CORE_R)
   add_subdirectory(${ZEPHYR_CURRENT_MODULE_DIR} cmsis)
+endif()
+
+if(CONFIG_CPU_AARCH32_CORTEX_A OR CONFIG_CPU_AARCH32_CORTEX_R)
   zephyr_include_directories(.)
 endif()
diff --git a/modules/cmsis/Kconfig b/modules/cmsis/Kconfig
index 9cc5ee79630..617dfae4952 100644
--- a/modules/cmsis/Kconfig
+++ b/modules/cmsis/Kconfig
@@ -4,6 +4,9 @@
 config ZEPHYR_CMSIS_MODULE
        bool

+config HAS_CMSIS_5_CORE_M
+       bool
+
 config HAS_CMSIS_CORE
        bool
        select HAS_CMSIS_CORE_A if CPU_AARCH32_CORTEX_A

[fabiobaltieri]

This seems to fix things locally for me and covers all the current CI failures

mmh that would enable HAS_CMSIS_5_CORE_M for everything, that set should go somewhere else, but listen I think I'd rather have the failing platform fixed and hold this rather than introducing another temporary workaround

[fabiobaltieri]

Blocked on #93725

[github-actions]

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_ambiq	zephyrproject-rtos/hal_ambiq@84ccbfc	zephyrproject-rtos/hal_ambiq@e921258	zephyrproject-rtos/[email protected]
hal_microchip	zephyrproject-rtos/hal_microchip@32a79d4	zephyrproject-rtos/hal_microchip@415c92d (master)	zephyrproject-rtos/[email protected]

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[wearyzen]

wow that was quick 😃 Looks like you covered everything that Zephyr's ci covers!
Adding a small comment otherwise LGTM, thanks again for adding this change!

[fabiobaltieri]

wow that was quick 😃 Looks like you covered everything that Zephyr's ci covers!

Yeah well, that's what this specific run covers, there must be more in the hals but I really don't have an effective way of chasing that down so I'm afraid this is the best we can do.

[AlessandroLuo]

FYI zephyrproject-rtos/hal_ambiq#56 is merged.

[fabiobaltieri]

thanks, just missing the microchip one then this is good to go

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[fabiobaltieri]

Alright good to go.