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

[PATCH v3 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.

v2 of this patchset has been submitted by Aurelien Jarno in November
2022. A follow-up submission addressing the comments received for v2
never happened.

As I believe that using the TRNG is generally desireable as it reduces
the time needed to boot significantly as userspace no longer waits due
blocking read of /dev/random while still lacking entropy I have picked
up Aurelien's previous work and completed it.

Link to v2: https://patchwork.kernel.org/project/linux-arm-kernel/cover/20221128184718.1963353-1-aurelien@aurel32.net/

v2 -> v3:
 * 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     |  60 +++++
 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         | 229 ++++++++++++++++++
 6 files changed, 320 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 v3 1/3] dt-bindings: RNG: Add Rockchip RNG bindings

[Daniel Golle]

From: Aurelien Jarno <aurelien@aurel32.net>

Add the RNG bindings for the RK3568 SoC from Rockchip

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 66 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..d45f03683fbe
--- /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 Rokchip 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 f45881e982d2..77d449c89bf2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19488,6 +19488,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 v3 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 | 229 ++++++++++++++++++++++++++
 4 files changed, 245 insertions(+)
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 77d449c89bf2..299b8c1a5fb5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19493,6 +19493,7 @@ M:	Daniel Golle <daniel@makrotopia.org>
 M:	Aurelien Jarno <aurelien@aurel32.net>
 S:	Maintained
 F:	Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
+F:	drivers/char/hw_random/rockchip-rng.c
 
 ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
 M:	Jacob Chen <jacob-chen@iotwrt.com>
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 442c40efb200..2b62cd08f91a 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -573,6 +573,20 @@ config HW_RANDOM_JH7110
 	  To compile this driver as a module, choose M here.
 	  The module will be called jh7110-trng.
 
+config HW_RANDOM_ROCKCHIP
+	tristate "Rockchip True Random Number Generator"
+	depends on HW_RANDOM && (ARCH_ROCKCHIP || COMPILE_TEST)
+	depends on HAS_IOMEM
+	default HW_RANDOM
+	help
+	  This driver provides kernel-side support for the True Random Number
+	  Generator hardware found on some Rockchip SoC like RK3566 or RK3568.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called rockchip-rng.
+
+	  If unsure, say Y.
+
 endif # HW_RANDOM
 
 config UML_RANDOM
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 32549a1186dc..01f012eab440 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -48,4 +48,5 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-trng.o
 obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
 obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
 obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
+obj-$(CONFIG_HW_RANDOM_ROCKCHIP) += rockchip-rng.o
 obj-$(CONFIG_HW_RANDOM_JH7110) += jh7110-trng.o
