Please see the comment inline. Regards, Kshitiz
-----Original Message----- From: Heinrich Schuchardt <xypron.g...@gmx.de> Sent: Friday, September 9, 2022 1:00 AM To: Kshitiz Varshney <kshitiz.varsh...@nxp.com> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Horia Geanta <horia.gea...@nxp.com>; Pankaj Gupta <pankaj.gu...@nxp.com>; Varun Sethi <v.se...@nxp.com>; Gaurav Jain <gaurav.j...@nxp.com>; Rahul Kumar Yadav <rahulkumar.ya...@nxp.com>; Vabhav Sharma <vabhav.sha...@nxp.com>; Sahil Malhotra <sahil.malho...@nxp.com>; Ye Li <ye...@nxp.com>; Stefano Babic <sba...@denx.de>; Fabio Estevam <feste...@gmail.com>; Peng Fan <peng....@nxp.com>; Sughosh Ganu <sughosh.g...@linaro.org>; Simon Glass <s...@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.varsh...@nxp.com> > wrote: >> >> From: Kshitiz <kshitiz.varsh...@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.varsh...@nxp.com> CC the maintainer for RNG Sughosh Ganu <sughosh.g...@linaro.org>. Will take care of this in next patch. >> Reviewed-by: Ye Li <ye...@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 <s...@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 <mar...@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.g...@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