[EXT] Re: [PATCH v1] 2: Uboot RNG Driver using Data Co-processor

Kshitiz Varshney kshitiz.varshney at nxp.com
Wed Dec 21 12:11:13 CET 2022


Please see the comment inline.

Regards,
Kshitiz

-----Original Message-----
From: Heinrich Schuchardt <xypron.glpk at gmx.de> 
Sent: Friday, September 9, 2022 1:00 AM
To: Kshitiz Varshney <kshitiz.varshney at nxp.com>
Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Horia Geanta <horia.geanta at nxp.com>; Pankaj Gupta <pankaj.gupta at nxp.com>; Varun Sethi <V.Sethi at nxp.com>; Gaurav Jain <gaurav.jain at nxp.com>; Rahul Kumar Yadav <rahulkumar.yadav at nxp.com>; Vabhav Sharma <vabhav.sharma at nxp.com>; Sahil Malhotra <sahil.malhotra at nxp.com>; Ye Li <ye.li at nxp.com>; Stefano Babic <sbabic at denx.de>; Fabio Estevam <festevam at gmail.com>; Peng Fan <peng.fan at nxp.com>; Sughosh Ganu <sughosh.ganu at linaro.org>; Simon Glass <sjg at chromium.org>
Subject: [EXT] Re: [PATCH v1] 2: Uboot RNG Driver using Data Co-processor

Caution: EXT Email

On 9/8/22 20:19, Simon Glass wrote:
> Hi Kshitiz,
>
> On Thu, 8 Sept 2022 at 02:59, Kshitiz Varshney <kshitiz.varshney at nxp.com> wrote:
>>
>> From: Kshitiz <kshitiz.varshney at nxp.com>
>>
>> This commit introduces Random number generator to uboot. It uses DCP 
>> driver for number generation.
>> RNG driver can be invoked by using below command on uboot prompt:-
>>             rng <number of bytes>
>>
>> Signed-off-by: Kshitiz Varshney <kshitiz.varshney at nxp.com>

CC the maintainer for RNG Sughosh Ganu <sughosh.ganu at linaro.org>.

Will take care of this in next patch.

>> Reviewed-by: Ye Li <ye.li at nxp.com>
>> ---
>>   drivers/crypto/fsl/Kconfig   |  10 ++
>>   drivers/crypto/fsl/Makefile  |   1 +
>>   drivers/crypto/fsl/dcp_rng.c | 184 +++++++++++++++++++++++++++++++++++
>>   3 files changed, 195 insertions(+)
>>   create mode 100644 drivers/crypto/fsl/dcp_rng.c
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> but please see below
>
>>
>> diff --git a/drivers/crypto/fsl/Kconfig b/drivers/crypto/fsl/Kconfig 
>> index 702d204a3d..da5955e31d 100644
>> --- a/drivers/crypto/fsl/Kconfig
>> +++ b/drivers/crypto/fsl/Kconfig
>> @@ -96,3 +96,13 @@ config RNG_SELF_TEST
>>           must be run before running any RNG based crypto implementation.
>>
>>   endif
>> +
>> +config FSL_DCP_RNG
>> +       bool "Enable Random Number Generator support"
>> +       depends on DM_RNG
>> +       default n
>> +       help
>> +       Enable support for the hardware based random number generator
>> +       module of the DCP.It uses the True Random Number Generator 
>> +(TRNG)

Please, use the same indentation as the other Kconfig entries in this
file: Add two space in each line of the help text.

Done

