[PATCH v2 0/3] hwrng: starfive - Add driver for TRNG module

[PATCH v2 0/3] hwrng: starfive - Add driver for TRNG module

[Jia Jie Ho]

This patch series adds kernel support for StarFive hardware random
number generator. First 2 patches add bindings documentation and driver
for this module. Patch 3 adds devicetree entry for VisionFive v2 SoC.

Patch 3 needs to be applied on top of:
https://patchwork.kernel.org/project/linux-riscv/patch/20221220011247.35560-7-hal.feng@starfivetech.com/

Changes since v1:
- updated of_match_ptr and added pm_sleep_ptr in Patch 2. (by Krzysztof)
- drop "status" in dts as module is always on in Patch 3. (by Krzysztof)

Jia Jie Ho (3):
  dt-bindings: rng: Add StarFive TRNG module
  hwrng: starfive - Add TRNG driver for StarFive SoC
  riscv: dts: starfive: Add TRNG node for VisionFive 2

 .../bindings/rng/starfive,jh7110-trng.yaml    |  55 +++
 MAINTAINERS                                   |   6 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  10 +
 drivers/char/hw_random/Kconfig                |  11 +
 drivers/char/hw_random/Makefile               |   1 +
 drivers/char/hw_random/starfive-trng.c        | 388 ++++++++++++++++++
 6 files changed, 471 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
 create mode 100644 drivers/char/hw_random/starfive-trng.c

-- 
2.25.1

[PATCH v2 1/3] dt-bindings: rng: Add StarFive TRNG module

[Jia Jie Ho]

Add documentation to describe Starfive true random number generator
module.

Co-developed-by: Jenny Zhang <jenny.zhang@starfivetech.com>
Signed-off-by: Jenny Zhang <jenny.zhang@starfivetech.com>
Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/rng/starfive,jh7110-trng.yaml    | 55 +++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml

diff --git a/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml b/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
new file mode 100644
index 000000000000..2b76ce25acc4
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rng/starfive,jh7110-trng.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive SoC TRNG Module
+
+maintainers:
+  - Jia Jie Ho <jiajie.ho@starfivetech.com>
+
+properties:
+  compatible:
+    const: starfive,jh7110-trng
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Hardware reference clock
+      - description: AHB reference clock
+
+  clock-names:
+    items:
+      - const: hclk
+      - const: ahb
+
+  resets:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    rng: rng@1600C000 {
+        compatible = "starfive,jh7110-trng";
+        reg = <0x1600C000 0x4000>;
+        clocks = <&clk 15>, <&clk 16>;
+        clock-names = "hclk", "ahb";
+        resets = <&reset 3>;
+        interrupts = <30>;
+    };
+...
-- 
2.25.1

[PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[Jia Jie Ho]

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@starfivetech.com>
Signed-off-by: Jenny Zhang <jenny.zhang@starfivetech.com>
Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
---
 MAINTAINERS                            |   6 +
 drivers/char/hw_random/Kconfig         |  11 +
 drivers/char/hw_random/Makefile        |   1 +
 drivers/char/hw_random/starfive-trng.c | 388 +++++++++++++++++++++++++
 4 files changed, 406 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@starfivetech.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/rng/starfive*
+F:	drivers/char/hw_random/starfive-trng.c
+
 STATIC BRANCH/CALL
 M:	Peter Zijlstra <peterz@infradead.org>
 M:	Josh Poimboeuf <jpoimboe@kernel.org>
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 3da8e85f8aae..ddd7ada7f877 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -549,6 +549,17 @@ config HW_RANDOM_CN10K
 	 To compile this driver as a module, choose M here.
 	 The module will be called cn10k_rng. If unsure, say Y.
 
+config HW_RANDOM_STARFIVE
+	tristate "StarFive HW Random Number Generator support"
+	depends on SOC_STARFIVE
+	depends on HW_RANDOM
+	help
+	  This driver provides support for the Hardware Random Number
+	  Generator in StarFive SoCs.
+
+	  To compile this driver as a module, choose M here.
+	  The module will be called starfive-trng.
+
 endif # HW_RANDOM
 
 config UML_RANDOM
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
diff --git a/drivers/char/hw_random/starfive-trng.c b/drivers/char/hw_random/starfive-trng.c
new file mode 100644
index 000000000000..a6fb369de814
--- /dev/null
+++ b/drivers/char/hw_random/starfive-trng.c
@@ -0,0 +1,388 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * TRNG driver for the StarFive SoC
+ *
+ * Copyright (C) 2022 StarFive Technology Co.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/hw_random.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/random.h>
+#include <linux/reset.h>
+
+/* trng register offset */
+#define STARFIVE_CTRL			0x00
+#define STARFIVE_STAT			0x04
+#define STARFIVE_MODE			0x08
+#define STARFIVE_SMODE			0x0C
+#define STARFIVE_IE			0x10
+#define STARFIVE_ISTAT			0x14
+#define STARFIVE_RAND0			0x20
+#define STARFIVE_RAND1			0x24
+#define STARFIVE_RAND2			0x28
+#define STARFIVE_RAND3			0x2C
+#define STARFIVE_RAND4			0x30
+#define STARFIVE_RAND5			0x34
+#define STARFIVE_RAND6			0x38
+#define STARFIVE_RAND7			0x3C
+#define STARFIVE_AUTO_RQSTS		0x60
+#define STARFIVE_AUTO_AGE		0x64
+
+/* CTRL CMD */
+#define STARFIVE_CTRL_EXEC_NOP		0x0
+#define STARFIVE_CTRL_GENE_RANDNUM	0x1
+#define STARFIVE_CTRL_EXEC_RANDRESEED	0x2
+
+/* STAT */
+#define STARFIVE_STAT_NONCE_MODE	BIT(2)
+#define STARFIVE_STAT_R256		BIT(3)
+#define STARFIVE_STAT_MISSION_MODE	BIT(8)
+#define STARFIVE_STAT_SEEDED		BIT(9)
+#define STARFIVE_STAT_LAST_RESEED(x)	((x) << 16)
+#define STARFIVE_STAT_SRVC_RQST		BIT(27)
+#define STARFIVE_STAT_RAND_GENERATING	BIT(30)
+#define STARFIVE_STAT_RAND_SEEDING	BIT(31)
+
+/* MODE */
+#define STARFIVE_MODE_R256		BIT(3)
+
+/* SMODE */
+#define STARFIVE_SMODE_NONCE_MODE	BIT(2)
+#define STARFIVE_SMODE_MISSION_MODE	BIT(8)
+#define STARFIVE_SMODE_MAX_REJECTS(x)	((x) << 16)
+
+/* IE */
+#define STARFIVE_IE_RAND_RDY_EN		BIT(0)
+#define STARFIVE_IE_SEED_DONE_EN	BIT(1)
+#define STARFIVE_IE_LFSR_LOCKUP_EN	BIT(4)
+#define STARFIVE_IE_GLBL_EN		BIT(31)
+
+#define STARFIVE_IE_ALL			(STARFIVE_IE_GLBL_EN | \
+					 STARFIVE_IE_RAND_RDY_EN | \
+					 STARFIVE_IE_SEED_DONE_EN | \
+					 STARFIVE_IE_LFSR_LOCKUP_EN)
+
+/* ISTAT */
+#define STARFIVE_ISTAT_RAND_RDY		BIT(0)
+#define STARFIVE_ISTAT_SEED_DONE	BIT(1)
+#define STARFIVE_ISTAT_LFSR_LOCKUP	BIT(4)
+
+#define STARFIVE_RAND_LEN		sizeof(u32)
+
+#define to_trng(p)			container_of(p, struct starfive_trng, rng)
+
+enum reseed {
+	RANDOM_RESEED,
+	NONCE_RESEED,
+};
+
+enum mode {
+	PRNG_128BIT,
+	PRNG_256BIT,
+};
+
+struct starfive_trng {
+	struct device		*dev;
+	void __iomem		*base;
+	struct clk		*hclk;
+	struct clk		*ahb;
+	struct reset_control	*rst;
+	struct hwrng		rng;
+	struct completion	random_done;
+	struct completion	reseed_done;
+	u32			mode;
+	u32			mission;
+	u32			reseed;
+};
+
+static u16 autoreq;
+module_param(autoreq, ushort, 0);
+MODULE_PARM_DESC(autoreq, "Auto-reseeding after random number requests by host reaches specified counter:\n"
+				" 0 - disable counter\n"
+				" other - reload value for internal counter");
+
+static u16 autoage;
+module_param(autoage, ushort, 0);
+MODULE_PARM_DESC(autoage, "Auto-reseeding after specified timer countdowns to 0:\n"
+				" 0 - disable timer\n"
+				" other - reload value for internal timer");
+
+static inline int starfive_trng_wait_idle(struct starfive_trng *trng)
+{
+	u32 stat;
+
+	return readl_relaxed_poll_timeout(trng->base + STARFIVE_STAT, stat,
+					  !(stat & (STARFIVE_STAT_RAND_GENERATING |
+						    STARFIVE_STAT_RAND_SEEDING)),
+					  10, 100000);
+}
+
+static inline void starfive_trng_irq_mask_clear(struct starfive_trng *trng)
+{
+	/* clear register: ISTAT */
+	u32 data = readl(trng->base + STARFIVE_ISTAT);
+
+	writel(data, trng->base + STARFIVE_ISTAT);
+}
+
+static int starfive_trng_cmd(struct starfive_trng *trng, u32 cmd)
+{
+	int ret;
+
+	ret = starfive_trng_wait_idle(trng);
+	if (ret)
+		return -ETIMEDOUT;
+
+	switch (cmd) {
+	case STARFIVE_CTRL_EXEC_NOP:
+		writel(cmd, trng->base + STARFIVE_CTRL);
+		break;
+	case STARFIVE_CTRL_GENE_RANDNUM:
+		reinit_completion(&trng->random_done);
+		writel(cmd, trng->base + STARFIVE_CTRL);
+		ret = wait_for_completion_timeout(&trng->random_done, 3000);
+		if (!ret)
+			return -ETIMEDOUT;
+		break;
+	case STARFIVE_CTRL_EXEC_RANDRESEED:
+		reinit_completion(&trng->reseed_done);
+		writel(cmd, trng->base + STARFIVE_CTRL);
+		ret = wait_for_completion_timeout(&trng->reseed_done, 3000);
+		if (!ret)
+			return -ETIMEDOUT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int starfive_trng_init(struct hwrng *rng)
+{
+	struct starfive_trng *trng = to_trng(rng);
+	u32 mode, intr = 0;
+
+	/* disable Auto Request/Age register */
+	writel(autoage, trng->base + STARFIVE_AUTO_AGE);
+	writel(autoreq, trng->base + STARFIVE_AUTO_RQSTS);
+
+	/* clear register: ISTAT */
+	starfive_trng_irq_mask_clear(trng);
+
+	/* set smode/mode */
+	mode  = readl(trng->base + STARFIVE_MODE);
+
+	switch (trng->mode) {
+	case PRNG_128BIT:
+		mode &= ~STARFIVE_MODE_R256;
+		break;
+	case PRNG_256BIT:
+		mode |= STARFIVE_MODE_R256;
+		break;
+	default:
+		mode |= STARFIVE_MODE_R256;
+		break;
+	}
+
+	intr |= STARFIVE_IE_ALL;
+	writel(intr, trng->base + STARFIVE_IE);
+	writel(mode, trng->base + STARFIVE_MODE);
+
+	starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_NOP);
+
+	return starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_RANDRESEED);
+}
+
+static irqreturn_t starfive_trng_irq(int irq, void *priv)
+{
+	u32 status;
+	struct starfive_trng *trng = (struct starfive_trng *)priv;
+
+	status = readl(trng->base + STARFIVE_ISTAT);
+	if (status & STARFIVE_ISTAT_RAND_RDY) {
+		writel(STARFIVE_ISTAT_RAND_RDY, trng->base + STARFIVE_ISTAT);
+		complete(&trng->random_done);
+	}
+
+	if (status & STARFIVE_ISTAT_SEED_DONE) {
+		writel(STARFIVE_ISTAT_SEED_DONE, trng->base + STARFIVE_ISTAT);
+		complete(&trng->reseed_done);
+	}
+
+	if (status & STARFIVE_ISTAT_LFSR_LOCKUP) {
+		writel(STARFIVE_ISTAT_LFSR_LOCKUP, trng->base + STARFIVE_ISTAT);
+		starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_RANDRESEED);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void starfive_trng_cleanup(struct hwrng *rng)
+{
+	struct starfive_trng *trng = to_trng(rng);
+
+	writel(0, trng->base + STARFIVE_CTRL);
+
+	reset_control_assert(trng->rst);
+	clk_disable_unprepare(trng->hclk);
+	clk_disable_unprepare(trng->ahb);
+}
+
+static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
+{
+	struct starfive_trng *trng = to_trng(rng);
+	int ret;
+
+	pm_runtime_get_sync(trng->dev);
+
+	if (trng->mode == PRNG_256BIT)
+		max = min_t(size_t, max, (STARFIVE_RAND_LEN * 8));
+	else
+		max = min_t(size_t, max, (STARFIVE_RAND_LEN * 4));
+
+	ret = starfive_trng_cmd(trng, STARFIVE_CTRL_GENE_RANDNUM);
+	if (ret)
+		return ret;
+
+	memcpy_fromio(buf, trng->base + STARFIVE_RAND0, max);
+
+	pm_runtime_put_sync(trng->dev);
+
+	return max;
+}
+
+static int starfive_trng_probe(struct platform_device *pdev)
+{
+	int ret;
+	int irq;
+	struct starfive_trng *trng;
+
+	trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
+	if (!trng)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, trng);
+	trng->dev = &pdev->dev;
+
+	trng->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(trng->base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(trng->base),
+				     "Error remapping memory for platform device.\n");
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(&pdev->dev, irq, starfive_trng_irq, 0, pdev->name,
+			       (void *)trng);
+	if (ret)
+		return dev_err_probe(&pdev->dev, irq,
+				     "Failed to register interrupt handler\n");
+
+	trng->hclk = devm_clk_get(&pdev->dev, "hclk");
+	if (IS_ERR(trng->hclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(trng->hclk),
+				     "Error getting hardware reference clock\n");
+
+	trng->ahb = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(trng->ahb))
+		return dev_err_probe(&pdev->dev, PTR_ERR(trng->ahb),
+				     "Error getting ahb reference clock\n");
+
+	trng->rst = devm_reset_control_get_shared(&pdev->dev, NULL);
+	if (IS_ERR(trng->rst))
+		return dev_err_probe(&pdev->dev, PTR_ERR(trng->rst),
+				     "Error getting hardware reset line\n");
+
+	clk_prepare_enable(trng->hclk);
+	clk_prepare_enable(trng->ahb);
+	reset_control_deassert(trng->rst);
+
+	init_completion(&trng->random_done);
+	init_completion(&trng->reseed_done);
+
+	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);
+
+	reset_control_assert(trng->rst);
+	clk_disable_unprepare(trng->ahb);
+	clk_disable_unprepare(trng->hclk);
+
+	return ret;
+}
+
+static int __maybe_unused starfive_trng_suspend(struct device *dev)
+{
+	struct starfive_trng *trng = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(trng->hclk);
+	clk_disable_unprepare(trng->ahb);
+
+	return 0;
+}
+
+static int __maybe_unused starfive_trng_resume(struct device *dev)
+{
+	struct starfive_trng *trng = dev_get_drvdata(dev);
+
+	clk_prepare_enable(trng->hclk);
+	clk_prepare_enable(trng->ahb);
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(starfive_trng_pm_ops, starfive_trng_suspend,
+				starfive_trng_resume);
+
+static const struct of_device_id trng_dt_ids[] __maybe_unused = {
+	{ .compatible = "starfive,jh7110-trng" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, trng_dt_ids);
+
+static struct platform_driver starfive_trng_driver = {
+	.probe	= starfive_trng_probe,
+	.driver	= {
+		.name		= "starfive-trng",
+		.pm		= &starfive_trng_pm_ops,
+		.of_match_table	= of_match_ptr(trng_dt_ids),
+	},
+};
+
+module_platform_driver(starfive_trng_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("StarFive True Random Number Generator");
-- 
2.25.1

Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[Herbert Xu]

On Wed, Dec 28, 2022 at 03:11:02PM +0800, Jia Jie Ho wrote:
>
> +static int starfive_trng_cmd(struct starfive_trng *trng, u32 cmd)
> +{
> +	int ret;
> +
> +	ret = starfive_trng_wait_idle(trng);
> +	if (ret)
> +		return -ETIMEDOUT;
> +
> +	switch (cmd) {
> +	case STARFIVE_CTRL_EXEC_NOP:
> +		writel(cmd, trng->base + STARFIVE_CTRL);
> +		break;
> +	case STARFIVE_CTRL_GENE_RANDNUM:
> +		reinit_completion(&trng->random_done);
> +		writel(cmd, trng->base + STARFIVE_CTRL);
> +		ret = wait_for_completion_timeout(&trng->random_done, 3000);

Please don't use a constant jiffies value, because it may vary
in length.  Instead use a constant millisecond value and convert
it to jiffies.

> +static irqreturn_t starfive_trng_irq(int irq, void *priv)
> +{
> +	u32 status;
> +	struct starfive_trng *trng = (struct starfive_trng *)priv;
> +
> +	status = readl(trng->base + STARFIVE_ISTAT);
> +	if (status & STARFIVE_ISTAT_RAND_RDY) {
> +		writel(STARFIVE_ISTAT_RAND_RDY, trng->base + STARFIVE_ISTAT);
> +		complete(&trng->random_done);
> +	}
> +
> +	if (status & STARFIVE_ISTAT_SEED_DONE) {
> +		writel(STARFIVE_ISTAT_SEED_DONE, trng->base + STARFIVE_ISTAT);
> +		complete(&trng->reseed_done);
> +	}
> +
> +	if (status & STARFIVE_ISTAT_LFSR_LOCKUP) {
> +		writel(STARFIVE_ISTAT_LFSR_LOCKUP, trng->base + STARFIVE_ISTAT);
> +		starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_RANDRESEED);

You should not sleep in an IRQ handler.

> +static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wait)

You should respect the wait argument and not do polling/sleeping
if it is false.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[JiaJie Ho]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 6 January, 2023 4:39 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Wed, Dec 28, 2022 at 03:11:02PM +0800, Jia Jie Ho wrote:
> >
> > +static int starfive_trng_cmd(struct starfive_trng *trng, u32 cmd) {
> > +	int ret;
> > +
> > +	ret = starfive_trng_wait_idle(trng);
> > +	if (ret)
> > +		return -ETIMEDOUT;
> > +
> > +	switch (cmd) {
> > +	case STARFIVE_CTRL_EXEC_NOP:
> > +		writel(cmd, trng->base + STARFIVE_CTRL);
> > +		break;
> > +	case STARFIVE_CTRL_GENE_RANDNUM:
> > +		reinit_completion(&trng->random_done);
> > +		writel(cmd, trng->base + STARFIVE_CTRL);
> > +		ret = wait_for_completion_timeout(&trng->random_done,
> 3000);
> 
> Please don't use a constant jiffies value, because it may vary in length.
> Instead use a constant millisecond value and convert it to jiffies.
> 

I'll fix this in next version.

> > +static irqreturn_t starfive_trng_irq(int irq, void *priv) {
> > +	u32 status;
> > +	struct starfive_trng *trng = (struct starfive_trng *)priv;
> > +
> > +	status = readl(trng->base + STARFIVE_ISTAT);
> > +	if (status & STARFIVE_ISTAT_RAND_RDY) {
> > +		writel(STARFIVE_ISTAT_RAND_RDY, trng->base +
> STARFIVE_ISTAT);
> > +		complete(&trng->random_done);
> > +	}
> > +
> > +	if (status & STARFIVE_ISTAT_SEED_DONE) {
> > +		writel(STARFIVE_ISTAT_SEED_DONE, trng->base +
> STARFIVE_ISTAT);
> > +		complete(&trng->reseed_done);
> > +	}
> > +
> > +	if (status & STARFIVE_ISTAT_LFSR_LOCKUP) {
> > +		writel(STARFIVE_ISTAT_LFSR_LOCKUP, trng->base +
> STARFIVE_ISTAT);
> > +		starfive_trng_cmd(trng,
> STARFIVE_CTRL_EXEC_RANDRESEED);
> 
> You should not sleep in an IRQ handler.
> 

Will fix this too.

> > +static int starfive_trng_read(struct hwrng *rng, void *buf, size_t
> > +max, bool wait)
> 
> You should respect the wait argument and not do polling/sleeping if it is false.

I'll add this in the next version.

Thanks for reviewing this patch.

Best regards,
Jia Jie

RE: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[JiaJie Ho]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 6 January, 2023 4:39 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Wed, Dec 28, 2022 at 03:11:02PM +0800, Jia Jie Ho wrote:
> > +static int starfive_trng_read(struct hwrng *rng, void *buf, size_t
> > +max, bool wait)
> 
> You should respect the wait argument and not do polling/sleeping if it is false.
> 

Hi Herbert, 

My trng device requires sending a generate new number cmd before each read.
It then only populates the data registers with new random number and raise an interrupt when ready.
If user choose to not wait, they will always get stale bits. 
Is it okay to always return error if wait=false ?

Thanks
Jia Jie

Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[Herbert Xu]

On Mon, Jan 09, 2023 at 02:58:14AM +0000, JiaJie Ho wrote:
>
> My trng device requires sending a generate new number cmd before each read.
> It then only populates the data registers with new random number and raise an interrupt when ready.
> If user choose to not wait, they will always get stale bits. 
> Is it okay to always return error if wait=false ?

What is the length of the wait time? Is there an upper bound?
What is the average wait time?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[JiaJie Ho]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 9 January, 2023 11:04 AM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Mon, Jan 09, 2023 at 02:58:14AM +0000, JiaJie Ho wrote:
> >
> > My trng device requires sending a generate new number cmd before each
> read.
> > It then only populates the data registers with new random number and
> raise an interrupt when ready.
> > If user choose to not wait, they will always get stale bits.
> > Is it okay to always return error if wait=false ?
> 
> What is the length of the wait time? Is there an upper bound?
> What is the average wait time?
> 

The average wait time is around 20 microseconds.
I measured from writel cmd to wait_for_completion done.

Thanks
Jia Jie

Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[Herbert Xu]

On Mon, Jan 09, 2023 at 03:41:14AM +0000, JiaJie Ho wrote:
>
> The average wait time is around 20 microseconds.
> I measured from writel cmd to wait_for_completion done.

Do you know of an upper bound? E.g., if we limit it to 40us how
many requests would fail on average?

Having a maximum delay of 40us would be OK with wait == 0.  So
you could implement it in a way such that if the wait time exceeded
40us then you return if wait == 0, otherwise you can wait longer.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive SoC

[JiaJie Ho]

> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 9 January, 2023 12:00 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Mon, Jan 09, 2023 at 03:41:14AM +0000, JiaJie Ho wrote:
> >
> > The average wait time is around 20 microseconds.
> > I measured from writel cmd to wait_for_completion done.
> 
> Do you know of an upper bound? E.g., if we limit it to 40us how many
> requests would fail on average?
> 

I don't have the upper bound. 
I ran the rngtest with 1000 blocks, none of the wait time exceeded 30us.

> Having a maximum delay of 40us would be OK with wait == 0.  So you could
> implement it in a way such that if the wait time exceeded 40us then you
> return if wait == 0, otherwise you can wait longer.
> 

I'll do this then.

Thanks
Jia Jie

[PATCH v2 3/3] riscv: dts: starfive: Add TRNG node for VisionFive 2

[Jia Jie Ho]

Adding StarFive TRNG controller node to VisionFive 2 SoC.

Co-developed-by: Jenny Zhang <jenny.zhang@starfivetech.com>
Signed-off-by: Jenny Zhang <jenny.zhang@starfivetech.com>
Signed-off-by: Jia Jie Ho <jiajie.ho@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index 4ac159d79d66..3c29e0bc6246 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -455,5 +455,15 @@ uart5: serial@12020000 {
 			reg-shift = <2>;
 			status = "disabled";
 		};
+
+		rng: rng@1600c000 {
+			compatible = "starfive,jh7110-trng";
+			reg = <0x0 0x1600C000 0x0 0x4000>;
+			clocks = <&stgcrg JH7110_STGCLK_SEC_HCLK>,
+				 <&stgcrg JH7110_STGCLK_SEC_MISCAHB>;
+			clock-names = "hclk", "ahb";
+			resets = <&stgcrg JH7110_STGRST_SEC_TOP_HRESETN>;
+			interrupts = <30>;
+		};
 	};
 };
-- 
2.25.1