Skip to content

Conversation

@benpicco
Copy link
Contributor

This adds a driver for the True Random Number Generator found in some
Atmel SAM0 SoCs.
The Code is based on the driver for the SAM TRNG, but uses different
register and clock definitions.

split off from #14685

@benpicco benpicco added area: Crypto / RNG platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) labels Oct 23, 2019
@benpicco benpicco requested a review from galak October 23, 2019 10:55
Copy link
Contributor

@ulfalizer ulfalizer left a comment

Choose a reason for hiding this comment

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

Kconfig/devictree nits fixed.

Copy link
Member

@ioannisg ioannisg left a comment

Choose a reason for hiding this comment

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

Quick question: is this driver compatible with the Atmel SAM0x SoCs? E.g. samd21?

@benpicco
Copy link
Contributor Author

The samd2x series does not contain a HWRNG.
But it's compatible with the one found on saml2x (not yet supported by zephyr)

@ioannisg
Copy link
Member

The samd2x series does not contain a HWRNG.
But it's compatible with the one found on saml2x (not yet supported by zephyr)

So, you name this as: SAM0 entropy driver, so I expect it to be supported in ATMEL SoCs under soc/arm/sam0. If this is not the case, I guess we could change the name of the driver. Or, am I missing something?

@benpicco
Copy link
Contributor Author

SAMD5x, SAML2x, SAML1x and SAMD2x all share the same peripheral IP.

This is part of a larger PR that adds support for SAMD5x/SAME5x in soc/arm/sam0 (SAME5x is just SAMD5x, but with Ethernet - just like SAMR21 is SAMD21 with a radio)

@benpicco
Copy link
Contributor Author

benpicco commented Oct 23, 2019

tbh entropy_sam0.c and entropy_sam.c are extremely similar.
Maybe it's a better idea to add some #ifdefs to entropy_sam.c to avoid the code duplication.

It was justifiable when the driver was so simple, but now that the driver has grown in complexity, it feel like a bad idea.

This adds a driver for the True Random Number Generator found in some
Atmel SAM0 SoCs.
The Code is based on the driver for the SAM TRNG, but uses different
register and clock definitions.

Signed-off-by: Benjamin Valentin <[email protected]>
@ioannisg
Copy link
Member

I wonder: isn't the 0 in sam supposed to indicate cortex m0 (armv6)? Instead or cortex m3,4,7 (armv7)?

Is your samd5x an armv7? if so it shouldn't really fit under sam0. Or we need to seriously reorg the atmel Soc directories?

@benpicco
Copy link
Contributor Author

benpicco commented Oct 23, 2019

Yes historically samd2x was the first one of the 'new generation' of Atmel MCUs - based on Cortex-M0+.
The peripherals from this line were than reused in saml2x (Cortex-M0+), saml1x (Cortex-M23) and samd5x (Cortex-M4F).

The Atmel SAM MCUs are based on ARM7 cores iirc.

I don't think renaming would be worth the hassle.

@ioannisg
Copy link
Member

Yes historically samd2x was the first one of the 'new generation' of Atmel MCUs - based on Cortex-M0+.
The peripherals from this line were than reused in saml2x (Cortex-M0+), saml1x (Cortex-M23) and samd5x (Cortex-M4F).

The Atmel SAM MCUs are based on ARM7 cores iirc.

I don't think renaming would be worth the hassle.

OK, so you say that the distinction between sam and sam0 is not the ARM architecture, but rather, the Atmel processor family (sam is the old and sam0 the more modern). Correct?

@benpicco
Copy link
Contributor Author

Yes. Actually the Atmel SAM family is also based around Coretx-M* cores but the peripherals are different. E.g. SAM have different peripherals for SPI, I2C and UART whereas SAM0 uses universal SERCOM peripherals for that.

When I added support for samd5x I just had to change how the peripherals are clocked, other than that I could use the existing sam0 drivers and things just forked for the most part, so I concluded it belongs to that family.

@ioannisg
Copy link
Member

tbh entropy_sam0.c and entropy_sam.c are extremely similar.
Maybe it's a better idea to add some #ifdefs to entropy_sam.c to avoid the code duplication.

It was justifiable when the driver was so simple, but now that the driver has grown in complexity, it feel like a bad idea.

I think i agree. It seems very similar to the sam driver - it is not worth the duplicate file, imho

@jhedberg
Copy link
Member

jhedberg commented Jan 7, 2020

What's the latest on this PR? Based on the latest comments it sounds like a different approach will be taken. Would that be a separate PR or an update to this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Crypto / RNG area: Devicetree platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants