Re: [PATCH v2 2/3] hwrng: add Rockchip SoC hwrng driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/11/2022 19:47, Aurelien Jarno wrote:
> Rockchip SoCs used to have a random number generator as part of their
> crypto device, and support for it has to be added to the corresponding
> driver. However newer Rockchip SoCs like the RK356x have an independent
> True Random Number Generator device. This patch adds a driver for it,
> greatly inspired from the downstream driver.
> 
> The TRNG device does not seem to have a signal conditionner and the FIPS
> 140-2 test returns a lot of failures. They can be reduced by increasing
> RK_RNG_SAMPLE_CNT, in a tradeoff between quality and speed. This value
> has been adjusted to get ~90% of successes and the quality value has
> been set accordingly.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 250 ++++++++++++++++++++++++++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
> 
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 3da8e85f8aae..8e5c88504f72 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -549,6 +549,20 @@ config HW_RANDOM_CN10K
>  	 To compile this driver as a module, choose M here.
>  	 The module will be called cn10k_rng. If unsure, say Y.
>  
> +config HW_RANDOM_ROCKCHIP
> +        tristate "Rockchip True Random Number Generator"
> +        depends on HW_RANDOM && (ARCH_ROCKCHIP || COMPILE_TEST)
> +        depends on HAS_IOMEM
> +        default HW_RANDOM
> +        help
> +          This driver provides kernel-side support for the True Random Number
> +          Generator hardware found on some Rockchip SoC like RK3566 or RK3568.
> +
> +          To compile this driver as a module, choose M here: the
> +          module will be called rockchip-rng.
> +
> +          If unsure, say Y.
> +
>  endif # HW_RANDOM
>  
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 3e948cf04476..b7e989535fd6 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -47,3 +47,4 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-trng.o
>  obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
>  obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
>  obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
> +obj-$(CONFIG_HW_RANDOM_ROCKCHIP) += rockchip-rng.o
> diff --git a/drivers/char/hw_random/rockchip-rng.c b/drivers/char/hw_random/rockchip-rng.c
> new file mode 100644
> index 000000000000..18cdd91ad8c3
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rockchip-rng.c True Random Number Generator driver for Rockchip SoCs
> + *
> + * Copyright (c) 2018, Fuzhou Rockchip Electronics Co., Ltd.
> + * Copyright (c) 2022, Aurelien Jarno
> + * Authors:
> + *  Lin Jinhan <troy.lin@rock-chips.com>
> + *  Aurelien Jarno <aurelien@aurel32.net>
> + */
> +#include <linux/clk.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define RK_RNG_AUTOSUSPEND_DELAY	100
> +#define RK_RNG_MAX_BYTE			32
> +#define RK_RNG_POLL_PERIOD_US		100
> +#define RK_RNG_POLL_TIMEOUT_US		10000
> +
> +/*
> + * TRNG collects osc ring output bit every RK_RNG_SAMPLE_CNT time. The value is
> + * a tradeoff between speed and quality and has been adjusted to get a quality
> + * of ~900 (~90% of FIPS 140-2 successes).
> + */
> +#define RK_RNG_SAMPLE_CNT		1000
> +
> +/* TRNG registers from RK3568 TRM-Part2, section 5.4.1 */
> +#define TRNG_RST_CTL			0x0004
> +#define TRNG_RNG_CTL			0x0400
> +#define TRNG_RNG_CTL_LEN_64_BIT		(0x00 << 4)
> +#define TRNG_RNG_CTL_LEN_128_BIT	(0x01 << 4)
> +#define TRNG_RNG_CTL_LEN_192_BIT	(0x02 << 4)
> +#define TRNG_RNG_CTL_LEN_256_BIT	(0x03 << 4)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_0	(0x00 << 2)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_1	(0x01 << 2)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_2	(0x02 << 2)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_3	(0x03 << 2)
> +#define TRNG_RNG_CTL_ENABLE		BIT(1)
> +#define TRNG_RNG_CTL_START		BIT(0)
> +#define TRNG_RNG_SAMPLE_CNT		0x0404
> +#define TRNG_RNG_DOUT_0			0x0410
> +#define TRNG_RNG_DOUT_1			0x0414
> +#define TRNG_RNG_DOUT_2			0x0418
> +#define TRNG_RNG_DOUT_3			0x041c
> +#define TRNG_RNG_DOUT_4			0x0420
> +#define TRNG_RNG_DOUT_5			0x0424
> +#define TRNG_RNG_DOUT_6			0x0428
> +#define TRNG_RNG_DOUT_7			0x042c
> +
> +struct rk_rng {
> +	struct hwrng rng;
> +	void __iomem *base;
> +	struct reset_control *rst;
> +	int clk_num;
> +	struct clk_bulk_data *clk_bulks;
> +};
> +
> +/* The mask determine the bits that are updated */
> +static void rk_rng_write_ctl(struct rk_rng *rng, u32 val, u32 mask)
> +{
> +	writel_relaxed((mask << 16) | val, rng->base + TRNG_RNG_CTL);
> +}
> +
> +static int rk_rng_init(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	u32 reg;
> +	int ret;
> +
> +	/* start clocks */
> +	ret = clk_bulk_prepare_enable(rk_rng->clk_num, rk_rng->clk_bulks);
> +	if (ret < 0) {
> +		dev_err((struct device *) rk_rng->rng.priv,
> +			"Failed to enable clks %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* set the sample period */
> +	writel(RK_RNG_SAMPLE_CNT, rk_rng->base + TRNG_RNG_SAMPLE_CNT);
> +
> +	/* set osc ring speed and enable it */
> +	reg = TRNG_RNG_CTL_LEN_256_BIT |

It's not reg. It's val.

> +		   TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> +		   TRNG_RNG_CTL_ENABLE;
> +	rk_rng_write_ctl(rk_rng, reg, 0xffff);
> +
> +	return 0;
> +}
> +
> +static void rk_rng_cleanup(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	u32 reg;
> +
> +	/* stop TRNG */
> +	reg = 0;

It's not reg. It's val.

> +	rk_rng_write_ctl(rk_rng, reg, 0xffff);
> +
> +	/* stop clocks */
> +	clk_bulk_disable_unprepare(rk_rng->clk_num, rk_rng->clk_bulks);
> +}
> +
> +static int rk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	u32 reg;
> +	int ret = 0;
> +	int i;
> +
> +	pm_runtime_get_sync((struct device *) rk_rng->rng.priv);

Missing error handling.

> +
> +	/* Start collecting random data */
> +	reg = TRNG_RNG_CTL_START;

This is not usefull. Just use it directly in write call. Actually this
is heavy confusing, as reg suggests address. This would be val instead...

> +	rk_rng_write_ctl(rk_rng, reg, reg);
> +
> +	ret = readl_poll_timeout(rk_rng->base + TRNG_RNG_CTL, reg,
> +				 !(reg & TRNG_RNG_CTL_START),
> +				 RK_RNG_POLL_PERIOD_US,
> +				 RK_RNG_POLL_TIMEOUT_US);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Read random data stored in the registers */
> +	ret = min_t(size_t, max, RK_RNG_MAX_BYTE);
> +	for (i = 0; i < ret; i += 4) {
> +		*(u32 *)(buf + i) = readl_relaxed(rk_rng->base + TRNG_RNG_DOUT_0 + i);
> +	}

This cannot be just memcpy_fromio?

> +
> +out:
> +	pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> +	pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> +
> +	return ret;
> +}
> +
> +static int rk_rng_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rk_rng *rk_rng;
> +	int ret;
> +
> +	rk_rng = devm_kzalloc(dev, sizeof(struct rk_rng), GFP_KERNEL);

sizeof(*rk_rng)

> +	if (!rk_rng)
> +		return -ENOMEM;
> +

Best regards,
Krzysztof