[v3,2/3] hwrng: add Rockchip SoC hwrng driver

Commit Message

Daniel Golle June 21, 2024, 1:25 a.m. UTC

From: Aurelien Jarno <[e-mail]>

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 <[e-mail]>
[[e-mail]: code style fixes]
Signed-off-by: Daniel Golle <[e-mail]>
---
 MAINTAINERS                           |   1 +
 drivers/char/hw_random/Kconfig        |  14 ++
 drivers/char/hw_random/Makefile       |   1 +
 drivers/char/hw_random/rockchip-rng.c | 229 ++++++++++++++++++++++++++
 4 files changed, 245 insertions(+)
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

Comments

Diederik de Haas June 21, 2024, 9:32 a.m. UTC | #1

Hi,

On Friday, 21 June 2024 03:25:18 CEST Daniel Golle wrote:
> diff --git a/drivers/char/hw_random/rockchip-rng.c
> b/drivers/char/hw_random/rockchip-rng.c new file mode 100644
> index 000000000000..6070abb73847
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,229 @@
> +// 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 <[e-mail]>
> + *  Aurelien Jarno <[e-mail]>
> + */
> +#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>

I've been using a modified version of Aurelien's patch myself and I added the 
following to my commit description:

```
hwrng: rockchip: Explicitly include correct DT includes

Similar to commit 
045a44d4c9b3 ("regulator: Explicitly include correct DT includes")
replace ``of_platform.h`` include with ``of.h`` and ``platform_device.h``.

Link: https://git.kernel.org/linus/045a44d4c9b32578aacf0811063e5bb741c7c32c
```

BUT I don't (really) know what I'm doing, so could you verify whether there is 
some merit to it?

Cheers,
  Diederik

Krzysztof Kozlowski June 21, 2024, 9:57 a.m. UTC | #2

On 21/06/2024 03:25, Daniel Golle wrote:
> From: Aurelien Jarno <[e-mail]>
> 

> +
> +static int rk_rng_init(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	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 */
> +	writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> +		       TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> +		       TRNG_RNG_CTL_ENABLE,
> +		       rk_rng->base + TRNG_RNG_CTL);

I doubt relaxed write is here intentional. Enabling should be last
instruction, so this should be ordered write.

> +
> +	return 0;
> +}
> +
> +static void rk_rng_cleanup(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +
> +	/* stop TRNG */
> +	writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);

This should not be relaxed. This might lead to very tricky to debug issues.

> +
> +	/* 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);
> +	size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> +	u32 reg;
> +	int ret = 0;
> +
> +	ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);

Why this cannot be just simpler pm_runtime_resume_and_get()?

> +	if (ret < 0)
> +		goto out;

This does not look like correct error path. Device was not busy here.

> +
> +	/* Start collecting random data */
> +	writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> +
> +	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 */
> +	memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> +out:
> +	pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> +	pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> +
> +	return to_read;
> +}
> +
> +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(*rk_rng), GFP_KERNEL);
> +	if (!rk_rng)
> +		return -ENOMEM;
> +
> +	rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rk_rng->base))
> +		return PTR_ERR(rk_rng->base);
> +
> +	rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> +	if (rk_rng->clk_num < 0)
> +		return dev_err_probe(dev, rk_rng->clk_num,
> +				     "Failed to get clks property\n");
> +
> +	rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> +	if (IS_ERR(rk_rng->rst))
> +		return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> +				     "Failed to get reset property\n");
> +
> +	reset_control_assert(rk_rng->rst);
> +	udelay(2);
> +	reset_control_deassert(rk_rng->rst);
> +
> +	platform_set_drvdata(pdev, rk_rng);
> +
> +	rk_rng->rng.name = dev_driver_string(dev);
> +#ifndef CONFIG_PM
> +	rk_rng->rng.init = rk_rng_init;
> +	rk_rng->rng.cleanup = rk_rng_cleanup;
> +#endif
> +	rk_rng->rng.read = rk_rng_read;
> +	rk_rng->rng.priv = (unsigned long) dev;
> +	rk_rng->rng.quality = 900;
> +
> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_enable(dev);
> +
> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
> +
> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");

Drop, driver should be silent on success.


Best regards,
Krzysztof

Chen-Yu Tsai June 21, 2024, 10:04 a.m. UTC | #3

