cpu/riscv: model Kconfig #16909

leandrolanzieri · 2021-09-28T14:47:37Z

Contribution description

This models the Kconfig modules for risc-v and adds two boards for the CI tests. For this I had to model libc modules, I'd like some feedback on those.

Testing procedure

CI should take care of comparing binaries.
Check list of modules compiled with and without Kconfig

Issues/PRs references

#16875

fjmolinas · 2021-09-28T14:58:20Z

cpu/riscv_common/periph/Kconfig

+# TODO: this modules are actually just artifacts from the way periph_init_%
+# modules are handled in Makefile. We need to define them to keep the list the
+# same for now. We should be able to remove them later on.
+config MODULE_PERIPH_INIT_CLIC
+    bool
+    default MODULE_PERIPH_CLIC
+
+config MODULE_PERIPH_INIT_CORETIMER
+    bool
+    default MODULE_PERIPH_CORETIMER
+
+config MODULE_PERIPH_INIT_PLIC
+    bool
+    default MODULE_PERIPH_PLIC
+
+endif # MODULE_RISCV_COMMON_PERIPH


You could add them here as well

RIOT/makefiles/features_modules.inc.mk

Lines 13 to 18 in 7bfcda9

ifneq (,$(filter periph_init, $(USEMODULE)))

PERIPH_IGNORE_MODULES := \

periph_init% \

periph_common \

periph_rtc_rtt \

#

Oh, did not know that one

I ran into the same a decided against adding modules just for the sake of it hahaha

Now added in c9dc338

fjmolinas · 2021-09-28T14:59:53Z

Generally looks good, lets trigger the ci!

leandrolanzieri · 2021-09-28T15:09:13Z

sys/Kconfig

+choice LIBC_IMPLEMENTATION
+    bool "Libc implementation"
+    depends on TEST_KCONFIG
+    default MODULE_NEWLIB if USE_LIBC_NEWLIB
+    default MODULE_PICOLIBC if USE_LIBC_PICOLIBC


I have an alternative proposal for this one, which uses less symbols:

choice LIBC_IMPLEMENTATION bool "Libc implementation" depends on TEST_KCONFIG config MODULE_NEWLIB bool "NewLib" config MODULE_PICOLIBC bool "Picolibc" endchoice

The default can be chosen in riscv_common/Kconfig as:

choice LIBC_IMPLEMENTATION default MODULE_NEWLIB endchoice

This works this way only because there are no dependencies? Or would we be able to override it in any case?

It actually depends on TEST_KCONFIG, and that does not seem to be weakened when I tested locally.

I can't find references to this behaviour in the official docs, but in Zephyr's docs they mention:

To change the default symbol of FOO to A, you would add the following definition to Kconfig.defconfig:

choice FOO default A endchoice

The Kconfig.defconfig method should be used when the dependencies of the choice might not be satisfied. In that case, you’re setting the default selection whenever the user makes the choice visible.

leandrolanzieri · 2021-09-28T16:11:57Z

Something seems to be wrong with newlib and native

fjmolinas · 2021-09-28T20:20:37Z

cpu/fe310/Kconfig

+    select MODULE_PERIPH_PLIC if TEST_KCONFIG
+    select MODULE_PERIPH_CORETIMER if MODULE_PERIPH_TIMER && HAS_PERIPH_CORETIMER
+    select MODULE_PERIPH_RTT if MODULE_PERIPH_RTC && HAS_PERIPH_RTT


Seem like ztimer_periph_rtc is not the default backend in Makefile.dep you could either force it in the application through Kconfig or add the following

ifneq (,$(filter ztimer_sec,$(USEMODULE))) USEMODULE += ztimer # native rtc is based on xtimer, no sense in using it and its actually buggy ifeq (,$(filter native,$(BOARD))) FEATURES_OPTIONAL += periph_rtc endif include $(RIOTMAKE)/features_check.inc.mk ifneq (,$(filter periph_rtc,$(FEATURES_USED))) USEMODULE += ztimer_periph_rtc else FEATURES_OPTIONAL += periph_rtt include $(RIOTMAKE)/features_check.inc.mk ifneq (,$(filter periph_rtt,$(FEATURES_USED))) USEMODULE += ztimer_periph_rtt else USEMODULE += ztimer_periph_timer endif endif endif

Thanks! Added

fjmolinas · 2021-09-29T09:05:43Z

cpu/riscv_common/Kconfig

    select HAS_SSP

