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

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

[Aurelien Jarno]

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.

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 +++++
 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         | 250 ++++++++++++++++++
 5 files changed, 334 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

-- 
2.35.1

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

[Aurelien Jarno]

Add the RNG bindings for the RK3568 SoC from Rockchip

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
 1 file changed, 60 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..c2f5ef69cf07
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
@@ -0,0 +1,60 @@
+# 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 for some Rockchip SoCs
+
+maintainers:
+  - Aurelien Jarno <aurelien@aurel32.net>
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3568-rng
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: TRNG clock
+      - description: TRNG AHB clock
+
+  clock-names:
+    items:
+      - const: trng_clk
+      - const: trng_hclk
+
+  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 = "trng_clk", "trng_hclk";
+        resets = <&cru SRST_TRNG_NS>;
+      };
+    };
+
+...
-- 
2.35.1

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

[Aurelien Jarno]

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>
---
 drivers/char/hw_random/Kconfig        |  14 ++
 drivers/char/hw_random/Makefile       |   1 +
 drivers/char/hw_random/rockchip-rng.c | 250 ++++++++++++++++++++++++++
 3 files changed, 265 insertions(+)
 create mode 100644 drivers/char/hw_random/rockchip-rng.c

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 3da8e85f8aae..8e5c88504f72 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -549,6 +549,20 @@ config HW_RANDOM_CN10K
 	 To compile this driver as a module, choose M here.
 	 The module will be called cn10k_rng. If unsure, say Y.
 
+config HW_RANDOM_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 3e948cf04476..b7e989535fd6 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -47,3 +47,4 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-trng.o
 obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
 obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
 obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
+obj-$(CONFIG_HW_RANDOM_ROCKCHIP) += rockchip-rng.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..18cdd91ad8c3
--- /dev/null
+++ b/drivers/char/hw_random/rockchip-rng.c
@@ -0,0 +1,250 @@
+// 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_0			0x0410
+#define TRNG_RNG_DOUT_1			0x0414
+#define TRNG_RNG_DOUT_2			0x0418
+#define TRNG_RNG_DOUT_3			0x041c
+#define TRNG_RNG_DOUT_4			0x0420
+#define TRNG_RNG_DOUT_5			0x0424
+#define TRNG_RNG_DOUT_6			0x0428
+#define TRNG_RNG_DOUT_7			0x042c
+
+struct rk_rng {
+	struct hwrng rng;
+	void __iomem *base;
+	struct reset_control *rst;
+	int clk_num;
+	struct clk_bulk_data *clk_bulks;
+};
+
+/* The mask determine the bits that are updated */
+static void rk_rng_write_ctl(struct rk_rng *rng, u32 val, u32 mask)
+{
+	writel_relaxed((mask << 16) | val, rng->base + TRNG_RNG_CTL);
+}
+
+static int rk_rng_init(struct hwrng *rng)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+	u32 reg;
+	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 */
+	reg = TRNG_RNG_CTL_LEN_256_BIT |
+		   TRNG_RNG_CTL_OSC_RING_SPEED_0 |
+		   TRNG_RNG_CTL_ENABLE;
+	rk_rng_write_ctl(rk_rng, reg, 0xffff);
+
+	return 0;
+}
+
+static void rk_rng_cleanup(struct hwrng *rng)
+{
+	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
+	u32 reg;
+
+	/* stop TRNG */
+	reg = 0;
+	rk_rng_write_ctl(rk_rng, reg, 0xffff);
+
+	/* 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);
+	u32 reg;
+	int ret = 0;
+	int i;
+
+	pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
+
+	/* Start collecting random data */
+	reg = TRNG_RNG_CTL_START;
+	rk_rng_write_ctl(rk_rng, reg, reg);
+
+	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 */
+	ret = min_t(size_t, max, RK_RNG_MAX_BYTE);
+	for (i = 0; i < ret; i += 4) {
+		*(u32 *)(buf + i) = readl_relaxed(rk_rng->base + TRNG_RNG_DOUT_0 + i);
+	}
+
+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;
+}
+
+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(struct 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",
+	},
+	{},
+};
+
+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 v2");
-- 
2.35.1

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

[Aurelien Jarno]

