cpu/cortexm: cleanup dependencies#13738
Conversation
leandrolanzieri
added
Platform: ARM
leandrolanzieri
requested review from
DipSwitch,
basilfx,
dylad,
haukepetersen,
keestux,
smlng,
thomaseichinger and
vincent-d
as code owners
leandrolanzieri
removed request for
DipSwitch,
basilfx,
dylad,
haukepetersen,
keestux,
smlng,
thomaseichinger and
vincent-d
|
Are you aware of #13236 ? (might not be better than this one though but could have been reviewed first...) |
Yes! I read your comment where you state that the approach of turning Here I'm using both a feature and a module, and the conditional expansion approach to set the CFLAGS. |
So why not mentioning it in this PR comment ? Why not reviewing it in the first place ?
Exactly what I had in mind in #13236. Just didn't have time to do it... |
No, let's keep this one. It does what I wanted to do. Why doing again there what is already done here ? |
|
For the next times, just ask before rushing ;) |
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
Changes make sense and look good, some testing:
toolchain=llvm BOARD=nucleo-f767zi make -C examples/hello-world/ info-debug-variable-CFLAGS_FPU --no-print-directory
-mfloat-abi=hard -mfpu=fpv5-sp-d16
TOOLCHAIN=llvm BOARD=nucleo-f767zi make -C examples/hello-world/ info-debug-variable-CFLAGS_FPU --no-print-directory
#empty
DISABLE_MODULE=cortexm_fpu BOARD=nucleo-f767zi make -C examples/hello-world/ info-debug-variable-CFLAGS_FPU --no-print-directory
-mfloat-abi=soft
|
Lets give murdock a run. |
fjmolinas
added
the
CI: ready for build
|
I I think this one will need giving |
On it... |
|
No failed builds, so lets just wait for the script output! |
|
@fjmolinas I could not create a gist with the diff. I uploaded it here. For what I can see, the differences are the new feature that now appears on the list, and the module showing up in the |
Yes I agree! Seem that its all good! |
fjmolinas
left a comment
fjmolinas
left a comment
There was a problem hiding this comment.
ACK, good changes, dependencies haven't changed and murdock is happy. Please squash @leandrolanzieri!
This moves the following modules to a architecture-specific Makefile.dep file: - cortexm_common - cortexm_common_periph - newlib - newlib_nano - periph
This adds cortexm_fpu to the DEFAULT_MODULE list when the feature cortexm_fpu is provided by the architecture. It also moves the dependency resolution of this module to the architecture-specific Makefile.dep file.
leandrolanzieri
force-pushed
the
pr/cpu/cortexm/cleanup_dependencies
branch
from
7b2ced0 to
ea2f963
Compare
leandrolanzieri
added
CI: ready for build
|
All green go! |
|
Seem something here affected the output of
This all dependencies where compared, so its jsut with |
|
I'll take a look |
|
Seems like the cache is not working properly since the blacklisting depend on board info that is not available before doing the first resolution: I would remove this. |
3 participants
Contribution description
This PR does some cleanup of dependencies that were added/resolved in
cortexm.inc.mk. I added a architecture-specificMakefile.depwhich is included by CPUs.It also changes the way
CFLAGS_FPUis handled, so we can now definecortexm_fpuas a default module on architectures that support it.Testing procedure
Check that dependency resolution still yields the same list of modules and CFLAGS
Check that FPU is handled correctly: Test with cortex-m7 (e.g.
nucleo-f746zg), cortex-m4f (e.g.nucleo-l452re) and some architecture that provides no FPU (e.g.samr21-xpro):cortexm_fpushould be there-mfloat-abi=hardand the correspondent-mfpu(depending on the architecture) should be present.DISABLE_MODULE=cortexm_fpu.mfloat-abi=softshould be present.Issues/PRs references
Part of #9913