Skip to content

Refactor core optional parameters (FPU + DSP + Security extensions) #9480

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 31, 2019
Merged

Refactor core optional parameters (FPU + DSP + Security extensions) #9480

merged 5 commits into from
Jan 31, 2019

Conversation

deepikabhavnani
Copy link

@deepikabhavnani deepikabhavnani commented Jan 23, 2019 •
edited
Loading

Description

Core variant options were set differently in all toolchains and bit complex. Trying to have similar mechanism here for all toolchains and cleanup. Commit messages provide more details.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@kjbracey-arm @theotherjimmy

@deepikabhavnani
Copy link
Author

@jeromecoutant @mikisch81 @cyliangtw @mmahadevan108
Change in this PR is not breaking, but it will be good to test for all Armv8M devices locally to make sure behavior or functionality is not modified.

@ciarmcom ciarmcom requested review from kjbracey, theotherjimmy and a team January 23, 2019 22:00
@ciarmcom
Copy link
Member

@deepikabhavnani, thank you for your changes.
@theotherjimmy @kjbracey-arm @ARMmbed/mbed-os-tools @ARMmbed/mbed-os-maintainers please review.

kjbracey
kjbracey approved these changes Jan 24, 2019
View reviewed changes
theotherjimmy
theotherjimmy suggested changes Jan 24, 2019
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

Since this is quickly becoming a lookup table of if statements, could we make this a lookup table?

@deepikabhavnani deepikabhavnani force-pushed the core_arch_v8m branch 3 times, most recently from f7da0a7 to ff35021 Compare January 24, 2019 21:12
@cyliangtw
Copy link
Contributor

@deepikabhavnani, draft test by some sample on M2351 device and the behavior or functionality is not modified in both of GCC_ARM and ARM toolchain.

theotherjimmy
theotherjimmy suggested changes Jan 25, 2019
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

I think we're headed in the correct direction. I marked a few more things for conversion into lookup tables. Let me know if you would like me to do the conversion.

deepikabhavnani added 5 commits January 25, 2019 09:28
Use core arch for setting secure/non-secure flags
907c517
Armc6 - Set floating point and CPU options for all core variants expl…
c0750de 
…icitly

Below are the options read from the toolchains/arm
armclang --target=arm-arm-none-eabi -mcpu=list
The following arguments to option 'mcpu' can be selected:
  -mcpu=cortex-m0
  -mcpu=cortex-m0plus
  -mcpu=cortex-m1
  -mcpu=cortex-m3
  -mcpu=cortex-m4
  -mcpu=cortex-m7
  -mcpu=cortex-m23
  -mcpu=cortex-m33
  ...

armlink --cpu=list
The following arguments to option 'cpu' can be selected:
 --cpu=Cortex-M0
 --cpu=Cortex-M0plus
 --cpu=Cortex-M1
 --cpu=Cortex-M1.os_extension
 --cpu=Cortex-M1.no_os_extension
 --cpu=Cortex-M4
 --cpu=Cortex-M4.no_fp
 --cpu=Cortex-M7
 --cpu=Cortex-M7.fp.sp
 --cpu=Cortex-M7.no_fp
 --cpu=Cortex-M23
 --cpu=Cortex-M33
 --cpu=Cortex-M33.no_fp
 --cpu=Cortex-M33.no_dsp
 --cpu=Cortex-M33.no_dsp.no_fp
...

armclang --target=arm-arm-none-eabi -mfpu=list
The following arguments to option 'mfpu' can be selected:
  -mfpu=fpv4-sp-d16
  -mfpu=fpv5-sp-d16
  -mfpu=fpv5-d16
...
GCC_ARM: Strip the -NS from core option before setting floating point…
c472005 
… options
Change DSP variant symbol to E from D(d-double floating point)
f7d49fd
Change if statements to lookup tables
febbeff
theotherjimmy
theotherjimmy approved these changes Jan 25, 2019
View reviewed changes
Copy link
Contributor

@theotherjimmy theotherjimmy left a comment

Choose a reason for hiding this comment

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

I'll make a PR to convert the FPU arguments to tables.

@deepikabhavnani deepikabhavnani mentioned this pull request Jan 25, 2019
Merged
bridadan
bridadan approved these changes Jan 25, 2019
View reviewed changes
Copy link
Contributor

@bridadan bridadan left a comment

Choose a reason for hiding this comment

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

Nice work @deepikabhavnani

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 28, 2019

Ci started

@mbed-ci
Copy link

mbed-ci commented Jan 28, 2019

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@cmonr cmonr removed the needs: CI label Jan 28, 2019
@cmonr
Copy link
Contributor

cmonr commented Jan 28, 2019

@deepikabhavnani Could you take a look at the greentea test failures?

@cmonr
Copy link
Contributor

cmonr commented Jan 28, 2019

CI job restarted: jenkins-ci/mbed-os-ci_greentea-test

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 29, 2019

Restarted again

K66F failure, @ARMmbed/mbed-os-test we reported earlier today similar failures in another PR, related to kinetis devices

@0xc0170
Copy link
Contributor

0xc0170 commented Jan 29, 2019

Based on the changes, I marked this to 5.11.4. But Refactor in PR type would go to the next feature release.

@cmonr
Copy link
Contributor

cmonr commented Jan 29, 2019

@deepikabhavnani I'm confused by #9480 (comment). Should this build not have succeeded?

@deepikabhavnani
Copy link
Author

@cmonr - Sorry comment on wrong PR.. Will delete it to avoid confusion

@cmonr cmonr merged commit c9e00cf into ARMmbed:master Jan 31, 2019
@deepikabhavnani deepikabhavnani deleted the core_arch_v8m branch January 31, 2019 16:23
@mikisch81 mikisch81 mentioned this pull request Feb 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theotherjimmy theotherjimmy theotherjimmy approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@bridadan bridadan bridadan approved these changes

@kjbracey kjbracey kjbracey approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@deepikabhavnani @ciarmcom @cyliangtw @0xc0170 @mbed-ci @cmonr @theotherjimmy @bridadan @kjbracey