On Fri, Jun 21, 2024 at 9:25 AM Daniel Golle <[e-mail]> wrote:
>
> From: Aurelien Jarno <[e-mail]>
>
> 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 <[e-mail]>
> [[e-mail]: code style fixes]
> Signed-off-by: Daniel Golle <[e-mail]>
> ---
>  MAINTAINERS                           |   1 +
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 229 ++++++++++++++++++++++++++
>  4 files changed, 245 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77d449c89bf2..299b8c1a5fb5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19493,6 +19493,7 @@ M:      Daniel Golle <[e-mail]>
>  M:     Aurelien Jarno <[e-mail]>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> +F:     drivers/char/hw_random/rockchip-rng.c
>
>  ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
>  M:     Jacob Chen <[e-mail]>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 442c40efb200..2b62cd08f91a 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -573,6 +573,20 @@ config HW_RANDOM_JH7110
>           To compile this driver as a module, choose M here.
>           The module will be called jh7110-trng.
>
> +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 32549a1186dc..01f012eab440 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -48,4 +48,5 @@ 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
>  obj-$(CONFIG_HW_RANDOM_JH7110) += jh7110-trng.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..6070abb73847
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,229 @@
> +// 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 <[e-mail]>
> + *  Aurelien Jarno <[e-mail]>
> + */
> +#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>

Need to explicitly include linux/platform_device.h for |struct platform_device|
and devm_platform_iomap_resource().

> +#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                  0x0410
> +
> +struct rk_rng {
> +       struct hwrng rng;
> +       void __iomem *base;
> +       struct reset_control *rst;
> +       int clk_num;
> +       struct clk_bulk_data *clk_bulks;
> +};
> +
> +static int rk_rng_init(struct hwrng *rng)
> +{
> +       struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +       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 */
> +       writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> +                      TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> +                      TRNG_RNG_CTL_ENABLE,
> +                      rk_rng->base + TRNG_RNG_CTL);
> +
> +       return 0;
> +}
> +
> +static void rk_rng_cleanup(struct hwrng *rng)
> +{
> +       struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +
> +       /* stop TRNG */
> +       writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
> +
> +       /* 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);
> +       size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> +       u32 reg;
> +       int ret = 0;
> +
> +       ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
> +       if (ret < 0)
> +               goto out;
> +
> +       /* Start collecting random data */
> +       writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> +
> +       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 */
> +       memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> +out:
> +       pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> +       pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> +
> +       return to_read;
> +}
> +
> +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(*rk_rng), GFP_KERNEL);
> +       if (!rk_rng)
> +               return -ENOMEM;
> +
> +       rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(rk_rng->base))
> +               return PTR_ERR(rk_rng->base);
> +
> +       rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> +       if (rk_rng->clk_num < 0)
> +               return dev_err_probe(dev, rk_rng->clk_num,
> +                                    "Failed to get clks property\n");
> +
> +       rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> +       if (IS_ERR(rk_rng->rst))
> +               return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> +                                    "Failed to get reset property\n");
> +
> +       reset_control_assert(rk_rng->rst);
> +       udelay(2);
> +       reset_control_deassert(rk_rng->rst);
> +
> +       platform_set_drvdata(pdev, rk_rng);
> +
> +       rk_rng->rng.name = dev_driver_string(dev);
> +#ifndef CONFIG_PM
> +       rk_rng->rng.init = rk_rng_init;
> +       rk_rng->rng.cleanup = rk_rng_cleanup;
> +#endif
> +       rk_rng->rng.read = rk_rng_read;
> +       rk_rng->rng.priv = (unsigned long) dev;
> +       rk_rng->rng.quality = 900;
> +
> +       pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_enable(dev);

You can use devm_pm_runtime_enable(dev) here and simply get rid of the
remove function, and also no explicit cleanup needed.

> +
> +       ret = devm_hwrng_register(dev, &rk_rng->rng);
> +       if (ret)
> +               return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");

This is missing cleanup for pm_runtime_enable().

> +
> +       dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> +
> +       return 0;
> +}
> +
> +static int rk_rng_remove(struct platform_device *pdev)

Return type of remove callback has been changed to void. This needs to be
updated.


ChenYu