diff --git a/drivers/char/hw_random/rockchip-rng.c b/drivers/char/hw_random/rockchip-rng.c
new file mode 100644
index 000000000000..6070abb73847
--- /dev/null
+++ b/drivers/char/hw_random/rockchip-rng.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * rockchip-rng.c True Random Number Generator driver for Rockchip SoCs
+ *
+ * Copyright (c) 2018, Fuzhou Rockchip Electronics Co., Ltd.
+ * Copyright (c) 2022, Aurelien Jarno
+ * Authors:
+ *  Lin Jinhan <troy.lin@rock-chips.com>
+ *  Aurelien Jarno <aurelien@aurel32.net>
+ */
+#include <linux/clk.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define RK_RNG_AUTOSUSPEND_DELAY	100
+#define RK_RNG_MAX_BYTE			32
+#define RK_RNG_POLL_PERIOD_US		100
+#define RK_RNG_POLL_TIMEOUT_US		10000
+
+/*
+ * TRNG collects osc ring output bit every RK_RNG_SAMPLE_CNT time. The value is
+ * a tradeoff between speed and quality and has been adjusted to get a quality
+ * of ~900 (~90% of FIPS 140-2 successes).
+ */
+#define RK_RNG_SAMPLE_CNT		1000
+
+/* TRNG registers from RK3568 TRM-Part2, section 5.4.1 */
+#define TRNG_RST_CTL			0x0004
+#define TRNG_RNG_CTL			0x0400
+#define TRNG_RNG_CTL_LEN_64_BIT		(0x00 << 4)
+#define TRNG_RNG_CTL_LEN_128_BIT	(0x01 << 4)
+#define TRNG_RNG_CTL_LEN_192_BIT	(0x02 << 4)
+#define TRNG_RNG_CTL_LEN_256_BIT	(0x03 << 4)
+#define TRNG_RNG_CTL_OSC_RING_SPEED_0	(0x00 << 2)
+#define TRNG_RNG_CTL_OSC_RING_SPEED_1	(0x01 << 2)
+#define TRNG_RNG_CTL_OSC_RING_SPEED_2	(0x02 << 2)
+#define TRNG_RNG_CTL_OSC_RING_SPEED_3	(0x03 << 2)
+#define TRNG_RNG_CTL_ENABLE		BIT(1)
+#define TRNG_RNG_CTL_START		BIT(0)
+#define TRNG_RNG_SAMPLE_CNT		0x0404
+#define TRNG_RNG_DOUT			0x0410
+
+struct rk_rng {
+	struct hwrng rng;
+	void __iomem *base;
+	struct reset_control *rst;
+	int clk_num;
+	struct clk_bulk_data *clk_bulks;
+};
+
+static int rk_rng_init(struct hwrng *rng)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+	int ret;
+
+	/* start clocks */
+	ret = clk_bulk_prepare_enable(rk_rng->clk_num, rk_rng->clk_bulks);
+	if (ret < 0) {
+		dev_err((struct device *) rk_rng->rng.priv,
+			"Failed to enable clks %d\n", ret);
+		return ret;
+	}
+
+	/* set the sample period */
+	writel(RK_RNG_SAMPLE_CNT, rk_rng->base + TRNG_RNG_SAMPLE_CNT);
+
+	/* set osc ring speed and enable it */
+	writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
+		       TRNG_RNG_CTL_OSC_RING_SPEED_0 |
+		       TRNG_RNG_CTL_ENABLE,
+		       rk_rng->base + TRNG_RNG_CTL);
+
+	return 0;
+}
+
+static void rk_rng_cleanup(struct hwrng *rng)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+
+	/* stop TRNG */
+	writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
+
+	/* stop clocks */
+	clk_bulk_disable_unprepare(rk_rng->clk_num, rk_rng->clk_bulks);
+}
+
+static int rk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+	size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
+	u32 reg;
+	int ret = 0;
+
+	ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
+	if (ret < 0)
+		goto out;
+
+	/* Start collecting random data */
+	writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
+
+	ret = readl_poll_timeout(rk_rng->base + TRNG_RNG_CTL, reg,
+				 !(reg & TRNG_RNG_CTL_START),
+				 RK_RNG_POLL_PERIOD_US,
+				 RK_RNG_POLL_TIMEOUT_US);
+	if (ret < 0)
+		goto out;
+
+	/* Read random data stored in the registers */
+	memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
+out:
+	pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
+	pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
+
+	return to_read;
+}
+
+static int rk_rng_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rk_rng *rk_rng;
+	int ret;
+
+	rk_rng = devm_kzalloc(dev, sizeof(*rk_rng), GFP_KERNEL);
+	if (!rk_rng)
+		return -ENOMEM;
+
+	rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rk_rng->base))
+		return PTR_ERR(rk_rng->base);
+
+	rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
+	if (rk_rng->clk_num < 0)
+		return dev_err_probe(dev, rk_rng->clk_num,
+				     "Failed to get clks property\n");
+
+	rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
+	if (IS_ERR(rk_rng->rst))
+		return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
+				     "Failed to get reset property\n");
+
+	reset_control_assert(rk_rng->rst);
+	udelay(2);
+	reset_control_deassert(rk_rng->rst);
+
+	platform_set_drvdata(pdev, rk_rng);
+
+	rk_rng->rng.name = dev_driver_string(dev);
+#ifndef CONFIG_PM
+	rk_rng->rng.init = rk_rng_init;
+	rk_rng->rng.cleanup = rk_rng_cleanup;
+#endif
+	rk_rng->rng.read = rk_rng_read;
+	rk_rng->rng.priv = (unsigned long) dev;
+	rk_rng->rng.quality = 900;
+
+	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_enable(dev);
+
+	ret = devm_hwrng_register(dev, &rk_rng->rng);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
+
+	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
+
+	return 0;
+}
+
+static int rk_rng_remove(struct platform_device *pdev)
+{
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int rk_rng_runtime_suspend(struct device *dev)
+{
+	struct rk_rng *rk_rng = dev_get_drvdata(dev);
+
+	rk_rng_cleanup(&rk_rng->rng);
+
+	return 0;
+}
+
+static int rk_rng_runtime_resume(struct device *dev)
+{
+	struct rk_rng *rk_rng = dev_get_drvdata(dev);
+
+	return rk_rng_init(&rk_rng->rng);
+}
+#endif
+
+static const struct dev_pm_ops rk_rng_pm_ops = {
+	SET_RUNTIME_PM_OPS(rk_rng_runtime_suspend,
+				rk_rng_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
+static const struct of_device_id rk_rng_dt_match[] = {
+	{ .compatible = "rockchip,rk3568-rng", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, rk_rng_dt_match);
+
+static struct platform_driver rk_rng_driver = {
+	.driver	= {
+		.name	= "rockchip-rng",
+		.pm	= &rk_rng_pm_ops,
+		.of_match_table = rk_rng_dt_match,
+	},
+	.probe	= rk_rng_probe,
+	.remove = rk_rng_remove,
+};
+
+module_platform_driver(rk_rng_driver);
+
+MODULE_DESCRIPTION("Rockchip True Random Number Generator driver");
+MODULE_AUTHOR("Lin Jinhan <troy.lin@rock-chips.com>, Aurelien Jarno <aurelien@aurel32.net>");
+MODULE_LICENSE("GPL");
-- 
2.45.2

[PATCH v3 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..57c8103500ea 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -1855,6 +1855,15 @@ usb2phy1_otg: otg-port {
 		};
 	};
 
+	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";
+	};
+
 	pinctrl: pinctrl {
 		compatible = "rockchip,rk3568-pinctrl";
 		rockchip,grf = <&grf>;
-- 
2.45.2

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

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 1565 bytes --]

Hi,

https://lore.kernel.org/all/89b16ec5-f9a5-f836-f51a-8325448e4775@linaro.org/ 
also mentions some remarks about the subject, so if I interpreted those 
remarks correctly, it should be:

dt-bindings: rng: rockchip: Add rk3568 TRNG

On Friday, 21 June 2024 03:25:01 CEST Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
> 
> Add the RNG bindings for the RK3568 SoC from Rockchip
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 66 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..d45f03683fbe
> --- /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 Rokchip RK3568 SoC

I *think* that the TRNG for rk3588 is different, so shouldn't the title be:

Rockchip TRNG for rk356x SoCs

Cheers,
  Diederik

PS: Heiko's name is without `memcpy_fromio` ;-P

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

Hi,

On Friday, 21 June 2024 03:25:18 CEST Daniel Golle wrote:
> diff --git a/drivers/char/hw_random/rockchip-rng.c
> b/drivers/char/hw_random/rockchip-rng.c new file mode 100644
> index 000000000000..6070abb73847
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rockchip-rng.c True Random Number Generator driver for Rockchip SoCs
> + *
> + * Copyright (c) 2018, Fuzhou Rockchip Electronics Co., Ltd.
> + * Copyright (c) 2022, Aurelien Jarno
> + * Authors:
> + *  Lin Jinhan <troy.lin@rock-chips.com>
> + *  Aurelien Jarno <aurelien@aurel32.net>
> + */
> +#include <linux/clk.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>

I've been using a modified version of Aurelien's patch myself and I added the 
following to my commit description:

```
hwrng: rockchip: Explicitly include correct DT includes

Similar to commit 
045a44d4c9b3 ("regulator: Explicitly include correct DT includes")
replace ``of_platform.h`` include with ``of.h`` and ``platform_device.h``.

Link: https://git.kernel.org/linus/045a44d4c9b32578aacf0811063e5bb741c7c32c
```

BUT I don't (really) know what I'm doing, so could you verify whether there is 
some merit to it?

Cheers,
  Diederik

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Friday, 21 June 2024 03:25:30 CEST Daniel Golle wrote:
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..57c8103500ea
> 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -1855,6 +1855,15 @@ usb2phy1_otg: otg-port {
>                 };
>         };
> 
> +       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";
> +       };
> +
>         pinctrl: pinctrl {
>                 compatible = "rockchip,rk3568-pinctrl";
>                 rockchip,grf = <&grf>;
> --

I had placed the node between ``sdhci: mmc@fe310000`` and
``i2s0_8ch: i2s@fe400000`` which I think is the proper order.

Cheers,
  Diederik

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

On Friday, 21 June 2024 11:12:35 CEST Diederik de Haas wrote:
> > +title: Rockchip TRNG
> > +
> > +description: True Random Number Generator on Rokchip RK3568 SoC
> 
> I *think* that the TRNG for rk3588 is different, so shouldn't the title be:
> 
> Rockchip TRNG for rk356x SoCs

And I just noticed `Rokchip` in the description, so that's missing a `c`.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Heiko Stübner]

Am Freitag, 21. Juni 2024, 11:36:45 CEST schrieb Diederik de Haas:
> On Friday, 21 June 2024 03:25:30 CEST Daniel Golle wrote:
> > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk356x.dtsi index d8543b5557ee..57c8103500ea
> > 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > @@ -1855,6 +1855,15 @@ usb2phy1_otg: otg-port {
> >                 };
> >         };
> > 
> > +       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";
> > +       };
> > +
> >         pinctrl: pinctrl {
> >                 compatible = "rockchip,rk3568-pinctrl";
> >                 rockchip,grf = <&grf>;
> > --
> 
> I had placed the node between ``sdhci: mmc@fe310000`` and
> ``i2s0_8ch: i2s@fe400000`` which I think is the proper order.

correct.
If a node has an address in its name, sort by that address.

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

[Krzysztof Kozlowski]

On 21/06/2024 03:25, Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
> 
> Add the RNG bindings for the RK3568 SoC from Rockchip
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

<form letter>
This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.
</form letter>


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 21/06/2024 03:25, Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
> 

> +
> +static int rk_rng_init(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	int ret;
> +
> +	/* start clocks */
> +	ret = clk_bulk_prepare_enable(rk_rng->clk_num, rk_rng->clk_bulks);
> +	if (ret < 0) {
> +		dev_err((struct device *) rk_rng->rng.priv,
> +			"Failed to enable clks %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* set the sample period */
> +	writel(RK_RNG_SAMPLE_CNT, rk_rng->base + TRNG_RNG_SAMPLE_CNT);
> +
> +	/* set osc ring speed and enable it */
> +	writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> +		       TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> +		       TRNG_RNG_CTL_ENABLE,
> +		       rk_rng->base + TRNG_RNG_CTL);

I doubt relaxed write is here intentional. Enabling should be last
instruction, so this should be ordered write.

> +
> +	return 0;
> +}
> +
> +static void rk_rng_cleanup(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +
> +	/* stop TRNG */
> +	writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);

This should not be relaxed. This might lead to very tricky to debug issues.

> +
> +	/* stop clocks */
> +	clk_bulk_disable_unprepare(rk_rng->clk_num, rk_rng->clk_bulks);
> +}
> +
> +static int rk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> +	u32 reg;
> +	int ret = 0;
> +
> +	ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);

