Skip to content

Refactor CMSIS #19875

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion arch/arm/core/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ config CPU_CORTEX_M
bool
select CPU_CORTEX
select ARCH_HAS_CUSTOM_SWAP_TO_MAIN
select HAS_CMSIS
select HAS_CMSIS_CORE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

select HAS_FLASH_LOAD_OFFSET
select ARCH_HAS_THREAD_ABORT
select ARCH_HAS_TRUSTED_EXECUTION if ARM_TRUSTZONE_M
Expand Down
12 changes: 2 additions & 10 deletions ext/hal/cmsis/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,11 +1,3 @@
if(CONFIG_HAS_CMSIS)
zephyr_include_directories(Include)
endif()
# SPDX-License-Identifier: Apache-2.0

# As of CMSIS v5.6.0, __PROGRAM_START is to indicate whether the
# ARM vendor or the OS supplies data/bss init routine, otherwise
# the default data/bss init routine for the selected toolchain is
# added. We set the macro in build-time to guarantee compatibility
# with all existing ARM platforms.

zephyr_compile_definitions(__PROGRAM_START)
add_subdirectory_ifdef(CONFIG_HAS_CMSIS_CORE_M Core)
11 changes: 11 additions & 0 deletions ext/hal/cmsis/Core/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# SPDX-License-Identifier: Apache-2.0

zephyr_include_directories(Include)

# As of CMSIS v5.6.0, __PROGRAM_START is to indicate whether the
# ARM vendor or the OS supplies data/bss init routine, otherwise
# the default data/bss init routine for the selected toolchain is
# added. We set the macro in build-time to guarantee compatibility
# with all existing ARM platforms.

zephyr_compile_definitions(__PROGRAM_START)
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
13 changes: 9 additions & 4 deletions ext/hal/cmsis/Kconfig
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
#
# Copyright (c) 2016 Intel Corporation
#
# SPDX-License-Identifier: Apache-2.0
#

config HAS_CMSIS
config HAS_CMSIS_CORE
bool
select HAS_CMSIS_CORE_M if CPU_CORTEX_M
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ulfalizer thoughts?

Copy link
Member Author

@stephanosio stephanosio Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if HAS_CMSIS_CORE

config HAS_CMSIS_CORE_M
bool

endif
16 changes: 8 additions & 8 deletions ext/hal/cmsis/README
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
The ARM Cortex Microcontroller Software Interface Standard (CMSIS) defines a
set of standard interfaces to ARM Cortex-M SOCs. In particular, the CMSIS-CORE
component standardizes the software interface to core and peripheral registers,
as well as exception names and the system clock frequency. Multiple SOC
vendors, including NXP and Nordic Semiconductor, include the CMSIS-CORE header
files in their SOC header files. These SOC header files are in turn used by
the vendor's peripheral drivers.
set of standard interfaces to ARM Cortex family SOCs. In particular, the
CMSIS-CORE component standardizes the software interface to core and peripheral
registers, as well as exception names and the system clock frequency. Multiple
SOC vendors, including NXP and Nordic Semiconductor, include the CMSIS-CORE
header files in their SOC header files. These SOC header files are in turn used
by the vendor's peripheral drivers.

http://www.arm.com/products/processors/cortex-m/cortex-microcontroller-software-interface-standard.php

The sources in this directory are imported from
https://github.com/ARM-software/CMSIS_5.git

The current version supported in Zephyr is
https://github.com/ARM-software/CMSIS_5/releases/tag/5.5.1
The current version supported in Zephyr is
https://github.com/ARM-software/CMSIS_5/releases/tag/5.6.0
2 changes: 1 addition & 1 deletion modules/Kconfig.atmel
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

config ASF
bool
select HAS_CMSIS
select HAS_CMSIS_CORE
depends on SOC_FAMILY_SAM || SOC_FAMILY_SAM0

config ATMEL_WINC1500
Expand Down
2 changes: 1 addition & 1 deletion modules/Kconfig.cypress
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@

config HAS_CYPRESS_DRIVERS
bool
select HAS_CMSIS
select HAS_CMSIS_CORE
2 changes: 1 addition & 1 deletion modules/Kconfig.imx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

config HAS_IMX_HAL
bool
select HAS_CMSIS
select HAS_CMSIS_CORE
depends on SOC_FAMILY_IMX

if HAS_IMX_HAL
Expand Down
2 changes: 1 addition & 1 deletion modules/Kconfig.mcux
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

config HAS_MCUX
bool
select HAS_CMSIS
select HAS_CMSIS_CORE
depends on SOC_FAMILY_KINETIS || SOC_FAMILY_IMX || SOC_FAMILY_LPC

if HAS_MCUX
Expand Down
1 change: 1 addition & 0 deletions modules/Kconfig.nordic
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ config HAS_NORDIC_DRIVERS

config HAS_NRFX
bool
select HAS_CMSIS_CORE

menu "nrfx drivers"
depends on HAS_NRFX
Expand Down
2 changes: 1 addition & 1 deletion modules/Kconfig.silabs
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,5 @@

config HAS_SILABS_GECKO
bool
select HAS_CMSIS
select HAS_CMSIS_CORE
depends on SOC_FAMILY_EXX32
2 changes: 1 addition & 1 deletion modules/Kconfig.simplelink
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ config SIMPLELINK_HOST_DRIVER

config HAS_MSP432P4XXSDK
bool
select HAS_CMSIS
select HAS_CMSIS_CORE

# Kconfig - CC13X2 / CC26X2 SDK HAL configuration

Expand Down
2 changes: 1 addition & 1 deletion modules/Kconfig.stm32
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

config HAS_STM32CUBE
bool
select HAS_CMSIS
select HAS_CMSIS_CORE
depends on SOC_FAMILY_STM32

if HAS_STM32CUBE
Expand Down
1 change: 0 additions & 1 deletion soc/arm/nordic_nrf/nrf51/Kconfig.series
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ config SOC_SERIES_NRF51X
select SOC_FAMILY_NRF
select HAS_SYS_POWER_STATE_DEEP_SLEEP_1
select XIP
select HAS_CMSIS
select HAS_NRFX
select HAS_SEGGER_RTT
help
Expand Down
1 change: 0 additions & 1 deletion soc/arm/nordic_nrf/nrf52/Kconfig.series
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ config SOC_SERIES_NRF52X
select SOC_FAMILY_NRF
select HAS_SYS_POWER_STATE_DEEP_SLEEP_1
select XIP
select HAS_CMSIS
select HAS_NRFX
select HAS_SEGGER_RTT
help
Expand Down
1 change: 0 additions & 1 deletion soc/arm/nordic_nrf/nrf91/Kconfig.series
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ config SOC_SERIES_NRF91X
select SOC_FAMILY_NRF
select HAS_SYS_POWER_STATE_DEEP_SLEEP_1
select XIP
select HAS_CMSIS
select HAS_NRFX
select HAS_SEGGER_RTT
help
Expand Down
1 change: 0 additions & 1 deletion soc/arm/nxp_kinetis/k2x/Kconfig.soc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ config SOC_MK22F51212
select HAS_MCUX_SIM
select HAS_OSC
select HAS_MCG
select HAS_CMSIS
select CPU_HAS_FPU

endchoice
Expand Down