> +{
> +       pm_runtime_disable(&pdev->dev);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int rk_rng_runtime_suspend(struct device *dev)
> +{
> +       struct rk_rng *rk_rng = dev_get_drvdata(dev);
> +
> +       rk_rng_cleanup(&rk_rng->rng);
> +
> +       return 0;
> +}
> +
> +static int rk_rng_runtime_resume(struct device *dev)
> +{
> +       struct rk_rng *rk_rng = dev_get_drvdata(dev);
> +
> +       return rk_rng_init(&rk_rng->rng);
> +}
> +#endif
> +
> +static const struct dev_pm_ops rk_rng_pm_ops = {
> +       SET_RUNTIME_PM_OPS(rk_rng_runtime_suspend,
> +                               rk_rng_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +};
> +
> +static const struct of_device_id rk_rng_dt_match[] = {
> +       { .compatible = "rockchip,rk3568-rng", },
> +       { /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rk_rng_dt_match);
> +
> +static struct platform_driver rk_rng_driver = {
> +       .driver = {
> +               .name   = "rockchip-rng",
> +               .pm     = &rk_rng_pm_ops,
> +               .of_match_table = rk_rng_dt_match,
> +       },
> +       .probe  = rk_rng_probe,
> +       .remove = rk_rng_remove,
> +};
> +
> +module_platform_driver(rk_rng_driver);
> +
> +MODULE_DESCRIPTION("Rockchip True Random Number Generator driver");
> +MODULE_AUTHOR("Lin Jinhan <[e-mail]>, Aurelien Jarno <[e-mail]>");
> +MODULE_LICENSE("GPL");
> --
> 2.45.2
>

Chen-Yu Tsai June 21, 2024, 10:18 a.m. UTC | #4

On Fri, Jun 21, 2024 at 5:58 PM Krzysztof Kozlowski <[e-mail]> wrote:
>
> On 21/06/2024 03:25, Daniel Golle wrote:
> > From: Aurelien Jarno <[e-mail]>
> >
>
> > +
> > +static int rk_rng_init(struct hwrng *rng)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +     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 */
> > +     writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> > +                    TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> > +                    TRNG_RNG_CTL_ENABLE,
> > +                    rk_rng->base + TRNG_RNG_CTL);
>
> I doubt relaxed write is here intentional. Enabling should be last
> instruction, so this should be ordered write.

I agree that the driver should just do all non-relaxed writes for simplicity.
The penalty isn't that severe since commit 22ec71615d82 ("arm64: io: Relax
implicit barriers in default I/O accessors").

Just to clarify, writes to devices are always ordered. The non-relaxed
writes are ordered to _memory writes_, which doesn't really matter for
this driver.

ChenYu


> > +
> > +     return 0;
> > +}
> > +
> > +static void rk_rng_cleanup(struct hwrng *rng)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +
> > +     /* stop TRNG */
> > +     writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
>
> This should not be relaxed. This might lead to very tricky to debug issues.
>
> > +
> > +     /* 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);
> > +     size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> > +     u32 reg;
> > +     int ret = 0;
> > +
> > +     ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
>
> Why this cannot be just simpler pm_runtime_resume_and_get()?
>
> > +     if (ret < 0)
> > +             goto out;
>
> This does not look like correct error path. Device was not busy here.
>
> > +
> > +     /* Start collecting random data */
> > +     writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> > +
> > +     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 */
> > +     memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> > +out:
> > +     pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> > +     pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> > +
> > +     return to_read;
> > +}
> > +
> > +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(*rk_rng), GFP_KERNEL);
> > +     if (!rk_rng)
> > +             return -ENOMEM;
> > +
> > +     rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(rk_rng->base))
> > +             return PTR_ERR(rk_rng->base);
> > +
> > +     rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> > +     if (rk_rng->clk_num < 0)
> > +             return dev_err_probe(dev, rk_rng->clk_num,
> > +                                  "Failed to get clks property\n");
> > +
> > +     rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> > +     if (IS_ERR(rk_rng->rst))
> > +             return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> > +                                  "Failed to get reset property\n");
> > +
> > +     reset_control_assert(rk_rng->rst);
> > +     udelay(2);
> > +     reset_control_deassert(rk_rng->rst);
> > +
> > +     platform_set_drvdata(pdev, rk_rng);
> > +
> > +     rk_rng->rng.name = dev_driver_string(dev);
> > +#ifndef CONFIG_PM
> > +     rk_rng->rng.init = rk_rng_init;
> > +     rk_rng->rng.cleanup = rk_rng_cleanup;
> > +#endif
> > +     rk_rng->rng.read = rk_rng_read;
> > +     rk_rng->rng.priv = (unsigned long) dev;
> > +     rk_rng->rng.quality = 900;
> > +
> > +     pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> > +     pm_runtime_use_autosuspend(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     ret = devm_hwrng_register(dev, &rk_rng->rng);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
> > +
> > +     dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>
> Drop, driver should be silent on success.
>
>
> Best regards,
> Krzysztof
>
>

Philipp Zabel June 21, 2024, 11:41 a.m. UTC | #5

Hi,

On Fr, 2024-06-21 at 02:25 +0100, Daniel Golle wrote:
> From: Aurelien Jarno <[e-mail]>
> 
> 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 <[e-mail]>
> [[e-mail]: code style fixes]
> Signed-off-by: Daniel Golle <[e-mail]>
> ---
>  MAINTAINERS                           |   1 +
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 229 ++++++++++++++++++++++++++
>  4 files changed, 245 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
> 
[...]
> diff --git a/drivers/char/hw_random/rockchip-rng.c b/drivers/char/hw_random/rockchip-rng.c
> new file mode 100644
> index 000000000000..6070abb73847
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,229 @@
[...]
> 
> +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(*rk_rng), GFP_KERNEL);
> +	if (!rk_rng)
> +		return -ENOMEM;
> +
> +	rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rk_rng->base))
> +		return PTR_ERR(rk_rng->base);
> +
> +	rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> +	if (rk_rng->clk_num < 0)
> +		return dev_err_probe(dev, rk_rng->clk_num,
> +				     "Failed to get clks property\n");
> +
> +	rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);

