[PATCH v6 0/3] hwrng: add hwrng support for Rockchip RK3568

[PATCH v6 0/3] hwrng: add hwrng support for Rockchip RK3568

[Daniel Golle]

Rockchip SoCs used to have a random number generator as part of their
crypto device.

However newer Rockchip SoCs like the RK3568 have an independent True
Random Number Generator device. This patchset adds a driver for it and
enables it in the device tree.

v5 -> v6:
 * Patch 1: unchanged

 * Patch 2: get rid of #ifdef
   - use if (IS_ENABLED(...)) { ... }instead of #ifdef inside functions
   - use __maybe_unused for functions previously enclosed by #ifdef'ery

 * Patch 3: unchanged

v4 -> v5:
 * Patch 1: always use RK3568 name
   - use full RK3568 name in patch description
   - add RK3568 to title in binding

 * Patch 2: full name and cosmetics
   - also always mention RK3568 as there may be other RNG in other
     (future) Rockchip SoCs
   - remove debug output on successful probe
   - use MODULE_AUTHOR several times instead of single comma-separated

 * Patch 3: unchanged

v3 -> v4:
 * Patch 1: minor corrections
   - fix Rokchip -> Rockchip typo
   - change commit title as requested

 * Patch 2: improved error handling and resource management
   - Always use writel() instead of writel_relaxed()
   - Use pm_runtime_resume_and_get
   - Correctly return error code in rk_rng_read()
   - Make use of devm_reset_control_array_get_exclusive
   - Use devm_pm_runtime_enable and there by get rid of rk_rng_remove()

 * Patch 3:
   - Move node to conform with ordering by address

v2 -> v3: patch adopted by Daniel Golle
 * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS
   - improved description
   - meaningful clock-names
   - add entry in MAINTAINERS files

 * Patch 2: numerous code-style improvements
   - drop misleading rk_rng_write_ctl(), simplify I/O writes
   - drop unused TRNG_RNG_DOUT_[1-7] macros
   - handle error handling for pm_runtime_get_sync()
   - use memcpy_fromio() instead of open coding for-loop
   - some minor white-spaces fixes

 * Patch 3:
   - use clock-names as defined in dt-bindings

v1 -> v2:
 * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski:
   - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml
   - Fix binding title and description
   - Fix compatible property
   - Rename clocks and add the corresponding descriptions
   - Drop reset-names
   - Add a bus definition with #address-cells and #size-cells to the
     example.

 * Patch 2: fix issue reported by kernel test robot <lkp@intel.com>
   - Do not read the random registers as big endian, looking at the
     RK3568 TRM this is actually not needed. This fixes a sparse
     warning.

 * Patch 3: unchanged

Aurelien Jarno (3):
  dt-bindings: rng: Add Rockchip RK3568 TRNG
  hwrng: add hwrng driver for Rockchip RK3568 SoC
  arm64: dts: rockchip: add DT entry for RNG to RK356x

 .../bindings/rng/rockchip,rk3568-rng.yaml     |  61 +++++
 MAINTAINERS                                   |   7 +
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   9 +
 drivers/char/hw_random/Kconfig                |  14 ++
 drivers/char/hw_random/Makefile               |   1 +
 drivers/char/hw_random/rockchip-rng.c         | 220 ++++++++++++++++++
 6 files changed, 312 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

-- 
2.45.2

[PATCH v6 1/3] dt-bindings: rng: Add Rockchip RK3568 TRNG

[Daniel Golle]

From: Aurelien Jarno <aurelien@aurel32.net>

Add the True Random Number Generator on the Rockchip RK3568 SoC.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/rng/rockchip,rk3568-rng.yaml     | 61 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml

diff --git a/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
new file mode 100644
index 000000000000..e0595814a6d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rng/rockchip,rk3568-rng.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3568 TRNG
+
+description: True Random Number Generator on Rockchip RK3568 SoC
+
+maintainers:
+  - Aurelien Jarno <aurelien@aurel32.net>
+  - Daniel Golle <daniel@makrotopia.org>
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3568-rng
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: TRNG clock
+      - description: TRNG AHB clock
+
+  clock-names:
+    items:
+      - const: core
+      - const: ahb
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3568-cru.h>
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      rng@fe388000 {
+        compatible = "rockchip,rk3568-rng";
+        reg = <0x0 0xfe388000 0x0 0x4000>;
+        clocks = <&cru CLK_TRNG_NS>, <&cru HCLK_TRNG_NS>;
+        clock-names = "core", "ahb";
+        resets = <&cru SRST_TRNG_NS>;
+      };
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index fb1df8c29f5a..7b2b8b1f526c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19693,6 +19693,12 @@ F:	Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
 F:	drivers/media/platform/rockchip/rkisp1
 F:	include/uapi/linux/rkisp1-config.h
 
+ROCKCHIP RK3568 RANDOM NUMBER GENERATOR SUPPORT
+M:	Daniel Golle <daniel@makrotopia.org>
+M:	Aurelien Jarno <aurelien@aurel32.net>
+S:	Maintained
+F:	Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
+
 ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
 M:	Jacob Chen <jacob-chen@iotwrt.com>
 M:	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
-- 
2.45.2

[PATCH v6 2/3] hwrng: add hwrng driver for Rockchip RK3568 SoC

[Daniel Golle]

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 RK3568 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>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 MAINTAINERS                           |   1 +
 drivers/char/hw_random/Kconfig        |  14 ++
 drivers/char/hw_random/Makefile       |   1 +
 drivers/char/hw_random/rockchip-rng.c | 220 ++++++++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7b2b8b1f526c..2745cfe56774 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19698,6 +19698,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..7e45e9880e3a
--- /dev/null
+++ b/drivers/char/hw_random/rockchip-rng.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * rockchip-rng.c True Random Number Generator driver for Rockchip RK3568 SoC
+ *
+ * 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.h>
+#include <linux/platform_device.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(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(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_resume_and_get((struct device *) rk_rng->rng.priv);
+	if (ret < 0)
+		return ret;
+
+	/* Start collecting random data */
+	writel(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 (ret < 0) ? ret : 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_exclusive(&pdev->dev);
+	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);
+	if (!IS_ENABLED(CONFIG_PM)) {
+		rk_rng->rng.init = rk_rng_init;
+		rk_rng->rng.cleanup = rk_rng_cleanup;
+	}
+	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);
+	devm_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");
+
+	return 0;
+}
+
+static int __maybe_unused 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 __maybe_unused rk_rng_runtime_resume(struct device *dev)
+{
+	struct rk_rng *rk_rng = dev_get_drvdata(dev);
+
+	return rk_rng_init(&rk_rng->rng);
+}
+
+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,
+};
+
+module_platform_driver(rk_rng_driver);
+
+MODULE_DESCRIPTION("Rockchip RK3568 True Random Number Generator driver");
+MODULE_AUTHOR("Lin Jinhan <troy.lin@rock-chips.com>");
+MODULE_AUTHOR("Aurelien Jarno <aurelien@aurel32.net>");
+MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
+MODULE_LICENSE("GPL");
-- 
2.45.2

[PATCH v6 3/3] arm64: dts: rockchip: add DT entry for RNG to RK356x

[Daniel Golle]

From: Aurelien Jarno <aurelien@aurel32.net>

Enable the just added Rockchip RNG driver for RK356x SoCs.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 4690be841a1c..78377e7d1d6d 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -1113,6 +1113,15 @@ sdhci: mmc@fe310000 {
 		status = "disabled";
 	};
 
