Skip to content
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

RFC: arch: arm: Remove aarch32 directory and split Cortex-M and Cortex-A/r code #60031

Conversation

SgrrZhf
Copy link
Member

@SgrrZhf SgrrZhf commented Jul 5, 2023

This PR tries to do some cleaning work in the arch/arm/core directory and split some common code into Cortex-M and Cortex-A/R parts. This is a huge work, hope you can give your comments on this.

Below is the major changes:

  1. It doesn't make sense to keep the aarch32 directory in the 'arch/arm/core' directory as the aarch64 has been moved out. This PR try to clean the 'aarch32' directory in 'arch/arm/core' and 'include/zephyr/arch/arm'.

  2. There are too many differences between Cortex-M and Cortex-A/R, For easier to maintain, this PR try to split some common code in 'arch/arm/core'. They are:

    • arch/arm/core/swap.c
    • arch/arm/core/swap_helper.S
    • arch/arm/core/irq_manage.c
    • arch/arm/core/isr_wrapper.S
  3. Move 'arch/arm/core/irq_relay.S to the cortex_m directory because it is only used by Cortex-M architecture.

There is a known issue about cleaning the 'aarch32' directory in the include/zephyr/arch/arm directory that boot/zephyr/arm_cleanup.c in the mcuboot module includes a header file resided in include/zephyr/arch/arm/aarch32 directory. It will lead to a build error.

https://github.com/zephyrproject-rtos/mcuboot/blob/74c4d1c52fd51d07904b27a7aa9b2303e896a4e3/boot/zephyr/arm_cleanup.c#L7C43-L7C52

@SgrrZhf SgrrZhf force-pushed the topics/huizha01/split-cortex-m-and-cortex-ar branch from 102b8a8 to 61b0703 Compare September 13, 2023 07:48
@SgrrZhf
Copy link
Member Author

SgrrZhf commented Sep 13, 2023

@SgrrZhf could you rebase please? I think it clashed with 1fd3171, couple header removals

Done

@fabiobaltieri fabiobaltieri merged commit 80e1b37 into zephyrproject-rtos:main Sep 13, 2023
30 checks passed
@SgrrZhf SgrrZhf deleted the topics/huizha01/split-cortex-m-and-cortex-ar branch September 13, 2023 09:20
@tejlmand
Copy link
Collaborator

this PR has broken armclang support.

When doing major rework, please remember to test with armclang, or ping someone who can carry out such test.

With armclang i'm now seeing errors like this:

CMake Error at /projects/github/ncs/zephyr/cmake/linker/armlink/target.cmake:74 (target_link_libraries):
  Error evaluating generator expression:                                                                                                                                                                                                      

    $<TARGET_OBJECTS:arch__arm__core__aarch32__cortex_m>                                                                                                                                                                                      

  Objects of target "arch__arm__core__aarch32__cortex_m" referenced but no                                                                                                                                                                    
  such target exists.                                                                                                                                                                                                                         
Call Stack (most recent call first):                                                                                                                                                                                                          
  /projects/github/ncs/zephyr/CMakeLists.txt:1427 (toolchain_ld_link_elf)        

@tejlmand
Copy link
Collaborator

@fabiobaltieri FYI

@fabiobaltieri
Copy link
Member

fabiobaltieri commented Sep 13, 2023

When doing major rework, please remember to test with armclang, or ping someone who can carry out such test.

Is there any fundamental blocker from having an armclang run in CI? Sounds like the only sustainable option to me. We have a separate clang run already so unless there's some licensing issue I don't see why not.

@tejlmand
Copy link
Collaborator

We have a separate clang run already so unless there's some licensing issue I don't see why not.

we do have some licenses for armclang for develoment, but I guess no one has had the time to look into getting this up and running with CI.

Note also that we should checkup with arm that we are permitted to use those licenses in CI.

@tejlmand
Copy link
Collaborator

i'm curious why this PR, which touches so many files in 11 commits, only have a manifest label, and no build-system label or similar.

@tejlmand
Copy link
Collaborator

Fixed here: #62590

@fabiobaltieri
Copy link
Member

i'm curious why this PR, which touches so many files in 11 commits, only have a manifest label, and no build-system label or similar.

I don't think it touches any files under "Build system" (https://github.com/zephyrproject-rtos/zephyr/blob/main/MAINTAINERS.yml#L334-L342), but regardless, the script that does the tagging does not apply more than 10 tags, this check kicked in: https://github.com/zephyrproject-rtos/zephyr/blob/main/scripts/set_assignees.py#L158

tejlmand added a commit to tejlmand/zephyr that referenced this pull request Sep 13, 2023
Fixes: zephyrproject-rtos#62589
Follow-up: zephyrproject-rtos#60031

The PR zephyrproject-rtos#60031 moved CMake code to new folder location causing generated
library names to change.
This change impacted the use of those libraries in the Zephyr armlink
CMake code, causing CMake failures at configure time.

This PR fixes this failure by updating the armlink CMake code to use
the new library names.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
fabiobaltieri pushed a commit that referenced this pull request Sep 13, 2023
Fixes: #62589
Follow-up: #60031

The PR #60031 moved CMake code to new folder location causing generated
library names to change.
This change impacted the use of those libraries in the Zephyr armlink
CMake code, causing CMake failures at configure time.

This PR fixes this failure by updating the armlink CMake code to use
the new library names.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
@fabiobaltieri
Copy link
Member

When doing major rework, please remember to test with armclang, or ping someone who can carry out such test.

Note also that we should checkup with arm that we are permitted to use those licenses in CI.

This is not really a sustainable solution at the change volume that the project is dealing with. It also does not seem fair to block against a proprietary system that only few developer have access to.

If the tool is proprietary and we are not allowed to run in CI then regressions will have to be detected and fixed out band.

Dat-NguyenDuy pushed a commit to nxp-zephyr/zephyr that referenced this pull request Sep 20, 2023
Fixes: zephyrproject-rtos#62589
Follow-up: zephyrproject-rtos#60031

The PR zephyrproject-rtos#60031 moved CMake code to new folder location causing generated
library names to change.
This change impacted the use of those libraries in the Zephyr armlink
CMake code, causing CMake failures at configure time.

This PR fixes this failure by updating the armlink CMake code to use
the new library names.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
coran21 pushed a commit to coran21/zephyr that referenced this pull request Sep 21, 2023
Fixes: zephyrproject-rtos#62589
Follow-up: zephyrproject-rtos#60031

The PR zephyrproject-rtos#60031 moved CMake code to new folder location causing generated
library names to change.
This change impacted the use of those libraries in the Zephyr armlink
CMake code, causing CMake failures at configure time.

This PR fixes this failure by updating the armlink CMake code to use
the new library names.

Signed-off-by: Torsten Rasmussen <Torsten.Rasmussen@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants