cpu/efm32: add Kconfig by basilfx · Pull Request #13111 · RIOT-OS/RIOT

basilfx · 2020-01-13T19:45:52Z

Contribution description

This is my attempt to get acquainted with Kconfig. I tried to migrate the existing EFM32 features into Kconfig, while still being backwards compatible (note that some of the features are part of the documentation, e.g. this example).

Testing procedure

There are currently two config options available.

To test EFM32_FEATURE_LEUART, use the tests/cpu_efm32_features. Without Kconfig, this compiles as-is. When using Kconfig, deliberatly set EFM32_FEATURE_LEUART=y and notice that compilation will fail (as intended).

To test EFM32_EMLIB_ASSERTIONS, run (for instance) QUIET=0 BOARD=slstk3401a make -C examples/default and observe that -DDEBUG_EFM is not added to CFLAGS. Then set EFM32_EMLIB_ASSERTIONS=y and compile again. This time, the flag is present (and the resulting binary is now a bit bigger).

Issues/PRs references

None

This rename EFM32_LEUART_ENABLED to CONFIG_EFM32_FEATURE_LEUART. For backwards compatibility (for the time being), efm32-features.mk is still included if CONFIG_KCONFIG_CPU_EFM32 is undefined.

leandrolanzieri

Nice to start having CPUs in Kconfig! Changes look good, just some comments.

leandrolanzieri · 2020-01-14T08:40:55Z

cpu/efm32/efm32-features.mk

 # should override them from the command line, or in your Makefile. Note that
 # some features may not be applicable to all EFM32 boards or CPUs.
-export EFM32_LEUART_ENABLED ?= 1
+export CONFIG_EFM32_FEATURE_LEUART ?= 1


I think we should keep the CONFIG_ namespace in Makefile variables for Kconfig generated symbols.

True, but that would break backwards compatibility. Since Kconfig is still optional, I need to have a default value for this option, right?

If I would remove it, support for LEUART would always be disabled.

But the only use for this variable is to set a CFLAG when Kconfig is not being used. So you can just keep the old name for the Makefile variable, and only check for its value if Kconfig was not used:

ifndef CONFIG_KCONFIG_CPU_EFM32 ifeq (1,$(EFM32_FEATURE_LEUART)) CFLAGS += -DCONFIG_EFM32_FEATURE_LEUART=1 endif endif

leandrolanzieri · 2020-01-14T09:40:43Z

cpu/Kconfig

@@ -0,0 +1,11 @@
+# Copyright (c) 2019 HAW Hamburg


Suggested change

# Copyright (c) 2019 HAW Hamburg

# Copyright (c) 2020 Bas Stottelaar <[email protected]>

It was copy-paste, so I did not change the copyright. I don't mind taking it though ;-)

leandrolanzieri · 2020-01-14T11:12:23Z

cpu/efm32/Makefile.features

@@ -1,4 +1,6 @@
-include $(RIOTCPU)/efm32/efm32-features.mk
+ifndef CONFIG_KCONFIG_CPU_EFM32


Is the conditional include necessary? When setting the CFLAG in line you are already checking for CONFIG_KCONFIG_CPU_EFM32

If I understand correctly, setting CONFIG_KCONFIG_CPU_EFM32 indicates that Kconfig was used. In that case, I don't want to include the default value.

gschorcht · 2020-01-15T05:03:31Z

Kconfig

 # For now, get used modules as macros from this file (see kconfig.mk)
 osource "$(KCONFIG_GENERATED_DEPENDENCIES)"

+rsource "cpu/Kconfig"


@leandrolanzieri Is it planned that the board will also be selected via Kconfig?

If not, at least the board must be specified at make command line, so the CPU is known. In this case I wonder if it wouldn't be better to include the CPU configuration as follows

Suggested change

rsource "cpu/Kconfig"

# The CPU may declare new symbols as well

osource "$(RIOTBASE)/cpu/$(CPU)/Kconfig"

This would avoid the user having to select the CPU from a list. He would only have one top-level menu entry for the specified CPU, provided the CPU has a `Kconfig' file.

@leandrolanzieri Is it planned that the board will also be selected via Kconfig?

We can move towards this, but not just yet as it has huge implications in the resolution of module dependencies.

If not, at least the board must be specified at make command line, so the CPU is known.

The configurator gets executed after $(BOARD) and $(CPU) being defined, always.

This would avoid the user having to select the CPU from a list. He would only have one top-level menu entry for the specified CPU, provided the CPU has a `Kconfig' file.

I think the way @basilfx is adding the menuconfig entry is correct, as it depends on the modules defined for EFM32 CPUs. That means that the user will only see the entry for the CPU that's being used. Right now this is not for selecting a particular CPU but to configure its parameters.

Ok, my concern is the usability. To configure the CPU, you have to select the CPU in main, then you have to enable the CPU for configuration first, and then you can enter into the CPU configuration

CPU ---> [*] Configure EFM32 CPU (NEW) ---> Features ---> [ ] Enable EMLIB assertions (NEW)

If there is only one CPU at the same time, why not to add Configure EFM32 CPU (NEW) directly to the main menu.

I'm just asking because I have the same issue for ESPs and solved it in different way. If you define that there should only be menu entries in the main menu and no menuconfig entries, that would be fine with me and I would do it in the same for the ESPs.

We could, in the root Kconfig file, do:

rsource "$(RIOTCPU)/*/Kconfig"

But if we then start having 'common' Kconfig files then this will add more than one entry in the Top level. Maybe then having a CPU menu is better.

Also for that we can just include every CPU using glob patterns:

# in /cpu/Kconfig menu "CPU" rsource "*/Kconfig" endmenu

I tried the latter, it works. So it is not necessary to include each cpu/*/Kconfig file separately.

tcschmidt · 2020-04-01T00:19:24Z

@leandrolanzieri next steps to move?

leandrolanzieri · 2020-04-01T12:44:31Z

In #13377 it was decided to go with the approach proposed by @gschorcht, so this needs rebasing now to adapt to it.

miri64 · 2020-06-25T13:49:21Z

@leandrolanzieri @basilfx is this still valid?

leandrolanzieri · 2020-06-25T14:03:11Z

The PR is valid but needs some rebasing and adaption

aabadie · 2020-09-29T10:40:28Z

It seems that #14310 also addressed the Kconfig support for EFM32 and is already merged. Is this PR still relevant or should it be closed ?

leandrolanzieri · 2020-09-29T11:14:55Z

It seems that #14310 also addressed the Kconfig support for EFM32 and is already merged. Is this PR still relevant or should it be closed ?

This PR exposes in addition a couple of configurations:

config EFM32_FEATURE_LEUART
    bool "Enable LEUART peripheral support"
    default y
    help
        Enable the LEUART peripheral in the UART driver.

config EFM32_EMLIB_ASSERTIONS
    bool "Enable EMLIB assertions"
    help
        Additional assertions can be enabled within the EMLIB peripheral
        library, for debugging. EMLIB is part of the Gecko SDK by Silicon Labs.

So those changes are still valid.
EDIT: but still would need to be adapted (e.g. the UART configuration moved to periph/Kconfig.uart like the similar timer configuration that is already there).

stale · 2021-06-03T19:59:58Z

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.

basilfx added 5 commits January 13, 2020 17:37

cpu: add Kconfig section

19313a6

cpu/efm32: add Kconfig

61336ca

cpu/efm32: rename EFM32_LEUART_ENABLED

1dd60d0

This rename EFM32_LEUART_ENABLED to CONFIG_EFM32_FEATURE_LEUART. For backwards compatibility (for the time being), efm32-features.mk is still included if CONFIG_KCONFIG_CPU_EFM32 is undefined.

tests/cpu_efm32_features: rename EFM32_LEUART_ENABLED

82655dd

cpu/efm32: add CONFIG_EFM32_EMLIB_ASSERTIONS

3e526ec

basilfx added the Area: boards Area: Board ports label Jan 13, 2020

basilfx requested a review from leandrolanzieri January 13, 2020 19:45

leandrolanzieri added TF: Config Marks issues and PRs related to the work of the Configuration Task Force Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jan 13, 2020

leandrolanzieri reviewed Jan 14, 2020

View reviewed changes

leandrolanzieri mentioned this pull request Jan 14, 2020

[CTF-tracker] Kconfig: Configuration exposure list #12888

Open

71 tasks

gschorcht reviewed Jan 15, 2020

View reviewed changes

leandrolanzieri added the Area: Kconfig Area: Kconfig integration label Feb 3, 2020

basilfx mentioned this pull request Feb 4, 2020

cpu/efm32: cleanup #13174

Merged

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Jun 3, 2021

stale bot closed this Jul 8, 2021

basilfx deleted the feature/efm32_kconfig branch April 12, 2023 22:05

	# Copyright (c) 2019 HAW Hamburg
	# Copyright (c) 2020 Bas Stottelaar <[email protected]>

		@@ -1,4 +1,6 @@
		include $(RIOTCPU)/efm32/efm32-features.mk
		ifndef CONFIG_KCONFIG_CPU_EFM32

-rsource "cpu/Kconfig"
+# The CPU may declare new symbols as well
+osource "$(RIOTBASE)/cpu/$(CPU)/Kconfig"

Conversation

basilfx commented Jan 13, 2020

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

leandrolanzieri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschorcht Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcschmidt commented Apr 1, 2020

Uh oh!

leandrolanzieri commented Apr 1, 2020

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

leandrolanzieri commented Jun 25, 2020

Uh oh!

aabadie commented Sep 29, 2020

Uh oh!

leandrolanzieri commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

leandrolanzieri Jan 14, 2020 •

edited

Loading

gschorcht Jan 15, 2020 •

edited

Loading

leandrolanzieri commented Sep 29, 2020 •

edited

Loading