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

cpu/cortexm_common: make SVC call always available #20603

Closed
wants to merge 3 commits into from

Conversation

dylad
Copy link
Member

@dylad dylad commented Apr 19, 2024

Contribution description

This PR proposes to make SVC call always available for user and thus removes the cortexm_svc pseudomodule.
SVCall is available on all ARMvx-M architecture. This exception was used before to trigger PendSV for the sheduler. Now PendSV is triggered manually and no longer require to use SVCall.
Since the feature was provided by default for all cortexm, I think it makes sense to remove the pseudomodule and to let it hardcoded in the code.

Testing procedure

CI should be happy.

Issues/PRs references

None.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: build system Area: Build system Area: pkg Area: External package ports Area: cpu Area: CPU/MCU ports labels Apr 19, 2024
Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

Looks reasonable, didn't test it

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 15, 2024
@riot-ci
Copy link

riot-ci commented May 15, 2024

Murdock results

✔️ PASSED

4dc8a4a pkg/wamr: replace cortexm_svc FEATURE by arch_arm

Success Failures Total Runtime
10177 0 10178 14m:15s

Artifacts

@dylad dylad force-pushed the pr/cpu/cortexm/remove_svc_module branch 2 times, most recently from 9d087c9 to 7b79f67 Compare May 15, 2024 18:43
@dylad dylad force-pushed the pr/cpu/cortexm/remove_svc_module branch from 7b79f67 to acf9882 Compare May 15, 2024 18:56
@dylad
Copy link
Member Author

dylad commented May 18, 2024

I didn't know arch_arm would also include ARM7TMDI.
I've added a fixup commit to blacklist arch_arm7 from pkg/wamr.
I'll squash if Murdock is happy about the fix.

@Teufelchen1
Copy link
Contributor

Hey @dylad what do you need to move this forward?

@dylad
Copy link
Member Author

dylad commented Jun 17, 2024

I forgot about this one !
If ACK is still valid, I just need to squash this.

Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

I can just give you second ACK :3

@dylad
Copy link
Member Author

dylad commented Jun 17, 2024

@kaspar030 Any objections here ?

@dylad dylad force-pushed the pr/cpu/cortexm/remove_svc_module branch from 05e8142 to 4dc8a4a Compare June 18, 2024 07:38
@dylad dylad added this pull request to the merge queue Jun 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 18, 2024
@dylad
Copy link
Member Author

dylad commented Jun 18, 2024

Looks like Murdock failed because this PR introduces some overhead.
But cortemx_svc was already there by default so I'm quite confused with this.

@dylad
Copy link
Member Author

dylad commented Jun 20, 2024

I'm starting to reconsider this. I was wrong thinking that cortexm_svc was pulled by default already. This is NOT the case.
Indeed the feature is provided for all Cortex-M but there is no FEATURES_REQUIRED=cortexm_svc anywhere. Thus, the default mode is still the legacy one.
Switching to the new behavior will increase ROM by A LOT. Since we do not have a real use case for SVC exception yet. (except for context switching) This PR is not worthwhile.
I'm closing this.

@dylad dylad closed this Jun 20, 2024
@Teufelchen1
Copy link
Contributor

That's what I get for not thinking stuff through 🥲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: cpu Area: CPU/MCU ports Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants