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

[PATCH v4 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, 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 patchset adds a driver for it and
enable it in the device tree.

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 RNG bindings
  hwrng: add Rockchip SoC hwrng driver
  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         | 222 ++++++++++++++++++
 6 files changed, 314 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 v4 1/3] dt-bindings: rng: Add Rockchip RNG bindings

[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>
---
 .../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..ad3648b96f82
--- /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 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 807feae089c4..5cd3bc2b034f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19498,6 +19498,12 @@ F:	Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst
 F:	drivers/media/platform/rockchip/rkisp1
 F:	include/uapi/linux/rkisp1-config.h
 
+ROCKCHIP 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 v4 2/3] hwrng: add Rockchip SoC hwrng driver

[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 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 | 222 ++++++++++++++++++++++++++
 4 files changed, 238 insertions(+)
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5cd3bc2b034f..580536b72d25 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19503,6 +19503,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..714d177ac45e
--- /dev/null
+++ b/drivers/char/hw_random/rockchip-rng.c
@@ -0,0 +1,222 @@
+// 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.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);
+#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);
+	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");
+
+	dev_dbg(&pdev->dev, "Registered Rockchip hwrng\n");
+
+	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,
+};
+
+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

[PATCH v4 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 d8543b5557ee..d43d59b44ea5 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -1112,6 +1112,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 v4 2/3] hwrng: add Rockchip SoC hwrng driver

[Krzysztof Kozlowski]

On 23/06/2024 05:33, Daniel Golle wrote:
> +
> +	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;

I doubt in this value. Usually SoC vendors do not provide datasheet with
any reliable and verifiable (so one which could be proven by 3rd party)
information. Can you provide a source? (and vendor downstream tree does
not really count)

> +
> +	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");
> +
> +	dev_dbg(&pdev->dev, "Registered Rockchip hwrng\n");

Drop, it is not useful at all. Srsly, we had already long enough talk,
which wasted time of three people. Why do you insist on wasting more?

There is no single benefit of such debug statement. sysfs already
provides you this information. Simple entry/exit  is provided by
tracing. You duplicate existing interfaces without any benefit, because
this prints nothing more.

Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: rng: Add Rockchip RNG bindings

[Krzysztof Kozlowski]

On 23/06/2024 05:32, Daniel Golle wrote:
> 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>

My comments from v2, which I reminded at v3, were not addressed.

Respond to each of them and acknowledge that you are going to implement
the change.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] hwrng: add Rockchip SoC hwrng driver

[Uwe Kleine-König]

Hello Krzysztof,

On 6/23/24 09:00, Krzysztof Kozlowski wrote:
> On 23/06/2024 05:33, Daniel Golle wrote:
>> +
>> +	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");
>> +
>> +	dev_dbg(&pdev->dev, "Registered Rockchip hwrng\n");
> 
> Drop, it is not useful at all. Srsly, we had already long enough talk,
 > [...]

And in this long talk using dev_dbg() was one of the suggestions for a 
compromise. For me this is ok.

> There is no single benefit of such debug statement. sysfs already
> provides you this information. Simple entry/exit  is provided by
> tracing. You duplicate existing interfaces without any benefit, because
> this prints nothing more.

There might be a (small) value if you want to know when during boot the 
device becomes available. So having a dev_dbg() that can be enabled 
dynamically (assuming DYNAMIC_DEBUG=y) and isn't in the way otherwise 
might be justified. IMHO a dev_dbg is lightweight enough that *I* won't 
continue the discussion.

Best regards
Uwe

Re: [PATCH v4 2/3] hwrng: add Rockchip SoC hwrng driver

[Aurelien Jarno]

Hi,

On 2024-06-23 09:00, Krzysztof Kozlowski wrote:
> On 23/06/2024 05:33, Daniel Golle wrote:
> > +
> > +	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;
> 
> I doubt in this value. Usually SoC vendors do not provide datasheet with
> any reliable and verifiable (so one which could be proven by 3rd party)
> information. Can you provide a source? (and vendor downstream tree does
> not really count)

As the original author of the patch, I am the one who have chosen the
value. I did it as explained in the commit message:

| 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.

It is also explained, admittedly more briefly, above the
RK_RNG_SAMPLE_CNT #define, as the commit messages are not really
relevant anymore once the patches are accepted:

| * 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).
| */

The decision to adjust RK_RNG_SAMPLE_CNT to reach ~90% of FIPS 140-2
successes was based on the quality chosen by most hw_random drivers
currently in the kernel sources. The FIPS 140-2 tests were performed
using rngtest from the rng-tools project.

