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