Why this cannot be just simpler pm_runtime_resume_and_get()?

> +	if (ret < 0)
> +		goto out;

This does not look like correct error path. Device was not busy here.

> +
> +	/* Start collecting random data */
> +	writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> +
> +	ret = readl_poll_timeout(rk_rng->base + TRNG_RNG_CTL, reg,
> +				 !(reg & TRNG_RNG_CTL_START),
> +				 RK_RNG_POLL_PERIOD_US,
> +				 RK_RNG_POLL_TIMEOUT_US);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* Read random data stored in the registers */
> +	memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> +out:
> +	pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> +	pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> +
> +	return to_read;
> +}
> +
> +static int rk_rng_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rk_rng *rk_rng;
> +	int ret;
> +
> +	rk_rng = devm_kzalloc(dev, sizeof(*rk_rng), GFP_KERNEL);
> +	if (!rk_rng)
> +		return -ENOMEM;
> +
> +	rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rk_rng->base))
> +		return PTR_ERR(rk_rng->base);
> +
> +	rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> +	if (rk_rng->clk_num < 0)
> +		return dev_err_probe(dev, rk_rng->clk_num,
> +				     "Failed to get clks property\n");
> +
> +	rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> +	if (IS_ERR(rk_rng->rst))
> +		return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> +				     "Failed to get reset property\n");
> +
> +	reset_control_assert(rk_rng->rst);
> +	udelay(2);
> +	reset_control_deassert(rk_rng->rst);
> +
> +	platform_set_drvdata(pdev, rk_rng);
> +
> +	rk_rng->rng.name = dev_driver_string(dev);
> +#ifndef CONFIG_PM
> +	rk_rng->rng.init = rk_rng_init;
> +	rk_rng->rng.cleanup = rk_rng_cleanup;
> +#endif
> +	rk_rng->rng.read = rk_rng_read;
> +	rk_rng->rng.priv = (unsigned long) dev;
> +	rk_rng->rng.quality = 900;
> +
> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_enable(dev);
> +
> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
> +
> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");

Drop, driver should be silent on success.


Best regards,
Krzysztof

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

[Chen-Yu Tsai]

On Fri, Jun 21, 2024 at 9:25 AM Daniel Golle <daniel@makrotopia.org> wrote:
>
> From: Aurelien Jarno <aurelien@aurel32.net>
>
> Rockchip SoCs used to have a random number generator as part of their
> crypto device, and support for it has to be added to the corresponding
> driver. However newer Rockchip SoCs like the RK356x have an independent
> True Random Number Generator device. This patch adds a driver for it,
> greatly inspired from the downstream driver.
>
> The TRNG device does not seem to have a signal conditionner and the FIPS
> 140-2 test returns a lot of failures. They can be reduced by increasing
> RK_RNG_SAMPLE_CNT, in a tradeoff between quality and speed. This value
> has been adjusted to get ~90% of successes and the quality value has
> been set accordingly.
>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> [daniel@makrotpia.org: code style fixes]
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  MAINTAINERS                           |   1 +
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 229 ++++++++++++++++++++++++++
>  4 files changed, 245 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 77d449c89bf2..299b8c1a5fb5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19493,6 +19493,7 @@ M:      Daniel Golle <daniel@makrotopia.org>
>  M:     Aurelien Jarno <aurelien@aurel32.net>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> +F:     drivers/char/hw_random/rockchip-rng.c
>
>  ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
>  M:     Jacob Chen <jacob-chen@iotwrt.com>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 442c40efb200..2b62cd08f91a 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -573,6 +573,20 @@ config HW_RANDOM_JH7110
>           To compile this driver as a module, choose M here.
>           The module will be called jh7110-trng.
>
> +config HW_RANDOM_ROCKCHIP
> +       tristate "Rockchip True Random Number Generator"
> +       depends on HW_RANDOM && (ARCH_ROCKCHIP || COMPILE_TEST)
> +       depends on HAS_IOMEM
> +       default HW_RANDOM
> +       help
> +         This driver provides kernel-side support for the True Random Number
> +         Generator hardware found on some Rockchip SoC like RK3566 or RK3568.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called rockchip-rng.
> +
> +         If unsure, say Y.
> +
>  endif # HW_RANDOM
>
>  config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index 32549a1186dc..01f012eab440 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -48,4 +48,5 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-trng.o
>  obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
>  obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
>  obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
> +obj-$(CONFIG_HW_RANDOM_ROCKCHIP) += rockchip-rng.o
>  obj-$(CONFIG_HW_RANDOM_JH7110) += jh7110-trng.o
> diff --git a/drivers/char/hw_random/rockchip-rng.c b/drivers/char/hw_random/rockchip-rng.c
> new file mode 100644
> index 000000000000..6070abb73847
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,229 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rockchip-rng.c True Random Number Generator driver for Rockchip SoCs
> + *
> + * Copyright (c) 2018, Fuzhou Rockchip Electronics Co., Ltd.
> + * Copyright (c) 2022, Aurelien Jarno
> + * Authors:
> + *  Lin Jinhan <troy.lin@rock-chips.com>
> + *  Aurelien Jarno <aurelien@aurel32.net>
> + */
> +#include <linux/clk.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>

Need to explicitly include linux/platform_device.h for |struct platform_device|
and devm_platform_iomap_resource().

> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define RK_RNG_AUTOSUSPEND_DELAY       100
> +#define RK_RNG_MAX_BYTE                        32
> +#define RK_RNG_POLL_PERIOD_US          100
> +#define RK_RNG_POLL_TIMEOUT_US         10000
> +
> +/*
> + * TRNG collects osc ring output bit every RK_RNG_SAMPLE_CNT time. The value is
> + * a tradeoff between speed and quality and has been adjusted to get a quality
> + * of ~900 (~90% of FIPS 140-2 successes).
> + */
> +#define RK_RNG_SAMPLE_CNT              1000
> +
> +/* TRNG registers from RK3568 TRM-Part2, section 5.4.1 */
> +#define TRNG_RST_CTL                   0x0004
> +#define TRNG_RNG_CTL                   0x0400
> +#define TRNG_RNG_CTL_LEN_64_BIT                (0x00 << 4)
> +#define TRNG_RNG_CTL_LEN_128_BIT       (0x01 << 4)
> +#define TRNG_RNG_CTL_LEN_192_BIT       (0x02 << 4)
> +#define TRNG_RNG_CTL_LEN_256_BIT       (0x03 << 4)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_0  (0x00 << 2)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_1  (0x01 << 2)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_2  (0x02 << 2)
> +#define TRNG_RNG_CTL_OSC_RING_SPEED_3  (0x03 << 2)
> +#define TRNG_RNG_CTL_ENABLE            BIT(1)
> +#define TRNG_RNG_CTL_START             BIT(0)
> +#define TRNG_RNG_SAMPLE_CNT            0x0404
> +#define TRNG_RNG_DOUT                  0x0410
> +
> +struct rk_rng {
> +       struct hwrng rng;
> +       void __iomem *base;
> +       struct reset_control *rst;
> +       int clk_num;
> +       struct clk_bulk_data *clk_bulks;
> +};
> +
> +static int rk_rng_init(struct hwrng *rng)
> +{
> +       struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +       int ret;
> +
> +       /* start clocks */
> +       ret = clk_bulk_prepare_enable(rk_rng->clk_num, rk_rng->clk_bulks);
> +       if (ret < 0) {
> +               dev_err((struct device *) rk_rng->rng.priv,
> +                       "Failed to enable clks %d\n", ret);
> +               return ret;
> +       }
> +
> +       /* set the sample period */
> +       writel(RK_RNG_SAMPLE_CNT, rk_rng->base + TRNG_RNG_SAMPLE_CNT);
> +
> +       /* set osc ring speed and enable it */
> +       writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> +                      TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> +                      TRNG_RNG_CTL_ENABLE,
> +                      rk_rng->base + TRNG_RNG_CTL);
> +
> +       return 0;
> +}
> +
> +static void rk_rng_cleanup(struct hwrng *rng)
> +{
> +       struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +
> +       /* stop TRNG */
> +       writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
> +
> +       /* stop clocks */
> +       clk_bulk_disable_unprepare(rk_rng->clk_num, rk_rng->clk_bulks);
> +}
> +
> +static int rk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> +       struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +       size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> +       u32 reg;
> +       int ret = 0;
> +
> +       ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
> +       if (ret < 0)
> +               goto out;
> +
> +       /* Start collecting random data */
> +       writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> +
> +       ret = readl_poll_timeout(rk_rng->base + TRNG_RNG_CTL, reg,
> +                                !(reg & TRNG_RNG_CTL_START),
> +                                RK_RNG_POLL_PERIOD_US,
> +                                RK_RNG_POLL_TIMEOUT_US);
> +       if (ret < 0)
> +               goto out;
> +
> +       /* Read random data stored in the registers */
> +       memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> +out:
> +       pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> +       pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> +
> +       return to_read;
> +}
> +
> +static int rk_rng_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rk_rng *rk_rng;
> +       int ret;
> +
> +       rk_rng = devm_kzalloc(dev, sizeof(*rk_rng), GFP_KERNEL);
> +       if (!rk_rng)
> +               return -ENOMEM;
> +
> +       rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(rk_rng->base))
> +               return PTR_ERR(rk_rng->base);
> +
> +       rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> +       if (rk_rng->clk_num < 0)
> +               return dev_err_probe(dev, rk_rng->clk_num,
> +                                    "Failed to get clks property\n");
> +
> +       rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> +       if (IS_ERR(rk_rng->rst))
> +               return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> +                                    "Failed to get reset property\n");
> +
> +       reset_control_assert(rk_rng->rst);
> +       udelay(2);
> +       reset_control_deassert(rk_rng->rst);
> +
> +       platform_set_drvdata(pdev, rk_rng);
> +
> +       rk_rng->rng.name = dev_driver_string(dev);
> +#ifndef CONFIG_PM
> +       rk_rng->rng.init = rk_rng_init;
> +       rk_rng->rng.cleanup = rk_rng_cleanup;
> +#endif
> +       rk_rng->rng.read = rk_rng_read;
> +       rk_rng->rng.priv = (unsigned long) dev;
> +       rk_rng->rng.quality = 900;
> +
> +       pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_enable(dev);