All that said, I am not an expert in that domain, so feel free to point
to the documentation or provide the correct method to determine the
quality.

Regards
Aurelien

[1] https://git.kernel.org/pub/scm/utils/kernel/rng-tools/rng-tools.git/

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                     http://aurel32.net

Re: [PATCH v4 2/3] hwrng: add Rockchip SoC hwrng driver

[Dragan Simic]

Hello Uwe,

On 2024-06-23 11:46, Uwe Kleine-König wrote:
> On 6/23/24 09:00, Krzysztof Kozlowski wrote:
>> On 23/06/2024 05:33, Daniel Golle wrote:
>>> +
>>> +	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");
>>> +
>>> +	dev_dbg(&pdev->dev, "Registered Rockchip hwrng\n");
>> 
>> Drop, it is not useful at all. Srsly, we had already long enough talk,
>> [...]
> 
> And in this long talk using dev_dbg() was one of the suggestions for a
> compromise. For me this is ok.
> 
>> There is no single benefit of such debug statement. sysfs already
>> provides you this information. Simple entry/exit  is provided by
>> tracing. You duplicate existing interfaces without any benefit, 
>> because
>> this prints nothing more.
> 
> There might be a (small) value if you want to know when during boot
> the device becomes available. So having a dev_dbg() that can be
> enabled dynamically (assuming DYNAMIC_DEBUG=y) and isn't in the way
> otherwise might be justified. IMHO a dev_dbg is lightweight enough
> that *I* won't continue the discussion.

For anyone interested, below is an example that shows the usability
of knowing when a device becomes available:

https://gitlab.manjaro.org/manjaro-arm/packages/core/linux/-/issues/21

Re: [PATCH v4 1/3] dt-bindings: rng: Add Rockchip RNG bindings

[Daniel Golle]

Hi Krzysztof,

thank you for your patiente and repeated review of this series.

On Sun, Jun 23, 2024 at 09:03:15AM +0200, Krzysztof Kozlowski wrote:
> On 23/06/2024 05:32, Daniel Golle wrote:
> > 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>
> 
> My comments from v2, which I reminded at v3, were not addressed.
> 
> Respond to each of them and acknowledge that you are going to implement
> the change.

Your comments to v1which I'm aware of are:
https://patchwork.kernel.org/comment/25087874/

> > +++ b/Documentation/devicetree/bindings/rng/rockchip-rng.yaml
> Filename matching compatible, so "rockchip,rk3568-rng.yaml"

I've changed the filename.

> > +title: Rockchip TRNG bindings

> Drop "bindings"

I've changed the title accordingly (now: "Rockchip TRNG" in v4).

> > +description:
> > +  This driver interface with the True Random Number Generator present in some
> 
> Drop "This driver interface" and make it a proper sentence. Bindings are
> not about drivers.

This has been addressed by Aurelien and further improved by me in v3.

> > +  clocks:
> > +    minItems: 2

> Drop minItems.

Aurelien did that in v2.

> > +  clock-names:
> > +    items:
> > +      - const: clk
> > +      - const: hclk
> 
> You need to explain what are these in clocks. Also you need better
> names. A clock name "clk" is useless.

Clocks now have meaningful names and descriptions.

> > +  reset-names:
> > +    items:
> > +      - const: reset
> 
> Drop reset-names entirely, not useful.

Aurelien did so in v2.

Your comments to v2 which I'm aware of are:
https://patchwork.kernel.org/comment/25111597/

> > Add the RNG bindings for the RK3568 SoC from Rockchip

> Use subject prefixes matching the subsystem (git log --oneline -- ...),
> so it is rng, not RNG. Also, you are not adding all-Rockhip RNG but a
> specific device.
> 
> Subject: drop second, redundant "bindings".

I've changed 'RNG' into 'rng' in the subject and spelled it out in the
commit message.

> > +description: True Random Number Generator for some Rockchip SoCs
> 
> s/for some Rockchip SoCs/on Rokchip RK3568 SoC/

I've adopted your suggestion in v3 and then fixed the typo in v4.

> 
> > +  clock-names:
> > +    items:
> > +      - const: trng_clk
> > +      - const: trng_hclk

> These are too vague names. Everything is a clk in clock-names, so no
> need usually to add it as name suffix. Give them some descriptive names,
> e.g. core and ahb.

If changed the names to the suggested 'core' and 'ahb'.

Before sending another round of patches, just to make sure we are on
the same page, please confirm that what remains is
Subject: dt-bindings: rng: Add Rockchip RNG bindings
which not only should be 'rng' in small letters but also name the exact
chip, eg.:
Subject: dt-bindings: rng: add TRNG on the Rockchip RK3568 SoC

