Skip to content

cpu/sam0_common: add hwrng driver#11647

Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:sam0-hwrng
Aug 3, 2019
Merged

cpu/sam0_common: add hwrng driver#11647
benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco:sam0-hwrng

Conversation

@benpicco
Copy link
Contributor

@benpicco benpicco commented Jun 6, 2019

SAML1X, SAML2X and SAMD5X all share the same HWRNG peripheral.

This adds a driver for it.

@dylad dylad self-assigned this Jun 6, 2019
@dylad dylad added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Jun 6, 2019
@dylad dylad added this to the Release 2019.07 milestone Jun 6, 2019
@dylad dylad added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 6, 2019
@benpicco benpicco force-pushed the sam0-hwrng branch 2 times, most recently from fc55180 to c46bd70 Compare June 6, 2019 15:29
@dylad dylad added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Jun 6, 2019
@benpicco
Copy link
Contributor Author

benpicco commented Jun 6, 2019

Hm, Murdock is confused

make: Entering directory '/tmp/dwq.0.8274662956598827/214d4a4189f5c8ea0f2c19669659fdab/tests/periph_hwrng'
There are unsatisfied feature requirements: periph_hwrng
\n\nEXPECT ERRORS!\n\n
Building application "tests_periph_hwrng" for "samr30-xpro" with MCU "saml21".

@dylad
Copy link
Member

dylad commented Jun 6, 2019

@benpicco I've open a ticket on Microchip support website to have an official answer from them. We should just wait a couple of days.

@benpicco
Copy link
Contributor Author

benpicco commented Jun 20, 2019

So how should we continue? Just drop the RNG for the entire saml21 family?

@dylad dylad 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 Jun 20, 2019
@dylad
Copy link
Member

dylad commented Jun 21, 2019

@cladmi Does the Makefile changes look good to you ?

@cladmi
Copy link
Contributor

cladmi commented Jun 25, 2019 •
edited
Loading

Processing Makefile.features must be done before Makefile.include.
Changing this would have required a dedicated pull request and testing procedure.

Also murdock does not use Makefile.include for checking supported boards

RIOT/makefiles/info-global.inc.mk

Lines 19 to 21 in 90e8da0

include $(RIOTBASE)/Makefile.features
include $(RIOTBASE)/Makefile.dep

So when doing make info-boards-supported the CPU_MODEL is empty and does not go in this section. Which I guess lead to the There are unsatisfied feature requirements: periph_hwrng and required BLACKLISTING.

The issue is that currently CPU_MODEL is defined in Makefile.include but is needed before. References:

Please give feedback over there if you are interested.

The current work around is to use BOARD until it is fixed with a comment. And I prefer whitelisting over blacklisting.

git grep '$(BOARD)' ':**/Makefile.features'
boards/common/arduino-atmega/Makefile.features:ifeq (,$(filter jiminy-mega256rfr2,$(BOARD)))
cpu/kinetis/Makefile.features:ifneq (,$(filter-out $(_KINETIS_BOARDS_WITHOUT_HWRNG),$(BOARD)))
cpu/stm32_common/Makefile.features:ifneq (,$(filter $(BOARDS_WITHOUT_HWRNG),$(BOARD)))
cpu/stm32f0/Makefile.features:ifeq (,$(filter nucleo-f031k6,$(BOARD)))

cladmi
cladmi suggested changes Jun 25, 2019
View reviewed changes
Copy link
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

Currently need to enable restricted by BOARD in cpu/saml21/Makefile.features.
It removes the need for the other changes.

@benpicco
Copy link
Contributor Author

benpicco commented Jun 25, 2019

I've followed the precedent set by stm32 and kinetis and introduced a BOARDS_WITHOUT_HWRNG list that currently only contains samr30-xpro.

@cladmi
Copy link
Contributor

cladmi commented Jun 25, 2019

Please do not push force/rebase to master during a review. It somehow forces to re-review everything each time as it is a pain to track what changed.

@benpicco
Copy link
Contributor Author

benpicco commented Jul 15, 2019

Sorry, I thought it would be cleaner to drop the commit and force-push instead of pushing a revert commit.

@cladmi
Copy link
Contributor