You can use devm_pm_runtime_enable(dev) here and simply get rid of the
remove function, and also no explicit cleanup needed.

> +
> +       ret = devm_hwrng_register(dev, &rk_rng->rng);
> +       if (ret)
> +               return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");

This is missing cleanup for pm_runtime_enable().

> +
> +       dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> +
> +       return 0;
> +}
> +
> +static int rk_rng_remove(struct platform_device *pdev)

Return type of remove callback has been changed to void. This needs to be
updated.


ChenYu

> +{
> +       pm_runtime_disable(&pdev->dev);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int rk_rng_runtime_suspend(struct device *dev)
> +{
> +       struct rk_rng *rk_rng = dev_get_drvdata(dev);
> +
> +       rk_rng_cleanup(&rk_rng->rng);
> +
> +       return 0;
> +}
> +
> +static int rk_rng_runtime_resume(struct device *dev)
> +{
> +       struct rk_rng *rk_rng = dev_get_drvdata(dev);
> +
> +       return rk_rng_init(&rk_rng->rng);
> +}
> +#endif
> +
> +static const struct dev_pm_ops rk_rng_pm_ops = {
> +       SET_RUNTIME_PM_OPS(rk_rng_runtime_suspend,
> +                               rk_rng_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +};
> +
> +static const struct of_device_id rk_rng_dt_match[] = {
> +       { .compatible = "rockchip,rk3568-rng", },
> +       { /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rk_rng_dt_match);
> +
> +static struct platform_driver rk_rng_driver = {
> +       .driver = {
> +               .name   = "rockchip-rng",
> +               .pm     = &rk_rng_pm_ops,
> +               .of_match_table = rk_rng_dt_match,
> +       },
> +       .probe  = rk_rng_probe,
> +       .remove = rk_rng_remove,
> +};
> +
> +module_platform_driver(rk_rng_driver);
> +
> +MODULE_DESCRIPTION("Rockchip True Random Number Generator driver");
> +MODULE_AUTHOR("Lin Jinhan <troy.lin@rock-chips.com>, Aurelien Jarno <aurelien@aurel32.net>");
> +MODULE_LICENSE("GPL");
> --
> 2.45.2
>

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

[Chen-Yu Tsai]

On Fri, Jun 21, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/06/2024 03:25, Daniel Golle wrote:
> > From: Aurelien Jarno <aurelien@aurel32.net>
> >
>
> > +
> > +static int rk_rng_init(struct hwrng *rng)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +     int ret;
> > +
> > +     /* start clocks */
> > +     ret = clk_bulk_prepare_enable(rk_rng->clk_num, rk_rng->clk_bulks);
> > +     if (ret < 0) {
> > +             dev_err((struct device *) rk_rng->rng.priv,
> > +                     "Failed to enable clks %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     /* set the sample period */
> > +     writel(RK_RNG_SAMPLE_CNT, rk_rng->base + TRNG_RNG_SAMPLE_CNT);
> > +
> > +     /* set osc ring speed and enable it */
> > +     writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> > +                    TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> > +                    TRNG_RNG_CTL_ENABLE,
> > +                    rk_rng->base + TRNG_RNG_CTL);
>
> I doubt relaxed write is here intentional. Enabling should be last
> instruction, so this should be ordered write.

I agree that the driver should just do all non-relaxed writes for simplicity.
The penalty isn't that severe since commit 22ec71615d82 ("arm64: io: Relax
implicit barriers in default I/O accessors").

Just to clarify, writes to devices are always ordered. The non-relaxed
writes are ordered to _memory writes_, which doesn't really matter for
this driver.

ChenYu


> > +
> > +     return 0;
> > +}
> > +
> > +static void rk_rng_cleanup(struct hwrng *rng)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +
> > +     /* stop TRNG */
> > +     writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
>
> This should not be relaxed. This might lead to very tricky to debug issues.
>
> > +
> > +     /* stop clocks */
> > +     clk_bulk_disable_unprepare(rk_rng->clk_num, rk_rng->clk_bulks);
> > +}
> > +
> > +static int rk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +     size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> > +     u32 reg;
> > +     int ret = 0;
> > +
> > +     ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
>
> Why this cannot be just simpler pm_runtime_resume_and_get()?
>
> > +     if (ret < 0)
> > +             goto out;
>
> This does not look like correct error path. Device was not busy here.
>
> > +
> > +     /* Start collecting random data */
> > +     writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> > +
> > +     ret = readl_poll_timeout(rk_rng->base + TRNG_RNG_CTL, reg,
> > +                              !(reg & TRNG_RNG_CTL_START),
> > +                              RK_RNG_POLL_PERIOD_US,
> > +                              RK_RNG_POLL_TIMEOUT_US);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     /* Read random data stored in the registers */
> > +     memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> > +out:
> > +     pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> > +     pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> > +
> > +     return to_read;
> > +}
> > +
> > +static int rk_rng_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct rk_rng *rk_rng;
> > +     int ret;
> > +
> > +     rk_rng = devm_kzalloc(dev, sizeof(*rk_rng), GFP_KERNEL);
> > +     if (!rk_rng)
> > +             return -ENOMEM;
> > +
> > +     rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(rk_rng->base))
> > +             return PTR_ERR(rk_rng->base);
> > +
> > +     rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> > +     if (rk_rng->clk_num < 0)
> > +             return dev_err_probe(dev, rk_rng->clk_num,
> > +                                  "Failed to get clks property\n");
> > +
> > +     rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> > +     if (IS_ERR(rk_rng->rst))
> > +             return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> > +                                  "Failed to get reset property\n");
> > +
> > +     reset_control_assert(rk_rng->rst);
> > +     udelay(2);
> > +     reset_control_deassert(rk_rng->rst);
> > +
> > +     platform_set_drvdata(pdev, rk_rng);
> > +
> > +     rk_rng->rng.name = dev_driver_string(dev);
> > +#ifndef CONFIG_PM
> > +     rk_rng->rng.init = rk_rng_init;
> > +     rk_rng->rng.cleanup = rk_rng_cleanup;
> > +#endif
> > +     rk_rng->rng.read = rk_rng_read;
> > +     rk_rng->rng.priv = (unsigned long) dev;
> > +     rk_rng->rng.quality = 900;
> > +
> > +     pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> > +     pm_runtime_use_autosuspend(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     ret = devm_hwrng_register(dev, &rk_rng->rng);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
> > +
> > +     dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>
> Drop, driver should be silent on success.
>
>
> Best regards,
> Krzysztof
>
>

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

[Philipp Zabel]

Hi,

On Fr, 2024-06-21 at 02:25 +0100, Daniel Golle wrote:
> From: Aurelien Jarno <aurelien@aurel32.net>
> 
> Rockchip SoCs used to have a random number generator as part of their
> crypto device, and support for it has to be added to the corresponding
> driver. However newer Rockchip SoCs like the RK356x have an independent
> True Random Number Generator device. This patch adds a driver for it,
> greatly inspired from the downstream driver.
> 
> The TRNG device does not seem to have a signal conditionner and the FIPS
> 140-2 test returns a lot of failures. They can be reduced by increasing
> RK_RNG_SAMPLE_CNT, in a tradeoff between quality and speed. This value
> has been adjusted to get ~90% of successes and the quality value has
> been set accordingly.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> [daniel@makrotpia.org: code style fixes]
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---
>  MAINTAINERS                           |   1 +
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 229 ++++++++++++++++++++++++++
>  4 files changed, 245 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
> 
[...]
> diff --git a/drivers/char/hw_random/rockchip-rng.c b/drivers/char/hw_random/rockchip-rng.c
> new file mode 100644
> index 000000000000..6070abb73847
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,229 @@
[...]
> 
> +static int rk_rng_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct rk_rng *rk_rng;
> +	int ret;
> +
> +	rk_rng = devm_kzalloc(dev, sizeof(*rk_rng), GFP_KERNEL);
> +	if (!rk_rng)
> +		return -ENOMEM;
> +
> +	rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(rk_rng->base))
> +		return PTR_ERR(rk_rng->base);
> +
> +	rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> +	if (rk_rng->clk_num < 0)
> +		return dev_err_probe(dev, rk_rng->clk_num,
> +				     "Failed to get clks property\n");
> +
> +	rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);

