getentropy() implementation on BBB
Christian Mauderer
list at c-mauderer.de
Wed Mar 7 18:10:51 UTC 2018
Hello Udit,
let me start with a hint: It's quite normal that the first patches of a
contributor get a lot of attention and have a lot of revisions. That can
be quite annoying for the one who writes the patch. But please
understand it in the way it's thought: As suggestions for improvement.
Did you already test that patches? There is a (quite basic) test in the
RTEMS testsuite: libtests/getentropy01. This test should run without
problems. If you have a debugger, you might also want to check whether
your getentropy implementation is reached and not the generic one.
The patch looks quite good already. But like I said: A lot of revisions.
I added some further remarks in the patch below ;-)
Best regards
Christian
Am 07.03.2018 um 11:26 schrieb Udit agarwal:
> Hi,
> I have updated the code, please have a look.
> From 74b8f4f5b9dd929b71ed5fb9dd0bc721a6f27a28 Mon Sep 17 00:00:00 2001
> From: Udit agarwal <dev.madaari at gmail.com <mailto:dev.madaari at gmail.com>>
> Date: Wed, 7 Mar 2018 15:52:13 +0530
> Subject: [PATCH] Added getentropy support to beagle BSP
>
> ---
> bsps/arm/include/libcpu/am335x.h | 34 ++++++++-
> c/src/lib/libbsp/arm/beagle/Makefile.am | 4 +
> .../libbsp/arm/beagle/getentropy/bbb_getentropy.c | 88
> ++++++++++++++++++++++
> 3 files changed, 125 insertions(+), 1 deletion(-)
> create mode 100644 c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
>
> diff --git a/bsps/arm/include/libcpu/am335x.h
> b/bsps/arm/include/libcpu/am335x.h
> index 367e97c..92af6dc 100644
> --- a/bsps/arm/include/libcpu/am335x.h
> +++ b/bsps/arm/include/libcpu/am335x.h
> @@ -14,6 +14,9 @@
> * Modified by Ben Gras <beng at shrike-systems.com
> <mailto:beng at shrike-systems.com>> to add lots
> * of beagleboard/beaglebone definitions, delete lpc32xx specific
> * ones, and merge with some other header files.
> + *
> + * Modified by Udit agarwal <dev.madaari at gmail.com
> <mailto:dev.madaari at gmail.com>> to add random
> + * number generating module definitions along with register structure.
> */
>
> #if !defined(_AM335X_H_)
> @@ -701,4 +704,33 @@
> #define AM335X_CM_PER_OCPWP_L3_CLKSTCTRL_CLKACTIVITY_OCPWP_L4_GCLK
> (0x00000020u)
> #define AM335X_I2C_INT_STOP_CONDITION AM335X_I2C_IRQSTATUS_BF
>
> -#endif
> +/* TRNG Register */
> +
> +/* RNG base address */
> +#define RNG_BASE 0x48310000
> +/* RNG clock control */
> +#define CM_PER_RNG_CLKCTRL (AM335X_CM_PER_ADDR | (9 << 4))
> +/* rng module clock status bits */
> +#define AM335X_CLK_RNG_BIT_MASK (0x30000)
> +/* Offset from RNG base for output ready flag */
> +#define RNG_STATUS_RDY (1u << 0)
> +/* Offset from RNG base for FRO related error */
> +#define RNG_STATUS_ERR (1u << 1)
> +/* Offset from RNG base for clock status */
> +#define RNG_STATUS_CLK (1u << 31)
> +/* enable module */
> +#define AM335X_RNG_ENABLE (1 << 10)
> +
> +/* TRNG register structure */
> +struct bbb_trng_register
> +{
> + uint64_t output; /*00*/
> + uint32_t status; /*08*/
> + uint32_t irq_en; /*0c*/
> + uint32_t status_clr; /*10*/
> + uint32_t control; /*14*/
> + uint32_t config; /*18*/
> +};
> +typedef struct bbb_trng_register bbb_trng_register;
> +
> +#endif
> \ No newline at end of file
> diff --git a/c/src/lib/libbsp/arm/beagle/Makefile.am
> b/c/src/lib/libbsp/arm/beagle/Makefile.am
> index 8251660..bf92081 100644
> --- a/c/src/lib/libbsp/arm/beagle/Makefile.am
> +++ b/c/src/lib/libbsp/arm/beagle/Makefile.am
> @@ -88,9 +88,13 @@ libbsp_a_SOURCES += gpio/bbb-gpio.c
> #pwm
> libbsp_a_SOURCES += pwm/pwm.c
>
> +#getentropy
> +libbsp_a_SOURCES += getentropy/bbb_getentropy.c
> +
Adding that one is good. But please also remove the default
implementation from the Makefile.am:
libbsp_a_SOURCES += ../../shared/getentropy-cpucounter.c
Otherwise you might get problems during linking.
> #RTC
> libbsp_a_SOURCES += rtc.c
> libbsp_a_SOURCES += ../../shared/tod.c
> +
You added a empty line in a section where you didn't change anything
else. Normally that's not a good idea because it slightly obfuscates the
history. Try to generate patches that only touch the area that is
related to your change. If you really think that this line should be
added, you should create a second patch with a description like "fix
spacing" or similar. But I wouldn't suggest to do that.
> # Clock
> libbsp_a_SOURCES += clock.c
> libbsp_a_SOURCES += ../../shared/clockdrv_shell.h
> diff --git a/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> b/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> new file mode 100644
> index 0000000..5b7c5d0
> --- /dev/null
> +++ b/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> @@ -0,0 +1,88 @@
> +/**
> +* @file
> +*
> +* @ingroup arm_beagle
> +*
> +* @brief Getentropy implementation on BeagleBone Black BSP
> +*/
> +
> +/*
> +* Copyright (c) 2018 Udit kumar agarwal <dev.madaari at gmail.com
> <http://gmail.com>>
> +*
> +* The license and distribution terms for this file may be
> +* found in the file LICENSE in this distribution or at
> +* http://www.rtems.org/license/LICENSE.
> +*/
> +
> +#include <libcpu/am335x.h>
> +#include <unistd.h>
> +#include <string.h>
> +#include <rtems/sysinit.h>
> +#include <rtems/types.h>
> +
> +/* max refill 34 * 256 cycles */
> +#define AM335X_RNG_MAX_REFILL (34 << 16)
> +/* min refill 33 * 64 cycles */
> +#define AM335X_RNG_MIN_REFILL (33 << 0)
> +/* startup 33 * 256 cycles */
> +#define AM335X_RNG_STARTUP_CYCLES (33 << 16)
> +
> +/* maximun and minimum refill cycle sets the number of samples to be taken
> + from FRO to generate random number */
> +static void am335x_rng_enable(bbb_trng_register *rng)
> +{
> + rng->control = rng->config = 0;
> + rng->config |= AM335X_RNG_MIN_REFILL | AM335X_RNG_MAX_REFILL ;
> + rng->control |= AM335X_RNG_STARTUP_CYCLES | AM335X_RNG_ENABLE ;
> +}
> +
> +static void am335x_rng_clock_enable(void)
> +{
> + *(volatile uint8_t *) CM_PER_RNG_CLKCTRL = 2;
> + while( *(volatile uint32_t *) CM_PER_RNG_CLKCTRL &
> AM335X_CLK_RNG_BIT_MASK ) {}
Is this line within 80 characters? The RTEMS style uses lines <= 80 chars.
When I'm already at style: Although it's not strictly enforced in BSPs,
RTEMS uses two spaces for indentation. See
https://devel.rtems.org/wiki/Developer/Coding/Conventions
Also I don't think we have a style rule for that I would suggest that
you mark it if you have an intentionally empty loop. That makes it clear
that you didn't just forget to write some code. E.g.
while( some condition ) {
/* wait */
}
> +}
> +
> +static uint64_t trng_getranddata(bbb_trng_register *rng)
> +{
> + uint64_t output = rng->output;
> + return output;
> +}
> +
> +int getentropy(void *ptr, size_t n)
> +{
> + volatile const bbb_trng_register *rng = (bbb_trng_register*) RNG_BASE;
> + am335x_rng_enable(rng);
> + while (n > 0)
> + {
> + uint64_t random;
> + size_t copy;
> +
> + /* wait untill RNG becomes ready with next set of random data */
> + while( ( rng->status & RNG_STATUS_RDY ) == 0 );
Again: I would suggest to mark it that this loop is intentionally empty.
> +
> + random = trng_getranddata(rng);
> +
> + /* Checking for error by masking all bits other then error bit
> in status register */
<= 80 Chars?
> + if( ((rng->status & RNG_STATUS_ERR)>>1) == 0)
> + {
> + /* clear the status flag after reading to generate new
> random value*/
<= 80 Chars?
> + rng->status_clr = RNG_STATUS_RDY;
> + copy = sizeof(random);
> + if ( n < copy )
> + {
> + copy = n;
> + }
> + memcpy(ptr, &random, copy);
> + n -= copy;
> + ptr = (char*)ptr + copy;
> + }
> + }
> +
> + return 0;
> +}
> +
> +RTEMS_SYSINIT_ITEM(
> + am335x_rng_clock_enable,
> + RTEMS_SYSINIT_DEVICE_DRIVERS,
> + RTEMS_SYSINIT_ORDER_LAST
> +);
> \ No newline at end of file
> --
> 1.9.1
>
> Regards,
> Udit agarwal
>
> On Tue, Mar 6, 2018 at 11:05 PM, Gedare Bloom <gedare at rtems.org
> <mailto:gedare at rtems.org>> wrote:
>
> On Tue, Mar 6, 2018 at 9:43 AM, Udit agarwal <dev.madaari at gmail.com
> <mailto:dev.madaari at gmail.com>> wrote:
> > Hi,
> > Here's the updated code(I have also attached the patch PFA):
> >
> > From 96e6e1bfd8cffeef5d309eb0a07fe2bfd086ef0a Mon Sep 17 00:00:00 2001
> > From: Udit agarwal <dev.madaari at gmail.com
> <mailto:dev.madaari at gmail.com>>
> > Date: Tue, 6 Mar 2018 20:07:44 +0530
> > Subject: [PATCH] Added getentropy support to BBB BSP
> >
> > ---
> > bsps/arm/beagle/headers.am <http://headers.am>
> | 1 +
> > bsps/arm/beagle/include/bsp/bbb_getentropy.h | 57
> +++++++++++++++
> > bsps/arm/include/libcpu/am335x.h | 16 +++++
> > c/src/lib/libbsp/arm/beagle/Makefile.am | 4 ++
> > .../libbsp/arm/beagle/getentropy/bbb_getentropy.c | 80
> > ++++++++++++++++++++++
> > 5 files changed, 158 insertions(+)
> > create mode 100644 bsps/arm/beagle/include/bsp/bbb_getentropy.h
> > create mode 100644
> c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> >
> > diff --git a/bsps/arm/beagle/headers.am <http://headers.am>
> b/bsps/arm/beagle/headers.am <http://headers.am>
> > index 6692d0b..ac4ff7c 100644
> > --- a/bsps/arm/beagle/headers.am <http://headers.am>
> > +++ b/bsps/arm/beagle/headers.am <http://headers.am>
> > @@ -12,3 +12,4 @@ include_bsp_HEADERS +=
> > ../../../../../../bsps/arm/beagle/include/bsp/bbb-pwm.h
> > include_bsp_HEADERS +=
> > ../../../../../../bsps/arm/beagle/include/bsp/beagleboneblack.h
> > include_bsp_HEADERS +=
> ../../../../../../bsps/arm/beagle/include/bsp/i2c.h
> > include_bsp_HEADERS +=
> ../../../../../../bsps/arm/beagle/include/bsp/irq.h
> > +include_bsp_HEADERS +=
> > ../../../../../../bsps/arm/beagle/include/bsp/bbb_getentropy.h
> > diff --git a/bsps/arm/beagle/include/bsp/bbb_getentropy.h
> > b/bsps/arm/beagle/include/bsp/bbb_getentropy.h
> > new file mode 100644
> > index 0000000..5e46f89
> > --- /dev/null
> > +++ b/bsps/arm/beagle/include/bsp/bbb_getentropy.h
> > @@ -0,0 +1,57 @@
> > +/**
> > + * @file
> > + *
> > + * @ingroup arm_beagle
> > + *
> > + * @brief TRNG Definations
> > + */
> > +
> > +/*
> > + * Copyright (c) 2018 Udit kumar agarwal <dev.madaari at
> gmail.com <http://gmail.com>>
> > + *
> > + * The license and distribution terms for this file may be
> > + * found in the file LICENSE in this distribution or at
> > + * http://www.rtems.org/license/LICENSE
> <http://www.rtems.org/license/LICENSE>.
> > + */
> > +
> > +
> > +#ifndef _TRNG_
> > +#define _TRNG_
> > +
> >
> +/*------------------------------------------------------------------------------
> Please look at similar files in RTEMS for how to add comments.
> We do not use this style /* --------------------
> And we don't put this kind of comment about the file sections.
>
> > + * Headers
> > +
> > *----------------------------------------------------------------------------*/
> > +
> > +#include <libcpu/am335x.h>
> > +
> > +
> > /*------------------------------------------------------------------------------
> > + * Register structure
> > +
> > *----------------------------------------------------------------------------*/
> > +
> > +struct rng {
> > + uint64_t output; /*00*/
> > + uint32_t status; /*08*/
> > + uint32_t irq_en; /*0c*/
> > + uint32_t status_clr; /*10*/
> > + uint32_t control; /*14*/
> > + uint32_t config; /*18*/
> > +};
> for an exported header, the struct namespace needs to be user
> friendly. See similar files at this layer for how to name things.
> Probably, this can be defined in the am335x.h header?
>
> > +
> > +typedef volatile struct rng *rng_ref_t;
> I'm not such a big fan of using the typedef to hide things like
> volatile or pointer notation. I don't see a need for this usually.
>
> > +
> > +/*----------------------------------------------------------------------------*/
> > +/* Exported functions
> > */
> > +/*----------------------------------------------------------------------------*/
> > +
> > +/* configure and enable RNG module */
> > +static void am335x_rng_enable(rng_ref_t);
> > +/* enable module clock for random number generator */
> > +static void am335x_rng_clock_enable(void);
> > +/* outputs random data */
> > +static uint64_t trng_getranddata(rng_ref_t);
>
> static keyword implies these are not exported.
>
> > +/* Copy random data of a specified size to the pointer supplied */
> > +int getentropy(void *ptr, size_t);
> I guess getentropy() is already exported by a different header file
> (unistd.h) in libc? This doesn't belong here.
>
> I suspect this header file is not actually necessary.
>
> > +
> > +
> > +
> Don't use multiple blank lines in a row. Only one empty line is ever
> needed by RTEMS style.
>
> > +#endif /* #ifndef _TRNG_ */
> > \ No newline at end of file
> > diff --git a/bsps/arm/include/libcpu/am335x.h
> > b/bsps/arm/include/libcpu/am335x.h
> > index 367e97c..8b58f1a 100644
> > --- a/bsps/arm/include/libcpu/am335x.h
> > +++ b/bsps/arm/include/libcpu/am335x.h
> > @@ -14,6 +14,9 @@
> > * Modified by Ben Gras <beng at shrike-systems.com <mailto:beng at shrike-systems.com>> to add lots
> > * of beagleboard/beaglebone definitions, delete lpc32xx specific
> > * ones, and merge with some other header files.
> > + *
> > + * Modified by Udit agarwal <dev.madaari at gmail.com <mailto:dev.madaari at gmail.com>> to add random
> > + * number generating module definations
> typo: definitions
> add a period (full stop).
>
> > */
> >
> > #if !defined(_AM335X_H_)
> > @@ -701,4 +704,17 @@
> > #define AM335X_CM_PER_OCPWP_L3_CLKSTCTRL_CLKACTIVITY_OCPWP_L4_GCLK
> > (0x00000020u)
> > #define AM335X_I2C_INT_STOP_CONDITION AM335X_I2C_IRQSTATUS_BF
> >
> > +/* TRNG Register */
> > +
> > +/* RNG base address */
> > +#define RNG_BASE 0x48310000
> > +/* RNG clock control */
> > +#define CM_PER_RNG_CLKCTRL AM335X_CM_PER_ADDR | 9<<4
> I'd put parentheses around this to avoid any problems with macro
> expansions and order of operations.
>
> > +/* Offset from RNG base for output ready flag */
> > +#define RNG_STATUS_RDY (1u << 0)
> > +/* Offset from RNG base for FRO related error*/
> > +#define RNG_STATUS_ERR (1u << 1)
> > +/* Offset from RNG base for clock status */
> > +#define RNG_STATUS_CLK (1u << 31)
> > +
> > #endif
> > diff --git a/c/src/lib/libbsp/arm/beagle/Makefile.am
> > b/c/src/lib/libbsp/arm/beagle/Makefile.am
> > index 8251660..bf92081 100644
> > --- a/c/src/lib/libbsp/arm/beagle/Makefile.am
> > +++ b/c/src/lib/libbsp/arm/beagle/Makefile.am
> > @@ -88,9 +88,13 @@ libbsp_a_SOURCES += gpio/bbb-gpio.c
> > #pwm
> > libbsp_a_SOURCES += pwm/pwm.c
> >
> > +#getentropy
> > +libbsp_a_SOURCES += getentropy/bbb_getentropy.c
> > +
> > #RTC
> > libbsp_a_SOURCES += rtc.c
> > libbsp_a_SOURCES += ../../shared/tod.c
> > +
> > # Clock
> > libbsp_a_SOURCES += clock.c
> > libbsp_a_SOURCES += ../../shared/clockdrv_shell.h
> > diff --git a/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> > b/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> > new file mode 100644
> > index 0000000..cab2102
> > --- /dev/null
> > +++ b/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> > @@ -0,0 +1,80 @@
> > +/**
> > +* @file
> > +*
> > +* @ingroup arm_beagle
> > +*
> > +* @brief Getentropy implementation on BeagleBone Black BSP
> > +*/
> > +
> > +/*
> > +* Copyright (c) 2018 Udit kumar agarwal <dev.madaari at gmail.com
> <http://gmail.com>>
> > +*
> > +* The license and distribution terms for this file may be
> > +* found in the file LICENSE in this distribution or at
> > +* http://www.rtems.org/license/LICENSE
> <http://www.rtems.org/license/LICENSE>.
> > +*/
> > +
> > +#include <unistd.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <rtems/sysinit.h>
> > +
> > +
> > +/* maximun and minimun refill cycle sets the number of samples to
> be taken
> typo: minimum
> > + from FRO to generate random number */
> > +static void am335x_rng_enable(rng_ref_t rng){
> put { on a new line.
>
> > + rng->config = 0
> > + | 34 << 16 /* max refill 34 * 256 cycles */
> indent broken lines, but
> > + | 33 << 0; /* min refill 33 * 64 cycles */
> You can fit this on one line, remove the comments and put them above
> the line, e.g.
> rng->config = 34<<16 | 33;
>
> I would prefer not to have these hard-coded values though it is
> fragile. what means 34, 16, and 33?
> #define AM335X_RNG_MAX_REFILL (34 << 16)
> #define AM335X_RNG_MIN_REFILL (33)
> #define AM335X_RNG_CONFIG_REFILL (AM335X_RNG_MAX_REFILL |
> AM335X_RNG_MIN_REFILL)
>
> rng.config = rng.control = 0;
> rng.config |= AM335X_RNG_CONFIG_REFILL_TIME;
>
> > + rng->control = 0
> > + | 33 << 16 /* startup 33 * 256 cycles */
> > + | 1 << 10; /* enable module */
>
> rng.control |= AM335X_RNG_CONTROL_STARTUP;
> rng.control |= AM335X_RNG_CONTROL_ENABLE;
>
> for example is more readable. look around for other examples.
>
> > +}
> > +
> > +static void am335x_rng_clock_enable(void)
> > +{
> > + *(volatile uint8_t *) CM_PER_RNG_CLKCTRL = 2;
> > + while( *(volatile uint32_t *) CM_PER_RNG_CLKCTRL & 0x30000 ) {}
> > +}
> > +
> > +static uint64_t trng_getranddata(rng_ref_t rng){
> > + uint64_t output = rng->output;
> > + return output;
> > +}
> > +
> > +int getentropy(void *ptr, size_t n)
> > +{
> > + rng_ref_t const rng = (rng_ref_t) RNG_BASE;
> > + am335x_rng_enable(rng);
> > + while (n > 0) {
> > + uint64_t random;
> indent blocks of code.
>
> > + size_t copy;
> > +
> > + /* wait untill RNG becomes ready with next set of random data */
> > + while( ( rng->status & RNG_STATUS_RDY ) == 0 );
> > +
> > + random = trng_getranddata(rng);
> > +
> > + /* Checking for error by masking all bits other then error bit in
> > status register */
> > + if( ((rng->status & RNG_STATUS_ERR)>>1) == 0){
> > +
> > + /* clear the status flag after reading to generate new random
> > value*/
> > + rng->status_clr = RNG_STATUS_RDY;
> > + copy = sizeof(random);
> > + if ( n < copy ) {
> > + copy = n;
> ditto.
>
> > + }
> > + memcpy(ptr, &random, copy);
> > + n -= copy;
> > + ptr += copy;
> pointer arithmetic on void pointer is not legal. You can portably use
> ptr = (char*)ptr + copy;
>
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +RTEMS_SYSINIT_ITEM(
> > + am335x_rng_clock_enable,
> > + RTEMS_SYSINIT_DEVICE_DRIVERS,
> > + RTEMS_SYSINIT_ORDER_LAST
> > +);
> > \ No newline at end of file
> > --
> > 1.9.1
> >
> > Regards,
> > Udit Agarwal
> >
> > On Tue, Mar 6, 2018 at 12:50 AM, Christian Mauderer
> <list at c-mauderer.de <mailto:list at c-mauderer.de>>
> > wrote:
> >>
> >> Am 05.03.2018 um 14:51 schrieb Udit agarwal:
> >> > Hi,
> >> > I tried implementing getentropy on BBB, below is the patch.
> Please have
> >> > a look.
> >> > I followed these(1
> >> > <https://e2e.ti.com/support/arm/sitara_arm/f/791/t/355064
> <https://e2e.ti.com/support/arm/sitara_arm/f/791/t/355064>> & 2
> >> >
> >> >
> <http://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/arch/arm/omap/am335x_trng.c
> <http://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/arch/arm/omap/am335x_trng.c>>)
> >> > links for code reference. and this
> >> >
> >> >
> <https://docs.google.com/spreadsheets/d/1VpghMlLtrWQIrcvCsRg3ueRZkX0eGSQkFnzjfsqrd8A/view#gid=0
> <https://docs.google.com/spreadsheets/d/1VpghMlLtrWQIrcvCsRg3ueRZkX0eGSQkFnzjfsqrd8A/view#gid=0>>
> >> > for register reference. Moreover, what further configuration in
> RTEMS is
> >> > needed to execute and test this code along with getentropy() sample
> >> > testcode?
> >>
> >> Hello Udit,
> >>
> >> regarding the patch: Please use 'git format-patch' for generating
> your
> >> patches and send them unchanged to the mailing list (for example with
> >> 'git send-email'. If you follow this workflow, it's easier to
> apply and
> >> test the patches. The preferred workflow for RTEMS is described here:
> >> https://devel.rtems.org/wiki/Developer/Git/Users#CreatingaPatch
> <https://devel.rtems.org/wiki/Developer/Git/Users#CreatingaPatch>
> >>
> >> Regarding documentation: It seems that there is really only few
> >> documentation regarding the crypto part available. The FreeBSD
> driver is
> >> most likely your best source.
> >>
> >> I added further comments down in your code.
> >>
> >> Best regards
> >>
> >> Christian
> >>
> >> >
> >> >
> >> > + /*
> >> > + * Copyright (c) 2018 embedded brains GmbH. All rights reserved.
> >> > + *
> >> > + * embedded brains GmbH
> >> > + * Dornierstr. 4
> >> > + * 82178 Puchheim
> >> > + * Germany
> >> > + * <rtems at embedded-brains.de
> <mailto:rtems at embedded-brains.de> <mailto:rtems at embedded-brains.de
> <mailto:rtems at embedded-brains.de>>>
> >> > + *
> >>
> >> Also it's nice if you add an embedded brains header, I don't
> think that
> >> I've seen you when I have been at work today. So please add your own
> >> name to the copyright line instead. With that you can even point
> to that
> >> file and tell everyone that it's from you.
> >>
> >> An address isn't really necessary except if you want it for some
> reason
> >> (like PR in case of a company).
> >>
> >> > + * The license and distribution terms for this file may be
> >> > + * found in the file LICENSE in this distribution or at
> >> > + * http://www.rtems.org/license/LICENSE
> <http://www.rtems.org/license/LICENSE>.
> >> > + */
> >> > +
> >> > + #include <libcpu/am335x.h>
> >> > + #include <unistd.h>
> >> > + #include <string.h>
> >> > + #include <rtems/sysinit.h>
> >> > +
> >> > + #define RNG_BASE 0x48310000
> >> > + #define CM_PER_RNG_CLKCTRL 0x44e00090
> >> > + #define RNG_STATUS_RDY (1u << 0) // output ready for reading
> >> > + #define RNG_STATUS_ERR (1u << 1) // FRO shutdown alarm
> >> > + #define RNG_STATUS_CLK (1u << 31) // module functional clock
> >> > active (no irq)
> >>
> >> Please add address definitions and the bit masks to the
> >> bsps/arm/include/libcpu/am335x.h instead of defining them here
> locally.
> >> Note that for the CM_PER_RNG_CLKCTRL, there is already a
> >> AM335X_CM_PER_ADDR that should be used as a base.
> >>
> >> > +
> >> > + typedef volatile struct rng *rng_ref_t;
> >> > +
> >> > + struct rng {
> >> > + /*00*/ uint64_t output; //r-
> >> > + /*08*/ uint32_t status; //r-
> >> > + /*0c*/ uint32_t irq_en; //rw
> >> > + /*10*/ uint32_t status_clr; //-c
> >> > + /*14*/ uint32_t control; //rw
> >> > + /*18*/ uint32_t config; //rw
> >> > + };
> >>
> >> Same is true here: It's a structure describing the registers. So it
> >> should be in the header instead.
> >>
> >> It seems that you copied that from the forum post. Please always
> check
> >> the license when you copy code. We have to avoid that we get any code
> >> into RTEMS that isn't compatible with RTEMS license.
> >>
> >> In this case the code should be obvious enough that - if you just
> >> rewrite it in RTEMS style, there shouldn't be a problem. But just
> >> remember that licensing is always a difficult topic.
> >>
> >> > +
> >> > + static void TRNG_Enable(rng_ref_t rng){
> >> > + rng->config = 0
> >> > + | 34 << 16 // max refill 34 * 256 cycles
> >> > + | 33 << 0; // min refill 33 * 64 cycles
> >> > + rng->control = 0
> >> > + | 33 << 16 // startup 33 * 256 cycles
> >> > + | 1 << 10; // enable module
> >> > + }
> >>
> >> Please use C-Style comments /* */ in all pure C code (most of RTEMS).
> >>
> >> > +
> >> > + static void beagle_trng_clock_enable(void)
> >> > + {
> >> > + // enable module clock
> >> > + *(volatile uint8_t *) CM_PER_RNG_CLKCTRL = 2;
> >> > + while( *(volatile uint32_t *) CM_PER_RNG_CLKCTRL &
> 0x30000 ) {}
> >> > + }
> >> > +
> >> > + static uint64_t TRNG_GetRandData(){
> >> > + uint64_t output = rng->output;
> >>
> >> 'rng' isn't defined here.
> >>
> >> > + return output;
> >> > + }
> >> > +
> >> > + int getentropy(void *ptr, size_t n)
> >> > + {
> >> > + rng_ref_t const rng = (rng_ref_t) RNG_BASE;
> >> > + TRNG_Enable(rng);
> >> > + while (n > 0) {
> >> > + uint64_t random;
> >> > + size_t copy;
> >> > +
> >> > + while( ( rng->status & RNG_STATUS_RDY ) == 0 ); //wait
> >>
> >> You should check specifically for the TRNG_STATUS_READY flag here
> like
> >> done in FreeBSD. I'm not sure what the other flags are but it's quite
> >> possible that there is also some error flag or similar. In that
> case it
> >> would not be a good idea to use the random number.
> >>
> >> > +
> >> > + random = TRNG_GetRandData(rng);
> >>
> >> Note that you don't have to copy the function names from the atsam
> >> exactly. In the atsam BSP, the library has provided these names.
> >> Otherwise it's quite unusual to start functions with upper case
> in RTEMS.
> >>
> >> > +
> >> > + /*
> >> > + * Read TRNG status one more time to avoid race
> condition.
> >> > + * Otherwise we can read (and clear) an old ready
> status but
> >> > get
> >> > + * a new value. The ready status for this value
> wouldn't be
> >> > + * reset.
> >> > + */
> >>
> >> That comment seems like it doesn't fit here any longer. You
> acknowledge
> >> the status instead in your next line. Just remove the comment.
> >>
> >> > + rng->status_clr = RNG_STATUS_RDY;
> >> > +
> >> > + copy = sizeof(random);
> >> > + if (n < copy ) {
> >> > + copy = n;
> >> > + }
> >> > + memcpy(ptr, &random, copy);
> >> > + n -= copy;
> >> > + ptr += copy;
> >> > + }
> >> > +
> >> > + return 0;
> >> > + }
> >> > +
> >> > + RTEMS_SYSINIT_ITEM(
> >> > + beagle_trng_clock_enable,
> >> > + RTEMS_SYSINIT_DEVICE_DRIVERS,
> >> > + RTEMS_SYSINIT_ORDER_LAST
> >> > + );
> >> >
> >> > Regards,
> >> > Udit agarwal
> >> >
> >> >
> >> > _______________________________________________
> >> > devel mailing list
> >> > devel at rtems.org <mailto:devel at rtems.org>
> >> > http://lists.rtems.org/mailman/listinfo/devel
> <http://lists.rtems.org/mailman/listinfo/devel>
> >> >
> >
> >
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org <mailto:devel at rtems.org>
> > http://lists.rtems.org/mailman/listinfo/devel
> <http://lists.rtems.org/mailman/listinfo/devel>
>
>
>
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
More information about the devel
mailing list