Please use devm_reset_control_array_get_exclusive() instead.


regards
Philipp

Dragan Simic June 21, 2024, 6:13 p.m. UTC | #6

Hello Krzysztof,

On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> On 21/06/2024 03:25, Daniel Golle wrote:
>> From: Aurelien Jarno <[e-mail]>

[snip]

>> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip 
>> hwrng\n");
>> +
>> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> 
> Drop, driver should be silent on success.

I respectfully disagree.  Many drivers print a single line upon
successful probing, which I find very useful.  In this particular
case, it's even more useful, because some people may be concerned
about the use of hardware TRNGs, so we should actually make sure
to announce it.

Uwe Kleine-König June 21, 2024, 10:16 p.m. UTC | #7

Hello Dragan,

On 6/21/24 20:13, Dragan Simic wrote:
> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> On 21/06/2024 03:25, Daniel Golle wrote:
>>> From: Aurelien Jarno <[e-mail]>
> 
> [snip]
> 
>>> +    pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>>> +    pm_runtime_use_autosuspend(dev);
>>> +    pm_runtime_enable(dev);
>>> +
>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
>>> +    if (ret)
>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register 
>>> Rockchip hwrng\n");
>>> +
>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>
>> Drop, driver should be silent on success.
> 
> I respectfully disagree.  Many drivers print a single line upon
> successful probing, which I find very useful.  In this particular
> case, it's even more useful, because some people may be concerned
> about the use of hardware TRNGs, so we should actually make sure
> to announce it.

I agree to Krzysztof here. From the POV of a driver author, your own
driver is very important and while you write it, it really interests
*you* if the driver is successfully probed. However from a system
perspective these are annoying: There are easily >50 devices[1] on a
system, if all of these print a message in probe, you have little chance
to see the relevant messages. Even if every driver author thinks their
work is a special snow flake that is worth announcing, in practice users
only care about your driver if there is a problem. Additionally each
message takes time and so delays the boot process. Additionally each 
message takes place in the printk ring buffer and so edges out earlier 
messages that might be more important.

So +1 for dropping the dev_info() or at least using dev_debug() for it.

Best regards
Uwe

[1] On my laptop if have:

	$ find /sys/devices -name driver | wc -l
	87

     On a Raspberrypi it yields 66.

Dragan Simic June 22, 2024, 10:29 a.m. UTC | #8

Hello Uwe,

On 2024-06-22 00:16, Uwe Kleine-König wrote:
> On 6/21/24 20:13, Dragan Simic wrote:
>> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>>> On 21/06/2024 03:25, Daniel Golle wrote:
>>>> From: Aurelien Jarno <[e-mail]>
>> 
>> [snip]
>> 
>>>> +    pm_runtime_set_autosuspend_delay(dev, 
>>>> RK_RNG_AUTOSUSPEND_DELAY);
>>>> +    pm_runtime_use_autosuspend(dev);
>>>> +    pm_runtime_enable(dev);
>>>> +
>>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
>>>> +    if (ret)
>>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register 
>>>> Rockchip hwrng\n");
>>>> +
>>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>> 
>>> Drop, driver should be silent on success.
>> 
>> I respectfully disagree.  Many drivers print a single line upon
>> successful probing, which I find very useful.  In this particular
>> case, it's even more useful, because some people may be concerned
>> about the use of hardware TRNGs, so we should actually make sure
>> to announce it.
> 
> I agree to Krzysztof here. From the POV of a driver author, your own
> driver is very important and while you write it, it really interests
> *you* if the driver is successfully probed. However from a system
> perspective these are annoying: There are easily >50 devices[1] on a
> system, if all of these print a message in probe, you have little 
> chance
> to see the relevant messages. Even if every driver author thinks their
> work is a special snow flake that is worth announcing, in practice 
> users
> only care about your driver if there is a problem. Additionally each
> message takes time and so delays the boot process. Additionally each
> message takes place in the printk ring buffer and so edges out earlier
> messages that might be more important.

