Skip to content

Correct the floating options for Cortex-M7 and Cortex-M4 #9477

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

Closed
wants to merge 5 commits into from
Closed

Correct the floating options for Cortex-M7 and Cortex-M4 #9477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0bd2e1b
Update IAR flags for Armv8M devices.
Jan 7, 2019
9a78756
M2351: Update code to support IAR 8.x builds
Jan 8, 2019
7292acf
Update `cmse` flag based on core_arch instead of different cores
Jan 23, 2019
659f083
Correct the floating+dsp options for Cortex-M33
Jan 23, 2019
afb559b
Correct the floating options for Cortex-M7 and Cortex-M4
Jan 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 0 additions & 13 deletions targets/TARGET_NUVOTON/TARGET_M2351/device/system_M2351.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,19 +130,6 @@ extern void SystemInit(void);
extern void SystemCoreClockUpdate(void);




#if defined (__ICCARM__)
uint32_t __TZ_get_PSP_NS(void);
void __TZ_set_PSP_NS(uint32_t topOfProcStack);
int32_t __TZ_get_MSP_NS(void);
void __TZ_set_MSP_NS(uint32_t topOfMainStack);
uint32_t __TZ_get_PRIMASK_NS(void);
void __TZ_set_PRIMASK_NS(uint32_t priMask);
#endif



Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Author

Choose a reason for hiding this comment

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

they are dummy defines in code, may be old trial code

Copy link
Contributor

@SenRamakri SenRamakri Jan 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I see, but it happens to be in target code which we are modifying/removing, hope its ok with Nuvoton.

Copy link
Author

Choose a reason for hiding this comment

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

Actual PR for this change is #9431
Please note: Commit for this PR is afb559b
Dependent on : #9431

#ifdef __cplusplus
}
#endif
Expand Down
3 changes: 3 additions & 0 deletions tools/export/iar/iar_definitions.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,5 +341,8 @@
"GBECoreSlave": 39,
"FPU2": 6,
"GFPUCoreSlave2": 39
},
"M2351KIAAEES": {
"OGChipSelectEditMenu": "M2351\tNuvoton M2351"
}
}
30 changes: 15 additions & 15 deletions tools/toolchains/iar.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from os.path import join, splitext, exists
from distutils.version import LooseVersion

from tools.targets import CORE_ARCH
from tools.toolchains import mbedToolchain, TOOLCHAIN_PATHS
from tools.hooks import hook_tool
from tools.utils import run_cmd, NotSupportedException
Expand All @@ -45,12 +46,18 @@ def __init__(self, target, notify=None, macros=None, build_profile=None,
build_dir=None):
mbedToolchain.__init__(self, target, notify, macros, build_dir=build_dir,
build_profile=build_profile)
if target.core == "Cortex-M7F" or target.core == "Cortex-M7FD":
cpuchoice = "Cortex-M7"
elif target.core.startswith("Cortex-M23"):
cpuchoice = "8-M.baseline"
if target.core == "Cortex-M7FD":
cpuchoice = "Cortex-M7.fp.dp"
elif target.core == "Cortex-M7F":
cpuchoice = "Cortex-M7.fp.sp"
elif target.core.startswith("Cortex-M33FD"):
cpuchoice = "Cortex-M33.fp"
Copy link
Contributor

@SenRamakri SenRamakri Jan 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Wonder why this is not fp.dp (double-prec) as in M7? Is this correct, please double-check?

Copy link
Author

Choose a reason for hiding this comment

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

D in case of Cortex-M33 is DSP and not double floating point.

Copy link
Contributor

@kjbracey kjbracey Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Which is a very bad idea, as I believe I've previously commented. Why are we making up non-standard inconsistent naming?

If you really want to have a single-letter name, "M33FE" would make more sense, as "E" is the letter historically used by ARM to represent the DSP Extensions. (Although that makes me think of the ARM7500FE where it meant EDO RAM, so maybe reverse to M33EF - putting the DSP extensions closer to the core than the FP unit is also logical).

But I would really rather go with what all the tools are consistently doing - using the "no_fp" and "no_dsp" suffixes. We're in the odd situation where all 3 toolchains agree on that, and Mbed OS is being different. (Okay, they don't totally agree on polarity of the FP flag)

Copy link
Author

Choose a reason for hiding this comment

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

While performing some changes recently, I see why Mbed OS used its own letters, as we do not have consistency among all toolchaina and even between assembler and compilers. Flags are +nodsp / .no_dsp / .dsp / +dsp

Copy link
Author

Choose a reason for hiding this comment

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

single-letter name, "M33FE"

Yes can change this now to some other letter for DSP, as no target is added.
@mbed-os-tools - thoughts on this please

Copy link
Contributor

Choose a reason for hiding this comment

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

Please make the core names consistent with the ARM historical single letter abbreviations; use E for DSP.

Copy link
Author

Choose a reason for hiding this comment

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

@theotherjimmy @kjbracey-arm - One more Query, for DSP and FPU. Should it be F-Eor FE
Floating point - F and FD looks fine, but with addition of E, FDE will be confusing. And will be allow any order here? like FD-E or E-FD?

elif target.core.startswith("Cortex-M33F"):
cpuchoice = "Cortex-M33.fp.no_dsp"
elif target.core.startswith("Cortex-M33"):
cpuchoice = "8-M.mainline"
cpuchoice = "Cortex-M33.no_dsp"
elif target.core.startswith("Cortex-M23"):
cpuchoice = "Cortex-M23"
else:
cpuchoice = target.core

Expand All @@ -68,18 +75,11 @@ def __init__(self, target, notify=None, macros=None, build_profile=None,
cxx_flags_cmd = [
"--c++", "--no_rtti", "--no_exceptions"
]
if target.core == "Cortex-M7FD":
asm_flags_cmd += ["--fpu", "VFPv5"]
c_flags_cmd.append("--fpu=VFPv5")
elif target.core == "Cortex-M7F":
asm_flags_cmd += ["--fpu", "VFPv5_sp"]
c_flags_cmd.append("--fpu=VFPv5_sp")
elif target.core == "Cortex-M23" or target.core == "Cortex-M33" or target.core == "Cortex-M33F":
self.flags["asm"] += ["--cmse"]
self.flags["common"] += ["--cmse"]

# Create Secure library
if target.core == "Cortex-M23" or self.target.core == "Cortex-M33" or self.target.core == "Cortex-M33F":
if CORE_ARCH[target.core] == 8 and not target.core.endswith("-NS"):
self.flags["asm"] += ["--cmse"]
self.flags["common"] += ["--cmse"]
secure_file = join(build_dir, "cmse_lib.o")
self.flags["ld"] += ["--import_cmse_lib_out=%s" % secure_file]

Expand Down