Enable the just added Rockchip RNG driver for RK356x SoCs.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 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 164708f1eb67..4be94ff45180 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -1770,6 +1770,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 = "trng_clk", "trng_hclk";
+		resets = <&cru SRST_TRNG_NS>;
+		reset-names = "reset";
+	};
+
 	pinctrl: pinctrl {
 		compatible = "rockchip,rk3568-pinctrl";
 		rockchip,grf = <&grf>;
-- 
2.35.1

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

[Krzysztof Kozlowski]

On 28/11/2022 19:47, Aurelien Jarno wrote:
> 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".

> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> ---
>  .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
>  1 file changed, 60 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..c2f5ef69cf07
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> @@ -0,0 +1,60 @@
> +# 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 for some Rockchip SoCs

s/for some Rockchip SoCs/on Rokchip RK3568 SoC/

> +
> +maintainers:
> +  - Aurelien Jarno <aurelien@aurel32.net>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3568-rng
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: TRNG clock
> +      - description: TRNG AHB clock
> +
> +  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.

> +
> +  resets:
> +    maxItems: 1
> +

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 28/11/2022 19:47, Aurelien Jarno 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 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>
> ---
>  drivers/char/hw_random/Kconfig        |  14 ++
>  drivers/char/hw_random/Makefile       |   1 +
>  drivers/char/hw_random/rockchip-rng.c | 250 ++++++++++++++++++++++++++
>  3 files changed, 265 insertions(+)
>  create mode 100644 drivers/char/hw_random/rockchip-rng.c
> 
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 3da8e85f8aae..8e5c88504f72 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -549,6 +549,20 @@ config HW_RANDOM_CN10K
>  	 To compile this driver as a module, choose M here.
>  	 The module will be called cn10k_rng. If unsure, say Y.
>  
> +config HW_RANDOM_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 3e948cf04476..b7e989535fd6 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -47,3 +47,4 @@ obj-$(CONFIG_HW_RANDOM_XIPHERA) += xiphera-trng.o
>  obj-$(CONFIG_HW_RANDOM_ARM_SMCCC_TRNG) += arm_smccc_trng.o
>  obj-$(CONFIG_HW_RANDOM_CN10K) += cn10k-rng.o
>  obj-$(CONFIG_HW_RANDOM_POLARFIRE_SOC) += mpfs-rng.o
> +obj-$(CONFIG_HW_RANDOM_ROCKCHIP) += rockchip-rng.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..18cdd91ad8c3
> --- /dev/null
> +++ b/drivers/char/hw_random/rockchip-rng.c
> @@ -0,0 +1,250 @@
> +// 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_0			0x0410
> +#define TRNG_RNG_DOUT_1			0x0414
> +#define TRNG_RNG_DOUT_2			0x0418
> +#define TRNG_RNG_DOUT_3			0x041c
> +#define TRNG_RNG_DOUT_4			0x0420
> +#define TRNG_RNG_DOUT_5			0x0424
> +#define TRNG_RNG_DOUT_6			0x0428
> +#define TRNG_RNG_DOUT_7			0x042c
> +
> +struct rk_rng {
> +	struct hwrng rng;
> +	void __iomem *base;
> +	struct reset_control *rst;
> +	int clk_num;
> +	struct clk_bulk_data *clk_bulks;
> +};
> +
> +/* The mask determine the bits that are updated */
> +static void rk_rng_write_ctl(struct rk_rng *rng, u32 val, u32 mask)
> +{
> +	writel_relaxed((mask << 16) | val, rng->base + TRNG_RNG_CTL);
> +}
> +
> +static int rk_rng_init(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	u32 reg;
> +	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 */
> +	reg = TRNG_RNG_CTL_LEN_256_BIT |

It's not reg. It's val.

> +		   TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> +		   TRNG_RNG_CTL_ENABLE;
> +	rk_rng_write_ctl(rk_rng, reg, 0xffff);
> +
> +	return 0;
> +}
> +
> +static void rk_rng_cleanup(struct hwrng *rng)
> +{
> +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> +	u32 reg;
> +
> +	/* stop TRNG */
> +	reg = 0;

It's not reg. It's val.

> +	rk_rng_write_ctl(rk_rng, reg, 0xffff);
> +
> +	/* 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);
> +	u32 reg;
> +	int ret = 0;
> +	int i;
> +
> +	pm_runtime_get_sync((struct device *) rk_rng->rng.priv);

Missing error handling.

> +
> +	/* Start collecting random data */
> +	reg = TRNG_RNG_CTL_START;

This is not usefull. Just use it directly in write call. Actually this
is heavy confusing, as reg suggests address. This would be val instead...

> +	rk_rng_write_ctl(rk_rng, reg, reg);
> +
> +	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 */
> +	ret = min_t(size_t, max, RK_RNG_MAX_BYTE);
> +	for (i = 0; i < ret; i += 4) {
> +		*(u32 *)(buf + i) = readl_relaxed(rk_rng->base + TRNG_RNG_DOUT_0 + i);
> +	}

This cannot be just memcpy_fromio?

> +
> +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;
> +}
> +
> +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(struct rk_rng), GFP_KERNEL);