Please use devm_reset_control_array_get_exclusive() instead.


regards
Philipp

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

[Dragan Simic]

Hello Krzysztof,

On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> On 21/06/2024 03:25, Daniel Golle wrote:
>> From: Aurelien Jarno <aurelien@aurel32.net>

[snip]

>> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>> +	pm_runtime_use_autosuspend(dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
>> +	if (ret)
>> +		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip 
>> hwrng\n");
>> +
>> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> 
> Drop, driver should be silent on success.

I respectfully disagree.  Many drivers print a single line upon
successful probing, which I find very useful.  In this particular
case, it's even more useful, because some people may be concerned
about the use of hardware TRNGs, so we should actually make sure
to announce it.

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

[Uwe Kleine-König]

Hello Dragan,

On 6/21/24 20:13, Dragan Simic wrote:
> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> On 21/06/2024 03:25, Daniel Golle wrote:
>>> From: Aurelien Jarno <aurelien@aurel32.net>
> 
> [snip]
> 
>>> +    pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>>> +    pm_runtime_use_autosuspend(dev);
>>> +    pm_runtime_enable(dev);
>>> +
>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
>>> +    if (ret)
>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register 
>>> Rockchip hwrng\n");
>>> +
>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>
>> Drop, driver should be silent on success.
> 
> I respectfully disagree.  Many drivers print a single line upon
> successful probing, which I find very useful.  In this particular
> case, it's even more useful, because some people may be concerned
> about the use of hardware TRNGs, so we should actually make sure
> to announce it.

I agree to Krzysztof here. From the POV of a driver author, your own
driver is very important and while you write it, it really interests
*you* if the driver is successfully probed. However from a system
perspective these are annoying: There are easily >50 devices[1] on a
system, if all of these print a message in probe, you have little chance
to see the relevant messages. Even if every driver author thinks their
work is a special snow flake that is worth announcing, in practice users
only care about your driver if there is a problem. Additionally each
message takes time and so delays the boot process. Additionally each 
message takes place in the printk ring buffer and so edges out earlier 
messages that might be more important.

So +1 for dropping the dev_info() or at least using dev_debug() for it.

Best regards
Uwe

[1] On my laptop if have:

	$ find /sys/devices -name driver | wc -l
	87

     On a Raspberrypi it yields 66.

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

[Aurelien Jarno]

Hi Daniel,

On 2024-06-21 02:24, Daniel Golle wrote:
> 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.
> 
> v2 of this patchset has been submitted by Aurelien Jarno in November
> 2022. A follow-up submission addressing the comments received for v2
> never happened.

I didn't have the time or energy to continue working on this. Thanks for
continuing the work, I'll give it a try at some point.

Regards
Aurelien

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

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

[Dragan Simic]

Hello Uwe,

On 2024-06-22 00:16, Uwe Kleine-König wrote:
> On 6/21/24 20:13, Dragan Simic wrote:
>> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>>> On 21/06/2024 03:25, Daniel Golle wrote:
>>>> From: Aurelien Jarno <aurelien@aurel32.net>
>> 
>> [snip]
>> 
>>>> +    pm_runtime_set_autosuspend_delay(dev, 
>>>> RK_RNG_AUTOSUSPEND_DELAY);
>>>> +    pm_runtime_use_autosuspend(dev);
>>>> +    pm_runtime_enable(dev);
>>>> +
>>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
>>>> +    if (ret)
>>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register 
>>>> Rockchip hwrng\n");
>>>> +
>>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>> 
>>> Drop, driver should be silent on success.
>> 
>> I respectfully disagree.  Many drivers print a single line upon
>> successful probing, which I find very useful.  In this particular
>> case, it's even more useful, because some people may be concerned
>> about the use of hardware TRNGs, so we should actually make sure
>> to announce it.
> 
> I agree to Krzysztof here. From the POV of a driver author, your own
> driver is very important and while you write it, it really interests
> *you* if the driver is successfully probed. However from a system
> perspective these are annoying: There are easily >50 devices[1] on a
> system, if all of these print a message in probe, you have little 
> chance
> to see the relevant messages. Even if every driver author thinks their
> work is a special snow flake that is worth announcing, in practice 
> users
> only care about your driver if there is a problem. Additionally each
> message takes time and so delays the boot process. Additionally each
> message takes place in the printk ring buffer and so edges out earlier
> messages that might be more important.

Well, I don't find those messages annoying, for the drivers I've had
nothing to do with.  Also, in my experience, 99.9% of users don't care
about the kernel messages at all, be it everything hunky-dory, or be
it something really wrong somewhere.

> So +1 for dropping the dev_info() or at least using dev_debug() for it.
> 
> Best regards
> Uwe
> 
> [1] On my laptop if have:
> 
> 	$ find /sys/devices -name driver | wc -l
> 	87
> 
>     On a Raspberrypi it yields 66.

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

[Krzysztof Kozlowski]

On 21/06/2024 20:13, Dragan Simic wrote:
> Hello Krzysztof,
> 
> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> On 21/06/2024 03:25, Daniel Golle wrote:
>>> From: Aurelien Jarno <aurelien@aurel32.net>
> 
> [snip]
> 
>>> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>>> +	pm_runtime_use_autosuspend(dev);
>>> +	pm_runtime_enable(dev);
>>> +
>>> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
>>> +	if (ret)
>>> +		return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip 
>>> hwrng\n");
>>> +
>>> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>
>> Drop, driver should be silent on success.
> 
> I respectfully disagree.  Many drivers print a single line upon
> successful probing, which I find very useful.  In this particular

No, it's duplicating existing interfaces and polluting log unnecessarily
without any useful information.

> case, it's even more useful, because some people may be concerned
> about the use of hardware TRNGs, so we should actually make sure
> to announce it.

Best regards,
Krzysztof

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

[Dragan Simic]

Hello Krzysztof,

On 2024-06-22 20:05, Krzysztof Kozlowski wrote:
> On 21/06/2024 20:13, Dragan Simic wrote:
>> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>>> On 21/06/2024 03:25, Daniel Golle wrote:
>>>> From: Aurelien Jarno <aurelien@aurel32.net>
>> 
>> [snip]
>> 
>>>> +	pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
>>>> +	pm_runtime_use_autosuspend(dev);
>>>> +	pm_runtime_enable(dev);
>>>> +
>>>> +	ret = devm_hwrng_register(dev, &rk_rng->rng);
>>>> +	if (ret)
>>>> +		return dev_err_probe(&pdev->dev, ret, "Failed to register 
>>>> Rockchip
>>>> hwrng\n");
>>>> +
>>>> +	dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>>> 
>>> Drop, driver should be silent on success.
>> 
>> I respectfully disagree.  Many drivers print a single line upon
>> successful probing, which I find very useful.  In this particular
> 
> No, it's duplicating existing interfaces and polluting log 
> unnecessarily
> without any useful information.

Would you, please, clarify what existing interfaces are you
referring to?

>> case, it's even more useful, because some people may be concerned
>> about the use of hardware TRNGs, so we should actually make sure
>> to announce it.

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

[Heiko Stübner]

Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
> Hello Uwe,
> 
> On 2024-06-22 00:16, Uwe Kleine-König wrote:
> > On 6/21/24 20:13, Dragan Simic wrote:
> >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> >>> On 21/06/2024 03:25, Daniel Golle wrote:
> >>>> From: Aurelien Jarno <aurelien@aurel32.net>
> >> 
> >> [snip]
> >> 
> >>>> +    pm_runtime_set_autosuspend_delay(dev, 
> >>>> RK_RNG_AUTOSUSPEND_DELAY);
> >>>> +    pm_runtime_use_autosuspend(dev);
> >>>> +    pm_runtime_enable(dev);
> >>>> +
> >>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
> >>>> +    if (ret)
> >>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register 
> >>>> Rockchip hwrng\n");
> >>>> +
> >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> >>> 
> >>> Drop, driver should be silent on success.
> >> 
> >> I respectfully disagree.  Many drivers print a single line upon
> >> successful probing, which I find very useful.  In this particular
> >> case, it's even more useful, because some people may be concerned
> >> about the use of hardware TRNGs, so we should actually make sure
> >> to announce it.
> > 
> > I agree to Krzysztof here. From the POV of a driver author, your own
> > driver is very important and while you write it, it really interests
> > *you* if the driver is successfully probed. However from a system
> > perspective these are annoying: There are easily >50 devices[1] on a
> > system, if all of these print a message in probe, you have little 
> > chance
> > to see the relevant messages. Even if every driver author thinks their
> > work is a special snow flake that is worth announcing, in practice 
> > users
> > only care about your driver if there is a problem. Additionally each
> > message takes time and so delays the boot process. Additionally each
> > message takes place in the printk ring buffer and so edges out earlier
> > messages that might be more important.
> 
> Well, I don't find those messages annoying, for the drivers I've had
> nothing to do with.  Also, in my experience, 99.9% of users don't care
> about the kernel messages at all, be it everything hunky-dory, or be
> it something really wrong somewhere.
> 
> > So +1 for dropping the dev_info() or at least using dev_debug() for it.

Just for 2ct ... I'm also in the don't print too much camp ;-) .
When parsing kernel logs to see where things fail, messages just
telling me about sucesses make things more difficult.

So really this message should be dropped or at least as Uwe suggests
made a dev_dbg.


Heiko

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

[Dragan Simic]

Hello Heiko,

On 2024-06-22 22:26, Heiko Stübner wrote:
> Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
>> On 2024-06-22 00:16, Uwe Kleine-König wrote:
>> > On 6/21/24 20:13, Dragan Simic wrote:
>> >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> >>> On 21/06/2024 03:25, Daniel Golle wrote:
>> >>>> From: Aurelien Jarno <aurelien@aurel32.net>
>> >>
>> >> [snip]
>> >>
>> >>>> +    pm_runtime_set_autosuspend_delay(dev,
>> >>>> RK_RNG_AUTOSUSPEND_DELAY);
>> >>>> +    pm_runtime_use_autosuspend(dev);
>> >>>> +    pm_runtime_enable(dev);
>> >>>> +
>> >>>> +    ret = devm_hwrng_register(dev, &rk_rng->rng);
>> >>>> +    if (ret)
>> >>>> +        return dev_err_probe(&pdev->dev, ret, "Failed to register
>> >>>> Rockchip hwrng\n");
>> >>>> +
>> >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>> >>>
>> >>> Drop, driver should be silent on success.
>> >>
>> >> I respectfully disagree.  Many drivers print a single line upon
>> >> successful probing, which I find very useful.  In this particular
>> >> case, it's even more useful, because some people may be concerned
>> >> about the use of hardware TRNGs, so we should actually make sure
>> >> to announce it.
>> >
>> > I agree to Krzysztof here. From the POV of a driver author, your own
>> > driver is very important and while you write it, it really interests
>> > *you* if the driver is successfully probed. However from a system
>> > perspective these are annoying: There are easily >50 devices[1] on a
>> > system, if all of these print a message in probe, you have little
>> > chance
>> > to see the relevant messages. Even if every driver author thinks their
>> > work is a special snow flake that is worth announcing, in practice
>> > users
>> > only care about your driver if there is a problem. Additionally each
>> > message takes time and so delays the boot process. Additionally each
>> > message takes place in the printk ring buffer and so edges out earlier
>> > messages that might be more important.
>> 
>> Well, I don't find those messages annoying, for the drivers I've had
>> nothing to do with.  Also, in my experience, 99.9% of users don't care
>> about the kernel messages at all, be it everything hunky-dory, or be
>> it something really wrong somewhere.
>> 
>> > So +1 for dropping the dev_info() or at least using dev_debug() for it.
> 
> Just for 2ct ... I'm also in the don't print too much camp ;-) .
> When parsing kernel logs to see where things fail, messages just
> telling me about sucesses make things more difficult.
> 
> So really this message should be dropped or at least as Uwe suggests
> made a dev_dbg.

As a note, "dmesg --level=err,warn", for example, is rather useful
when it comes to filtering the kernel messages to see only those that
are signs of a trouble.

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2392 bytes --]