cladmi commented Jul 15, 2019

The current process is to not squash/rebase during a review as mentioned in https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#squash-commits-after-review

The makefiles changes are now good for me. @dylad I let you handle the rest.

@MrKevinWeiss MrKevinWeiss removed this from the Release 2019.07 milestone Jul 16, 2019
@MrKevinWeiss MrKevinWeiss modified the milestone: Release 2019.10 Jul 16, 2019
@benpicco benpicco mentioned this pull request Jul 24, 2019
Merged
@PeterKietzmann
Copy link
Member

PeterKietzmann commented Jul 25, 2019

I've tested this PR on a saml11-xpro. First I ran tests/periph_hwrng which spits out "different" values. Then I ran tests/rng and its shell commands.

  • Rate of 529 KiB/s seems ok (a remote-reva containing cc2538 has 507 KiB/s).
  • FIPS 140-2 succeeds
  • Bit distributions look equally distributed.
#  speed
# Running speed test, with seed 0 using HW RNG.
# 
# Running speed test for 10 seconds
# Collected 1355927 samples in 10.000037 seconds (529 KiB/s).
> fips
#  fips
# Running FIPS 140-2 test, with seed 0 using HW RNG.
# 
# - Monobit test: passed
# - Poker test: passed
# - Run test: passed
# - Longrun test: passed
> distributions
#  distributions
# Running distributions test, with seed 0 using HW RNG.
# 
# For 32-bit samples (min = 4907, max = 5064, avg = 4985):
# 00: ##########################################################################
# 01: ###########################################################################
# 02: ###########################################################################
# 03: ##########################################################################
# 04: ##########################################################################
# 05: ##########################################################################
# 06: ##########################################################################
# 07: ##########################################################################
# 08: ###########################################################################
# 09: #########################################################################
# 10: #########################################################################
# 11: #########################################################################
# 12: #########################################################################
# 13: ##########################################################################
# 14: ##########################################################################
# 15: ##########################################################################
# 16: ##########################################################################
# 17: #########################################################################
# 18: ###########################################################################
# 19: ##########################################################################
# 20: ###########################################################################
# 21: ##########################################################################
# 22: ###########################################################################
# 23: #########################################################################
# 24: ##########################################################################
# 25: #########################################################################
# 26: ##########################################################################
# 27: ##########################################################################
# 28: ##########################################################################
# 29: #########################################################################
# 30: ##########################################################################
# 31: ##########################################################################

@PeterKietzmann
Copy link
Member

PeterKietzmann commented Jul 25, 2019

The code looks good to me too. I'll set all labels but "fundamentals" as I didn't participate in the initial discussion. @dylad please check and eventually remove your change request.

@PeterKietzmann PeterKietzmann added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 25, 2019
@dylad
Copy link
Member

dylad commented Jul 25, 2019

Same here, I'll take a look at it next week.

@benpicco benpicco mentioned this pull request Jul 29, 2019
Closed
@dylad
Copy link
Member

dylad commented Aug 2, 2019

I just retest this PR on SAML10, SAML11, SAML21 & SAME54 with tests/periph_hwrng.

Output looks good to me !

@dylad dylad added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Aug 2, 2019
@dylad dylad added this to the Release 2019.10 milestone Aug 2, 2019
dylad
dylad approved these changes Aug 2, 2019
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

ACK.
Tested this PR on several boards.

benpicco and others added 2 commits August 3, 2019 14:50
@benpicco
cpu/sam0_common: add hwrng driver
a9b0db3
@benpicco
cpu: saml21: don't silently ignore missing makefile
7e50889 
-include will throw no error if sam0_common/Makefile.features does not exist.
This may not have been intentional as none of the other sam0 implementations
do this.

Replace it with a normal include.
@benpicco benpicco merged commit 2ddfd1f into RIOT-OS:master Aug 3, 2019
@benpicco benpicco deleted the sam0-hwrng branch August 3, 2019 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dylad dylad dylad approved these changes

+1 more reviewer

@cladmi cladmi cladmi requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dylad dylad

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

5 participants

@benpicco @dylad @cladmi @PeterKietzmann @MrKevinWeiss