sizeof(*rk_rng)

> +	if (!rk_rng)
> +		return -ENOMEM;
> +

Best regards,
Krzysztof

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

[Aurelien Jarno]

Hi,

Thanks for your feedback.

On 2022-11-29 10:24, Krzysztof Kozlowski wrote:
> On 28/11/2022 19:47, Aurelien Jarno wrote:
> > 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".
> 
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > ---
> >  .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
> >  1 file changed, 60 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..c2f5ef69cf07
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
> > @@ -0,0 +1,60 @@
> > +# 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 for some Rockchip SoCs
> 
> s/for some Rockchip SoCs/on Rokchip RK3568 SoC/

My point there is that this driver should also work for other Rockchip
SoCs like the RK3588, but 1) it support for this SoC is being added and
not yet available in the Linux kernel 2) it hasn't been tested.

Should we mark it as RK3568 specific (or rather RK356x) and change that
once a compatible entry is added for the RK3588?

> > +
> > +maintainers:
> > +  - Aurelien Jarno <aurelien@aurel32.net>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3568-rng
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: TRNG clock
> > +      - description: TRNG AHB clock
> > +
> > +  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.

Those names are based on <include/dt-bindings/clock/rk3568-cru.h> and
other drivers seems to have used those for the names. But I understand
that broken things could have been merged, so I am fine changing that to
core and ahb.

> > +
> > +  resets:
> > +    maxItems: 1
> > +

Regards
Aurelien

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

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

[Aurelien Jarno]

Hi,

Thanks for your feedback.

On 2022-11-29 10:33, Krzysztof Kozlowski wrote:
> On 28/11/2022 19:47, Aurelien Jarno 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..18cdd91ad8c3
> > --- /dev/null
> > +++ b/drivers/char/hw_random/rockchip-rng.c
> > @@ -0,0 +1,250 @@
> > +// 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_0			0x0410
> > +#define TRNG_RNG_DOUT_1			0x0414
> > +#define TRNG_RNG_DOUT_2			0x0418
> > +#define TRNG_RNG_DOUT_3			0x041c
> > +#define TRNG_RNG_DOUT_4			0x0420
> > +#define TRNG_RNG_DOUT_5			0x0424
> > +#define TRNG_RNG_DOUT_6			0x0428
> > +#define TRNG_RNG_DOUT_7			0x042c
> > +
> > +struct rk_rng {
> > +	struct hwrng rng;
> > +	void __iomem *base;
> > +	struct reset_control *rst;
> > +	int clk_num;
> > +	struct clk_bulk_data *clk_bulks;
> > +};
> > +
> > +/* The mask determine the bits that are updated */
> > +static void rk_rng_write_ctl(struct rk_rng *rng, u32 val, u32 mask)
> > +{
> > +	writel_relaxed((mask << 16) | val, rng->base + TRNG_RNG_CTL);
> > +}
> > +
> > +static int rk_rng_init(struct hwrng *rng)
> > +{
> > +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +	u32 reg;
> > +	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 */
> > +	reg = TRNG_RNG_CTL_LEN_256_BIT |
> 
> It's not reg. It's val.

Ok, I'll fix that.

> > +		   TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> > +		   TRNG_RNG_CTL_ENABLE;
> > +	rk_rng_write_ctl(rk_rng, reg, 0xffff);
> > +
> > +	return 0;
> > +}
> > +
> > +static void rk_rng_cleanup(struct hwrng *rng)
> > +{
> > +	struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +	u32 reg;
> > +
> > +	/* stop TRNG */
> > +	reg = 0;
> 
> It's not reg. It's val.

I'll also fix that one.

> > +	rk_rng_write_ctl(rk_rng, reg, 0xffff);
> > +
> > +	/* 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);
> > +	u32 reg;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
> 
> Missing error handling.

I'll fix that.
 
> > +
> > +	/* Start collecting random data */
> > +	reg = TRNG_RNG_CTL_START;
> 
> This is not usefull. Just use it directly in write call. Actually this
> is heavy confusing, as reg suggests address. This would be val instead...

This was to avoid passing the value twice, as it is used for both the
mask and the value. But I can pass it directly if it's better.

> > +	rk_rng_write_ctl(rk_rng, reg, reg);
> > +
> > +	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 */
> > +	ret = min_t(size_t, max, RK_RNG_MAX_BYTE);
> > +	for (i = 0; i < ret; i += 4) {
> > +		*(u32 *)(buf + i) = readl_relaxed(rk_rng->base + TRNG_RNG_DOUT_0 + i);
> > +	}
> 
> This cannot be just memcpy_fromio?

According to the TRM, we should ensure 32-bit accesses, so I am not sure
memcpy_fromio will work. However it seems __ioread32_copy could be a
good replacement.

> > +
> > +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;
> > +}
> > +
> > +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(struct rk_rng), GFP_KERNEL);
> 
> sizeof(*rk_rng)