If there are any other comments you made which I'm not aware of, please
point me to them.


Cheers


Daniel

Re: [PATCH v4 2/3] hwrng: add Rockchip SoC hwrng driver

[Anand Moon]

Hi,

On Sun, 23 Jun 2024 at 16:13, Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> Hi,
>
> On 2024-06-23 09:00, Krzysztof Kozlowski wrote:
> > On 23/06/2024 05:33, Daniel Golle wrote:
> > > +
> > > +   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;
> >
> > I doubt in this value. Usually SoC vendors do not provide datasheet with
> > any reliable and verifiable (so one which could be proven by 3rd party)
> > information. Can you provide a source? (and vendor downstream tree does
> > not really count)
>
> As the original author of the patch, I am the one who have chosen the
> value. I did it as explained in the commit message:
>
> | 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.
>
> It is also explained, admittedly more briefly, above the
> RK_RNG_SAMPLE_CNT #define, as the commit messages are not really
> relevant anymore once the patches are accepted:
>
> | * 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).
> | */
>
> The decision to adjust RK_RNG_SAMPLE_CNT to reach ~90% of FIPS 140-2
> successes was based on the quality chosen by most hw_random drivers
> currently in the kernel sources. The FIPS 140-2 tests were performed
> using rngtest from the rng-tools project.
>
> All that said, I am not an expert in that domain, so feel free to point
> to the documentation or provide the correct method to determine the
> quality.
>
> Regards
> Aurelien
>
> [1] https://git.kernel.org/pub/scm/utils/kernel/rng-tools/rng-tools.git/

This is an old repository, the latest can be found below
[1] https://github.com/nhorman/rng-tools

However, I could not find the support from ARM and ARM64 in the repository below
because all the assembly is written for the X86 arch.

Thanks

-Anand

Re: [PATCH v4 1/3] dt-bindings: rng: Add Rockchip RNG bindings

[Krzysztof Kozlowski]

On 23/06/2024 15:08, Daniel Golle wrote:
> Hi Krzysztof,
> 
> thank you for your patiente and repeated review of this series.
> 
> On Sun, Jun 23, 2024 at 09:03:15AM +0200, Krzysztof Kozlowski wrote:
>> On 23/06/2024 05:32, Daniel Golle wrote:
>>> 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>
>>
>> My comments from v2, which I reminded at v3, were not addressed.
>>
>> Respond to each of them and acknowledge that you are going to implement
>> the change.
> 
> Your comments to v1which I'm aware of are:
> https://patchwork.kernel.org/comment/25087874/

We talk about comments from v2, not v1.

> 
>>> +++ b/Documentation/devicetree/bindings/rng/rockchip-rng.yaml
>> Filename matching compatible, so "rockchip,rk3568-rng.yaml"
> 
> I've changed the filename.
> 
>>> +title: Rockchip TRNG bindings
> 
>> Drop "bindings"
> 
> I've changed the title accordingly (now: "Rockchip TRNG" in v4).
> 
>>> +description:
>>> +  This driver interface with the True Random Number Generator present in some
>>
>> Drop "This driver interface" and make it a proper sentence. Bindings are
>> not about drivers.
> 
> This has been addressed by Aurelien and further improved by me in v3.
> 
>>> +  clocks:
>>> +    minItems: 2
> 
>> Drop minItems.
> 
> Aurelien did that in v2.
> 
>>> +  clock-names:
>>> +    items:
>>> +      - const: clk
>>> +      - const: hclk
>>
>> You need to explain what are these in clocks. Also you need better
>> names. A clock name "clk" is useless.
> 
> Clocks now have meaningful names and descriptions.
> 
>>> +  reset-names:
>>> +    items:
>>> +      - const: reset
>>
>> Drop reset-names entirely, not useful.
> 
> Aurelien did so in v2.
> 
> Your comments to v2 which I'm aware of are:
> https://patchwork.kernel.org/comment/25111597/
> 
>>> Add the RNG bindings for the RK3568 SoC from Rockchip
> 
>> Use subject prefixes matching the subsystem (git log --oneline -- ...),
>> so it is rng, not RNG. Also, you are not adding all-Rockhip RNG but a
>> specific device.
>>
>> Subject: drop second, redundant "bindings".
> 
> I've changed 'RNG' into 'rng' in the subject and spelled it out in the
> commit message.

And where did you drop the redundant bindings? You just quoted the
sentence and ignored it.


Best regards,
Krzysztof