Well, I don't find those messages annoying, for the drivers I've had
nothing to do with.  Also, in my experience, 99.9% of users don't care
about the kernel messages at all, be it everything hunky-dory, or be
it something really wrong somewhere.

> So +1 for dropping the dev_info() or at least using dev_debug() for it.
> 
> Best regards
> Uwe
> 
> [1] On my laptop if have:
> 
> 	$ find /sys/devices -name driver | wc -l
> 	87
> 
>     On a Raspberrypi it yields 66.

Krzysztof Kozlowski June 22, 2024, 6:05 p.m. UTC | #9

On 21/06/2024 20:13, Dragan Simic wrote:
> Hello Krzysztof,
> 
> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> On 21/06/2024 03:25, Daniel Golle wrote:
>>> From: Aurelien Jarno <[e-mail]>
> 
> [snip]
> 
>>> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>>> +	pm_runtime_use_autosuspend(dev);
>>> +	pm_runtime_enable(dev);
>>> +
>>> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
>>> +	if (ret)
>>> +		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip 
>>> hwrng\n");
>>> +
>>> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>
>> Drop, driver should be silent on success.
> 
> I respectfully disagree.  Many drivers print a single line upon
> successful probing, which I find very useful.  In this particular

No, it's duplicating existing interfaces and polluting log unnecessarily
without any useful information.

> case, it's even more useful, because some people may be concerned
> about the use of hardware TRNGs, so we should actually make sure
> to announce it.

Best regards,
Krzysztof

Dragan Simic June 22, 2024, 7:10 p.m. UTC | #10

Hello Krzysztof,

On 2024-06-22 20:05, Krzysztof Kozlowski wrote:
> On 21/06/2024 20:13, Dragan Simic wrote:
>> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>>> On 21/06/2024 03:25, Daniel Golle wrote:
>>>> From: Aurelien Jarno <[e-mail]>
>> 
>> [snip]
>> 
>>>> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>>>> +	pm_runtime_use_autosuspend(dev);
>>>> +	pm_runtime_enable(dev);
>>>> +
>>>> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
>>>> +	if (ret)
>>>> +		return dev_err_probe(&pdev->dev, ret, "Failed to register 
>>>> Rockchip
>>>> hwrng\n");
>>>> +
>>>> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>> 
>>> Drop, driver should be silent on success.
>> 
>> I respectfully disagree.  Many drivers print a single line upon
>> successful probing, which I find very useful.  In this particular
> 
> No, it's duplicating existing interfaces and polluting log 
> unnecessarily
> without any useful information.

Would you, please, clarify what existing interfaces are you
referring to?

>> case, it's even more useful, because some people may be concerned
>> about the use of hardware TRNGs, so we should actually make sure
>> to announce it.

Heiko Stübner June 22, 2024, 8:26 p.m. UTC | #11

Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
> Hello Uwe,
> 
> On 2024-06-22 00:16, Uwe Kleine-König wrote:
> > On 6/21/24 20:13, Dragan Simic wrote:
> >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> >>> On 21/06/2024 03:25, Daniel Golle wrote:
> >>>> From: Aurelien Jarno <[e-mail]>
> >> 
> >> [snip]
> >> 
> >>>> +    pm_runtime_set_autosuspend_delay(dev, 
> >>>> RK_RNG_AUTOSUSPEND_DELAY);
> >>>> +    pm_runtime_use_autosuspend(dev);
> >>>> +    pm_runtime_enable(dev);
> >>>> +
> >>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
> >>>> +    if (ret)
> >>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register 
> >>>> Rockchip hwrng\n");
> >>>> +
> >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> >>> 
> >>> Drop, driver should be silent on success.
> >> 
> >> I respectfully disagree.  Many drivers print a single line upon
> >> successful probing, which I find very useful.  In this particular
> >> case, it's even more useful, because some people may be concerned
> >> about the use of hardware TRNGs, so we should actually make sure
> >> to announce it.
> > 
> > I agree to Krzysztof here. From the POV of a driver author, your own
> > driver is very important and while you write it, it really interests
> > *you* if the driver is successfully probed. However from a system
> > perspective these are annoying: There are easily >50 devices[1] on a
> > system, if all of these print a message in probe, you have little 
> > chance
> > to see the relevant messages. Even if every driver author thinks their
> > work is a special snow flake that is worth announcing, in practice 
> > users
> > only care about your driver if there is a problem. Additionally each
> > message takes time and so delays the boot process. Additionally each
> > message takes place in the printk ring buffer and so edges out earlier
> > messages that might be more important.
> 
> Well, I don't find those messages annoying, for the drivers I've had
> nothing to do with.  Also, in my experience, 99.9% of users don't care
> about the kernel messages at all, be it everything hunky-dory, or be
> it something really wrong somewhere.
> 
> > So +1 for dropping the dev_info() or at least using dev_debug() for it.

Just for 2ct ... I'm also in the don't print too much camp ;-) .
When parsing kernel logs to see where things fail, messages just
telling me about sucesses make things more difficult.

