-
Notifications
You must be signed in to change notification settings - Fork 778
Add Cortex-R support #2261
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
base: main
Are you sure you want to change the base?
Add Cortex-R support #2261
Conversation
boot/zephyr/arm_cleanup.c
Outdated
uint8_t i; | ||
uint8_t num_regions; | ||
uint32_t mpu_type_register; | ||
|
||
// disable MPU | ||
__asm__( | ||
" mrc p15, 0, r0, c1, c0, 0\n" | ||
" bic r0, #1\n" | ||
" mcr p15, 0, r0, c1, c0, 0\n" | ||
" isb\n" | ||
::: "r0"); | ||
|
||
// the number of MPU regions is stored in bits 15:8 of the MPU type register | ||
__asm__ volatile("mrc p15, 0, %0, c0, c0, 4\n" : "=r" (mpu_type_register) ::); | ||
num_regions = (uint8_t) ((mpu_type_register >> 8) & BIT_MASK(8)); | ||
|
||
for (i = 0; i < num_regions; ++i) { | ||
// select region in the MPU and clear the region size field | ||
__asm__ volatile( | ||
" mov r0, #0\n" | ||
" mcr p15, 0, %0, c6, c2, 0\n" | ||
" mcr p15, 0, r0, c6, c1, 2\n" | ||
:: "r" (i) : "r0"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be using zephyr APIs, should not be adding code, let alone assembly, here to do this (applies to clearing MPU, the boot assembly code is fine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the inline assmebly isn't really good here. I was also thinking about using Zephyr functions but then noticed that arm_core_mpu_disable
is not public (as in: Not in any header) and only interally used, meaning it could change.
For disabling (clearing) the individual regions Zephyr has a non-public header that provides ARM_MPU_ClrRegion
(link), which is the same name CMSIS provides only for Cortex-M cores.
boot/zephyr/include/arm_cleanup.h
Outdated
@@ -1,6 +1,7 @@ | |||
|
|||
/* | |||
* Copyright (c) 2020 Nordic Semiconductor ASA | |||
* Copyright (c) 2025 Siemens Mobility GmbH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
604b494
to
d4a3922
Compare
Thanks for the review. I addressed the changes in the last force-push and added a comment to the MPU |
72d9beb
to
783871b
Compare
@nordicjm can you please take a look again? I changed the MPU code to be more readable via macros but if you insist I can also add a Also sorry but I didn't notice that I removed the Copyright notices for the small changes in the release note commit instead of the original commit while rebasing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is OK to add, just needs nit. fixes
boot/zephyr/arm_cleanup.c
Outdated
|
||
#endif /* CONFIG_CPU_CORTEX_R5 */ | ||
|
||
void cleanup_arm_interrupts(void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline for {
boot/zephyr/arm_cleanup.c
Outdated
@@ -24,11 +53,27 @@ void cleanup_arm_nvic(void) { | |||
for (uint8_t i = 0; i < ARRAY_SIZE(NVIC->ICPR); i++) { | |||
NVIC->ICPR[i] = 0xFFFFFFFF; | |||
} | |||
#else | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4ed8635
to
5b4dfa2
Compare
First force-push is a capitalize first letter in the comment fix that I previously missed; second force-push are your suggestions. Do you mean it's ok to add the internal header or the inline ASM to read/write coprocessor registers via the macros? |
PR is OK as-is, would be good to move from here to zephyr itself then just call from here but that is optional/nice to have not required |
@m-braunschweig seemingly this branch has conflicts that must be resolved? Not sure what those are but can you check and fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have explicate ifdefry
#ifdef CONFIG_CPU_CORTEX_R5
for code for supporting Cortex-R in that patch.
This would help wit maintenance and improve legibility of Cortex-R relevant code.
boot/zephyr/arm_cleanup.c
Outdated
#if CONFIG_CPU_HAS_NXP_SYSMPU | ||
#include <fsl_sysmpu.h> | ||
#endif | ||
|
||
void cleanup_arm_nvic(void) { | ||
#ifndef CONFIG_CPU_CORTEX_M |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this only for CONFIG_CPU_CORTEX_R5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently yes. The GIC is also ARM specific while z_soc_irq_eoi
should is architecture agnostic
5b4dfa2
to
25b2643
Compare
Resolved. Was due to the Copyright line in the
Yeah. Are you fine with me splitting the |
@m-braunschweig I actually men rather using |
25b2643
to
8bf4803
Compare
Updated it. One problem is that we can't use Edit: I just saw |
Add Cortex-R support by using ifdefs to change the vector table, remove CORTEX_M only code and do an explicit bx instruction to switch from Thumb to ARM mode on exit of the MCUboot application. Signed-off-by: Mika Braunschweig <[email protected]>
Add code to cleanup the Cortex-R processor state to the reset configuration and disable + acknowledge all interrupts before entering the booted application. Signed-off-by: Mika Braunschweig <[email protected]>
Add notes about the additions made in order to support Cortex-R booting. Signed-off-by: Mika Braunschweig <[email protected]>
8bf4803
to
a3cca64
Compare
Updated it to use |
__set_CONTROL(0x00); /* application will configures core on its own */ | ||
__ISB(); | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am good with it as it is but can we do #ifdef CONFIG_ARMV7_R
here just to make the use case clearer. Same with other #else
s as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition, aside from my other comment, this looks good enough as it is
This PR adds support for Booting on a Cortex-R core. It has been tested with the following branch which also includes some instructions what Kconfig options need to be set: https://github.com/siemens/zephyr/tree/mika/ti/ti-am2434-native-boot