cpu/efm32: cleanup

[fjmolinas]

Contribution description

efm32 uses shell calls to resolve a series of CPU parameters from the CPU_MODEL. All these shell calls are slow and make building and testing any efm32 slower than it needs to be.

This PR fixes this by aggregating all that data into one single file and using MAKE functions to extract the data.

A benchmark example is that building and running all tests without this PR takes about 1:49 on my machine, and it now takes 1:15.

This also move those variables to Makefile.include or Makefile.features respectively according to where they should be.

Names are changed to use the already existing and exported CPU_ARCH and CPU_FAM.

Testing procedure

Look a t the number of execv calls in this PR vs master:

• master

BOARD=slstk3402a strace -ff -e trace=execve make --no-print-directory -C examples/hello-world/ info-debug-variable-QUIET 2>&1 | grep 'execve' | wc -l
5571

• pr

BOARD=slstk3402a strace -ff -e trace=execve make --no-print-directory -C examples/hello-world/ info-debug-variable-QUIET 2>&1 | grep 'execve' | wc -l
83

You can also compare time used to resolve simple make targets e.g.:

• master

time BOARD=slstk3402a make --no-print-directory -C examples/hello-world/ info-debug-variable-USEMODULE
auto_init board boards_common_silabs core core_msg cortexm_common cortexm_common_periph cortexm_fpu cpu cpu_efm32pg12b gecko_sdk_emlib gecko_sdk_emlib_extra newlib newlib_nano newlib_syscalls_default periph periph_common periph_gpio periph_pm periph_uart pm_layered silabs_aem silabs_bc stdio_uart sys

real    0m0.741s
user    0m0.555s
sys     0m0.193s

-pr

time BOARD=slstk3402a make --no-print-directory -C examples/hello-world/ info-debug-variable-USEMODULE
auto_init board boards_common_silabs core core_msg cortexm_common cortexm_common_periph cortexm_fpu cpu cpu_efm32pg12b gecko_sdk_emlib gecko_sdk_emlib_extra newlib newlib_nano newlib_syscalls_default periph periph_common periph_gpio periph_pm periph_uart pm_layered silabs_aem silabs_bc stdio_uart sys

real    0m0.170s
user    0m0.115s
sys     0m0.027s

Issues/PRs references

Relates to #10850
Also relates to #9913
This is a first cleanup step, when #12407 is merged and agreed I will move towards a folder based approach.

[basilfx]

That's a very nice refactoring I never thought of. It's still very maintainable.

I have to test this in detail, hope to do that this week.

[aabadie]

A few inline comments.

[fjmolinas]

BTW @basilfx I didn't keep the message that was in cpu.txt:

# This file is automatically generated, and should not be changed. There is
# probably little reason to edit this file anyway, since it should already
# contain all information for the EFM32GG family of CPUs.

since this wasn't generated automatically but I did copy paste everything so not data was changed.

[fjmolinas]

Rebased and fixed typo.

[aabadie]

There are changes related to kinetis (introduced by 9bcc51d), but unrelated to this PR and stk3700 builds are failing (there might be others)

[fjmolinas]

@aabadie murdock is passing now.

[fjmolinas]

@aabadie @basilfx could you give a couple of boards a test?

[aabadie]

Tested on slstk3402a: build is successful, examples/default works. I also confirm the perform improvements introduced by this PR:

• System calls:

This PR

BOARD=slstk3402a strace -ff -e trace=execve make --no-print-directory -C examples/hello-world/ info-debug-variable-QUIET 2>&1 | grep 'execve' | wc -l
101

master

BOARD=slstk3402a strace -ff -e trace=execve make --no-print-directory -C examples/hello-world/ info-debug-variable-QUIET 2>&1 | grep 'execve' | wc -l
7667

• Dependency resolution time

this PR

time BOARD=slstk3402a make --no-print-directory -C examples/hello-world/ info-debug-variable-USEMODULE
auto_init board boards_common_silabs core core_init core_msg core_panic cortexm_common cortexm_common_periph cortexm_fpu cpu cpu_efm32pg12b gecko_sdk_emlib gecko_sdk_emlib_extra newlib newlib_nano newlib_syscalls_default periph periph_common periph_gpio periph_pm periph_uart pm_layered silabs_aem silabs_bc stdio_uart sys

real	0m0,080s
user	0m0,074s
sys	0m0,011s

master

time BOARD=slstk3402a make --no-print-directory -C examples/hello-world/ info-debug-variable-USEMODULE
auto_init board boards_common_silabs core core_init core_msg core_panic cortexm_common cortexm_common_periph cortexm_fpu cpu cpu_efm32pg12b gecko_sdk_emlib gecko_sdk_emlib_extra newlib newlib_nano newlib_syscalls_default periph periph_common periph_gpio periph_pm periph_uart pm_layered silabs_aem silabs_bc stdio_uart sys

real	0m1,472s
user	0m1,008s
sys	0m0,568s

ACK

@basilfx can you test this PR ? If not, are you ok with it ?

[basilfx]

Sorry for not being that active these weeks. I have other work that has priority.

I do have a few comments.

• efm32-info.mk isn't used anymore. That file was responsible for parsing the values, and provide meaningful errors (there are more supported CPU's in the 'database' than we have includes for).
• efm32-features.mk will be removed once we move on to Kconfig (cpu/efm32: add Kconfig #13111).
• I liked the idea of having the CPU-related information per family, instead of having them all in one file (efm32-vars.mk). That could be realized by defining CPU_FAM upfront in the board. I use this quite a lot, because I have some custom boards with another family not in that list.
• We should not use CPU_SERIES: it's an EFM32 thingy.
• the TRNG feature is broken on the SLSTK3402a, because EFM32_TNRG is undefined. While this is a typo (it should be EFM32_TRNG, my mistake!), it doesn't get defined anymore.

@fjmolinas I really like the improvements, and I want this to be merged. But let me check if I can propose some changes (I'll do that right away).

[basilfx]

@fjmolinas I have modified a few things that I think keeps things more self-contained. I have three additional commit in my own branch here.

Here is the summary:

• Define CPU_FAM per board. That variable is allowed/provided, according to here.
  • Having CPU_FAM defined, it saves the shell execution here, which you already fixed!
  • It's a small trade-off, to have this information provided once, and save a lot of troubles later, even though it feels 'redundant'.
• Remove CPU_SERIES, as that is something that is confusing next to CPU_FAM and CPU_ARCH. Now that CPU_FAM is provided, all other references to EFM32_FAMILY can be removed.
  • To the 'outside world', we only need CPU, CPU_FAM and CPU_MODEL.
• Restore families being self-contained, which makes it easy to drop-in/symlink additional families, for testing.
• Restore the assertions in efm32-info.mk to provide meaningful errors in case headers or family-specific information is missing.
  • Only headers for CPU_MODELs that are used by a board are included, but the list of family-specific information includes all information about the CPUs.
  • To support another EFM32 CPU_MODEL, it should be as simple adding the new header file only. I use that for my custom boards.
  • And yes, by switching branches a lot, you sometimes forget that your board isn't supported in that branch, so the errors help :-P
• Keep the logic of parsing family-specific information central at one place and only use the EFM32_ variables throughout the EFM32 CPU module only. This removes the need for the index variables, and keeps that logic at one place.
  • Only EFM32_ARCHITECTURE is used to provide CPU_ARCH.
  • No exports needed anymore.
  • Added more comments to clarify things.
• Fix support for periph_hwrng by providing EFM32_TRNG (and fix my typo).
• Remove logic from efm32-features.mk that will be removed eventually.
  • I think I chose a misleading file name, as it should only contain toggles to remove/fine-tune/experiment EFM32 related features.

Using the strace command, I'm at 59 execve syscalls (for hello-world), which is exactly the same as your branch. Compared to master, it is really much faster! I never thought of this issue.

I want to stress out that I don't want to annoy you by fixing some things myself, after you and @aabadie have almost finished this. But I hope you can consider my changes. You did the hard work, I only find-replaced/moved some parts. If you like them, then please take them over (no need for credit).

[fjmolinas]

Hi @basilfx thanks for your feedback, some comments

• CPU_ARCH can't be defined in Makefile.include, these goes against Build dependencies - processing order issues #9913, but it can be moved to Makefile.features

Remove logic from efm32-features.mk that will be removed eventually.

I don't mind removing this.

- DIRS += families/$(EFM32_FAMILY)
+ DIRS += families/$(CPU_FAM)

This shouldn't be done, because CPU_FAM is only available here because it is exported, but the is no real reason to export is, at some point that export should be removed, so I rather use an explicit variable EFM32_FAMILY that is exported just for efm32. You were the one that mentioned in a previous comment that CPU_FAM should not be used this way. Not using it for VECTORS_O := $(BINDIR)/cpu_$(CPU_FAM)/vectors.o and others related is for the same reason.

If you can recall or find that discussion it would be great, to either make clear that CPU_FAM can be use that way or move away from that kind of usage.

Restore families being self-contained, which makes it easy to drop-in/symlink additional families, for testing.

As I said in the PR eventually we will have to move towards #12407, which will mean one directory per CPU_MODEL, but if you prefer keeping these separate for now I don't mind. But if #12407 is implemented they will disappear anyway.

[fjmolinas]

If you can recall or find that discussion it would be great, to either make clear that CPU_FAM can be use that way or move away from that kind of usage.

I'm guessing its probably a comment made by @cladmi no?

[basilfx]

@fjmolinas I've send you an email to maybe discuss/explain this ;-)

[fjmolinas]

Talking offline with @basilfx and since CPU_FAM is already used for directory resolution (which is why it needs to be exported). I'll go ahead with @basilfx suggestion and use CPU_FAM as well.

@basilfx can we remove the column with the CPU_FAM from the "database" then?

[fjmolinas]

@basilfx can we remove the column with the CPU_FAM from the "database" then?

Maybe it is usefull for new boards?

[cladmi]

If you can recall or find that discussion it would be great, to either make clear that CPU_FAM can be use that way or move away from that kind of usage.

I'm guessing its probably a comment made by @cladmi no?

Talking offline with @basilfx and since CPU_FAM is already used for directory resolution (which is why it needs to be exported). I'll go ahead with @basilfx suggestion and use CPU_FAM as well.

TL;DR; I would not have exported either CPU_FAM or EFM32_FAMILY but used different module names for each family and done the DIRS resolution based on USEMODULE.

---

The reason I would not have exported CPU_FAM, but also not EFM32_FAMILY, for this use case, is that the reason to go into that families/cpu_something directory is not because this cpu is from this CPU_FAM but because the module in that directory is used.
Currently they may all have the same module name cpu or cpu_common (no idea), so a hack is used to find where the implementation is, but if being explicit and renaming it to different names cpu_efm32_family_CPU_FAM, then it would not matter in the rest of the building process that it is CPU_FAM, or CPU_LINE, or CPU_REVISION, the module name would be sufficient.

Then adding the families/cpu_something to DIRS would not depend of CPU_FAM but on USEMODULE. Which for me matched way more a declarative model where each module has a known directory. I was in favor of replacing board and cpu by board_BOARD_NAME and cpu_CPU_NAME.

With this being declarative, and moving the definition earlier in the build, it could allow moving this DIRS resolution in the main Makefile.include, instead of in many recursive Makefile, and so let make figure out how to parallelize the compilation of each module and maybe not even go in a module directory if it is up to date.

[basilfx]

@basilfx can we remove the column with the CPU_FAM from the "database" then?

I've updated the PR to remove that column as well. Makes sense!

TL;DR; I would not have exported either CPU_FAM or EFM32_FAMILY but used different module names for each family and done the DIRS resolution based on USEMODULE.

I guess that could be a next improvement? I see that this one already saves a lot, and the other approach would also include work outside of the EFM32 CPU folder, right?

[cladmi]

TL;DR; I would not have exported either CPU_FAM or EFM32_FAMILY but used different module names for each family and done the DIRS resolution based on USEMODULE.

I guess that could be a next improvement? I see that this one already saves a lot, and the other approach would also include work outside of the EFM32 CPU folder, right?

Exactly. I only answered to the why not export question.
But in the current situation, either EFM32_FAMILY or now CPU_FAM needs to be exported.

[fjmolinas]

I squashed and re-wrote the commit history to make it cleaner, it was compiling on every commit. I rebased to make sure I wasn't making any changes with respect to the past history, the only changes were to remove unneeded := and move an error message location. With the new commit history I think it should be easier to review individual changes.

If name-spacing is not wanted for the famlies/cpu_* modules I can rename them pack to just the family. I though it would be good to namespace to not mixup the module and the CPU_FAM variable.

[basilfx]

I prefer to have the folder in families/ as they were. Similar to the folders in cpu/, they are not prefixed with cpu_ either, and most of the files in families/<name>/ are vendor files to get the cpu/efm32/ working.

[fjmolinas]

I prefer to have the folder in families/ as they were. Similar to the folders in cpu/, they are not prefixed with cpu_ either, and most of the files in families// are vendor files to get the cpu/efm32/ working.

Ok, I'll se when I have time to do this. Probably not soon.

[fjmolinas]

Ok, I'll se when I have time to do this. Probably not soon.

Well did it right away, I think you will he happy with what is now @basilfx

[aabadie]

Found some "tabs instead of spaces" nit in cpu/efm32/Makefile.

You can squash directly.

I'm fine with the changes

[aabadie]

The CI is green, @basilfx are you ok with the current state of this PR ?

[kaspar030]

ACK from my side, but please @basilfx take a final look!

[basilfx]

I approve. Thanks for the hard work @fjmolinas!

[cladmi]

GG

[benpicco]

I'm trying to add support to another member of the efm32 family and now I wonder what tool was used to generate that kind of file.

[basilfx]

You might want to look here: https://github.com/basilfx/EFM2RIOT/tree/develop/dist

It should be reasonably up to date and contain all supported chips already.

[benpicco]

Uh that's nice.
@aabadie did something very similar for stm32.

I take we can't do that on the fly as Gecko SDK is required. But I don't like copying around folders either.

But we could just import that wholesale.

[basilfx]

I don't think it is necessary to do it on-the-fly. These families are quite stable (only new ones are added).

[benpicco]

So how about we import https://github.com/basilfx/EFM2RIOT/tree/develop/dist/cpu/efm32/families into RIOT as a whole?

[gschorcht]

@basilfx I have a THUNDERBOARD EFM32GG12 here, for whose MCU there is no entry in this file. Is this file really created automatically and if so, how. Or does the entry have to be added manually?

[basilfx]

This file is generated using https://github.com/basilfx/EFM2Riot. That script parses the Gecko SDK and generates the necessary files. I'll admit that it's a bit outdated now.