On Thu, Jan 12, 2023 at 12:38:11PM +0800, Jia Jie Ho wrote:
> This adds driver support for the hardware random number generator in
> Starfive SoCs and adds StarFive TRNG entry to MAINTAINERS.
>
> Co-developed-by: Jenny Zhang <jenny.zhang@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jenny Zhang <jenny.zhang@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jia Jie Ho <jiajie.ho@xxxxxxxxxxxxxxxx>
> ---
> MAINTAINERS | 6 +
> drivers/char/hw_random/Kconfig | 11 +
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/starfive-trng.c | 397 +++++++++++++++++++++++++
> 4 files changed, 415 insertions(+)
> create mode 100644 drivers/char/hw_random/starfive-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 65140500d9f8..b91e3fc11fc6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19626,6 +19626,12 @@ F: drivers/reset/reset-starfive.c
> F: include/linux/reset/starfive.h
> F: include/dt-bindings/reset/starfive*
>
> +STARFIVE TRNG DRIVER
> +M: Jia Jie Ho <jiajie.ho@xxxxxxxxxxxxxxxx>
> +S: Maintained
> +F: Documentation/devicetree/bindings/rng/starfive*
> +F: drivers/char/hw_random/starfive-trng.c
minor nit (so don't submit another version just to fix this):
This should be Supported, no?
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 3e948cf04476..f68ac370847f 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -47,3 +47,4 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-trng.o
> obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
> obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
> obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
> +obj-$(CONFIG_HW_RANDOM_STARFIVE) += starfive-trng.o
Is "STARFIVE" a bit too general of a name here and in the Kconfig entry?
I don't have a TRM for the JH7100, but this name (and the Kconfig text)
would give me the impression that I can use it there too.
Does this driver support both?
> +static int starfive_trng_probe(struct platform_device *pdev)
> + clk_prepare_enable(trng->hclk);
> + clk_prepare_enable(trng->ahb);
> + reset_control_deassert(trng->rst);
> +
> + trng->rng.name = dev_driver_string(&pdev->dev);
> + trng->rng.init = starfive_trng_init;
> + trng->rng.cleanup = starfive_trng_cleanup;
> + trng->rng.read = starfive_trng_read;
> +
> + trng->mode = PRNG_256BIT;
> + trng->mission = 1;
> + trng->reseed = RANDOM_RESEED;
> +
> + ret = devm_hwrng_register(&pdev->dev, &trng->rng);
> + if (ret) {
> + dev_err_probe(&pdev->dev, ret, "Failed to register hwrng\n");
> + goto err_fail_register;
> + }
> +
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> + pm_runtime_enable(&pdev->dev);
> +
> + return 0;
> +
> +err_fail_register:
> + pm_runtime_disable(&pdev->dev);
This was only enabled after the only goto for this label, does it
serve a purpose?
I know little about runtime PM, it just caught my eye.
I looked at the other rng drivers that had calls to pm_runtime_enable(),
but they all seem to do their pm enablement _before_ calling
hwrng_register().
Again, I am not familiar with runtime PM, but curious why you are doing
things differently, that's all.
> +
> + reset_control_assert(trng->rst);
> + clk_disable_unprepare(trng->ahb);
> + clk_disable_unprepare(trng->hclk);
> +
> + return ret;
> +}
Thanks,
Conor.
Attachment:
signature.asc
Description: PGP signature
