[PATCH v5 3/8] stm32mp1: rng: Add a driver for random number generator(rng) device

[Heinrich Schuchardt]

On 12/26/19 6:25 PM, Sughosh Ganu wrote:
> Add a driver for the rng device found on stm32mp1 platforms. The
> driver provides a routine for reading the random number seed from the
> hardware device.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> Reviewed-by: Patrice Chotard <patrice.chotard at st.com>
> Acked-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
> Changes since V4:
> * Return number of bytes read on a successful read, and a -ve value on
>    error.
>
>   drivers/rng/Kconfig        |   7 ++
>   drivers/rng/Makefile       |   1 +
>   drivers/rng/stm32mp1_rng.c | 165 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 173 insertions(+)
>   create mode 100644 drivers/rng/stm32mp1_rng.c
>
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index dd44cc0..c9c4751 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -5,3 +5,10 @@ config DM_RNG
>   	  Enable driver model for random number generator(rng) devices.
>   	  This interface is used to initialise the rng device and to
>   	  read the random seed from the device.
> +
> +config RNG_STM32MP1
> +	bool "Enable random number generator for STM32MP1"
> +	depends on ARCH_STM32MP && DM_RNG
> +	default n
> +	help
> +	  Enable STM32MP1 rng driver.
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 311705b..699beb3 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -4,3 +4,4 @@
>   #
>
>   obj-$(CONFIG_DM_RNG) += rng-uclass.o
> +obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
> diff --git a/drivers/rng/stm32mp1_rng.c b/drivers/rng/stm32mp1_rng.c
> new file mode 100644
> index 0000000..56ad848
> --- /dev/null
> +++ b/drivers/rng/stm32mp1_rng.c
> @@ -0,0 +1,165 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <reset.h>
> +#include <rng.h>
> +
> +#include <asm/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +
> +#define RNG_CR 0x00
> +#define RNG_CR_RNGEN BIT(2)
> +#define RNG_CR_CED BIT(5)
> +
> +#define RNG_SR 0x04
> +#define RNG_SR_SEIS BIT(6)
> +#define RNG_SR_CEIS BIT(5)
> +#define RNG_SR_SECS BIT(2)
> +#define RNG_SR_DRDY BIT(0)
> +
> +#define RNG_DR 0x08
> +
> +struct stm32_rng_platdata {
> +	fdt_addr_t base;
> +	struct clk clk;
> +	struct reset_ctl rst;
> +};
> +
> +static int stm32_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +	int retval = 0, i, nbytes;
> +	u32 sr, count, reg;
> +	size_t increment;
> +	struct stm32_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +	/*
> +	 * Only INT_MAX number of bytes can be returned. If more
> +	 * bytes need to be read, the caller must do it in a loop.
> +	 */
> +	if (len > INT_MAX)
> +		len = INT_MAX;
> +
> +	nbytes = len;
> +	while (len > 0) {
> +		retval = readl_poll_timeout(pdata->base + RNG_SR, sr,
> +					    sr & RNG_SR_DRDY, 10000);
> +		if (retval)
> +			return retval;
> +
> +		if (sr & (RNG_SR_SEIS | RNG_SR_SECS)) {
> +			/* As per SoC TRM */
> +			clrbits_le32(pdata->base + RNG_SR, RNG_SR_SEIS);
> +			for (i = 0; i < 12; i++)
> +				readl(pdata->base + RNG_DR);
> +			if (readl(pdata->base + RNG_SR) & RNG_SR_SEIS) {
> +				printf("RNG Noise");
> +				return -EIO;

The SEIS bit indicates a seed error. According to the RM0090 Reference
manual for the STM32F429/439 you should clear the SEIS bit and set the
RNGEN bit to restart the RNG.

https://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf,
page 768.

So why do you return an error code here? What do you expect the caller
to do?

Should we check the CEIS flag which indicates a clock error?

> +			}
> +			/* start again */
> +			continue;
> +		}
> +

It took me some time to understand what this loop does. Please, add a
comment indicating that we copy up to 16 random bytes from the RNG.

Best regards

Heinrich

> +		count = 4;
> +		while (len && count) {
> +			reg = readl(pdata->base + RNG_DR);
> +			memcpy(data, &reg, min(len, sizeof(u32)));
> +			increment = min(len, sizeof(u32));
> +			data += increment;
> +			len -= increment;
> +			count--;
> +		}
> +	}
> +
> +	return nbytes;
> +}
> +
> +static int stm32_rng_init(struct stm32_rng_platdata *pdata)
> +{
> +	int err;
> +
> +	err = clk_enable(&pdata->clk);
> +	if (err)
> +		return err;
> +
> +	/* Disable CED */
> +	writel(RNG_CR_RNGEN | RNG_CR_CED, pdata->base + RNG_CR);
> +
> +	/* clear error indicators */
> +	writel(0, pdata->base + RNG_SR);
> +
> +	return 0;
> +}
> +
> +static int stm32_rng_cleanup(struct stm32_rng_platdata *pdata)
> +{
> +
> +	writel(0, pdata->base + RNG_CR);
> +
> +	return clk_disable(&pdata->clk);
> +}
> +
> +static int stm32_rng_probe(struct udevice *dev)
> +{
> +	struct stm32_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +	reset_assert(&pdata->rst);
> +	udelay(20);
> +	reset_deassert(&pdata->rst);
> +
> +	return stm32_rng_init(pdata);
> +}
> +
> +static int stm32_rng_remove(struct udevice *dev)
> +{
> +	struct stm32_rng_platdata *pdata = dev_get_platdata(dev);
> +
> +	return stm32_rng_cleanup(pdata);
> +}
> +
> +static int stm32_rng_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct stm32_rng_platdata *pdata = dev_get_platdata(dev);
> +	int err;
> +
> +	pdata->base = dev_read_addr(dev);
> +	if (!pdata->base)
> +		return -ENOMEM;
> +
> +	err = clk_get_by_index(dev, 0, &pdata->clk);
> +	if (err)
> +		return err;
> +
> +	err = reset_get_by_index(dev, 0, &pdata->rst);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops stm32_rng_ops = {
> +	.read = stm32_rng_read,
> +};
> +
> +static const struct udevice_id stm32_rng_match[] = {
> +	{
> +		.compatible = "st,stm32-rng",
> +	},
> +	{},
> +};
> +
> +U_BOOT_DRIVER(stm32_rng) = {
> +	.name = "stm32-rng",
> +	.id = UCLASS_RNG,
> +	.of_match = stm32_rng_match,
> +	.ops = &stm32_rng_ops,
> +	.probe = stm32_rng_probe,
> +	.remove = stm32_rng_remove,
> +	.platdata_auto_alloc_size = sizeof(struct stm32_rng_platdata),
> +	.ofdata_to_platdata = stm32_rng_ofdata_to_platdata,
> +};
>