+	rng: rng@fe388000 {
+		compatible = "rockchip,rk3568-rng";
+		reg = <0x0 0xfe388000 0x0 0x4000>;
+		clocks = <&cru CLK_TRNG_NS>, <&cru HCLK_TRNG_NS>;
+		clock-names = "core", "ahb";
+		resets = <&cru SRST_TRNG_NS>;
+		reset-names = "reset";
+	};
+
 	i2s0_8ch: i2s@fe400000 {
 		compatible = "rockchip,rk3568-i2s-tdm";
 		reg = <0x0 0xfe400000 0x0 0x1000>;
-- 
2.45.2

Re: [PATCH v6 0/3] hwrng: add hwrng support for Rockchip RK3568

[Chen-Yu Tsai]

Hi,

On Sat, Jul 13, 2024 at 8:38 AM Daniel Golle <daniel@makrotopia.org> wrote:
>
> Rockchip SoCs used to have a random number generator as part of their
> crypto device.
>
> However newer Rockchip SoCs like the RK3568 have an independent True
> Random Number Generator device. This patchset adds a driver for it and
> enables it in the device tree.
>

Have you tested any of the later iterations? For me it stopped working
at v3. After v3 (including v3), all it spits out is zeros.

> v5 -> v6:
>  * Patch 1: unchanged
>
>  * Patch 2: get rid of #ifdef
>    - use if (IS_ENABLED(...)) { ... }instead of #ifdef inside functions
>    - use __maybe_unused for functions previously enclosed by #ifdef'ery
>
>  * Patch 3: unchanged
>
> v4 -> v5:
>  * Patch 1: always use RK3568 name
>    - use full RK3568 name in patch description
>    - add RK3568 to title in binding
>
>  * Patch 2: full name and cosmetics
>    - also always mention RK3568 as there may be other RNG in other
>      (future) Rockchip SoCs
>    - remove debug output on successful probe
>    - use MODULE_AUTHOR several times instead of single comma-separated
>
>  * Patch 3: unchanged
>
> v3 -> v4:
>  * Patch 1: minor corrections
>    - fix Rokchip -> Rockchip typo
>    - change commit title as requested
>
>  * Patch 2: improved error handling and resource management
>    - Always use writel() instead of writel_relaxed()
>    - Use pm_runtime_resume_and_get
>    - Correctly return error code in rk_rng_read()
>    - Make use of devm_reset_control_array_get_exclusive
>    - Use devm_pm_runtime_enable and there by get rid of rk_rng_remove()
>
>  * Patch 3:
>    - Move node to conform with ordering by address
>
> v2 -> v3: patch adopted by Daniel Golle
>  * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS
>    - improved description
>    - meaningful clock-names
>    - add entry in MAINTAINERS files
>
>  * Patch 2: numerous code-style improvements
>    - drop misleading rk_rng_write_ctl(), simplify I/O writes

This is probably the culprit. The RNG and RST control registers have
enable bits in their top 16 bits. Without those set together with the
actual bit values, the writes to the registers have no effect.

Please check all your writel calls against the TRM and add appropriate
bitmasks for the upper 16 bits.


ChenYu

>    - drop unused TRNG_RNG_DOUT_[1-7] macros
>    - handle error handling for pm_runtime_get_sync()
>    - use memcpy_fromio() instead of open coding for-loop
>    - some minor white-spaces fixes
>
>  * Patch 3:
>    - use clock-names as defined in dt-bindings
>
> v1 -> v2:
>  * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski:
>    - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml
>    - Fix binding title and description
>    - Fix compatible property
>    - Rename clocks and add the corresponding descriptions
>    - Drop reset-names
>    - Add a bus definition with #address-cells and #size-cells to the
>      example.
>
>  * Patch 2: fix issue reported by kernel test robot <lkp@intel.com>
>    - Do not read the random registers as big endian, looking at the
>      RK3568 TRM this is actually not needed. This fixes a sparse
>      warning.
>
>  * Patch 3: unchanged
>
> Aurelien Jarno (3):
>   dt-bindings: rng: Add Rockchip RK3568 TRNG
>   hwrng: add hwrng driver for Rockchip RK3568 SoC
>   arm64: dts: rockchip: add DT entry for RNG to RK356x
>
>  .../bindings/rng/rockchip,rk3568-rng.yaml     |  61 +++++
>  MAINTAINERS                                   |   7 +
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   9 +
>  drivers/char/hw_random/Kconfig                |  14 ++
>  drivers/char/hw_random/Makefile               |   1 +
>  drivers/char/hw_random/rockchip-rng.c         | 220 ++++++++++++++++++
>  6 files changed, 312 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
>
> --
> 2.45.2
>

Re: [PATCH v6 0/3] hwrng: add hwrng support for Rockchip RK3568

[Daniel Golle]

Hi Chen-Yu,

thank you for reviewing and testing.

On Sat, Jul 13, 2024 at 02:48:39PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Sat, Jul 13, 2024 at 8:38 AM Daniel Golle <daniel@makrotopia.org> wrote:
> >
> > Rockchip SoCs used to have a random number generator as part of their
> > crypto device.
> >
> > However newer Rockchip SoCs like the RK3568 have an independent True
> > Random Number Generator device. This patchset adds a driver for it and
> > enables it in the device tree.
> >
> 
> Have you tested any of the later iterations? For me it stopped working
> at v3. After v3 (including v3), all it spits out is zeros.

I've not examined the actual values it returns, I will do so in future
iterations.

Alsom, I misread the original rk_rng_write_ctl() function, I will bring
it back and also improve the comment describing it.

> 
> > v5 -> v6:
> >  * Patch 1: unchanged
> >
> >  * Patch 2: get rid of #ifdef
> >    - use if (IS_ENABLED(...)) { ... }instead of #ifdef inside functions
> >    - use __maybe_unused for functions previously enclosed by #ifdef'ery
> >
> >  * Patch 3: unchanged
> >
> > v4 -> v5:
> >  * Patch 1: always use RK3568 name
> >    - use full RK3568 name in patch description
> >    - add RK3568 to title in binding
> >
> >  * Patch 2: full name and cosmetics
> >    - also always mention RK3568 as there may be other RNG in other
> >      (future) Rockchip SoCs
> >    - remove debug output on successful probe
> >    - use MODULE_AUTHOR several times instead of single comma-separated
> >
> >  * Patch 3: unchanged
> >
> > v3 -> v4:
> >  * Patch 1: minor corrections
> >    - fix Rokchip -> Rockchip typo
> >    - change commit title as requested
> >
> >  * Patch 2: improved error handling and resource management
> >    - Always use writel() instead of writel_relaxed()
> >    - Use pm_runtime_resume_and_get
> >    - Correctly return error code in rk_rng_read()
> >    - Make use of devm_reset_control_array_get_exclusive
> >    - Use devm_pm_runtime_enable and there by get rid of rk_rng_remove()
> >
> >  * Patch 3:
> >    - Move node to conform with ordering by address
> >
> > v2 -> v3: patch adopted by Daniel Golle
> >  * Patch 1: address comments of Krzysztof Kozlowski, add MAINTAINERS
> >    - improved description
> >    - meaningful clock-names
> >    - add entry in MAINTAINERS files
> >
> >  * Patch 2: numerous code-style improvements
> >    - drop misleading rk_rng_write_ctl(), simplify I/O writes
> 
> This is probably the culprit. The RNG and RST control registers have
> enable bits in their top 16 bits. Without those set together with the
> actual bit values, the writes to the registers have no effect.
> 
> Please check all your writel calls against the TRM and add appropriate
> bitmasks for the upper 16 bits.

The upper 16 bits are apparently used as hardware mask when writing the
lower 16 bits...

I will send v7 after testing.


> 
> 
> ChenYu
> 
> >    - drop unused TRNG_RNG_DOUT_[1-7] macros
> >    - handle error handling for pm_runtime_get_sync()
> >    - use memcpy_fromio() instead of open coding for-loop
> >    - some minor white-spaces fixes
> >
> >  * Patch 3:
> >    - use clock-names as defined in dt-bindings
> >
> > v1 -> v2:
> >  * Patch 1: fix issues reported by Rob Herring and Krzysztof Kozlowski:
> >    - Rename rockchip-rng.yaml into rockchip,rk3568-rng.yaml
> >    - Fix binding title and description
> >    - Fix compatible property
> >    - Rename clocks and add the corresponding descriptions
> >    - Drop reset-names
> >    - Add a bus definition with #address-cells and #size-cells to the
> >      example.
> >
> >  * Patch 2: fix issue reported by kernel test robot <lkp@intel.com>
> >    - Do not read the random registers as big endian, looking at the
> >      RK3568 TRM this is actually not needed. This fixes a sparse
> >      warning.
> >
> >  * Patch 3: unchanged
> >
> > Aurelien Jarno (3):
> >   dt-bindings: rng: Add Rockchip RK3568 TRNG
> >   hwrng: add hwrng driver for Rockchip RK3568 SoC
> >   arm64: dts: rockchip: add DT entry for RNG to RK356x
> >
> >  .../bindings/rng/rockchip,rk3568-rng.yaml     |  61 +++++
> >  MAINTAINERS                                   |   7 +
> >  arch/arm64/boot/dts/rockchip/rk356x.dtsi      |   9 +
> >  drivers/char/hw_random/Kconfig                |  14 ++
> >  drivers/char/hw_random/Makefile               |   1 +
> >  drivers/char/hw_random/rockchip-rng.c         | 220 ++++++++++++++++++
> >  6 files changed, 312 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> >  create mode 100644 drivers/char/hw_random/rockchip-rng.c
> >
> > --
> > 2.45.2
> >

Re: [PATCH v6 2/3] hwrng: add hwrng driver for Rockchip RK3568 SoC

[Kamlesh Gurudasani]

Daniel Golle <daniel@makrotopia.org> writes:

> This message was sent from outside of Texas Instruments. 
> Do not click links or open attachments unless you recognize the source of this email and know the content is safe. If you wish
> to report this message to IT Security, please forward the message as an attachment to phishing@list.ti.com 
>  
> 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 RK3568 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>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  MAINTAINERS                           |   1 +
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 220 ++++++++++++++++++++++++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7b2b8b1f526c..2745cfe56774 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19698,6 +19698,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..7e45e9880e3a
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rockchip-rng.c True Random Number Generator driver for Rockchip RK3568 SoC
> + *
> + * 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.h>
> +#include <linux/platform_device.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(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(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_resume_and_get((struct device *) rk_rng->rng.priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Start collecting random data */
> +	writel(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 (ret < 0) ? ret : to_read;
Do we really need to check ret < 0 here?
readl_poll_timeout returns either 0 or -ve.

return ret ? ret : to_read;
would work as well. we save extra evaluation of (ret<0).

Let me know what you think.

Kamlesh

> +}
> +
> +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_exclusive(&pdev->dev);
> +	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);
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		rk_rng->rng.init = rk_rng_init;
> +		rk_rng->rng.cleanup = rk_rng_cleanup;
> +	}
> +	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);
> +	devm_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");
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused 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 __maybe_unused rk_rng_runtime_resume(struct device *dev)
> +{
> +	struct rk_rng *rk_rng = dev_get_drvdata(dev);
> +
> +	return rk_rng_init(&rk_rng->rng);
> +}
> +
> +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,
> +};
> +
> +module_platform_driver(rk_rng_driver);
> +
> +MODULE_DESCRIPTION("Rockchip RK3568 True Random Number Generator driver");
> +MODULE_AUTHOR("Lin Jinhan <troy.lin@rock-chips.com>");
> +MODULE_AUTHOR("Aurelien Jarno <aurelien@aurel32.net>");
> +MODULE_AUTHOR("Daniel Golle <daniel@makrotopia.org>");
> +MODULE_LICENSE("GPL");
> -- 
> 2.45.2