So really this message should be dropped or at least as Uwe suggests
made a dev_dbg.


Heiko

Dragan Simic June 22, 2024, 8:45 p.m. UTC | #12

Hello Heiko,

On 2024-06-22 22:26, Heiko Stübner wrote:
> Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
>> On 2024-06-22 00:16, Uwe Kleine-König wrote:
>> > On 6/21/24 20:13, Dragan Simic wrote:
>> >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> >>> On 21/06/2024 03:25, Daniel Golle wrote:
>> >>>> From: Aurelien Jarno <[e-mail]>
>> >>
>> >> [snip]
>> >>
>> >>>> +    pm_runtime_set_autosuspend_delay(dev,
>> >>>> RK_RNG_AUTOSUSPEND_DELAY);
>> >>>> +    pm_runtime_use_autosuspend(dev);
>> >>>> +    pm_runtime_enable(dev);
>> >>>> +
>> >>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
>> >>>> +    if (ret)
>> >>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register
>> >>>> Rockchip hwrng\n");
>> >>>> +
>> >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>> >>>
>> >>> Drop, driver should be silent on success.
>> >>
>> >> I respectfully disagree.  Many drivers print a single line upon
>> >> successful probing, which I find very useful.  In this particular
>> >> case, it's even more useful, because some people may be concerned
>> >> about the use of hardware TRNGs, so we should actually make sure
>> >> to announce it.
>> >
>> > I agree to Krzysztof here. From the POV of a driver author, your own
>> > driver is very important and while you write it, it really interests
>> > *you* if the driver is successfully probed. However from a system
>> > perspective these are annoying: There are easily >50 devices[1] on a
>> > system, if all of these print a message in probe, you have little
>> > chance
>> > to see the relevant messages. Even if every driver author thinks their
>> > work is a special snow flake that is worth announcing, in practice
>> > users
>> > only care about your driver if there is a problem. Additionally each
>> > message takes time and so delays the boot process. Additionally each
>> > message takes place in the printk ring buffer and so edges out earlier
>> > messages that might be more important.
>> 
>> Well, I don't find those messages annoying, for the drivers I've had
>> nothing to do with.  Also, in my experience, 99.9% of users don't care
>> about the kernel messages at all, be it everything hunky-dory, or be
>> it something really wrong somewhere.
>> 
>> > So +1 for dropping the dev_info() or at least using dev_debug() for it.
> 
> Just for 2ct ... I'm also in the don't print too much camp ;-) .
> When parsing kernel logs to see where things fail, messages just
> telling me about sucesses make things more difficult.
> 
> So really this message should be dropped or at least as Uwe suggests
> made a dev_dbg.

As a note, "dmesg --level=err,warn", for example, is rather useful
when it comes to filtering the kernel messages to see only those that
are signs of a trouble.

Uwe Kleine-König June 23, 2024, 12:20 a.m. UTC | #13

Hello Dragan,

On Sat, Jun 22, 2024 at 10:45:22PM +0200, Dragan Simic wrote:
> On 2024-06-22 22:26, Heiko Stübner wrote:
> > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
> > > On 2024-06-22 00:16, Uwe Kleine-König wrote:
> > > > On 6/21/24 20:13, Dragan Simic wrote:
> > > >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> > > >>> On 21/06/2024 03:25, Daniel Golle wrote:
> > > >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> > > >>>
> > > >>> Drop, driver should be silent on success.
> > > >>
> > [...]
> > So really this message should be dropped or at least as Uwe suggests
> > made a dev_dbg.
> 
> As a note, "dmesg --level=err,warn", for example, is rather useful
> when it comes to filtering the kernel messages to see only those that
> are signs of a trouble.