Hello Dragan,

On Sat, Jun 22, 2024 at 10:45:22PM +0200, Dragan Simic wrote:
> On 2024-06-22 22:26, Heiko Stübner wrote:
> > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
> > > On 2024-06-22 00:16, Uwe Kleine-König wrote:
> > > > On 6/21/24 20:13, Dragan Simic wrote:
> > > >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
> > > >>> On 21/06/2024 03:25, Daniel Golle wrote:
> > > >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
> > > >>>
> > > >>> Drop, driver should be silent on success.
> > > >>
> > [...]
> > So really this message should be dropped or at least as Uwe suggests
> > made a dev_dbg.
> 
> As a note, "dmesg --level=err,warn", for example, is rather useful
> when it comes to filtering the kernel messages to see only those that
> are signs of a trouble.

IMHO it's a bit sad, that there is such a long thread about something so
trivial, but I want to make a few points:

 - not all dmesg implementations support this:

	root@machine:~ dmesg --level=err,warn
	dmesg: unrecognized option '--level=err,warn'
	BusyBox v1.36.1 () multi-call binary.

	Usage: dmesg [-cr] [-n LEVEL] [-s SIZE]

	Print or control the kernel ring buffer

		-c              Clear ring buffer after printing
		-n LEVEL        Set console logging level
		-s SIZE         Buffer size
		-r              Print raw message buffer

 - Your argument that the output of this dev_info can easily be ignored
   is a very weak reason to keep it.

 - I personally consider it hard sometimes to accept feedback if I think
   it's wrong and there is a good reason to do it the way I want it.
   But there are now three people opposing your position, who brought
   forward (IMHO) good reasons and even a constructive alternative was
   presented. Please stay open minded while weighting the options.
   The questions to ask here include:

    - How many people care for this message? During every boot? Is it
      maybe enough for these people to check in /sys if the device
      probed successfully? Or maybe even the absence of an error message
      is enough?
    - How many people get this message and don't care about the
      presented information? How many people are even annoyed by it?
    - Is the delay and memory usage introduced by this message justified
      then, even if it's small?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Dragan Simic]

