treewide: clean up architecture specific toolchain setup by maribu · Pull Request #15996 · RIOT-OS/RIOT

maribu · 2021-02-12T16:54:45Z

Contribution description

Architecture specific toolchain setup is done mostly in makefiles/arch/<ARCH>.inc.mk, except for ESP and ARM9. Those oddballs are changed to the common pattern for consistency.

Secondly, each CPU family included the architecture specific settings on its. However, as the architecture was is already known once the features are resolved, one can simple include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk directly in $(RIOTBASE)/Makefile.include.

Testing procedure

If the toolchains are not properly set up, Murdock will complain. In addition, it might make sense to pick one example application and board for each platform and confirm that binaries doesn't change.

Issues/PRs references

One step towards making #15993 possible without reordering the processing of Makefile.dep and Makefile.include.

cpu/arm7_common/Makefile.features

fjmolinas

This one looks good, there are just two changes I would put in different commits. This is a nice cleanup and I don't expect any impact since thee include still happens after the CPU one so any board/cpu specific configuration can still override the generic one.

fjmolinas · 2021-02-16T10:40:54Z

cpu/esp_common/Kconfig

    select HAS_ARCH_32BIT
    select HAS_ARCH_ESP

+config CPU_ARCH_XTENSA_LX106


I think these changes should be In another commit.

fjmolinas · 2021-02-16T10:42:56Z

cpu/arm7_common/Makefile.features

-CPU_ARCH = armv4t
-CPU_CORE = arm7tdmi_s
+CPU_ARCH := armv4t
+CPU_CORE := arm7tdmi_s


Do you mind putting these changes in its own commit, they are not really related to the whole PR. And you can add the argument for doing this to the commit message.

fjmolinas · 2021-02-16T10:45:19Z

cpu/riscv_common/Makefile.features

@@ -1,3 +1,5 @@
+CPU_ARCH := risc-v


Same for this one, can you add to a commit where you say "adding missing arch or something like that", or fix missing arch and you can ad comment on the := change in that commit as well.

fjmolinas · 2021-02-16T10:46:24Z

Makefile.include

 include $(RIOTCPU)/$(CPU)/Makefile.include

+# include architecture specific settings
+include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk


Suggested change

include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk

-include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk

With this we can avoid the empty files I believe.

maribu · 2021-02-16T12:55:16Z

Done. And a rebase seems to also be needed due to a merge conflict.

fjmolinas

ACK, lets see what the ci says.

fjmolinas · 2021-02-22T08:54:47Z

Sorry @maribu I forgot to check the ci result and now this one needs a rebase :(

This is where it is located for all other platforms, let's have this consistently.

Use xtensa-lx106 for the ES8266 and xtensa-lx6 for the ESP32, instead of xtensa for both. This is later needed to allow differentiating between those two for the toolchin selection.

Instead of having each CPU family including $(RIOTMAKE)/arch/<ARCH>.inc.mk, just include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk at Makefile.include

Simple expansion is recommended by official documentation [1] for a multitude of reasons over recursive expansion, of which predictable behavior (and, thus, maintainability of Makefiles) and performance are the biggest arguments. [1]: https://www.gnu.org/software/make/manual/html_node/Flavors.html

maribu · 2021-02-22T11:08:59Z

Strange, git was able to auto-merge locally during rebase.

fjmolinas

ACK

benpicco · 2021-02-22T13:15:17Z

looks like mips is missing

maribu · 2021-02-22T14:25:24Z

looks like mips is missing

No, it worked fine prior to the XFA merge. The issue seems to be that the order of linker flags is slightly different with this PR for MIPS. All other arch but MIPS include makefiles/arch/$(CPU_ARCH).inc.mk at the end of cpu/$(CPU)/Makefile.include. In MIPS, it is included at the top of that file. Apparently, order didn't matte before, but with XFA it does now.

I will investigate this later. I believe there is some value in having the toolchain, CFLAGS and LDFLAGS setup as consistent as possible between all archs, as this eases refactoring and maintaining stuff.

stale · 2022-03-02T13:18:34Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

OlegHahm · 2022-04-08T12:12:45Z

@maribu, ping :)

maribu · 2022-04-08T13:06:58Z

This is the type of PRs that horribly break things in a way that Murdock won't catch it, e.g. by triggering a build system bug that ends up half of the test cases not being build anymore. I agree that I should get back to this PR and push it forward, but maybe it is better to wait for the next development cycle before I break everything :)

gschorcht · 2022-04-08T14:30:16Z

@maribu Sorry for jumping into this discussion so late. To be honest, I would prefer to drop the changes for ESPs for now, as they conflict with PRs #17842, #17844, and the PR for ESP32S3 support that is hopefully coming in the next few days. In these PRs xtensa or riscv-esp32 first define the CPU architecture and xtensa-lx6, xtensa-lx7 and riscv32 then define the CPU core used but not the architecture:

ifneq (,$(filter esp32,$(CPU_FAM)))
  CPU_ARCH = xtensa
  CPU_CORE = xtensa-lx6
else ifneq (,$(filter esp32c3,$(CPU_FAM)))
  CPU_ARCH = riscv_esp32
  CPU_CORE = riscv32
else ifneq (,$(filter esp32s3,$(CPU_FAM)))
  CPU_ARCH = xtensa
  CPU_CORE = xtensa-lx7
else
  $(error Unkwnown ESP32x SoC variant (family))
endif

Merging your changes now would cause a lot of conflicts when these very large PRs are rebased, and I would have to check everything again to make sure it still works. So I would prefer to clean it up later if it is still necessary.

maribu · 2022-04-08T18:08:27Z

OK, let's put this on hold until #17842 and #17844 are in.

gschorcht · 2022-08-09T06:13:05Z

OK, let's put this on hold until #17842 and #17844 are in.

@maribu The changes of these PRs are merged now but there are a number of new PRs (#18410, #18411, ...) for ESP32-S3 support. Since the PR seems to diverge already, the question is whether we should get this PR merged before #18410, #18411. It will require a number of changes for ESPs.

maribu · 2022-08-09T07:27:37Z

I currently have no time to push this PR. Let's get ESP32-C3 in first. I can rebase and adapt this afterwards when I have time.

maribu requested review from DipSwitch, aabadie, basilfx, benpicco, biboc, dylad, fjmolinas, gschorcht, haukepetersen, kaspar030, smlng and vincent-d as code owners February 12, 2021 16:54

maribu added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: build system Area: Build system Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Feb 12, 2021

maribu requested review from cgundogan, jia200x and leandrolanzieri as code owners February 12, 2021 19:21

maribu force-pushed the cleanup_arch branch from e602080 to 56f3d9d Compare February 12, 2021 21:03

fjmolinas reviewed Feb 15, 2021

View reviewed changes

cpu/arm7_common/Makefile.features Show resolved Hide resolved

fjmolinas reviewed Feb 16, 2021

View reviewed changes

maribu force-pushed the cleanup_arch branch from 56f3d9d to b53bdee Compare February 16, 2021 12:54

maribu force-pushed the cleanup_arch branch from b53bdee to 501e346 Compare February 16, 2021 12:55

fjmolinas approved these changes Feb 16, 2021

View reviewed changes

kaspar030 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 16, 2021

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 17, 2021

maribu added 2 commits February 22, 2021 12:07

cpu/arm7_common: move arch specific compiler setup to makefiles/arch

8f30d17

This is where it is located for all other platforms, let's have this consistently.

cpu/esp*: move arch specific compiler setup to makefiles/arch

d07950b

This is where it is located for all other platforms, let's have this consistently.

maribu added 5 commits February 22, 2021 12:07

cpu/esp*: use more explicit CPU arch

6e49a0c

Use xtensa-lx106 for the ES8266 and xtensa-lx6 for the ESP32, instead of xtensa for both. This is later needed to allow differentiating between those two for the toolchin selection.

cpu/riscv_common: Add missing CPU_ARCH declaration

5b35127

cpu/avr8_common: Add missing CPU_ARCH declaration

1723eab

treewide: clean up includes of makefiles/arch/...

dcc2dec

Instead of having each CPU family including $(RIOTMAKE)/arch/<ARCH>.inc.mk, just include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk at Makefile.include

maribu force-pushed the cleanup_arch branch from 501e346 to b468610 Compare February 22, 2021 11:08

fjmolinas approved these changes Feb 22, 2021

View reviewed changes

MrKevinWeiss added this to the Release 2021.07 milestone Jun 21, 2021

MrKevinWeiss removed this from the Release 2021.07 milestone Jul 15, 2021

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 2, 2022

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Apr 8, 2022

maribu added the State: waiting for other PR State: The PR requires another PR to be merged first label Apr 8, 2022

maribu closed this Jan 21, 2023

maribu deleted the cleanup_arch branch January 21, 2023 21:42

	include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk
	-include $(RIOTMAKE)/arch/$(CPU_ARCH).inc.mk

Conversation

maribu commented Feb 12, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 16, 2021

Choose a reason for hiding this comment

Uh oh!

maribu commented Feb 16, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Feb 22, 2021

Uh oh!

maribu commented Feb 22, 2021

Uh oh!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco commented Feb 22, 2021

Uh oh!

maribu commented Feb 22, 2021

Uh oh!

stale bot commented Mar 2, 2022

Uh oh!

OlegHahm commented Apr 8, 2022

Uh oh!

maribu commented Apr 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gschorcht commented Apr 8, 2022

Uh oh!

maribu commented Apr 8, 2022

Uh oh!

gschorcht commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maribu commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maribu commented Apr 8, 2022 •

edited

Loading

gschorcht commented Aug 9, 2022 •

edited

Loading