Skip to content

boards/hifive1: include cpu/fe310 features#10063

Merged
smlng merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/hifive1/use_fe310_features
Oct 1, 2018
Merged

boards/hifive1: include cpu/fe310 features#10063
smlng merged 1 commit intoRIOT-OS:masterfrom
cladmi:pr/hifive1/use_fe310_features

Conversation

@cladmi
Copy link
Copy Markdown
Contributor

@cladmi cladmi commented Sep 27, 2018

Contribution description

The board should process its CPU features.
This has the consequence to make 'periph_pm' visible to the board.

I also removed the duplicate FEATURES_PROVIDED from the board defined in
the cpu.

Testing procedure

Verify its ok to have periph_pm for hifive1:

Verify that indeed board hifive1 provides periph_pm, by design and in the implementation.
I can correctly build tests_pm with this PR but not sure if I should test something else.

List features provided

We now also see periph_pm for hifive1 provided features (cpuid moved but is still there).

make --no-print-directory -C examples/hello-world info-debug-variable-FEATURES_PROVIDED BOARD=hifive1
periph_gpio periph_gpio_irq periph_rtc periph_rtt periph_timer periph_uart periph_cpuid periph_pm

# No `periph_pm` in riot/master
git checkout riot/master
make --no-print-directory -C examples/hello-world info-debug-variable-FEATURES_PROVIDED BOARD=hifive1
periph_cpuid periph_gpio periph_gpio_irq periph_rtc periph_rtt periph_timer periph_uart

Issues/PRs references

Found while working on #9913

@cladmi
boards/hifive1: include cpu/fe310 features
1c89d23 
The board should process its CPU features.
This has the consequence to make 'periph_pm' visible to the board.

I also removed the duplicate FEATURES_PROVIDED from the board defined in
the cpu.
@cladmi cladmi added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Sep 27, 2018
@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Sep 27, 2018

@kenrabold Also some other changes to the hifive1 features. You are the best placed to know if the result makes sense I guess.

My goal is to have the include $(RIOTCPU)/fe310/Makefile.features and see if anything should be adapted to make it correct.

@cladmi cladmi mentioned this pull request Sep 27, 2018
Open
22 tasks
smlng
smlng reviewed Sep 28, 2018
View reviewed changes
boards/hifive1/Makefile.features
# The board MPU family (used for grouping by the CI system)
FEATURES_MCU_GROUP = risc_v

include $(RIOTCPU)/fe310/Makefile.features
Copy link
Copy Markdown
Member

@smlng smlng Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (all) other boards we use the -include notation, does it make sense here to?

Copy link
Copy Markdown
Contributor Author

@cladmi cladmi Sep 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unnecessary to do '-include' as the file exist. And it is my next step to change all to include

Copy link
Copy Markdown
Contributor Author

@cladmi cladmi Sep 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boards: always include cpu features #10078

Copy link
Copy Markdown
Member

@smlng smlng Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, I just opt for consistency when ever possible

@cladmi cladmi mentioned this pull request Sep 28, 2018
Merged
smlng
smlng approved these changes Oct 1, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK!

@smlng smlng merged commit dc86533 into RIOT-OS:master Oct 1, 2018
@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Oct 1, 2018

@smlng Did you test the testing procedure ?

Verify its ok to have periph_pm for hifive1:

Verify that indeed board hifive1 provides periph_pm, by design and in the implementation.
I can correctly build tests_pm with this PR but not sure if I should test something else.

Because that was a consequence of merging this that was not there before.

@cladmi
Copy link
Copy Markdown
Contributor Author

cladmi commented Oct 1, 2018

If it is not the case, the declaration should "just" be removed from cpu/fe310/Makefile.features and let on the board responsibility to define it.

@cladmi cladmi deleted the pr/hifive1/use_fe310_features branch October 1, 2018 13:05
@jia200x jia200x added this to the Release 2018.10 milestone Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@smlng smlng smlng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: boards Area: Board ports Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2018.10

Development

Successfully merging this pull request may close these issues.

3 participants

@cladmi @smlng @jia200x