Hello Uwe,

On 2024-06-23 02:20, Uwe Kleine-König wrote:
> On Sat, Jun 22, 2024 at 10:45:22PM +0200, Dragan Simic wrote:
>> On 2024-06-22 22:26, Heiko Stübner wrote:
>> > Am Samstag, 22. Juni 2024, 12:29:33 CEST schrieb Dragan Simic:
>> > > On 2024-06-22 00:16, Uwe Kleine-König wrote:
>> > > > On 6/21/24 20:13, Dragan Simic wrote:
>> > > >> On 2024-06-21 11:57, Krzysztof Kozlowski wrote:
>> > > >>> On 21/06/2024 03:25, Daniel Golle wrote:
>> > > >>>> +    dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>> > > >>>
>> > > >>> Drop, driver should be silent on success.
>> > > >>
>> > [...]
>> > So really this message should be dropped or at least as Uwe suggests
>> > made a dev_dbg.
>> 
>> As a note, "dmesg --level=err,warn", for example, is rather useful
>> when it comes to filtering the kernel messages to see only those that
>> are signs of a trouble.
> 
> IMHO it's a bit sad, that there is such a long thread about something 
> so
> trivial, but I want to make a few points:
> 
>  - not all dmesg implementations support this:
> 
> 	root@machine:~ dmesg --level=err,warn
> 	dmesg: unrecognized option '--level=err,warn'
> 	BusyBox v1.36.1 () multi-call binary.
> 
> 	Usage: dmesg [-cr] [-n LEVEL] [-s SIZE]
> 
> 	Print or control the kernel ring buffer
> 
> 		-c              Clear ring buffer after printing
> 		-n LEVEL        Set console logging level
> 		-s SIZE         Buffer size
> 		-r              Print raw message buffer
> 
>  - Your argument that the output of this dev_info can easily be ignored
>    is a very weak reason to keep it.
> 
>  - I personally consider it hard sometimes to accept feedback if I 
> think
>    it's wrong and there is a good reason to do it the way I want it.
>    But there are now three people opposing your position, who brought
>    forward (IMHO) good reasons and even a constructive alternative was
>    presented. Please stay open minded while weighting the options.
>    The questions to ask here include:
> 
>     - How many people care for this message? During every boot? Is it
>       maybe enough for these people to check in /sys if the device
>       probed successfully? Or maybe even the absence of an error 
> message
>       is enough?
>     - How many people get this message and don't care about the
>       presented information? How many people are even annoyed by it?
>     - Is the delay and memory usage introduced by this message 
> justified
>       then, even if it's small?

I'm sorry if my responses caused any inconvenience.  I find most of your
points totally valid, but there are a couple of them I could continue
arguing about.  Though, as you also pointed out, my opinion has been
already outvoted, so I'll remain silent.