-
Notifications
You must be signed in to change notification settings - Fork 7.4k
modules: CMSIS_6: fix path and use it for TF-M and Cortex-M #89370
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
The following west manifest projects have changed revision in this Pull Request:
Additional metadata changed:
⛔ DNM label due to: 1 project with metadata changes Note: This message is automatically posted and updated by the Manifest GitHub Action. |
Ah. Good idea just enabling it for M only. |
7f6707d
to
320db7b
Compare
The twister failures are observed on main branch as well so they are not related to this PR. |
Thanks for this. Could you also put up a PR in the TF-M fork that removes the copy of the CMSIS 6 files (just reverting the commit)? |
|
Hi @stephanosio , could you please have a look at this PR and help with approval/merge? |
Replacing the assignee @stephanosio as he hasn't been active, the last GitHub activity is from two weeks ago, and this PR is now blocking other TF-M PRs. |
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.
Doesn't this change warrant a line in the release notes?
(CC @kartben @danieldegrasse @dkalowsk)
(I'm ok with that coming as a follow up PR)
@@ -0,0 +1,9 @@ | |||
# Copyright (c) 2023 Nordic Semiconductor ASA |
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.
Suggestion:
This file content is below the trivial threshold, you would not need to keep somebody else's copyright if you copy and modify it.
You can also just add a Copyright The Zephyr Project Contributors
notice instead of adding a new line for ARM (if the file has more copyright and license lines than code it is probably overkill :))
Same applies to modules/cmsis_6/cmsis_core.h
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.
Thanks, I'll push a follow up PR removing the original copyright and a note in the release note.
Hey, this breaks any downstream project that has hal module filtering enabled does not have cmsis listed and is using ARM. Going to break the example app too:
it really needs this to be mentioned in the release notes, something about the requirement of using the cmsis_6 module instead of cmsis if Cortex-M is used. Opened zephyrproject-rtos/example-application#84 for the example application. |
Or is it meant to need both
if it only has |
@fabiobaltieri, the combination is expected atm because some of the hal's like stm32 might be using the older cmsis and we'll have to wait for them to upgrade. This wasn't possible before because there was no CMSIS_6 module for them to update to but now they should be able to do so. BTW, could you please share the command you tried for the above error? This looks like something that can be easily fixed with the right path. |
Tried to build a few stm32 projects (including nucleo_f429zi/stm32f429xx which includes the file dwt.h) but couldn't see the issue reported, let me know if you find the board used to reproduce this error. Thanks! |
All good, don't worry about it.
Ah it was from one of my test projects, had a CI run with minimal modules, let's see you can do:
I think it's enabled from these in the specific project
|
Thanks, I found the issue and will be posting a fix soon with the release notes update. |
As noted in PR zephyrproject-rtos#89370, the content of these files falls below the trivial threshold. Therefore, it is not necessary to retain the original copyright, which was carried over when the files were copied from the `cmsis` module. Signed-off-by: Sudan Landge <sudan.landge@arm.com>
As noted in PR zephyrproject-rtos#89370, the content of these files falls below the trivial threshold. Therefore, it is not necessary to retain the original copyright, which was carried over when the files were copied from the `cmsis` module. Signed-off-by: Sudan Landge <sudan.landge@arm.com>
Zephyr has migrated to CMSIS v6. While the plan was to transition all ARM architectures, issues with Cortex-A and Cortex-R prevented full adoption. As a result, only Cortex-M was updated to v6 in zephyrproject-rtos/zephyr#89370. Since the SC-OBC module A1 uses a Cortex-M3, we need to adopt CMSIS v6. That PR was merged after Zephyr v4.1.0, so older versions may still require the legacy CMSIS module. To maintain compatibility, we are keeping the old module alongside v6.
Zephyr has migrated to CMSIS v6. While the plan was to transition all ARM architectures, issues with Cortex-A and Cortex-R prevented full adoption. As a result, only Cortex-M was updated to v6 in zephyrproject-rtos/zephyr#89370. Since the SC-OBC module A1 uses a Cortex-M3, we need to adopt CMSIS v6. That PR was merged after Zephyr v4.1.0, so older versions may still require the legacy CMSIS module. To maintain compatibility, we are keeping the old module alongside v6. Signed-off-by: Yasushi SHOJI <yashi@spacecubics.com>
What is the change?
Switch to using CMSIS_6 module for TF-M and Cortex-M while continue to use CMSIS for Cortex-A/R.
Why do we need this change?
Pls refer to the commit messages for more details.
Signed-off-by: Sudan Landge sudan.landge@arm.com