-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Build tool fixes for Musca support #7792
Conversation
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.
LGTM, just some questions
tools/toolchains/arm.py
Outdated
self.flags['common'].append("-mcpu=%s" % target.core.lower()) | ||
self.flags['ld'].append("--cpu=%s" % target.core.lower()) | ||
self.SHEBANG += " -mcpu=%s" % target.core.lower() | ||
if not target.core.startswith("Cortex-M23") and not target.core.startswith("Cortex-M33"): |
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.
Hmm, so I take it building for the NS side no longer needs to be differentiated?
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.
Differentiation is still there, here we are just not setting ‘-mcpu’ parameter for any of v8M device. If we set cpu as cortex-m33 in flags, compiler enables dsp as well which is optional. Hence we just set ‘-march’
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.
Please collapse the
else:
if: ...
into
elif:
@@ -256,8 +256,6 @@ def link(self, output, objects, libraries, lib_dirs, mem_map): | |||
# Exec command | |||
self.notify.cc_verbose("Link: %s" % ' '.join(cmd)) | |||
self.default_cmd(cmd) | |||
if self.target.core == "Cortex-M23" or self.target.core == "Cortex-M33": |
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.
Why is this output no longer needed? (Commit only mentioned something about a build issue)
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.
Code for creation of ‘secure_file’ was initially in this section and later moved to other part. Variable ‘secure_file’ is not available here and hence throws error
-mcpu option if set for v8M CPU;s it will add DSP feature as default which is optional. Hence setting just the architecture for Cortex-M23 and Cortex-M33
a84ebee
to
beab422
Compare
As per the link below, options for clang and armlink are diferrent for Cortex-M33 armlink --cpu 8-M.Main --import-cmse-lib-out importlib_v1.o armclang -march=armv8-m.main -mcmse http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0773h/pge1452794854109.html
Fix in 6366452 : Options for clang and armlink are diferrent for Cortex-M33
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0773h/pge1452794854109.html |
@theotherjimmy - We have few issues while building ASM files. All the C flags are added to asm build, as a result .o generated is not correct.
|
Resolved below warnings Warning: L3912W: Option 'legacyalign' is deprecated. Warning: L3912W: Option 'no_strict_wchar_size' is deprecated. Warning: L3912W: Option 'no_strict_enum_size' is deprecated.
Issue relate to ASM file build was related to assembly file extension ".s", and is resolved by updating it to be caps .S |
@theotherjimmy @gaborkertesz - Please review |
e9d53ab
to
3fcf14e
Compare
Basic hello world example has been tested with armclang and gcc on Musca and working as expected as a secure only application. |
@theotherjimmy @cmonr - Please review |
tools/toolchains/arm.py
Outdated
self.flags['c'].append("-mcpu=cortex-m33+nodsp") | ||
self.flags['c'].append("-mfpu=none") | ||
self.flags['cxx'].append("-mcpu=cortex-m33+nodsp") | ||
self.flags['cxx'].append("-mfpu=none") |
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.
Can these be added to common?
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.
Done 👍
|
||
if target.core == "Cortex-M23" or target.core == "Cortex-M33": | ||
self.flags['common'].append("-mcmse") | ||
self.flags['cxx'].append("-mcmse") | ||
self.flags['c'].append("-mcmse") |
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.
Could this be added to common?
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.
This flag gives warning with linker and armasm.
I got one question - removing those legacy flags from linker command - the warning is for ARMC6 only (can't be seen in ARMC5) ? This is safe to remove, no functionality change? |
Legacy linker flags are not present for ARM 5 compiler, I don;t see those flags even in history of ARM 5 compiler, @theotherjimmy - any idea why those flags were added for ARMC6.
Should be, as they have no impact. Deprecated as per this link http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0381b/CHDHCGGF.html |
Change shared by @gabor Kertesz Hard-fault on Musca was because of enabled floating point instructions, disabling DSP and FPU.
3fcf14e
to
ed58ff0
Compare
@theotherjimmy - I also see below warning in detail build dumps, not sure what is causing this.
|
Can we start the build for this PR? |
Does #7792 (comment) not need to be addressed? |
/morph build |
Build : SUCCESSBuild number : 2933 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2551 |
Test : SUCCESSBuild number : 2682 |
Description
Typos and minor fixes required to support Cortex-M33 devices. Support of M33 was not tested since we didn;t had any target. This PR will be tested internally with MUSCA.
Pull request type