IMHO it's a bit sad, that there is such a long thread about something so
trivial, but I want to make a few points:

 - not all dmesg implementations support this:

	root@machine:~ dmesg --level=err,warn
	dmesg: unrecognized option '--level=err,warn'
	BusyBox v1.36.1 () multi-call binary.

	Usage: dmesg [-cr] [-n LEVEL] [-s SIZE]

	Print or control the kernel ring buffer

		-c              Clear ring buffer after printing
		-n LEVEL        Set console logging level
		-s SIZE         Buffer size
		-r              Print raw message buffer

 - Your argument that the output of this dev_info can easily be ignored
   is a very weak reason to keep it.

 - I personally consider it hard sometimes to accept feedback if I think
   it's wrong and there is a good reason to do it the way I want it.
   But there are now three people opposing your position, who brought
   forward (IMHO) good reasons and even a constructive alternative was
   presented. Please stay open minded while weighting the options.
   The questions to ask here include:

    - How many people care for this message? During every boot? Is it
      maybe enough for these people to check in /sys if the device
      probed successfully? Or maybe even the absence of an error message
      is enough?
    - How many people get this message and don't care about the
      presented information? How many people are even annoyed by it?
    - Is the delay and memory usage introduced by this message justified
      then, even if it's small?

Best regards
Uwe

Dragan Simic June 23, 2024, 5:41 a.m. UTC | #14

Hello Uwe,

On 2024-06-23 02:20, Uwe Kleine-König wrote:
> On Sat, Jun 22, 2024 at 10:45:22PM +0200, Dragan Simic wrote:
>> On 2024-06-22 22:26, Heiko Stübner wrote:
>> > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
>> > > On 2024-06-22 00:16, Uwe Kleine-König wrote:
>> > > > On 6/21/24 20:13, Dragan Simic wrote:
>> > > >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> > > >>> On 21/06/2024 03:25, Daniel Golle wrote:
>> > > >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>> > > >>>
>> > > >>> Drop, driver should be silent on success.
>> > > >>
>> > [...]
>> > So really this message should be dropped or at least as Uwe suggests
>> > made a dev_dbg.
>> 
>> As a note, "dmesg --level=err,warn", for example, is rather useful
>> when it comes to filtering the kernel messages to see only those that
>> are signs of a trouble.
> 
> IMHO it's a bit sad, that there is such a long thread about something 
> so
> trivial, but I want to make a few points:
> 
>  - not all dmesg implementations support this:
> 
> 	root@machine:~ dmesg --level=err,warn
> 	dmesg: unrecognized option '--level=err,warn'
> 	BusyBox v1.36.1 () multi-call binary.
> 
> 	Usage: dmesg [-cr] [-n LEVEL] [-s SIZE]
> 
> 	Print or control the kernel ring buffer
> 
> 		-c              Clear ring buffer after printing
> 		-n LEVEL        Set console logging level
> 		-s SIZE         Buffer size
> 		-r              Print raw message buffer
> 
>  - Your argument that the output of this dev_info can easily be ignored
>    is a very weak reason to keep it.
> 
>  - I personally consider it hard sometimes to accept feedback if I 
> think
>    it's wrong and there is a good reason to do it the way I want it.
>    But there are now three people opposing your position, who brought
>    forward (IMHO) good reasons and even a constructive alternative was
>    presented. Please stay open minded while weighting the options.
>    The questions to ask here include:
> 
>     - How many people care for this message? During every boot? Is it
>       maybe enough for these people to check in /sys if the device
>       probed successfully? Or maybe even the absence of an error 
> message
>       is enough?
>     - How many people get this message and don't care about the
>       presented information? How many people are even annoyed by it?
>     - Is the delay and memory usage introduced by this message 
> justified
>       then, even if it's small?

I'm sorry if my responses caused any inconvenience.  I find most of your
points totally valid, but there are a couple of them I could continue
arguing about.  Though, as you also pointed out, my opinion has been
already outvoted, so I'll remain silent.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 77d449c89bf2..299b8c1a5fb5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19493,6 +19493,7 @@  M:	Daniel Golle <[e-mail]>
 M:	Aurelien Jarno <[e-mail]>
 S:	Maintained
 F:	Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
