riscv_common: Refactor common fe310 code to riscv_common by bergzand · Pull Request #15718 · RIOT-OS/RIOT

bergzand · 2021-01-07T14:57:48Z

Contribution description

This PR refactors the lone fe310 RISC-V cpu into a separate riscv_common director keeping the fe310-specifics in their own directory. This makes it easier to add new RISC-V based processors in the future.

My original idea was to refactor into a rv32i_common directory, but I decided that riscv_common should be enough. Changes between rv64i and rv32i are minimal and mostly in the architecture size and doesn't warrant separate directories for the two (for now). It is however possible that some doxygen or variable names are still rv32i_, these should all be renamed to riscv_

Testing procedure

Changes to the binaries should be minimal or nonexistent.

Compilation from Murdock, together with a few applications tested on the hifive1b should be enough for code tests.
Please also verify that the documentation and the generated doxygen makes sense after the split.

Issues/PRs references

None

cpu/riscv_common/include/periph_cpu_common.h

benpicco

looks good to me, but I don't have any hardware to test this.

benpicco · 2021-01-18T16:16:38Z

cpu/riscv_common/include/cpu.h

+void riscv_fpu_init(void);
+
+/**
+ * @brief   Initialization of the interrupt controller
+ */
+void riscv_irq_init(void);


should those be public?

Doesn't have to be, but I'm going to leave that cleanup to a possible follow up :)

bergzand · 2021-01-18T16:43:27Z

There are still a few places where the riscv_common code uses code from the fe310. Slapping on a WIP label to prevent accidental merges while this is still the case.

aabadie · 2021-01-19T09:26:47Z

cpu/riscv_common/ldscripts/riscv.ld

 *
 * @file
- * @brief           Memory definitions for the SiFive FE310
+ * @brief           Memory definitions for for the RISC-V CPU


Suggested change

* @brief Memory definitions for for the RISC-V CPU

* @brief Memory definitions for the RISC-V CPU

bergzand · 2021-01-19T10:49:00Z

TODO:

Migrate _REG32(p, i) defines to a common place and allow periph implementations to define PERIPH_REG defines
Migrate PLIC config defines to a periph_cpu header: PLIC_CTRL_ADDR PLIC_NUM_INTERRUPTS and PLIC_NUM_PRIORITIES
Allow setting a TARGET_ARCH_MARCH string (rv32imac and a TARGET_ARCH_MABI (ilp32)

cpu/fe310/include/cpu_conf.h

aabadie · 2021-01-19T17:15:42Z

Without the following patch, the build is broken for application requiring irq/timer:

index c66b62719e..2582e92261 100644
--- a/cpu/riscv_common/Makefile.dep
+++ b/cpu/riscv_common/Makefile.dep
@@ -4,6 +4,8 @@ USEMODULE += riscv_common
 # include common periph code
 USEMODULE += riscv_periph_common
 
+FEATURES_REQUIRED += periph_coretimer periph_plic
+
 USEMODULE += periph_pm
 
 # Make calls to malloc and friends thread-safe
diff --git a/cpu/riscv_common/Makefile.features b/cpu/riscv_common/Makefile.features
index 6880670a79..f0eca40db5 100644
--- a/cpu/riscv_common/Makefile.features
+++ b/cpu/riscv_common/Makefile.features
@@ -6,6 +6,7 @@ FEATURES_PROVIDED += periph_cpuid
 FEATURES_PROVIDED += periph_plic
 FEATURES_PROVIDED += periph_pm
 FEATURES_PROVIDED += ssp
+FEATURES_PROVIDED += periph_coretimer periph_plic
 
 # RISC-V toolchain on CI does not work properly with picolibc yet
 ifeq (,$(RIOT_CI_BUILD))

I'm not sure that declaring the coretimer/plic as peripherals is the way to go. You could simply add them to the riscv_common module. But maybe I'm missing something here ?

fjmolinas · 2021-01-26T08:21:25Z

Is this still wip @bergzand ?

bergzand · 2021-01-26T10:20:38Z

Is this still wip @bergzand ?

Unfortunately

bergzand · 2021-01-26T11:00:40Z

I'm not sure that declaring the coretimer/plic as peripherals is the way to go. You could simply add them to the riscv_common module. But maybe I'm missing something here ?

Me neither. The issue is that the PLIC is one of the possible implementations for interrupt controllers on the RISC-V. There is also the CLIC. A CPU could use the CLINT timer for periph_timer, but it could also use a different (more advanced) timer peripheral

bergzand · 2021-01-26T11:08:10Z

@fjmolinas No longer WIP!

cpu/fe310/include/periph_cpu.h

cpu/riscv_common/Makefile.dep

aabadie · 2021-01-26T13:04:55Z

Please squash, I'd like to know what Murdock has to say with this PR :)

aabadie · 2021-01-26T14:36:01Z

This needs a rebase now.

bergzand · 2021-01-26T14:44:43Z

Rebased!

fjmolinas · 2021-02-02T09:09:44Z

This one needs a rebase again @bergzand

bergzand · 2021-02-02T15:46:37Z

Rebased!

aabadie

Still needs some work. With the following diff, things should be better:

diff --git a/cpu/fe310/Makefile b/cpu/fe310/Makefile
index 0dbf0f1807..fd024edf0e 100644
--- a/cpu/fe310/Makefile
+++ b/cpu/fe310/Makefile
@@ -2,6 +2,6 @@
 MODULE = cpu
 
 # add a list of subdirectories, that should also be built
-DIRS += $(RIOTCPU)/riscv_common periph vendor
+DIRS = $(RIOTCPU)/riscv_common periph vendor
 
-include $(RIOTCPU)/riscv_common/Makefile
+include $(RIOTBASE)/Makefile.base
diff --git a/cpu/fe310/Makefile.features b/cpu/fe310/Makefile.features
index 5715137124..66120f1516 100644
--- a/cpu/fe310/Makefile.features
+++ b/cpu/fe310/Makefile.features
:...skipping...
diff --git a/cpu/fe310/Makefile b/cpu/fe310/Makefile
index 0dbf0f1807..fd024edf0e 100644
--- a/cpu/fe310/Makefile
+++ b/cpu/fe310/Makefile
@@ -2,6 +2,6 @@
 MODULE = cpu
 
 # add a list of subdirectories, that should also be built
-DIRS += $(RIOTCPU)/riscv_common periph vendor
+DIRS = $(RIOTCPU)/riscv_common periph vendor
 
-include $(RIOTCPU)/riscv_common/Makefile
+include $(RIOTBASE)/Makefile.base
diff --git a/cpu/fe310/Makefile.features b/cpu/fe310/Makefile.features
index 5715137124..66120f1516 100644
--- a/cpu/fe310/Makefile.features
+++ b/cpu/fe310/Makefile.features
@@ -4,5 +4,6 @@ FEATURES_PROVIDED += cpp
 FEATURES_PROVIDED += libstdcpp
 FEATURES_PROVIDED += newlib
 FEATURES_PROVIDED += periph_cpuid
+FEATURES_PROVIDED += periph_gpio periph_gpio_irq
 
 include $(RIOTCPU)/riscv_common/Makefile.features
diff --git a/cpu/fe310/cpu.c b/cpu/fe310/cpu.c
index a244c1f49b..a10ea94dd6 100644
--- a/cpu/fe310/cpu.c
+++ b/cpu/fe310/cpu.c
@@ -18,7 +18,6 @@
  */
 
 #include "cpu.h"
-#include "nanostubs.h"
 #include "periph/init.h"
 #include "periph_conf.h"
 
diff --git a/cpu/riscv_common/Makefile b/cpu/riscv_common/Makefile
index 880129078d..e09377cd1e 100644
--- a/cpu/riscv_common/Makefile
+++ b/cpu/riscv_common/Makefile
@@ -1,3 +1,3 @@
-DIRS += periph
+DIRS = periph
 
 include $(RIOTBASE)/Makefile.base
diff --git a/cpu/riscv_common/Makefile.dep b/cpu/riscv_common/Makefile.dep
index 4c9bbe5ef7..70aa523b8c 100644
--- a/cpu/riscv_common/Makefile.dep
+++ b/cpu/riscv_common/Makefile.dep
@@ -2,7 +2,7 @@
 USEMODULE += riscv_common
 
 # include common periph code
-USEMODULE += riscv_periph_common
+USEMODULE += riscv_common_periph
 
 # Make calls to malloc and friends thread-safe
 USEMODULE += malloc_thread_safe
diff --git a/cpu/riscv_common/periph/Makefile b/cpu/riscv_common/periph/Makefile
index b83a4e8257..db019f82ea 100644
--- a/cpu/riscv_common/periph/Makefile
+++ b/cpu/riscv_common/periph/Makefile
@@ -1,2 +1,2 @@
-MODULE = riscv_periph_common
+MODULE = riscv_common_periph
 include $(RIOTMAKE)/periph.mk

cpu/fe310/Makefile

bergzand · 2021-02-04T22:33:09Z

@aabadie Murdock is all green, do you mind if I squash here again?

aabadie · 2021-02-05T08:10:46Z

do you mind if I squash here again?

No problem, go ahead!

bergzand · 2021-02-05T09:21:42Z

Squashed!

aabadie · 2021-02-05T15:36:47Z

I ran compile_and_test_for_board on this PR and there are a lot of tests failing. Some are unrelated to this PR and are a regression compared to 2020.10.
I'll report here later.

Example with tests/thread_flags:

main(): This is RIOT! (Version: 2021.01-devel-1905-g4736a-HEAD)
START
thread(): waiting for 0x1...
main(): setting flag 0x0001
thread(): received flags: 0x0001
thread(): waiting for 0x1 || 0x64...
main(): setting flag 0x0064
thread(): received flags: 0x0064
thread(): waiting for 0x2 && 0x4...
main(): setting flag 0x0001
main(): setting flag 0x0008
main(): setting flag 0x0002
main(): setting flag 0x0004
thread(): received flags: 0x0006
thread(): waiting for any flag, one by one
thread(): received flags: 0x0001
thread(): waiting for any flag, one by one
thread(): received flags: 0x0008
main: setting 100ms timeout...
main: timeout triggered. time passed: 100068us
main: setting 100ms timeout (using uint64)...
Unhandled trap:
  mcause: 0x1
  mepc:   0x20010a94
  mtval:  0x20010a94
*** RIOT kernel panic:
Unhandled trap

*** halted.

Bisecting points to 1b2adb4 from #15736

aabadie · 2021-02-05T19:43:18Z

Here is the output of compile_and_test_for_board run on hifive1b with this PR:

ERROR:hifive1b:Tests failed: 10
Failures during test:
- [tests/event_wait_timeout](tests/event_wait_timeout/test.failed)
- [tests/gnrc_ipv6_fwd_w_sub](tests/gnrc_ipv6_fwd_w_sub/test.failed)
- [tests/periph_wdt](tests/periph_wdt/test.failed)
- [tests/pkg_utensor](tests/pkg_utensor/test.failed)
- [tests/ps_schedstatistics](tests/ps_schedstatistics/test.failed)
- [tests/sys_architecture](tests/sys_architecture/test.failed)
- [tests/thread_flags](tests/thread_flags/test.failed)
- [tests/thread_flags_xtimer](tests/thread_flags_xtimer/test.failed)
- [tests/xtimer_periodic_wakeup](tests/xtimer_periodic_wakeup/test.failed)
- [tests/xtimer_usleep](tests/xtimer_usleep/test.failed)

As said above, some of the failing tests are unrelated to this PR and some are a regression introduced by #15736 (unfortunately in the latest release), so I compared them against master (just re-ran the failing tests with the master branch):

ERROR:hifive1b:Tests failed: 10
Failures during test:
- [tests/event_wait_timeout](tests/event_wait_timeout/test.failed)
- [tests/gnrc_ipv6_fwd_w_sub](tests/gnrc_ipv6_fwd_w_sub/test.failed)
- [tests/periph_wdt](tests/periph_wdt/test.failed)
- [tests/pkg_utensor](tests/pkg_utensor/test.failed)
- [tests/ps_schedstatistics](tests/ps_schedstatistics/test.failed)
- [tests/sys_architecture](tests/sys_architecture/test.failed)
- [tests/thread_flags](tests/thread_flags/test.failed)
- [tests/thread_flags_xtimer](tests/thread_flags_xtimer/test.failed)
- [tests/xtimer_periodic_wakeup](tests/xtimer_periodic_wakeup/test.failed)
- [tests/xtimer_usleep](tests/xtimer_usleep/test.failed)

As a reference, here are results compared to 2021.01-devel:

ERROR:hifive1b:Tests failed: 7
Failures during test:
- [tests/gnrc_ipv6_fwd_w_sub](tests/gnrc_ipv6_fwd_w_sub/test.failed)
- [tests/periph_wdt](tests/periph_wdt/test.failed)
- [tests/pkg_utensor](tests/pkg_utensor/test.failed)
- [tests/ps_schedstatistics](tests/ps_schedstatistics/test.failed)
- [tests/sys_architecture](tests/sys_architecture/test.failed)
- [tests/xtimer_periodic_wakeup](tests/xtimer_periodic_wakeup/test.failed)
- [tests/xtimer_usleep](tests/xtimer_usleep/test.failed)

So #15736 introduced a regression for tests/event_wait_timeout, tests/thread_flags and tests/thread_flags_xtimer.

bergzand · 2021-02-06T12:04:39Z

So all failures are unrelated to this PR?

aabadie

Compared to master, there's no regression introduced by this PR. I added a #13086 (comment) in #13086 regarding the regression I found in master, so we don't loose track of them.

Anyway, let's not block this PR because of them.

I'm fine with the changes here, they all look good.

ACK

bergzand · 2021-02-08T09:28:30Z

Thanks for the review! I'll try to pick up the regression asap

bergzand added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Platform: RISC-V Platform: This PR/issue effects RISC-V-based platforms labels Jan 7, 2021

bergzand requested review from MrKevinWeiss, aabadie, cgundogan, jia200x, kaspar030 and leandrolanzieri as code owners January 7, 2021 14:57

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch 3 times, most recently from 2248a49 to a9eae0e Compare January 7, 2021 15:19

benpicco reviewed Jan 7, 2021

View reviewed changes

cpu/riscv_common/include/periph_cpu_common.h Outdated Show resolved Hide resolved

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch 2 times, most recently from 5c8a422 to b045327 Compare January 15, 2021 15:22

benpicco reviewed Jan 18, 2021

View reviewed changes

bergzand added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 18, 2021

aabadie reviewed Jan 19, 2021

View reviewed changes

aabadie mentioned this pull request Jan 19, 2021

vera++: increase line length #15793

Merged

aabadie reviewed Jan 19, 2021

View reviewed changes

cpu/fe310/include/cpu_conf.h Show resolved Hide resolved

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch from 515c5c0 to f9c960d Compare January 26, 2021 10:22

bergzand removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 26, 2021

aabadie reviewed Jan 26, 2021

View reviewed changes

cpu/fe310/include/periph_cpu.h Outdated Show resolved Hide resolved

aabadie reviewed Jan 26, 2021

View reviewed changes

cpu/riscv_common/Makefile.dep Outdated Show resolved Hide resolved

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch from 61ae50a to f7b149c Compare January 26, 2021 13:06

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch from f7b149c to 43a894d Compare January 26, 2021 14:42

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 26, 2021

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch from 43a894d to 423268b Compare January 26, 2021 14:44

aabadie self-assigned this Jan 26, 2021

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch 2 times, most recently from 6c65c5b to 6414885 Compare February 2, 2021 15:46

aabadie requested changes Feb 2, 2021

View reviewed changes

cpu/fe310/Makefile Outdated Show resolved Hide resolved

bergzand added 2 commits February 5, 2021 09:32

riscv_common: Refactor common fe310 code to riscv_common

2692957

uncrustify: also include riscv_common in the whitelist

19bb182

bergzand force-pushed the pr/rv32i/fe310_rv32i_refactor branch from 55a4760 to 19bb182 Compare February 5, 2021 08:32

aabadie mentioned this pull request Feb 8, 2021

Failing tests on FE310 (Hifive1b) #13086

Open

aabadie approved these changes Feb 8, 2021

View reviewed changes

aabadie merged commit 50cf93c into RIOT-OS:master Feb 8, 2021

bergzand deleted the pr/rv32i/fe310_rv32i_refactor branch February 8, 2021 09:47

This was referenced Feb 8, 2021

riscv_common: make thread_yield_higher IRQ compatible #15942

Merged

fe310: make thread_yield_higher IRQ compatible [backport 2021.01] #15961

Merged

kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021

	* @brief Memory definitions for for the RISC-V CPU
	* @brief Memory definitions for the RISC-V CPU

Conversation

bergzand commented Jan 7, 2021

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

benpicco Jan 18, 2021

Choose a reason for hiding this comment

Uh oh!

bergzand Jan 26, 2021

Choose a reason for hiding this comment

Uh oh!

bergzand commented Jan 18, 2021

Uh oh!

aabadie Jan 19, 2021

Choose a reason for hiding this comment

Uh oh!

bergzand commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

aabadie commented Jan 19, 2021

Uh oh!

fjmolinas commented Jan 26, 2021

Uh oh!

bergzand commented Jan 26, 2021

Uh oh!

bergzand commented Jan 26, 2021

Uh oh!

bergzand commented Jan 26, 2021

Uh oh!

Uh oh!

Uh oh!

aabadie commented Jan 26, 2021

Uh oh!

aabadie commented Jan 26, 2021

Uh oh!

bergzand commented Jan 26, 2021

Uh oh!

fjmolinas commented Feb 2, 2021

Uh oh!

bergzand commented Feb 2, 2021

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bergzand commented Feb 4, 2021

Uh oh!

aabadie commented Feb 5, 2021

Uh oh!

bergzand commented Feb 5, 2021

Uh oh!

aabadie commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Feb 5, 2021

Uh oh!

bergzand commented Feb 6, 2021

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

bergzand commented Feb 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bergzand commented Jan 19, 2021 •

edited

Loading

aabadie commented Feb 5, 2021 •

edited

Loading