I'll fix that.

> > +	if (!rk_rng)
> > +		return -ENOMEM;
> > +

Regards
Aurelien


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

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

[Krzysztof Kozlowski]

On 02/12/2022 20:20, Aurelien Jarno wrote:
> Hi,
> 
> Thanks for your feedback.
> 
> On 2022-11-29 10:24, Krzysztof Kozlowski wrote:
>> On 28/11/2022 19:47, Aurelien Jarno wrote:
>>> 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".
>>
>>>
>>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>>> ---
>>>  .../bindings/rng/rockchip,rk3568-rng.yaml     | 60 +++++++++++++++++++
>>>  1 file changed, 60 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..c2f5ef69cf07
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/rng/rockchip,rk3568-rng.yaml
>>> @@ -0,0 +1,60 @@
>>> +# 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 for some Rockchip SoCs
>>
>> s/for some Rockchip SoCs/on Rokchip RK3568 SoC/
> 
> My point there is that this driver should also work for other Rockchip
> SoCs like the RK3588, but 1)

Driver maybe less, but bindings might not.

> it support for this SoC is being added and
> not yet available in the Linux kernel 2) it hasn't been tested.
> 
> Should we mark it as RK3568 specific (or rather RK356x) and change that
> once a compatible entry is added for the RK3588?

Describe what you are adding here, not something else.

> 
>>> +
>>> +maintainers:
>>> +  - Aurelien Jarno <aurelien@aurel32.net>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - rockchip,rk3568-rng
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: TRNG clock
>>> +      - description: TRNG AHB clock
>>> +
>>> +  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.
> 
> Those names are based on <include/dt-bindings/clock/rk3568-cru.h> and

clock-names is not for the actual name of the clock feeding it, but
rather name of input of the device. Reader-friendly.

> other drivers seems to have used those for the names. But I understand
> that broken things could have been merged, so I am fine changing that to
> core and ahb.
> 
>>> +
>>> +  resets:
>>> +    maxItems: 1
>>> +
> 
> Regards
> Aurelien
> 

Best regards,
Krzysztof

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

[Jason A. Donenfeld]

On Mon, Nov 28, 2022 at 07:47:17PM +0100, Aurelien Jarno wrote:
> 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.

Can't you reduce it even more to get 100%? All we need is 32 bytes every
once in a while.

> +	rk_rng->rng.quality = 900;

If your intention is "90%", this should be 921 or 922, because the
quality knob is out of 1024, not 1000.

Herbert - this seems like a fairly common pitfall I've seen all over the
place. It might be worth making a mental memo to reject or ask questions
about numbers that seem "too round", when you look at these sorts of
patches.

Jason

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

[Jason A. Donenfeld]

On Mon, Dec 05, 2022 at 02:13:45PM +0100, Jason A. Donenfeld wrote:
> If your intention is "90%", this should be 921 or 922, because the
> quality knob is out of 1024, not 1000.
> 
> Herbert - this seems like a fairly common pitfall I've seen all over the
> place. It might be worth making a mental memo to reject or ask questions
> about numbers that seem "too round", when you look at these sorts of
> patches.

Or alternatively we could introduce a cheesy macro like:

   #define HWRNG_PERCENTAGE(p) ((p) * 1024 / 100)