+F:	drivers/char/hw_random/rockchip-rng.c
 
 ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
 M:	Jacob Chen <[e-mail]>
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 442c40efb200..2b62cd08f91a 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -573,6 +573,20 @@  config HW_RANDOM_JH7110
 	  To compile this driver as a module, choose M here.
 	  The module will be called jh7110-trng.
 
+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 32549a1186dc..01f012eab440 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -48,4 +48,5 @@  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
 obj-$(CONFIG_HW_RANDOM_JH7110) += jh7110-trng.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..6070abb73847
--- /dev/null
+++ b/drivers/char/hw_random/rockchip-rng.c
@@ -0,0 +1,229 @@ 
+// 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 <[e-mail]>
+ *  Aurelien Jarno <[e-mail]>
+ */
+#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			0x0410
+
+struct rk_rng {
+	struct hwrng rng;
+	void __iomem *base;
+	struct reset_control *rst;
+	int clk_num;
+	struct clk_bulk_data *clk_bulks;
+};
+
+static int rk_rng_init(struct hwrng *rng)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+	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 */
+	writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
+		       TRNG_RNG_CTL_OSC_RING_SPEED_0 |
+		       TRNG_RNG_CTL_ENABLE,
+		       rk_rng->base + TRNG_RNG_CTL);
+
+	return 0;
+}
+
+static void rk_rng_cleanup(struct hwrng *rng)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+
+	/* stop TRNG */
+	writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
+
+	/* 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);
+	size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
+	u32 reg;
+	int ret = 0;
+
+	ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
+	if (ret < 0)
+		goto out;
+
+	/* Start collecting random data */
+	writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
+
+	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 */
+	memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
+out:
+	pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
+	pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
+
+	return to_read;
+}
+
+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(*rk_rng), GFP_KERNEL);
+	if (!rk_rng)
+		return -ENOMEM;
+
+	rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rk_rng->base))
+		return PTR_ERR(rk_rng->base);
+
+	rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
+	if (rk_rng->clk_num < 0)
+		return dev_err_probe(dev, rk_rng->clk_num,
+				     "Failed to get clks property\n");
+
+	rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
+	if (IS_ERR(rk_rng->rst))
+		return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
+				     "Failed to get reset property\n");
+
+	reset_control_assert(rk_rng->rst);
+	udelay(2);
+	reset_control_deassert(rk_rng->rst);
+
+	platform_set_drvdata(pdev, rk_rng);
+
+	rk_rng->rng.name = dev_driver_string(dev);
+#ifndef CONFIG_PM
+	rk_rng->rng.init = rk_rng_init;
+	rk_rng->rng.cleanup = rk_rng_cleanup;
+#endif
+	rk_rng->rng.read = rk_rng_read;
+	rk_rng->rng.priv = (unsigned long) dev;
+	rk_rng->rng.quality = 900;
+
+	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+
+	ret = devm_hwrng_register(dev, &rk_rng->rng);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
+
+	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
+
+	return 0;
+}
+
+static int rk_rng_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int rk_rng_runtime_suspend(struct device *dev)
+{
+	struct rk_rng *rk_rng = dev_get_drvdata(dev);
+
+	rk_rng_cleanup(&rk_rng->rng);
+
+	return 0;
+}
+
+static int rk_rng_runtime_resume(struct device *dev)
+{
+	struct rk_rng *rk_rng = dev_get_drvdata(dev);
+
+	return rk_rng_init(&rk_rng->rng);
+}
+#endif
+
+static const struct dev_pm_ops rk_rng_pm_ops = {
+	SET_RUNTIME_PM_OPS(rk_rng_runtime_suspend,
+				rk_rng_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
+static const struct of_device_id rk_rng_dt_match[] = {
+	{ .compatible = "rockchip,rk3568-rng", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, rk_rng_dt_match);
+
+static struct platform_driver rk_rng_driver = {
+	.driver	= {
+		.name	= "rockchip-rng",
+		.pm	= &rk_rng_pm_ops,
+		.of_match_table = rk_rng_dt_match,
+	},
+	.probe	= rk_rng_probe,
+	.remove = rk_rng_remove,
+};
+
+module_platform_driver(rk_rng_driver);
+
+MODULE_DESCRIPTION("Rockchip True Random Number Generator driver");
+MODULE_AUTHOR("Lin Jinhan <[e-mail]>, Aurelien Jarno <[e-mail]>");
+MODULE_LICENSE("GPL");