cpu/fe310: rtc depend on the rtt feature and hifive1 update

[cladmi]

Contribution description

It is the role of boards based on 'cpu/fe310' to give the configuration
for the rtc/rtt.

The fe310/periph/rtc implementation depends on having periph/rtt configured
by the board so depends on the board 'providing' the periph_rtt feature
and declaring the required macros.

It should not simply depend of the 'periph_rtt' module as this does not
enforce having a configuration for the module in the board.

In practice, when compiling, it would result in undefined 'RTT' symbols,
instead of the build system detecting it.

This also updates board/hifive1 as it does not need to handle this anymore.

Testing procedure

To test this, you can modify the hifive1 board as if it did not support periph_rtt:

diff --git a/boards/hifive1/Makefile.features b/boards/hifive1/Makefile.features
index 3920db679..a40f0c0e2 100644
--- a/boards/hifive1/Makefile.features
+++ b/boards/hifive1/Makefile.features
@@ -3,14 +3,9 @@ FEATURES_PROVIDED += periph_cpuid
 FEATURES_PROVIDED += periph_gpio periph_gpio_irq
 #FEATURES_PROVIDED += periph_pwm
 FEATURES_PROVIDED += periph_rtc
-FEATURES_PROVIDED += periph_rtt
 #FEATURES_PROVIDED += periph_spi
 FEATURES_PROVIDED += periph_timer
 FEATURES_PROVIDED += periph_uart
 
-ifneq (,$(filter periph_rtc,$(FEATURES_REQUIRED)))
-  FEATURES_REQUIRED += periph_rtt
-endif
-
 # The board MPU family (used for grouping by the CI system)
 FEATURES_MCU_GROUP = risc_v