and then enforce that everyone use that. But that's a bit wacky too, in
the sense of - why is anybody using a non-obvious percentage in the first
place. Like if you see "512" (or better, "1024 / 2"), okay fine, it's a
device that guarantees 50%, which seems like a common enough physical
thing. But if we see "HWRNG_PERCENTAGE(90)", the first question is why?
What causes that?  Seems very weird. And it's probably wrong.

But if it *is* right, that deserves a big comment with explanation,
where the calculation for that "921" literal can be explained in full,
or, better, evaluated as a constant expression in terms of hardware
constants -- something like
HW_CLOCKRATE/FROBNICATOR_INTENSITY*1024/TURBOENCABULATION_MODE_WEIGHT,
and then it all makes sense.

So maybe rather than a macro or accepting barebones "921" values, if the
value isn't 1024 (0), then it needs a comment + an expression computing
the value.

Seem reasonable?

Jason

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

[Aurelien Jarno]

Hi,

On 2022-12-05 14:13, Jason A. Donenfeld wrote:
> On Mon, Nov 28, 2022 at 07:47:17PM +0100, Aurelien Jarno wrote:
> > 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.
> 
> Can't you reduce it even more to get 100%? All we need is 32 bytes every
> once in a while.

From what I understood, we get the raw stream of the TRNG, there is no
conditionner and the TRNG is not FIPS compliant. So even with the
slowest speed, you don't reach 100% and you only get a very small
increase in the quality while it's way more slower.

> > +	rk_rng->rng.quality = 900;
> 
> If your intention is "90%", this should be 921 or 922, because the
> quality knob is out of 1024, not 1000.

Well I am not sure it really matters. 90% is actually conservative, it's
the worst case I have seen, rounded down. However I often get much
better quality, see for instance the following run:

| Copyright (c) 2004 by Henrique de Moraes Holschuh
| This is free software; see the source for copying conditions.  There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
| 
| rngtest: starting FIPS tests...
| rngtest: entropy source drained
| rngtest: bits received from input: 16777216
| rngtest: FIPS 140-2 successes: 819
| rngtest: FIPS 140-2 failures: 19
| rngtest: FIPS 140-2(2001-10-10) Monobit: 17
| rngtest: FIPS 140-2(2001-10-10) Poker: 0
| rngtest: FIPS 140-2(2001-10-10) Runs: 2
| rngtest: FIPS 140-2(2001-10-10) Long run: 2
| rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
| rngtest: input channel speed: (min=132.138; avg=137.848; max=147.308)Kibits/s
| rngtest: FIPS tests speed: (min=16.924; avg=20.272; max=20.823)Mibits/s
| rngtest: Program run time: 119647459 microseconds

Does the exact value has an importance there? I thought it was just
important to not overestimate the quality.

Regards
Aurelien

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

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

[Jason A. Donenfeld]

Hi Aurelien,

On Mon, Dec 05, 2022 at 10:34:54PM +0100, Aurelien Jarno wrote:
> Well I am not sure it really matters. 90% is actually conservative, it's
> the worst case I have seen, rounded down. However I often get much
> better quality, see for instance the following run:
> 
> | Copyright (c) 2004 by Henrique de Moraes Holschuh
> | This is free software; see the source for copying conditions.  There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> | 
> | rngtest: starting FIPS tests...
> | rngtest: entropy source drained
> | rngtest: bits received from input: 16777216
> | rngtest: FIPS 140-2 successes: 819
> | rngtest: FIPS 140-2 failures: 19
> | rngtest: FIPS 140-2(2001-10-10) Monobit: 17
> | rngtest: FIPS 140-2(2001-10-10) Poker: 0
> | rngtest: FIPS 140-2(2001-10-10) Runs: 2
> | rngtest: FIPS 140-2(2001-10-10) Long run: 2
> | rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> | rngtest: input channel speed: (min=132.138; avg=137.848; max=147.308)Kibits/s
> | rngtest: FIPS tests speed: (min=16.924; avg=20.272; max=20.823)Mibits/s
> | rngtest: Program run time: 119647459 microseconds
> 
> Does the exact value has an importance there? I thought it was just
> important to not overestimate the quality.

That's the right principle. I just worry about estimating it like that
from looking at the output, rather than being derived from some
knowledge about the hardware. Maybe 50% (quality=512) is more
reasonable, so that it collects two bits for every one?

Jason