+    select MODULE_MALLOC_THREAD_SAFE if TEST_KCONFIG
+    imply MODULE_NEWLIB_NANO


Isn't TEST_KCONFIG missing?

Imply will not force it, if TEST_KCONFIG is not there (or some other dependency for that matter), the symbol is not enabled.

That's why this also works when picolibc is selected, newlib_nano would not be enabled in that case.

fjmolinas

One last comment, but please squash afterwards

leandrolanzieri · 2021-09-29T09:11:51Z

Squashed!

fjmolinas · 2021-09-29T12:15:19Z

sys/ztimer/Makefile.dep

@@ -108,3 +108,27 @@ ifneq (,$(filter ztimer_msec,$(USEMODULE)))
    USEMODULE += ztimer_periph_timer
  endif
 endif
+
+ifneq (,$(filter ztimer_sec,$(USEMODULE)))


@benpicco how would this affet your rtc_rtt concerns

Anyway, this is already an issue in master... so this does not make the problem worse..

I'll try to find a solution for this in a follow up

fjmolinas

ACK, please rebase and squash (

leandrolanzieri · 2021-10-05T15:04:20Z

Thanks for the review!

leandrolanzieri added Type: new feature The issue requests / The PR implemements a new feature for RIOT Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Sep 28, 2021

leandrolanzieri requested review from MrKevinWeiss and fjmolinas September 28, 2021 14:47

leandrolanzieri requested review from cgundogan, jia200x and kaspar030 as code owners September 28, 2021 14:47

github-actions bot added Area: CI Area: Continuous Integration of RIOT components Area: cpu Area: CPU/MCU ports Area: Kconfig Area: Kconfig integration Area: sys Area: System labels Sep 28, 2021

leandrolanzieri mentioned this pull request Sep 28, 2021

[Tracking]: modeling dependencies in Kconfig #16875

Closed

fjmolinas requested a review from bergzand September 28, 2021 14:53

fjmolinas reviewed Sep 28, 2021

View reviewed changes

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

leandrolanzieri commented Sep 28, 2021

View reviewed changes

fjmolinas reviewed Sep 28, 2021

View reviewed changes

github-actions bot added Area: timers Area: timer subsystems Area: build system Area: Build system labels Sep 29, 2021

fjmolinas reviewed Sep 29, 2021

View reviewed changes

leandrolanzieri force-pushed the pr/cpu/riscv/model_kconfig branch from c9dc338 to 2d36f24 Compare September 29, 2021 09:11

fjmolinas reviewed Sep 29, 2021

View reviewed changes

fjmolinas approved these changes Oct 1, 2021

View reviewed changes

leandrolanzieri force-pushed the pr/cpu/riscv/model_kconfig branch from 2d36f24 to bda69da Compare October 1, 2021 09:25

leandrolanzieri added 2 commits October 1, 2021 11:26

sys/ztimer: add dependencies for ztimer_sec

34259e1

cpu/riscv_common: model Kconfig

cf753c6

leandrolanzieri added 4 commits October 1, 2021 11:26

sys/libc: model Kconfig

9557a9d

cpu/fe310: model Kconfig

fe8e043

cpu/gd32v: model Kconfig

a015508

.murdock: add hifive1 and seeedstudio-gd32 to Kconfig test

bda69da

benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 1, 2021

fjmolinas merged commit 93bff48 into RIOT-OS:master Oct 5, 2021

leandrolanzieri deleted the pr/cpu/riscv/model_kconfig branch October 5, 2021 15:04

kfessel mentioned this pull request Nov 2, 2021

sys/ztimer: add 'ztimer_no_periph_rtt #17099

Closed

fjmolinas mentioned this pull request Nov 2, 2021

sys/ztimer: don't default to rtc for ztimer_sec #17113

Merged

fjmolinas added this to the Release 2022.01 milestone Nov 18, 2021

	ifneq (,$(filter periph_init, $(USEMODULE)))
	PERIPH_IGNORE_MODULES := \
	periph_init% \
	periph_common \
	periph_rtc_rtt \
	#

cpu/riscv: model Kconfig #16909

cpu/riscv: model Kconfig #16909

Uh oh!

Conversation

leandrolanzieri commented Sep 28, 2021

Contribution description

Testing procedure

Issues/PRs references

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!

fjmolinas commented Sep 28, 2021

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!

leandrolanzieri commented Sep 28, 2021

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!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Sep 29, 2021

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!

fjmolinas left a comment

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Oct 5, 2021

Uh oh!

Uh oh!