diff --git a/boards/hifive1/include/periph_conf.h b/boards/hifive1/include/periph_conf.h
index e0dd67612..cae1eca9f 100644
--- a/boards/hifive1/include/periph_conf.h
+++ b/boards/hifive1/include/periph_conf.h
@@ -54,15 +54,10 @@ extern "C" {
 /** @} */
 
 /**
- * @name    RTT/RTC configuration
+ * @name    RTC configuration
  *
  * @{
  */
-#define RTT_NUMOF                   (1)
-#define RTT_FREQUENCY               (1)             /* in Hz */
-#define RTT_MAX_VALUE               (0xFFFFFFFF)
-#define RTT_INTR_PRIORITY           (2)
-
 #define RTC_NUMOF                   (1)
 /** @} */

And when compiling you get error RTT_ undeclared errors

With this PR and the diff you correctly get the missing features error

make -C examples/default/ BOARD=hifive1 info-debug-variable-FEATURES_USED all
make: Entering directory '/home/cladmi/git/work/riot_test/examples/default'
There are unsatisfied feature requirements: periph_rtt


EXPECT ERRORS!

Issues/PRs references

This is part of #10060 and was found while working on #9913

[cladmi]

@kenrabold some cleanup for your board/cpu

[smlng]

this whole periph_rtt feature is handled differently for most boards right now, from the approach here. The closest is the kinetis but even there the CPU has the USEMODULE += periph_rtt and each kinetis-based board has FEATURE_PROVIDED += periph_rtt.

[smlng]

tough I understand that this fixes a bug for the hifive1 board and fe310 cpu combination

[cladmi]

Correct me if I am wrong it is really something I am not really good with.

From what I understood from with discussions with @kYc0o . The configuration of the rtc and rtt is board specific as it depends on the board wiring external crystals and things to the cpu.

So even if the implementation can be done by the CPU it is still only made available by the board having specific hardware for it, and for us, having header configuration for it.

If this is wrong, I can change to another direction you tell me.
It would need cleaning anyway as checking against FEATURES_REQUIRED is incomplete.

[cladmi]

this whole periph_rtt feature is handled differently for most boards right now, from the approach here. The closest is the kinetis but even there the CPU has the USEMODULE += periph_rtt and each kinetis-based board has FEATURE_PROVIDED += periph_rtt.

If for any reason, one board does not provide periph_rtt and not have headers it would break too as USEMODULE += periph_ does not checks if it is available.
I can provide another followup PR for it too if we go this direction.

[cladmi]

@gebart I think you would have a better knowledge on this and if it would apply to kinetis boards.

[kenrabold]

When I implemented the code for RISC-V CPU and HiFive1 board, I struggled with understanding the feature dependency make rules and getting them to work. In my opinion, all of the FEATURES_PROVIDED that are listed in the Makefile.features for the hifive1 board really should be in the CPU Makefile.features under cpu\fe310. Those "features" are provided by the SoC, not the board, and it should be the board that identifies which of those features it wants to turn on using a FEATURES_REQUIRED setting. However, I could not get the build to work with that setup (it could be fixed now?) Also, neither the FE310 CPU nor the HiFive1 board have an RTC, so I used the same RTC on top of the RTT implementation that I saw in the kinetis board. But you need a dependency linking the selection of the RTC feature by requiring the RTT for the FE310 CPU. Again, I could not get the compilation to work.

I'm open to suggestions on ways to make the configuration of the RISC-V CPU and board files more flexible

[cladmi]

@kenrabold thank you for your feedback. I will try to explain a bit more with my words.
With a lot of discussions with @jcarrano and @kYc0o I finally start to manage to put understandable words on.

There is also still somehow a mix between using FEATURES_OPTIONAL to define optional modules (which are otherwise currently not implemented) and the dependency on hardware. So some things are currently done with FEATURES_REQUIRED or FEATURES_OPTIONAL and may need to be done using USEMODULE and a non existing OPTIONAL_USEMODULE and I am still a bit unclear about these one so excuse the lack of precision around this.

Those "features" are provided by the SoC, not the board, and it should be the board that identifies which of those features it wants to turn on using a FEATURES_REQUIRED setting.

In fact, it is not the board that identifies which one to turn on or not but the application, or its dependencies.
You may have an rtc, if the application does not want to enable it, it is not included.

The FEATURES_PROVIDED that describe that it is possible to have the rtt and rtc for this board.

Why ? because there is:

• hardware available for it (what you call "features" provided by the SoC")
• an implementation that implements the api for this hardware
• a configuration to say how it is configured.

In your case:

• the hardware is in the cpu (and maybe the board has also has a crystal installed, I do not know if it is the case but then it is not 'SoC' only anymore)
• the implementation is done in cpu/fe310/periph
• the configuration is provided by the hifive1/include/periph_conf.h

Because of this requirement that the board provides the configuration and an external crystal it is the responsibility of the board to declare the FEATURES_PROVIDED.

An example that @kYc0o gave me, a cpu could have 4 uarts, but the board manufacturer decided to not wire them. There are there in the cpu, there is code to handle them, but they are not connected.

Should there be somewhere documented in the cpu that there are these 4 uarts that could be used if configured ? Yes. But maybe more in the documentation than in a build system dependency resolution file. There could be a comment in Makefile.features also of course.
Same for your rtc/rtt case.

Also, neither the FE310 CPU nor the HiFive1 board have an RTC, so I used the same RTC on top of the RTT implementation that I saw in the kinetis board.

Even if currently periph_rtc is actually not a hardware peripheral we still need to describe it as a feature provided to be homogeneous with the rest. That is where we are not really clear with modules and features.

But you need a dependency linking the selection of the RTC feature by requiring the RTT for the FE310 CPU. Again, I could not get the compilation to work.

That is what I described by saying that the periph_rtc module (== implementation in cpu/fe310) requires the periph_rtt feature (== hardware available) in cpu/fe310/Makefile.dep.
In practice checking FEATURES_REQUIRED as you did in the board is not enough, it should be more done with FEATURES_USED but in that case, it is the implementation that needs it so it makes more sense to do it with USEMODULE.

With this, does it makes my changes understandable or did I leave something unclear in my explanation? Feedback will help summarizing this later in documentation.

I still have many things I could add but prefer not put too much details first if they are not helping.

[kenrabold]

@cladmi I think your changes are perfectly fine. Thanks for clarifying the use of the various features and modules defines

[cladmi]

@kenrabold thank you for your feedback and for forcing me to explain this. I used your request to try writing something that could act as a base for a documentation ;)

[jcarrano]

@cladmi

Even if currently periph_rtc is actually not a hardware peripheral we still need to describe it as a feature provided to be homogeneous with the rest. That is where we are not really clear with modules and features.

The case with software-emulated features is something I cannot figure out how to handle. If the implementation is generic (works with any board that provides RTT) then there should not be any need for the board to declare the RTC feature explicitly. This also applies to any other sw-emulated features.

For now I think it is good to migrate features out of the CPU, but we have to start thinking how to sort this out.

[cladmi]

Thank you for reviewing.

this whole periph_rtt feature is handled differently for most boards right now, from the approach here. The closest is the kinetis but even there the CPU has the USEMODULE += periph_rtt and each kinetis-based board has FEATURE_PROVIDED += periph_rtt.

@smlng I should check these, thank you.

[cladmi]

@jcarrano I would say that is an api choice from the rtc functionality (I use different words on purpose).
If for 2 years, all the rtc implementations are done based on rtt and can be implemented by a generic module, it makes sense to only describe it as a module (that has a requirement on rtc feature). But the day there is an hardware rtc implementation, then the old scheme does not work anymore and the api must be updated to say it is actually what we describe as a feature.

Here we are already at the point where there are rtc only implementations and some of the implementations are based on an rtt. It is just a small overhead for these ones.

But definitely these are the refinements I am still missing on how to define what should and what should not be described as a feature.
Also there are currently limitations that prevent us from doing it with only module.
I keep this concerns for the next step :)