* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 1:25 ` [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver Daniel Golle
@ 2024-06-21 9:32 ` Diederik de Haas
2024-06-21 9:57 ` Krzysztof Kozlowski
` (2 subsequent siblings)
3 siblings, 0 replies; 24+ messages in thread
From: Diederik de Haas @ 2024-06-21 9:32 UTC (permalink / raw)
To: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Uwe Kleine-König, Sebastian Reichel,
Anand Moon, Dragan Simic, Sascha Hauer, Martin Kaiser,
Ard Biesheuvel, linux-crypto, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
Cc: Daniel Golle
[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]
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 <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>
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
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 1:25 ` [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver Daniel Golle
2024-06-21 9:32 ` Diederik de Haas
@ 2024-06-21 9:57 ` Krzysztof Kozlowski
2024-06-21 10:18 ` Chen-Yu Tsai
2024-06-21 18:13 ` Dragan Simic
2024-06-21 10:04 ` Chen-Yu Tsai
2024-06-21 11:41 ` Philipp Zabel
3 siblings, 2 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2024-06-21 9:57 UTC (permalink / raw)
To: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Uwe Kleine-König, Sebastian Reichel,
Anand Moon, Dragan Simic, Sascha Hauer, Martin Kaiser,
Ard Biesheuvel, linux-crypto, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
On 21/06/2024 03:25, Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
>
> +
> +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
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 9:57 ` Krzysztof Kozlowski
@ 2024-06-21 10:18 ` Chen-Yu Tsai
2024-06-21 18:13 ` Dragan Simic
1 sibling, 0 replies; 24+ messages in thread
From: Chen-Yu Tsai @ 2024-06-21 10:18 UTC (permalink / raw)
To: Daniel Golle
Cc: Krzysztof Kozlowski, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Uwe Kleine-König, Sebastian Reichel,
Anand Moon, Dragan Simic, Sascha Hauer, Martin Kaiser,
Ard Biesheuvel, linux-crypto, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
On Fri, Jun 21, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/06/2024 03:25, Daniel Golle wrote:
> > From: Aurelien Jarno <aurelien@aurel32.net>
> >
>
> > +
> > +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
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 9:57 ` Krzysztof Kozlowski
2024-06-21 10:18 ` Chen-Yu Tsai
@ 2024-06-21 18:13 ` Dragan Simic
2024-06-21 22:16 ` Uwe Kleine-König
2024-06-22 18:05 ` Krzysztof Kozlowski
1 sibling, 2 replies; 24+ messages in thread
From: Dragan Simic @ 2024-06-21 18:13 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Uwe Kleine-König, Sebastian Reichel,
Anand Moon, Sascha Hauer, Martin Kaiser, Ard Biesheuvel,
linux-crypto, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hello Krzysztof,
On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> On 21/06/2024 03:25, Daniel Golle wrote:
>> From: Aurelien Jarno <aurelien@aurel32.net>
[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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 18:13 ` Dragan Simic
@ 2024-06-21 22:16 ` Uwe Kleine-König
2024-06-22 10:29 ` Dragan Simic
2024-06-22 18:05 ` Krzysztof Kozlowski
1 sibling, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-06-21 22:16 UTC (permalink / raw)
To: Dragan Simic, Krzysztof Kozlowski
Cc: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Sebastian Reichel, Anand Moon, Sascha Hauer,
Martin Kaiser, Ard Biesheuvel, linux-crypto, devicetree,
linux-arm-kernel, linux-rockchip, linux-kernel
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 <aurelien@aurel32.net>
>
> [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.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 22:16 ` Uwe Kleine-König
@ 2024-06-22 10:29 ` Dragan Simic
2024-06-22 20:26 ` Heiko Stübner
0 siblings, 1 reply; 24+ messages in thread
From: Dragan Simic @ 2024-06-22 10:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Krzysztof Kozlowski, Daniel Golle, Aurelien Jarno, Olivia Mackall,
Herbert Xu, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Heiko Stuebner, Philipp Zabel, Sebastian Reichel, Anand Moon,
Sascha Hauer, Martin Kaiser, Ard Biesheuvel, linux-crypto,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel
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 <aurelien@aurel32.net>
>>
>> [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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-22 10:29 ` Dragan Simic
@ 2024-06-22 20:26 ` Heiko Stübner
2024-06-22 20:45 ` Dragan Simic
0 siblings, 1 reply; 24+ messages in thread
From: Heiko Stübner @ 2024-06-22 20:26 UTC (permalink / raw)
To: Uwe Kleine-König, Dragan Simic
Cc: Krzysztof Kozlowski, Daniel Golle, Aurelien Jarno, Olivia Mackall,
Herbert Xu, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Philipp Zabel, Sebastian Reichel, Anand Moon, Sascha Hauer,
Martin Kaiser, Ard Biesheuvel, linux-crypto, devicetree,
linux-arm-kernel, linux-rockchip, linux-kernel
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 <aurelien@aurel32.net>
> >>
> >> [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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-22 20:26 ` Heiko Stübner
@ 2024-06-22 20:45 ` Dragan Simic
2024-06-23 0:20 ` Uwe Kleine-König
0 siblings, 1 reply; 24+ messages in thread
From: Dragan Simic @ 2024-06-22 20:45 UTC (permalink / raw)
To: Heiko Stübner
Cc: Uwe Kleine-König, Krzysztof Kozlowski, Daniel Golle,
Aurelien Jarno, Olivia Mackall, Herbert Xu, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel,
Sebastian Reichel, Anand Moon, Sascha Hauer, Martin Kaiser,
Ard Biesheuvel, linux-crypto, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
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 <aurelien@aurel32.net>
>> >>
>> >> [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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-22 20:45 ` Dragan Simic
@ 2024-06-23 0:20 ` Uwe Kleine-König
2024-06-23 5:41 ` Dragan Simic
0 siblings, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-06-23 0:20 UTC (permalink / raw)
To: Dragan Simic
Cc: Heiko Stübner, Krzysztof Kozlowski, Daniel Golle,
Aurelien Jarno, Olivia Mackall, Herbert Xu, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel,
Sebastian Reichel, Anand Moon, Sascha Hauer, Martin Kaiser,
Ard Biesheuvel, linux-crypto, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-23 0:20 ` Uwe Kleine-König
@ 2024-06-23 5:41 ` Dragan Simic
0 siblings, 0 replies; 24+ messages in thread
From: Dragan Simic @ 2024-06-23 5:41 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Heiko Stübner, Krzysztof Kozlowski, Daniel Golle,
Aurelien Jarno, Olivia Mackall, Herbert Xu, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Philipp Zabel,
Sebastian Reichel, Anand Moon, Sascha Hauer, Martin Kaiser,
Ard Biesheuvel, linux-crypto, devicetree, linux-arm-kernel,
linux-rockchip, linux-kernel
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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 18:13 ` Dragan Simic
2024-06-21 22:16 ` Uwe Kleine-König
@ 2024-06-22 18:05 ` Krzysztof Kozlowski
2024-06-22 19:10 ` Dragan Simic
1 sibling, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2024-06-22 18:05 UTC (permalink / raw)
To: Dragan Simic
Cc: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Uwe Kleine-König, Sebastian Reichel,
Anand Moon, Sascha Hauer, Martin Kaiser, Ard Biesheuvel,
linux-crypto, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
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 <aurelien@aurel32.net>
>
> [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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-22 18:05 ` Krzysztof Kozlowski
@ 2024-06-22 19:10 ` Dragan Simic
0 siblings, 0 replies; 24+ messages in thread
From: Dragan Simic @ 2024-06-22 19:10 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Philipp Zabel, Uwe Kleine-König, Sebastian Reichel,
Anand Moon, Sascha Hauer, Martin Kaiser, Ard Biesheuvel,
linux-crypto, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
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 <aurelien@aurel32.net>
>>
>> [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.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 1:25 ` [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver Daniel Golle
2024-06-21 9:32 ` Diederik de Haas
2024-06-21 9:57 ` Krzysztof Kozlowski
@ 2024-06-21 10:04 ` Chen-Yu Tsai
2024-06-21 11:41 ` Philipp Zabel
3 siblings, 0 replies; 24+ messages in thread
From: Chen-Yu Tsai @ 2024-06-21 10:04 UTC (permalink / raw)
To: Daniel Golle
Cc: Aurelien Jarno, Olivia Mackall, Herbert Xu, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner, Philipp Zabel,
Uwe Kleine-König, Sebastian Reichel, Anand Moon,
Dragan Simic, Sascha Hauer, Martin Kaiser, Ard Biesheuvel,
linux-crypto, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
On Fri, Jun 21, 2024 at 9:25 AM Daniel Golle <daniel@makrotopia.org> wrote:
>
> From: Aurelien Jarno <aurelien@aurel32.net>
>
> 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>
> [daniel@makrotpia.org: code style fixes]
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> 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 <daniel@makrotopia.org>
> M: Aurelien Jarno <aurelien@aurel32.net>
> 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 <jacob-chen@iotwrt.com>
> 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 <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>
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 <troy.lin@rock-chips.com>, Aurelien Jarno <aurelien@aurel32.net>");
> +MODULE_LICENSE("GPL");
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver
2024-06-21 1:25 ` [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver Daniel Golle
` (2 preceding siblings ...)
2024-06-21 10:04 ` Chen-Yu Tsai
@ 2024-06-21 11:41 ` Philipp Zabel
3 siblings, 0 replies; 24+ messages in thread
From: Philipp Zabel @ 2024-06-21 11:41 UTC (permalink / raw)
To: Daniel Golle, Aurelien Jarno, Olivia Mackall, Herbert Xu,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner,
Uwe Kleine-König, Sebastian Reichel, Anand Moon,
Dragan Simic, Sascha Hauer, Martin Kaiser, Ard Biesheuvel,
linux-crypto, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hi,
On Fr, 2024-06-21 at 02:25 +0100, Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
>
> 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>
> [daniel@makrotpia.org: code style fixes]
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
> 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
^ permalink raw reply [flat|nested] 24+ messages in thread