>
> Space before It
>
>> +       and a Pseudo-Random Number Generator (PRNG) to achieve a true
>> +       randomness and cryptographic strength.
>> diff --git a/drivers/crypto/fsl/Makefile 
>> b/drivers/crypto/fsl/Makefile index 926300e2ab..c653208d23 100644
>> --- a/drivers/crypto/fsl/Makefile
>> +++ b/drivers/crypto/fsl/Makefile
>> @@ -7,6 +7,7 @@ obj-$(CONFIG_FSL_CAAM) += jr.o fsl_hash.o jobdesc.o error.o
>>   obj-$(CONFIG_FSL_BLOB) += fsl_blob.o
>>   obj-$(CONFIG_RSA_FREESCALE_EXP) += fsl_rsa.o
>>   obj-$(CONFIG_FSL_CAAM_RNG) += rng.o
>> +obj-$(CONFIG_FSL_DCP_RNG) += dcp_rng.o
>>   obj-$(CONFIG_IMX_CAAM_MFG_PROT) += fsl_mfgprot.o
>>   obj-$(CONFIG_RNG_SELF_TEST) += rng_self_test.o
>>   obj-$(CONFIG_CMD_PROVISION_KEY) += fsl_aes.o tag_object.o diff 
>> --git a/drivers/crypto/fsl/dcp_rng.c b/drivers/crypto/fsl/dcp_rng.c 
>> new file mode 100644 index 0000000000..a797710c2e
>> --- /dev/null
>> +++ b/drivers/crypto/fsl/dcp_rng.c
>> @@ -0,0 +1,184 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * RNG driver for Freescale RNGC
>> + *
>> + * Copyright (C) 2008-2012 Freescale Semiconductor, Inc.
>> + * Copyright (C) 2017 Martin Kaiser <martin at kaiser.cx>
>> + *     Copyright 2022 NXP
>> + *
>> + * Based on RNGC driver in drivers/char/hw_random/imx-rngc.c in 
>> +Linux  */
>> +
>> +#include <asm/cache.h>
>> +#include <common.h>
>> +#include <cpu_func.h>
>> +#include <dm.h>
>> +#include <rng.h>
>> +#include <linux/kernel.h>
>> +#include <linux/delay.h>
>> +#include <asm/io.h>
>> +#include <dm/root.h>
>
> Should be:
>
>> +#include <common.h>
>> +#include <cpu_func.h>
>> +#include <dm.h>
>> +#include <rng.h>
>> +#include <asm/cache.h>
>> +#include <asm/io.h>
>> +#include <dm/root.h>
>> +#include <linux/delay.h>
>> +#include <linux/kernel.h>
>
>
>> +
>> +#define DCP_RNG_MAX_FIFO_STORE_SIZE    4
>> +#define RNGC_VER_ID                    0x0000
>> +#define RNGC_COMMAND                   0x0004
>> +#define RNGC_CONTROL                   0x0008
>> +#define RNGC_STATUS                    0x000C
>> +#define RNGC_ERROR                     0x0010
>> +#define RNGC_FIFO                      0x0014
>> +
>> +/* the fields in the ver id register */
>> +#define RNGC_TYPE_SHIFT                        28
>> +
>> +/* the rng_type field */
>> +#define RNGC_TYPE_RNGB                 0x1
>> +#define RNGC_TYPE_RNGC                 0x2
>> +
>> +#define RNGC_CMD_CLR_ERR               0x00000020
>> +#define RNGC_CMD_SEED                  0x00000002
>> +
>> +#define RNGC_CTRL_AUTO_SEED            0x00000010
>> +
>> +#define RNGC_STATUS_ERROR              0x00010000
>> +#define RNGC_STATUS_FIFO_LEVEL_MASK    0x00000f00
>> +#define RNGC_STATUS_FIFO_LEVEL_SHIFT   8
>> +#define RNGC_STATUS_SEED_DONE          0x00000020
>> +#define RNGC_STATUS_ST_DONE            0x00000010
>
> Why all the leading zeroes?
>
>> +
>> +#define RNGC_ERROR_STATUS_STAT_ERR     0x00000008
>> +
>> +#define RNGC_TIMEOUT                   3000000U /* 3 sec */
>> +
>> +struct imx_rngc {
>
> Normally the priv data should have a _priv suffix.
>
>> +       unsigned long base;
>> +};
>> +
>> +static int rngc_read(struct udevice *dev, void *data, size_t len) {
>> +       struct imx_rngc *rngc = dev_get_priv(dev);
>
> Normally the var should be called priv

This is described in doc/develop/codingstyle.rst line 192ff.

>
>> +       u8 buffer[DCP_RNG_MAX_FIFO_STORE_SIZE];
>> +       u32 status, level;
>> +       size_t size;
>> +
>> +       while (len) {
>> +               status = readl(rngc->base + RNGC_STATUS);
>> +
>> +               /* is there some error while reading this random number? */
>> +               if (status & RNGC_STATUS_ERROR)
>> +                       break;
>> +               /* how many random numbers are in FIFO? [0-16] */
>> +               level = (status & RNGC_STATUS_FIFO_LEVEL_MASK) >>
>> +                       RNGC_STATUS_FIFO_LEVEL_SHIFT;
>> +
>> +               if (level) {
>> +                       /* retrieve a random number from FIFO */
>> +                       *(u32 *)buffer = readl(rngc->base + RNGC_FIFO);
>> +                       size = min(len, sizeof(u32));
>> +                       memcpy(data, buffer, size);
>> +                       data += size;
>> +                       len -= size;
>> +               }
>> +       }
>> +
>> +       return len ? -EIO : 0;
>> +}
>> +
>> +static int rngc_init(struct imx_rngc *rngc) {
>> +       u32 cmd, ctrl, status, err_reg = 0;
>> +       unsigned long long timeval = 0;
>> +       unsigned long long timeout = RNGC_TIMEOUT;
>> +
>> +       /* clear error */
>> +       cmd = readl(rngc->base + RNGC_COMMAND);
>> +       writel(cmd | RNGC_CMD_CLR_ERR, rngc->base + RNGC_COMMAND);
>> +
>> +       /* create seed, repeat while there is some statistical error */
>> +       do {
>> +               /* seed creation */
>> +               cmd = readl(rngc->base + RNGC_COMMAND);
>> +               writel(cmd | RNGC_CMD_SEED, rngc->base + 
>> + RNGC_COMMAND);
>> +
>> +               udelay(1);
>> +               timeval += 1;

As this loop can take rather long, should we call WATCHDOG_RESET() before and after the loop?

Otherwise looks good to me.

This is in reference with imx-rngc.c in kernel code. 
See this link for more info:- https://elixir.bootlin.com/linux/latest/source/drivers/char/hw_random/imx-rngc.c

Acked-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

>> +
>> +               status = readl(rngc->base + RNGC_STATUS);
>> +               err_reg = readl(rngc->base + RNGC_ERROR);
>> +
>> +               if (status & (RNGC_STATUS_SEED_DONE | RNGC_STATUS_ST_DONE))
>> +                       break;
>> +
>> +               if (timeval > timeout) {
>> +                       debug("rngc timed out\n");
>> +                       return -ETIMEDOUT;
>> +               }
>> +       } while (err_reg == RNGC_ERROR_STATUS_STAT_ERR);
>> +
>> +       if (err_reg)
>> +               return -EIO;
>> +
>> +       /*
>> +        * enable automatic seeding, the rngc creates a new seed automatically
>> +        * after serving 2^20 random 160-bit words
>> +        */
>> +       ctrl = readl(rngc->base + RNGC_CONTROL);
>> +       ctrl |= RNGC_CTRL_AUTO_SEED;
>> +       writel(ctrl, rngc->base + RNGC_CONTROL);
>
> setbits_le32(rngc->base + RNGC_CONTROL, RNGC_CTRL_AUTO_SEED);
>
>> +       return 0;
>> +}
>> +
>> +static int rngc_probe(struct udevice *dev) {
>> +       struct imx_rngc *rngc = dev_get_priv(dev);
>> +       fdt_addr_t addr;
>> +       u32 ver_id;
>> +       u8  rng_type;
>> +       int ret;
>> +
>> +       addr = dev_read_addr(dev);
>> +       if (addr == FDT_ADDR_T_NONE) {
>> +               ret = -EINVAL;
>> +               goto err;
>> +       }
>> +
>> +       rngc->base = addr;
>> +       ver_id = readl(rngc->base + RNGC_VER_ID);
>> +       rng_type = ver_id >> RNGC_TYPE_SHIFT;
>> +       /*
>> +        * This driver supports only RNGC and RNGB. (There's a different
>> +        * driver for RNGA.)
>> +        */
>> +       if (rng_type != RNGC_TYPE_RNGC && rng_type != RNGC_TYPE_RNGB) {
>> +               ret = -ENODEV;
>> +               goto err;
>> +       }
>> +
>> +       ret = rngc_init(rngc);
>> +       if (ret)
>> +               goto err;
>> +
>> +       return 0;
>> +
>> +err:
>> +       printf("%s error = %d\n", __func__, ret);
>> +       return ret;
>> +}
>> +
>> +static const struct dm_rng_ops rngc_ops = {
>> +       .read = rngc_read,
>> +};
>> +
>> +static const struct udevice_id rngc_dt_ids[] = {
>> +       { .compatible = "fsl,imx25-rngb" },
>> +       { }
>> +};
>> +
>> +U_BOOT_DRIVER(dcp_rng) = {
>> +       .name = "dcp_rng",
>> +       .id = UCLASS_RNG,
>> +       .of_match = rngc_dt_ids,
>> +       .ops = &rngc_ops,
>> +       .probe = rngc_probe,
>> +       .priv_auto  = sizeof(struct imx_rngc),
>> +       .flags = DM_FLAG_ALLOC_PRIV_DMA, };
>> --
>> 2.25.1
>>
>
> Regards,
> Simon



More information about the U-Boot mailing list