[PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Hadar Gat]

The Arm CryptoCell is a hardware security engine.
This patch introduces driver for its TRNG (True Random Number Generator)
engine.

v7 change: in arm-cctrng.yaml, removed unneeded 'minitems'

v6 change: add missing initialization of hwrng quality.

v5 changes:
	1. in arm-cctrng.yaml, fixed error in 'make dt_binding_check'
	2. in cctrng.c, clean up cctrng clock handling

v4 changes: update arm-cctrng.yaml to conform with json-schema standard.

v3 change: removed few unneeded "#ifdef CONFIG_PM" from the code.

v2 changes: fixed 'make dt_bnding_check' errors.

Hadar Gat (3):
  dt-bindings: add device tree binding for Arm CryptoCell trng engine
  hw_random: cctrng: introduce Arm CryptoCell driver
  MAINTAINERS: add HG as cctrng maintainer

 .../devicetree/bindings/rng/arm-cctrng.yaml        |  54 ++
 MAINTAINERS                                        |   9 +
 drivers/char/hw_random/Kconfig                     |  12 +
 drivers/char/hw_random/Makefile                    |   1 +
 drivers/char/hw_random/cctrng.c                    | 736 +++++++++++++++++++++
 drivers/char/hw_random/cctrng.h                    |  72 ++
 6 files changed, 884 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/arm-cctrng.yaml
 create mode 100644 drivers/char/hw_random/cctrng.c
 create mode 100644 drivers/char/hw_random/cctrng.h

-- 
2.7.4

[PATCH v7 1/3] dt-bindings: add device tree binding for Arm CryptoCell trng engine

[Hadar Gat]

The Arm CryptoCell is a hardware security engine. This patch adds DT
bindings for its TRNG (True Random Number Generator) engine.

Signed-off-by: Hadar Gat <hadar.gat@arm.com>
---
 .../devicetree/bindings/rng/arm-cctrng.yaml        | 54 ++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/arm-cctrng.yaml

diff --git a/Documentation/devicetree/bindings/rng/arm-cctrng.yaml b/Documentation/devicetree/bindings/rng/arm-cctrng.yaml
new file mode 100644
index 0000000..ca6aad1
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/arm-cctrng.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/rng/arm-cctrng.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm TrustZone CryptoCell TRNG engine
+
+maintainers:
+  - Hadar Gat <hadar.gat@arm.com>
+
+description: |+
+  Arm TrustZone CryptoCell TRNG (True Random Number Generator) engine.
+
+properties:
+  compatible:
+    enum:
+      - arm,cryptocell-713-trng
+      - arm,cryptocell-703-trng
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+  arm,rosc-ratio:
+    description:
+      Arm TrustZone CryptoCell TRNG engine has 4 ring oscillators.
+      Sampling ratio values for these 4 ring oscillators. (from calibration)
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32-array
+      - items:
+          maxItems: 4
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - interrupts
+  - reg
+  - arm,rosc-ratio
+
+additionalProperties: false
+
+examples:
+  - |
+    arm_cctrng: rng@60000000 {
+        compatible = "arm,cryptocell-713-trng";
+        interrupts = <0 29 4>;
+        reg = <0x60000000 0x10000>;
+        arm,rosc-ratio = <5000 1000 500 0>;
+    };
-- 
2.7.4

[PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Hadar Gat]

Introduce low level Arm CryptoCell TRNG HW support.

Signed-off-by: Hadar Gat <hadar.gat@arm.com>
---
 drivers/char/hw_random/Kconfig  |  12 +
 drivers/char/hw_random/Makefile |   1 +
 drivers/char/hw_random/cctrng.c | 736 ++++++++++++++++++++++++++++++++++++++++
 drivers/char/hw_random/cctrng.h |  72 ++++
 4 files changed, 821 insertions(+)
 create mode 100644 drivers/char/hw_random/cctrng.c
 create mode 100644 drivers/char/hw_random/cctrng.h

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 9bc46da..848f26f 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -474,6 +474,18 @@ config HW_RANDOM_KEYSTONE
 	help
 	  This option enables Keystone's hardware random generator.
 
+config HW_RANDOM_CCTRNG
+	tristate "Arm CryptoCell True Random Number Generator support"
+	default HW_RANDOM
+	help
+	  This driver provides support for the True Random Number
+	  Generator available in Arm TrustZone CryptoCell.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called cctrng.
+
+	  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 a7801b4..2c67247 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -41,3 +41,4 @@ obj-$(CONFIG_HW_RANDOM_S390) += s390-trng.o
 obj-$(CONFIG_HW_RANDOM_KEYSTONE) += ks-sa-rng.o
 obj-$(CONFIG_HW_RANDOM_OPTEE) += optee-rng.o
 obj-$(CONFIG_HW_RANDOM_NPCM) += npcm-rng.o
+obj-$(CONFIG_HW_RANDOM_CCTRNG) += cctrng.o
diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
new file mode 100644
index 0000000..bdcd562
--- /dev/null
+++ b/drivers/char/hw_random/cctrng.c
@@ -0,0 +1,736 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2019-2020 ARM Limited or its affiliates. */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/workqueue.h>
+#include <linux/circ_buf.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/bitfield.h>
+
+#include "cctrng.h"
+
+#define CC_REG_LOW(name)  (name ## _BIT_SHIFT)
+#define CC_REG_HIGH(name) (CC_REG_LOW(name) + name ## _BIT_SIZE - 1)
+#define CC_GENMASK(name)  GENMASK(CC_REG_HIGH(name), CC_REG_LOW(name))
+
+#define CC_REG_FLD_GET(reg_name, fld_name, reg_val)     \
+	(FIELD_GET(CC_GENMASK(CC_ ## reg_name ## _ ## fld_name), reg_val))
+
+#define CC_HW_RESET_LOOP_COUNT 10
+#define CC_TRNG_SUSPEND_TIMEOUT 3000
+
+/* data circular buffer in words must be:
+ *  - of a power-of-2 size (limitation of circ_buf.h macros)
+ *  - at least 6, the size generated in the EHR according to HW implementation
+ */
+#define CCTRNG_DATA_BUF_WORDS 32
+
+/* The timeout for the TRNG operation should be calculated with the formula:
+ * Timeout = EHR_NUM * VN_COEFF * EHR_LENGTH * SAMPLE_CNT * SCALE_VALUE
+ * while:
+ *  - SAMPLE_CNT is input value from the characterisation process
+ *  - all the rest are constants
+ */
+#define EHR_NUM 1
+#define VN_COEFF 4
+#define EHR_LENGTH CC_TRNG_EHR_IN_BITS
+#define SCALE_VALUE 2
+#define CCTRNG_TIMEOUT(smpl_cnt) \
+	(EHR_NUM * VN_COEFF * EHR_LENGTH * smpl_cnt * SCALE_VALUE)
+
+struct cctrng_drvdata {
+	struct platform_device *pdev;
+	void __iomem *cc_base;
+	struct clk *clk;
+	struct hwrng rng;
+	u32 active_rosc;
+	/* Sampling interval for each ring oscillator:
+	 * count of ring oscillator cycles between consecutive bits sampling.
+	 * Value of 0 indicates non-valid rosc
+	 */
+	u32 smpl_ratio[CC_TRNG_NUM_OF_ROSCS];
+
+	u32 data_buf[CCTRNG_DATA_BUF_WORDS];
+	struct circ_buf circ;
+	struct work_struct compwork;
+	struct work_struct startwork;
+
+	/* pending_hw - 1 when HW is pending, 0 when it is idle */
+	atomic_t pending_hw;
+
+	/* protects against multiple concurrent consumers of data_buf */
+	spinlock_t read_lock;
+};
+
+
+/* functions for write/read CC registers */
+static inline void cc_iowrite(struct cctrng_drvdata *drvdata, u32 reg, u32 val)
+{
+	iowrite32(val, (drvdata->cc_base + reg));
+}
+static inline u32 cc_ioread(struct cctrng_drvdata *drvdata, u32 reg)
+{
+	return ioread32(drvdata->cc_base + reg);
+}
+
+
+static int cc_trng_pm_get(struct device *dev)
+{
+	int rc = 0;
+
+	rc = pm_runtime_get_sync(dev);
+
+	/* pm_runtime_get_sync() can return 1 as a valid return code */
+	return (rc == 1 ? 0 : rc);
+}
+
+static void cc_trng_pm_put_suspend(struct device *dev)
+{
+	int rc = 0;
+
+	pm_runtime_mark_last_busy(dev);
+	rc = pm_runtime_put_autosuspend(dev);
+	if (rc)
+		dev_err(dev, "pm_runtime_put_autosuspend returned %x\n", rc);
+}
+
+static int cc_trng_pm_init(struct cctrng_drvdata *drvdata)
+{
+	struct device *dev = &(drvdata->pdev->dev);
+
+	/* must be before the enabling to avoid redundant suspending */
+	pm_runtime_set_autosuspend_delay(dev, CC_TRNG_SUSPEND_TIMEOUT);
+	pm_runtime_use_autosuspend(dev);
+	/* set us as active - note we won't do PM ops until cc_trng_pm_go()! */
+	return pm_runtime_set_active(dev);
+}
+
+static void cc_trng_pm_go(struct cctrng_drvdata *drvdata)
+{
+	struct device *dev = &(drvdata->pdev->dev);
+
+	/* enable the PM module*/
+	pm_runtime_enable(dev);
+}
+
+static void cc_trng_pm_fini(struct cctrng_drvdata *drvdata)
+{
+	struct device *dev = &(drvdata->pdev->dev);
+
+	pm_runtime_disable(dev);
+}
+
+
+static inline int cc_trng_parse_sampling_ratio(struct cctrng_drvdata *drvdata)
+{
+	struct device *dev = &(drvdata->pdev->dev);
+	struct device_node *np = drvdata->pdev->dev.of_node;
+	int rc;
+	int i;
+	/* ret will be set to 0 if at least one rosc has (sampling ratio > 0) */
+	int ret = -EINVAL;
+
+	rc = of_property_read_u32_array(np, "arm,rosc-ratio",
+					drvdata->smpl_ratio,
+					CC_TRNG_NUM_OF_ROSCS);
+	if (rc) {
+		/* arm,rosc-ratio was not found in device tree */
+		return rc;
+	}
+
+	/* verify that at least one rosc has (sampling ratio > 0) */
+	for (i = 0; i < CC_TRNG_NUM_OF_ROSCS; ++i) {
+		dev_dbg(dev, "rosc %d sampling ratio %u",
+			i, drvdata->smpl_ratio[i]);
+
+		if (drvdata->smpl_ratio[i] > 0)
+			ret = 0;
+	}
+
+	return ret;
+}
+
+static int cc_trng_change_rosc(struct cctrng_drvdata *drvdata)
+{
+	struct device *dev = &(drvdata->pdev->dev);
+
+	dev_dbg(dev, "cctrng change rosc (was %d)\n", drvdata->active_rosc);
+	drvdata->active_rosc += 1;
+
+	while (drvdata->active_rosc < CC_TRNG_NUM_OF_ROSCS) {
+		if (drvdata->smpl_ratio[drvdata->active_rosc] > 0)
+			return 0;
+
+		drvdata->active_rosc += 1;
+	}
+	return -EINVAL;
+}
+
+
+static void cc_trng_enable_rnd_source(struct cctrng_drvdata *drvdata)
+{
+	u32 max_cycles;
+
+	/* Set watchdog threshold to maximal allowed time (in CPU cycles) */
+	max_cycles = CCTRNG_TIMEOUT(drvdata->smpl_ratio[drvdata->active_rosc]);
+	cc_iowrite(drvdata, CC_RNG_WATCHDOG_VAL_REG_OFFSET, max_cycles);
+
+	/* enable the RND source */
+	cc_iowrite(drvdata, CC_RND_SOURCE_ENABLE_REG_OFFSET, 0x1);
+
+	/* unmask RNG interrupts */
+	cc_iowrite(drvdata, CC_RNG_IMR_REG_OFFSET, (u32)~CC_RNG_INT_MASK);
+}
+
+
+/* increase circular data buffer index (head/tail) */
+static inline void circ_idx_inc(int *idx, int bytes)
+{
+	*idx += (bytes + 3) >> 2;
+	*idx &= (CCTRNG_DATA_BUF_WORDS - 1);
+}
+
+static inline size_t circ_buf_space(struct cctrng_drvdata *drvdata)
+{
+	return CIRC_SPACE(drvdata->circ.head,
+			  drvdata->circ.tail, CCTRNG_DATA_BUF_WORDS);
+
+}
+
+static int cctrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+	/* current implementation ignores "wait" */
+
+	struct cctrng_drvdata *drvdata = (struct cctrng_drvdata *)rng->priv;
+	struct device *dev = &(drvdata->pdev->dev);
+	u32 *buf = (u32 *)drvdata->circ.buf;
+	size_t copied = 0;
+	size_t cnt_w;
+	size_t size;
+	size_t left;
+
+	if (!spin_trylock(&drvdata->read_lock)) {
+		/* concurrent consumers from data_buf cannot be served */
+		dev_dbg_ratelimited(dev, "unable to hold lock\n");
+		return 0;
+	}
+
+	/* copy till end of data buffer (without wrap back) */
+	cnt_w = CIRC_CNT_TO_END(drvdata->circ.head,
+				drvdata->circ.tail, CCTRNG_DATA_BUF_WORDS);
+	size = min((cnt_w<<2), max);
+	memcpy(data, &(buf[drvdata->circ.tail]), size);
+	copied = size;
+	circ_idx_inc(&drvdata->circ.tail, size);
+	/* copy rest of data in data buffer */
+	left = max - copied;
+	if (left > 0) {
+		cnt_w = CIRC_CNT(drvdata->circ.head,
+				 drvdata->circ.tail, CCTRNG_DATA_BUF_WORDS);
+		size = min((cnt_w<<2), left);
+		memcpy(data, &(buf[drvdata->circ.tail]), size);
+		copied += size;
+		circ_idx_inc(&drvdata->circ.tail, size);
+	}
+
+	spin_unlock(&drvdata->read_lock);
+
+	if (circ_buf_space(drvdata) >= CC_TRNG_EHR_IN_WORDS) {
+		if (atomic_cmpxchg(&drvdata->pending_hw, 0, 1) == 0) {
+			/* re-check space in buffer to avoid potential race */
+			if (circ_buf_space(drvdata) >= CC_TRNG_EHR_IN_WORDS) {
+				/* increment device's usage counter */
+				int rc = cc_trng_pm_get(dev);
+
+				if (rc) {
+					dev_err(dev,
+						"cc_trng_pm_get returned %x\n",
+						rc);
+					return rc;
+				}
+
+				/* schedule execution of deferred work handler
+				 * for filling of data buffer
+				 */
+				schedule_work(&drvdata->startwork);
+			} else {
+				atomic_set(&drvdata->pending_hw, 0);
+			}
+		}
+	}
+
+	return copied;
+}
+
+static void cc_trng_hw_trigger(struct cctrng_drvdata *drvdata)
+{
+	u32 tmp_smpl_cnt = 0;
+	struct device *dev = &(drvdata->pdev->dev);
+
+	dev_dbg(dev, "cctrng hw trigger.\n");
+
+	/* enable the HW RND clock */
+	cc_iowrite(drvdata, CC_RNG_CLK_ENABLE_REG_OFFSET, 0x1);
+
+	/* do software reset */
+	cc_iowrite(drvdata, CC_RNG_SW_RESET_REG_OFFSET, 0x1);
+	/* in order to verify that the reset has completed,
+	 * the sample count need to be verified
+	 */
+	do {
+		/* enable the HW RND clock   */
+		cc_iowrite(drvdata, CC_RNG_CLK_ENABLE_REG_OFFSET, 0x1);
+
+		/* set sampling ratio (rng_clocks) between consecutive bits */
+		cc_iowrite(drvdata, CC_SAMPLE_CNT1_REG_OFFSET,
+			   drvdata->smpl_ratio[drvdata->active_rosc]);
+
+		/* read the sampling ratio  */
+		tmp_smpl_cnt = cc_ioread(drvdata, CC_SAMPLE_CNT1_REG_OFFSET);
+
+	} while (tmp_smpl_cnt != drvdata->smpl_ratio[drvdata->active_rosc]);
+
+	/* disable the RND source for setting new parameters in HW */
+	cc_iowrite(drvdata, CC_RND_SOURCE_ENABLE_REG_OFFSET, 0);
+
+	cc_iowrite(drvdata, CC_RNG_ICR_REG_OFFSET, 0xFFFFFFFF);
+
+	cc_iowrite(drvdata, CC_TRNG_CONFIG_REG_OFFSET, drvdata->active_rosc);
+
+	/* Debug Control register: set to 0 - no bypasses */
+	cc_iowrite(drvdata, CC_TRNG_DEBUG_CONTROL_REG_OFFSET, 0);
+
+	cc_trng_enable_rnd_source(drvdata);
+}
+
+void cc_trng_compwork_handler(struct work_struct *w)
+{
+	u32 isr = 0;
+	u32 ehr_valid = 0;
+	struct cctrng_drvdata *drvdata =
+			container_of(w, struct cctrng_drvdata, compwork);
+	struct device *dev = &(drvdata->pdev->dev);
+	int i;
+
+	/* stop DMA and the RNG source */
+	cc_iowrite(drvdata, CC_RNG_DMA_ENABLE_REG_OFFSET, 0);
+	cc_iowrite(drvdata, CC_RND_SOURCE_ENABLE_REG_OFFSET, 0);
+
+	/* read RNG_ISR and check for errors */
+	isr = cc_ioread(drvdata, CC_RNG_ISR_REG_OFFSET);
+	ehr_valid = CC_REG_FLD_GET(RNG_ISR, EHR_VALID, isr);
+	dev_dbg(dev, "Got RNG_ISR=0x%08X (EHR_VALID=%u)\n", isr, ehr_valid);
+
+#ifdef CONFIG_CRYPTO_FIPS
+	if (CC_REG_FLD_GET(RNG_ISR, CRNGT_ERR, isr) && fips_enabled) {
+		fips_fail_notify();
+		/* FIPS error is fatal */
+		panic("Got HW CRNGT error while fips is enabled!\n");
+	}
+#endif
+
+	/* Clear all pending RNG interrupts */
+	cc_iowrite(drvdata, CC_RNG_ICR_REG_OFFSET, isr);
+
+
+	if (!ehr_valid) {
+		/* in case of AUTOCORR/TIMEOUT error, try the next ROSC */
+		if (CC_REG_FLD_GET(RNG_ISR, AUTOCORR_ERR, isr) ||
+				CC_REG_FLD_GET(RNG_ISR, WATCHDOG, isr)) {
+			dev_dbg(dev, "cctrng autocorr/timeout error.\n");
+			goto next_rosc;
+		}
+
+		/* in case of VN error, ignore it */
+	}
+
+	/* read EHR data from registers */
+	for (i = 0; i < CC_TRNG_EHR_IN_WORDS; i++) {
+		/* calc word ptr in data_buf */
+		u32 *buf = (u32 *)drvdata->circ.buf;
+
+		buf[drvdata->circ.head] = cc_ioread(drvdata,
+				CC_EHR_DATA_0_REG_OFFSET + (i*sizeof(u32)));
+
+		/* EHR_DATA registers are cleared on read. In case 0 value was
+		 * returned, restart the entropy collection.
+		 */
+		if (buf[drvdata->circ.head] == 0) {
+			dev_dbg(dev, "Got 0 value in EHR. active_rosc %u\n",
+				drvdata->active_rosc);
+			goto next_rosc;
+		}
+
+		circ_idx_inc(&drvdata->circ.head, 1<<2);
+	}
+
+	atomic_set(&drvdata->pending_hw, 0);
+
+	/* continue to fill data buffer if needed */
+	if (circ_buf_space(drvdata) >= CC_TRNG_EHR_IN_WORDS) {
+		if (atomic_cmpxchg(&drvdata->pending_hw, 0, 1) == 0) {
+			/* Re-enable rnd source */
+			cc_trng_enable_rnd_source(drvdata);
+			return;
+		}
+	}
+
+	cc_trng_pm_put_suspend(dev);
+
+	dev_dbg(dev, "compwork handler done\n");
+	return;
+
+next_rosc:
+	if ((circ_buf_space(drvdata) >= CC_TRNG_EHR_IN_WORDS) &&
+			(cc_trng_change_rosc(drvdata) == 0)) {
+		/* trigger trng hw with next rosc */
+		cc_trng_hw_trigger(drvdata);
+	} else {
+		atomic_set(&drvdata->pending_hw, 0);
+		cc_trng_pm_put_suspend(dev);
+	}
+}
+
+static irqreturn_t cc_isr(int irq, void *dev_id)
+{
+	struct cctrng_drvdata *drvdata = (struct cctrng_drvdata *)dev_id;
+	struct device *dev = &(drvdata->pdev->dev);
+	u32 irr;
+
+	/* if driver suspended return, probably shared interrupt */
+	if (pm_runtime_suspended(dev))
+		return IRQ_NONE;
+
+	/* read the interrupt status */
+	irr = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
+	dev_dbg(dev, "Got IRR=0x%08X\n", irr);
+
+	if (irr == 0) /* Probably shared interrupt line */
+		return IRQ_NONE;
+
+	/* clear interrupt - must be before processing events */
+	cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, irr);
+
+	/* RNG interrupt - most probable */
+	if (irr & CC_HOST_RNG_IRQ_MASK) {
+		/* Mask RNG interrupts - will be unmasked in deferred work */
+		cc_iowrite(drvdata, CC_RNG_IMR_REG_OFFSET, 0xFFFFFFFF);
+
+		/* We clear RNG interrupt here,
+		 * to avoid it from firing as we'll unmask RNG interrupts.
+		 */
+		cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET,
+			   CC_HOST_RNG_IRQ_MASK);
+
+		irr &= ~CC_HOST_RNG_IRQ_MASK;
+
+		/* schedule execution of deferred work handler */
+		schedule_work(&drvdata->compwork);
+	}
+
+	if (irr) {
+		dev_dbg_ratelimited(dev,
+				"IRR includes unknown cause bits (0x%08X)\n",
+				irr);
+		/* Just warning */
+	}
+
+	return IRQ_HANDLED;
+}
+
+void cc_trng_startwork_handler(struct work_struct *w)
+{
+	struct cctrng_drvdata *drvdata =
+			container_of(w, struct cctrng_drvdata, startwork);
+
+	drvdata->active_rosc = 0;
+	cc_trng_hw_trigger(drvdata);
+}
+
+
+static int cc_trng_clk_init(struct cctrng_drvdata *drvdata)
+{
+	struct clk *clk;
+	struct device *dev = &(drvdata->pdev->dev);
+	int rc = 0;
+
+	clk = devm_clk_get_optional(dev, NULL);
+	if (IS_ERR(clk)) {
+		if (PTR_ERR(clk) != -EPROBE_DEFER)
+			dev_err(dev, "Error getting clock: %pe\n", clk);
+		return PTR_ERR(clk);
+	}
+	drvdata->clk = clk;
+
+	rc = clk_prepare_enable(drvdata->clk);
+	if (rc) {
+		dev_err(dev, "Failed to enable clock\n");
+		return rc;
+	}
+
+	return 0;
+}
+
+static void cc_trng_clk_fini(struct cctrng_drvdata *drvdata)
+{
+	clk_disable_unprepare(drvdata->clk);
+}
+
+
+static int cctrng_probe(struct platform_device *pdev)
+{
+	struct resource *req_mem_cc_regs = NULL;
+	struct cctrng_drvdata *drvdata;
+	struct device *dev = &pdev->dev;
+	int rc = 0;
+	u32 val;
+	int irq;
+
+	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+
+	drvdata->rng.name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!drvdata->rng.name)
+		return -ENOMEM;
+
+	drvdata->rng.read = cctrng_read;
+	drvdata->rng.priv = (unsigned long)drvdata;
+	drvdata->rng.quality = CC_TRNG_QUALITY;
+
+	platform_set_drvdata(pdev, drvdata);
+	drvdata->pdev = pdev;
+
+	drvdata->circ.buf = (char *)drvdata->data_buf;
+
+	/* Get device resources */
+	/* First CC registers space */
+	req_mem_cc_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	/* Map registers space */
+	drvdata->cc_base = devm_ioremap_resource(dev, req_mem_cc_regs);
+	if (IS_ERR(drvdata->cc_base)) {
+		dev_err(dev, "Failed to ioremap registers");
+		return PTR_ERR(drvdata->cc_base);
+	}
+
+	dev_dbg(dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs->name,
+		req_mem_cc_regs);
+	dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
+		&req_mem_cc_regs->start, drvdata->cc_base);
+
+	/* Then IRQ */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "Failed getting IRQ resource\n");
+		return irq;
+	}
+
+	/* parse sampling rate from device tree */
+	rc = cc_trng_parse_sampling_ratio(drvdata);
+	if (rc) {
+		dev_err(dev, "Failed to get legal sampling ratio for rosc\n");
+		return rc;
+	}
+
+	rc = cc_trng_clk_init(drvdata);
+	if (rc) {
+		dev_err(dev, "cc_trng_clk_init failed\n");
+		return rc;
+	}
+
+	INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
+	INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
+	spin_lock_init(&drvdata->read_lock);
+
+	/* register the driver isr function */
+	rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng", drvdata);
+	if (rc) {
+		dev_err(dev, "Could not register to interrupt %d\n", irq);
+		goto post_clk_err;
+	}
+	dev_dbg(dev, "Registered to IRQ: %d\n", irq);
+
+	/* Clear all pending interrupts */
+	val = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
+	dev_dbg(dev, "IRR=0x%08X\n", val);
+	cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, val);
+
+	/* unmask HOST RNG interrupt */
+	cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
+		   cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
+		   ~CC_HOST_RNG_IRQ_MASK);
+
+	/* init PM */
+	rc = cc_trng_pm_init(drvdata);
+	if (rc) {
+		dev_err(dev, "cc_trng_pm_init failed\n");
+		goto post_clk_err;
+	}
+
+	/* increment device's usage counter */
+	rc = cc_trng_pm_get(dev);
+	if (rc) {
+		dev_err(dev, "cc_trng_pm_get returned %x\n", rc);
+		goto post_pm_err;
+	}
+
+	/* set pending_hw to verify that HW won't be triggered from read */
+	atomic_set(&drvdata->pending_hw, 1);
+
+	/* registration of the hwrng device */
+	rc = hwrng_register(&drvdata->rng);
+	if (rc) {
+		dev_err(dev, "Could not register hwrng device.\n");
+		goto post_pm_err;
+	}
+
+	/* trigger HW to start generate data */
+	drvdata->active_rosc = 0;
+	cc_trng_hw_trigger(drvdata);
+
+	/* All set, we can allow auto-suspend */
+	cc_trng_pm_go(drvdata);
+
+	dev_info(dev, "ARM cctrng device initialized\n");
+
+	return 0;
+
+post_pm_err:
+	cc_trng_pm_fini(drvdata);
+
+post_clk_err:
+	cc_trng_clk_fini(drvdata);
+
+	return rc;
+}
+
+static int cctrng_remove(struct platform_device *pdev)
+{
+	struct cctrng_drvdata *drvdata = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+
+	dev_dbg(dev, "Releasing cctrng resources...\n");
+
+	hwrng_unregister(&drvdata->rng);
+
+	cc_trng_pm_fini(drvdata);
+
+	cc_trng_clk_fini(drvdata);
+
+	dev_info(dev, "ARM cctrng device terminated\n");
+
+	return 0;
+}
+
+static int __maybe_unused cctrng_suspend(struct device *dev)
+{
+	struct cctrng_drvdata *drvdata = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "set HOST_POWER_DOWN_EN\n");
+	cc_iowrite(drvdata, CC_HOST_POWER_DOWN_EN_REG_OFFSET,
+			POWER_DOWN_ENABLE);
+
+	clk_disable_unprepare(drvdata->clk);
+
+	return 0;
+}
+
+static bool cctrng_wait_for_reset_completion(struct cctrng_drvdata *drvdata)
+{
+	unsigned int val;
+	unsigned int i;
+
+	for (i = 0; i < CC_HW_RESET_LOOP_COUNT; i++) {
+		/* in cc7x3 NVM_IS_IDLE indicates that CC reset is
+		 *  completed and device is fully functional
+		 */
+		val = cc_ioread(drvdata, CC_NVM_IS_IDLE_REG_OFFSET);
+		if (val & BIT(CC_NVM_IS_IDLE_VALUE_BIT_SHIFT)) {
+			/* hw indicate reset completed */
+			return true;
+		}
+		/* allow scheduling other process on the processor */
+		schedule();
+	}
+	/* reset not completed */
+	return false;
+}
+
+static int __maybe_unused cctrng_resume(struct device *dev)
+{
+	struct cctrng_drvdata *drvdata = dev_get_drvdata(dev);
+	int rc;
+
+	dev_dbg(dev, "unset HOST_POWER_DOWN_EN\n");
+	/* Enables the device source clk */
+	rc = clk_prepare_enable(drvdata->clk);
+	if (rc) {
+		dev_err(dev, "failed getting clock back on. We're toast.\n");
+		return rc;
+	}
+
+	/* wait for Cryptocell reset completion */
+	if (!cctrng_wait_for_reset_completion(drvdata)) {
+		dev_err(dev, "Cryptocell reset not completed");
+		return -EBUSY;
+	}
+
+	/* unmask HOST RNG interrupt */
+	cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
+		   cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
+		   ~CC_HOST_RNG_IRQ_MASK);
+
+	cc_iowrite(drvdata, CC_HOST_POWER_DOWN_EN_REG_OFFSET,
+		   POWER_DOWN_DISABLE);
+
+	return 0;
+}
+
+static UNIVERSAL_DEV_PM_OPS(cctrng_pm, cctrng_suspend, cctrng_resume, NULL);
+
+static const struct of_device_id arm_cctrng_dt_match[] = {
+	{ .compatible = "arm,cryptocell-713-trng", },
+	{ .compatible = "arm,cryptocell-703-trng", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, arm_cctrng_dt_match);
+
+static struct platform_driver cctrng_driver = {
+	.driver = {
+		.name = "cctrng",
+		.of_match_table = arm_cctrng_dt_match,
+		.pm = &cctrng_pm,
+	},
+	.probe = cctrng_probe,
+	.remove = cctrng_remove,
+};
+
+static int __init cctrng_mod_init(void)
+{
+	/* Compile time assertion checks */
+	BUILD_BUG_ON(CCTRNG_DATA_BUF_WORDS < 6);
+	BUILD_BUG_ON((CCTRNG_DATA_BUF_WORDS & (CCTRNG_DATA_BUF_WORDS-1)) != 0);
+
+	return platform_driver_register(&cctrng_driver);
+}
+module_init(cctrng_mod_init);
+
+static void __exit cctrng_mod_exit(void)
+{
+	platform_driver_unregister(&cctrng_driver);
+}
+module_exit(cctrng_mod_exit);
+
+/* Module description */
+MODULE_DESCRIPTION("ARM CryptoCell TRNG Driver");
+MODULE_AUTHOR("ARM");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/char/hw_random/cctrng.h b/drivers/char/hw_random/cctrng.h
new file mode 100644
index 0000000..1f2fde9
--- /dev/null
+++ b/drivers/char/hw_random/cctrng.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2019-2020 ARM Limited or its affiliates. */
+
+#include <linux/bitops.h>
+
+#define POWER_DOWN_ENABLE 0x01
+#define POWER_DOWN_DISABLE 0x00
+
+/* hwrng quality: bits of true entropy per 1024 bits of input */
+#define CC_TRNG_QUALITY	1024
+
+/* CryptoCell TRNG HW definitions */
+#define CC_TRNG_NUM_OF_ROSCS	4
+/* The number of words generated in the entropy holding register (EHR)
+ * 6 words (192 bit) according to HW implementation
+ */
+#define CC_TRNG_EHR_IN_WORDS	6
+#define CC_TRNG_EHR_IN_BITS	(CC_TRNG_EHR_IN_WORDS * BITS_PER_TYPE(u32))
+
+#define CC_HOST_RNG_IRQ_MASK BIT(CC_HOST_RGF_IRR_RNG_INT_BIT_SHIFT)
+
+/* RNG interrupt mask */
+#define CC_RNG_INT_MASK (BIT(CC_RNG_IMR_EHR_VALID_INT_MASK_BIT_SHIFT) | \
+			 BIT(CC_RNG_IMR_AUTOCORR_ERR_INT_MASK_BIT_SHIFT) | \
+			 BIT(CC_RNG_IMR_CRNGT_ERR_INT_MASK_BIT_SHIFT) | \
+			 BIT(CC_RNG_IMR_VN_ERR_INT_MASK_BIT_SHIFT) | \
+			 BIT(CC_RNG_IMR_WATCHDOG_INT_MASK_BIT_SHIFT))
+
+// --------------------------------------
+// BLOCK: RNG
+// --------------------------------------
+#define CC_RNG_IMR_REG_OFFSET	0x0100UL
+#define CC_RNG_IMR_EHR_VALID_INT_MASK_BIT_SHIFT	0x0UL
+#define CC_RNG_IMR_AUTOCORR_ERR_INT_MASK_BIT_SHIFT	0x1UL
+#define CC_RNG_IMR_CRNGT_ERR_INT_MASK_BIT_SHIFT	0x2UL
+#define CC_RNG_IMR_VN_ERR_INT_MASK_BIT_SHIFT	0x3UL
+#define CC_RNG_IMR_WATCHDOG_INT_MASK_BIT_SHIFT	0x4UL
+#define CC_RNG_ISR_REG_OFFSET	0x0104UL
+#define CC_RNG_ISR_EHR_VALID_BIT_SHIFT	0x0UL
+#define CC_RNG_ISR_EHR_VALID_BIT_SIZE	0x1UL
+#define CC_RNG_ISR_AUTOCORR_ERR_BIT_SHIFT	0x1UL
+#define CC_RNG_ISR_AUTOCORR_ERR_BIT_SIZE	0x1UL
+#define CC_RNG_ISR_CRNGT_ERR_BIT_SHIFT	0x2UL
+#define CC_RNG_ISR_CRNGT_ERR_BIT_SIZE	0x1UL
+#define CC_RNG_ISR_WATCHDOG_BIT_SHIFT	0x4UL
+#define CC_RNG_ISR_WATCHDOG_BIT_SIZE	0x1UL
+#define CC_RNG_ICR_REG_OFFSET	0x0108UL
+#define CC_TRNG_CONFIG_REG_OFFSET	0x010CUL
+#define CC_EHR_DATA_0_REG_OFFSET	0x0114UL
+#define CC_RND_SOURCE_ENABLE_REG_OFFSET	0x012CUL
+#define CC_SAMPLE_CNT1_REG_OFFSET	0x0130UL
+#define CC_TRNG_DEBUG_CONTROL_REG_OFFSET	0x0138UL
+#define CC_RNG_SW_RESET_REG_OFFSET	0x0140UL
+#define CC_RNG_CLK_ENABLE_REG_OFFSET	0x01C4UL
+#define CC_RNG_DMA_ENABLE_REG_OFFSET	0x01C8UL
+#define CC_RNG_WATCHDOG_VAL_REG_OFFSET	0x01D8UL
+// --------------------------------------
+// BLOCK: SEC_HOST_RGF
+// --------------------------------------
+#define CC_HOST_RGF_IRR_REG_OFFSET	0x0A00UL
+#define CC_HOST_RGF_IRR_RNG_INT_BIT_SHIFT	0xAUL
+#define CC_HOST_RGF_IMR_REG_OFFSET	0x0A04UL
+#define CC_HOST_RGF_ICR_REG_OFFSET	0x0A08UL
+
+#define CC_HOST_POWER_DOWN_EN_REG_OFFSET	0x0A78UL
+
+// --------------------------------------
+// BLOCK: NVM
+// --------------------------------------
+#define CC_NVM_IS_IDLE_REG_OFFSET	0x0F10UL
+#define CC_NVM_IS_IDLE_VALUE_BIT_SHIFT	0x0UL
+#define CC_NVM_IS_IDLE_VALUE_BIT_SIZE	0x1UL
-- 
2.7.4

[PATCH v7 3/3] MAINTAINERS: add HG as cctrng maintainer

[Hadar Gat]

I work for Arm on maintaining the TrustZone CryptoCell TRNG driver.

Signed-off-by: Hadar Gat <hadar.gat@arm.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c312b65..3f27716 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3847,6 +3847,15 @@ S:	Supported
 F:	drivers/crypto/ccree/
 W:	https://developer.arm.com/products/system-ip/trustzone-cryptocell/cryptocell-700-family
 
+CCTRNG ARM TRUSTZONE CRYPTOCELL TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
+M:	Hadar Gat <hadar.gat@arm.com>
+L:	linux-crypto@vger.kernel.org
+S:	Supported
+F:	drivers/char/hw_random/cctrng.c
+F:	drivers/char/hw_random/cctrng.h
+F:	Documentation/devicetree/bindings/rng/arm-cctrng.txt
+W:	https://developer.arm.com/products/system-ip/trustzone-cryptocell/cryptocell-700-family
+
 CEC FRAMEWORK
 M:	Hans Verkuil <hverkuil-cisco@xs4all.nl>
 L:	linux-media@vger.kernel.org
-- 
2.7.4

Re: [PATCH v7 1/3] dt-bindings: add device tree binding for Arm CryptoCell trng engine

[Rob Herring]

On Fri, 27 Mar 2020 09:10:21 +0300, Hadar Gat wrote:
> The Arm CryptoCell is a hardware security engine. This patch adds DT
> bindings for its TRNG (True Random Number Generator) engine.
> 
> Signed-off-by: Hadar Gat <hadar.gat@arm.com>
> ---
>  .../devicetree/bindings/rng/arm-cctrng.yaml        | 54 ++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rng/arm-cctrng.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Herbert Xu]

On Fri, Mar 27, 2020 at 09:10:20AM +0300, Hadar Gat wrote:
> The Arm CryptoCell is a hardware security engine.
> This patch introduces driver for its TRNG (True Random Number Generator)
> engine.
> 
> v7 change: in arm-cctrng.yaml, removed unneeded 'minitems'
> 
> v6 change: add missing initialization of hwrng quality.
> 
> v5 changes:
> 	1. in arm-cctrng.yaml, fixed error in 'make dt_binding_check'
> 	2. in cctrng.c, clean up cctrng clock handling
> 
> v4 changes: update arm-cctrng.yaml to conform with json-schema standard.
> 
> v3 change: removed few unneeded "#ifdef CONFIG_PM" from the code.
> 
> v2 changes: fixed 'make dt_bnding_check' errors.
> 
> Hadar Gat (3):
>   dt-bindings: add device tree binding for Arm CryptoCell trng engine
>   hw_random: cctrng: introduce Arm CryptoCell driver
>   MAINTAINERS: add HG as cctrng maintainer
> 
>  .../devicetree/bindings/rng/arm-cctrng.yaml        |  54 ++
>  MAINTAINERS                                        |   9 +
>  drivers/char/hw_random/Kconfig                     |  12 +
>  drivers/char/hw_random/Makefile                    |   1 +
>  drivers/char/hw_random/cctrng.c                    | 736 +++++++++++++++++++++
>  drivers/char/hw_random/cctrng.h                    |  72 ++
>  6 files changed, 884 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/rng/arm-cctrng.yaml
>  create mode 100644 drivers/char/hw_random/cctrng.c
>  create mode 100644 drivers/char/hw_random/cctrng.h

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

Re: [PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Geert Uytterhoeven]

Hi Hadar,

On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> The Arm CryptoCell is a hardware security engine.
> This patch introduces driver for its TRNG (True Random Number Generator)
> engine.

Thanks for your series!

I am wondering what is the relation between this and
Documentation/devicetree/bindings/crypto/arm-cryptocell.txt?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Hadar Gat]

Hi Geert,

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, 20 April 2020 12:35
> 
> Hi Hadar,
> 
> On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > The Arm CryptoCell is a hardware security engine.
> > This patch introduces driver for its TRNG (True Random Number
> > Generator) engine.
> 
> Thanks for your series!
> 
> I am wondering what is the relation between this and
> Documentation/devicetree/bindings/crypto/arm-cryptocell.txt?

Arm TrustZone CryptoCell hardware contains both cryptographic engine (ccree) and true random number generator engine (cctrng).
These are separate engines with some sharing in logic and interface.
cctrng engine may not always be present.
The devicetree documentation is in:
For ccree - Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
For cctrng - Documentation/devicetree/bindings/rng/arm-cctrng.yaml

> 
> Gr{oetje,eeting}s,
> 
>                         Geert

Hadar

Re: [PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Geert Uytterhoeven]

Hi Hadar,

On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> Introduce low level Arm CryptoCell TRNG HW support.
>
> Signed-off-by: Hadar Gat <hadar.gat@arm.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/char/hw_random/cctrng.c

> +static int cctrng_probe(struct platform_device *pdev)
> +{
> +       struct resource *req_mem_cc_regs = NULL;
> +       struct cctrng_drvdata *drvdata;
> +       struct device *dev = &pdev->dev;
> +       int rc = 0;
> +       u32 val;
> +       int irq;
> +
> +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (!drvdata)
> +               return -ENOMEM;
> +
> +       drvdata->rng.name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +       if (!drvdata->rng.name)
> +               return -ENOMEM;
> +
> +       drvdata->rng.read = cctrng_read;
> +       drvdata->rng.priv = (unsigned long)drvdata;
> +       drvdata->rng.quality = CC_TRNG_QUALITY;
> +
> +       platform_set_drvdata(pdev, drvdata);
> +       drvdata->pdev = pdev;
> +
> +       drvdata->circ.buf = (char *)drvdata->data_buf;
> +
> +       /* Get device resources */
> +       /* First CC registers space */
> +       req_mem_cc_regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       /* Map registers space */
> +       drvdata->cc_base = devm_ioremap_resource(dev, req_mem_cc_regs);
> +       if (IS_ERR(drvdata->cc_base)) {
> +               dev_err(dev, "Failed to ioremap registers");
> +               return PTR_ERR(drvdata->cc_base);
> +       }
> +
> +       dev_dbg(dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs->name,
> +               req_mem_cc_regs);
> +       dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
> +               &req_mem_cc_regs->start, drvdata->cc_base);
> +
> +       /* Then IRQ */
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq < 0) {
> +               dev_err(dev, "Failed getting IRQ resource\n");
> +               return irq;
> +       }
> +
> +       /* parse sampling rate from device tree */
> +       rc = cc_trng_parse_sampling_ratio(drvdata);
> +       if (rc) {
> +               dev_err(dev, "Failed to get legal sampling ratio for rosc\n");
> +               return rc;
> +       }
> +
> +       rc = cc_trng_clk_init(drvdata);
> +       if (rc) {
> +               dev_err(dev, "cc_trng_clk_init failed\n");
> +               return rc;
> +       }
> +
> +       INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
> +       INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
> +       spin_lock_init(&drvdata->read_lock);
> +
> +       /* register the driver isr function */
> +       rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng", drvdata);

Shoudn't this be done after clearing the pending interrupts below?

> +       if (rc) {
> +               dev_err(dev, "Could not register to interrupt %d\n", irq);
> +               goto post_clk_err;
> +       }
> +       dev_dbg(dev, "Registered to IRQ: %d\n", irq);
> +
> +       /* Clear all pending interrupts */
> +       val = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
> +       dev_dbg(dev, "IRR=0x%08X\n", val);
> +       cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, val);

The above accesses the engine's registers...

> +
> +       /* unmask HOST RNG interrupt */
> +       cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
> +                  cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
> +                  ~CC_HOST_RNG_IRQ_MASK);
> +
> +       /* init PM */
> +       rc = cc_trng_pm_init(drvdata);
> +       if (rc) {
> +               dev_err(dev, "cc_trng_pm_init failed\n");
> +               goto post_clk_err;
> +       }

> +
> +       /* increment device's usage counter */
> +       rc = cc_trng_pm_get(dev);

... but only here is Runtime PM initialized, and the device guaranteed
to be powered.  If a device is accessed while powered down, this may
lead to an asynchronous external abort, or a plain lockup.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Geert Uytterhoeven]

Hi Hadar,

On Mon, Apr 20, 2020 at 2:27 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Monday, 20 April 2020 12:35
> >
> > On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > > The Arm CryptoCell is a hardware security engine.
> > > This patch introduces driver for its TRNG (True Random Number
> > > Generator) engine.
> >
> > Thanks for your series!
> >
> > I am wondering what is the relation between this and
> > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt?
>
> Arm TrustZone CryptoCell hardware contains both cryptographic engine (ccree) and true random number generator engine (cctrng).

OK.

> These are separate engines with some sharing in logic and interface.

Do they share the same register block?

> cctrng engine may not always be present.

I assume that applies to e.g. the older 630p?

> The devicetree documentation is in:
> For ccree - Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> For cctrng - Documentation/devicetree/bindings/rng/arm-cctrng.yaml

Thank you, I had already read both documents.


Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Hadar Gat]

Hi Geert,

To better explain the relationship between ccree and cctrng drivers, here an description of the underlying hardware and the relationship to the two drivers:

Arm TrustZone CryptoCell is a hardware block that implements two separate and discreet, although related, interfaces: one for the Rich Execution Environment  (read: Linux) and the other for the Trusted Execution Environment (e.g. Trusty, Op-TEE).

The ccree driver exposes the REE interface of CryptoCell to Linux. Where a SoC vendor implements both REE and TEE in their design, that is all that is needed.

However, we have some customers that make use CryptoCell but never implement a Trusted Execution Environment. This is a design decision taken when the SoC hardware is being designed and not a software controlled configuration, as it involves how the buses are laid out. Some of these customers have requested from us to allow making use in Linux of the TRNG resources which are normally associated with the TEE side when it is not in use. For these customers, the cctrng driver allows making use in Linux the TRNG which is normally part of the TEE side of CryptoCell.

I am guessing based on market segment that this is NOT the case with the Renesas boards you are maintain.

Nevertheless, we of course very much welcome your review, input  and contribution that has proved extremely valuable with the ccree driver. 🙂

BR,
Hadar & Gilad

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> Hi Hadar,
> 
> On Mon, Apr 20, 2020 at 2:27 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Sent: Monday, 20 April 2020 12:35
> > >
> > > On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > > > The Arm CryptoCell is a hardware security engine.
> > > > This patch introduces driver for its TRNG (True Random Number
> > > > Generator) engine.
> > >
> > > Thanks for your series!
> > >
> > > I am wondering what is the relation between this and
> > > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt?
> >
> > Arm TrustZone CryptoCell hardware contains both cryptographic engine
> (ccree) and true random number generator engine (cctrng).
> 
> OK.
> 
> > These are separate engines with some sharing in logic and interface.
> 
> Do they share the same register block?
> 
> > cctrng engine may not always be present.
> 
> I assume that applies to e.g. the older 630p?
> 
> > The devicetree documentation is in:
> > For ccree -
> > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > For cctrng - Documentation/devicetree/bindings/rng/arm-cctrng.yaml
> 
> Thank you, I had already read both documents.
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

RE: [PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Hadar Gat]

Thank you Geert for reviewing the cctrng code.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: Monday, 20 April 2020 16:45
> 
> Hi Hadar,
> 
> On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > Introduce low level Arm CryptoCell TRNG HW support.
> >
> > Signed-off-by: Hadar Gat <hadar.gat@arm.com>
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/char/hw_random/cctrng.c
> 
> > +static int cctrng_probe(struct platform_device *pdev) {
> > +       struct resource *req_mem_cc_regs = NULL;
> > +       struct cctrng_drvdata *drvdata;
> > +       struct device *dev = &pdev->dev;
> > +       int rc = 0;
> > +       u32 val;
> > +       int irq;
> > +
> > +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > +       if (!drvdata)
> > +               return -ENOMEM;
> > +
> > +       drvdata->rng.name = devm_kstrdup(dev, dev_name(dev),
> GFP_KERNEL);
> > +       if (!drvdata->rng.name)
> > +               return -ENOMEM;
> > +
> > +       drvdata->rng.read = cctrng_read;
> > +       drvdata->rng.priv = (unsigned long)drvdata;
> > +       drvdata->rng.quality = CC_TRNG_QUALITY;
> > +
> > +       platform_set_drvdata(pdev, drvdata);
> > +       drvdata->pdev = pdev;
> > +
> > +       drvdata->circ.buf = (char *)drvdata->data_buf;
> > +
> > +       /* Get device resources */
> > +       /* First CC registers space */
> > +       req_mem_cc_regs = platform_get_resource(pdev,
> IORESOURCE_MEM, 0);
> > +       /* Map registers space */
> > +       drvdata->cc_base = devm_ioremap_resource(dev,
> req_mem_cc_regs);
> > +       if (IS_ERR(drvdata->cc_base)) {
> > +               dev_err(dev, "Failed to ioremap registers");
> > +               return PTR_ERR(drvdata->cc_base);
> > +       }
> > +
> > +       dev_dbg(dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs-
> >name,
> > +               req_mem_cc_regs);
> > +       dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
> > +               &req_mem_cc_regs->start, drvdata->cc_base);
> > +
> > +       /* Then IRQ */
> > +       irq = platform_get_irq(pdev, 0);
> > +       if (irq < 0) {
> > +               dev_err(dev, "Failed getting IRQ resource\n");
> > +               return irq;
> > +       }
> > +
> > +       /* parse sampling rate from device tree */
> > +       rc = cc_trng_parse_sampling_ratio(drvdata);
> > +       if (rc) {
> > +               dev_err(dev, "Failed to get legal sampling ratio for rosc\n");
> > +               return rc;
> > +       }
> > +
> > +       rc = cc_trng_clk_init(drvdata);
> > +       if (rc) {
> > +               dev_err(dev, "cc_trng_clk_init failed\n");
> > +               return rc;
> > +       }
> > +
> > +       INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
> > +       INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
> > +       spin_lock_init(&drvdata->read_lock);
> > +
> > +       /* register the driver isr function */
> > +       rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng",
> > + drvdata);
> 
> Shoudn't this be done after clearing the pending interrupts below?

I'm not sure what do you mean in your question...
I assume you're suggesting that the registration of the driver ISR function should be done only after clearing the pending interrupts?!
Anyway, any pending interrupt that might exist is irrelevant to the current cctrng driver which just started (we're in the probe function)

> > +       if (rc) {
> > +               dev_err(dev, "Could not register to interrupt %d\n", irq);
> > +               goto post_clk_err;
> > +       }
> > +       dev_dbg(dev, "Registered to IRQ: %d\n", irq);
> > +
> > +       /* Clear all pending interrupts */
> > +       val = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
> > +       dev_dbg(dev, "IRR=0x%08X\n", val);
> > +       cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, val);
> 
> The above accesses the engine's registers...

That is right.

> > +
> > +       /* unmask HOST RNG interrupt */
> > +       cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
> > +                  cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
> > +                  ~CC_HOST_RNG_IRQ_MASK);
> > +
> > +       /* init PM */
> > +       rc = cc_trng_pm_init(drvdata);
> > +       if (rc) {
> > +               dev_err(dev, "cc_trng_pm_init failed\n");
> > +               goto post_clk_err;
> > +       }
> 
> > +
> > +       /* increment device's usage counter */
> > +       rc = cc_trng_pm_get(dev);
> 
> ... but only here is Runtime PM initialized, and the device guaranteed to be
> powered.  If a device is accessed while powered down, this may lead to an
> asynchronous external abort, or a plain lockup.

It is assumed that when the driver is probed it is already powered. Only then, the driver initializes and enables the runtime PM to allow power down of the HW when it is not in use.

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Geert Uytterhoeven]

Hi Hadar,

On Tue, Apr 21, 2020 at 3:16 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > Sent: Monday, 20 April 2020 16:45
> >
> > On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > > Introduce low level Arm CryptoCell TRNG HW support.
> > >
> > > Signed-off-by: Hadar Gat <hadar.gat@arm.com>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/drivers/char/hw_random/cctrng.c
> >
> > > +static int cctrng_probe(struct platform_device *pdev) {
> > > +       struct resource *req_mem_cc_regs = NULL;
> > > +       struct cctrng_drvdata *drvdata;
> > > +       struct device *dev = &pdev->dev;
> > > +       int rc = 0;
> > > +       u32 val;
> > > +       int irq;
> > > +
> > > +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > > +       if (!drvdata)
> > > +               return -ENOMEM;
> > > +
> > > +       drvdata->rng.name = devm_kstrdup(dev, dev_name(dev),
> > GFP_KERNEL);
> > > +       if (!drvdata->rng.name)
> > > +               return -ENOMEM;
> > > +
> > > +       drvdata->rng.read = cctrng_read;
> > > +       drvdata->rng.priv = (unsigned long)drvdata;
> > > +       drvdata->rng.quality = CC_TRNG_QUALITY;
> > > +
> > > +       platform_set_drvdata(pdev, drvdata);
> > > +       drvdata->pdev = pdev;
> > > +
> > > +       drvdata->circ.buf = (char *)drvdata->data_buf;
> > > +
> > > +       /* Get device resources */
> > > +       /* First CC registers space */
> > > +       req_mem_cc_regs = platform_get_resource(pdev,
> > IORESOURCE_MEM, 0);
> > > +       /* Map registers space */
> > > +       drvdata->cc_base = devm_ioremap_resource(dev,
> > req_mem_cc_regs);
> > > +       if (IS_ERR(drvdata->cc_base)) {
> > > +               dev_err(dev, "Failed to ioremap registers");
> > > +               return PTR_ERR(drvdata->cc_base);
> > > +       }
> > > +
> > > +       dev_dbg(dev, "Got MEM resource (%s): %pR\n", req_mem_cc_regs-
> > >name,
> > > +               req_mem_cc_regs);
> > > +       dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
> > > +               &req_mem_cc_regs->start, drvdata->cc_base);
> > > +
> > > +       /* Then IRQ */
> > > +       irq = platform_get_irq(pdev, 0);
> > > +       if (irq < 0) {
> > > +               dev_err(dev, "Failed getting IRQ resource\n");
> > > +               return irq;
> > > +       }
> > > +
> > > +       /* parse sampling rate from device tree */
> > > +       rc = cc_trng_parse_sampling_ratio(drvdata);
> > > +       if (rc) {
> > > +               dev_err(dev, "Failed to get legal sampling ratio for rosc\n");
> > > +               return rc;
> > > +       }
> > > +
> > > +       rc = cc_trng_clk_init(drvdata);
> > > +       if (rc) {
> > > +               dev_err(dev, "cc_trng_clk_init failed\n");
> > > +               return rc;
> > > +       }
> > > +
> > > +       INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
> > > +       INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
> > > +       spin_lock_init(&drvdata->read_lock);
> > > +
> > > +       /* register the driver isr function */
> > > +       rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED, "cctrng",
> > > + drvdata);
> >
> > Shoudn't this be done after clearing the pending interrupts below?
>
> I'm not sure what do you mean in your question...
> I assume you're suggesting that the registration of the driver ISR function should be done only after clearing the pending interrupts?!

Indeed.

> Anyway, any pending interrupt that might exist is irrelevant to the current cctrng driver which just started (we're in the probe function)

If there is a pending interrupt, your interrupt handler (which returns
IRQ_NONE in this case) will be called repeatedly, until the driver gets
to clearing the pending interrupts below, or until the interrupt core
decides to give up, and disable it for good.

> > > +       if (rc) {
> > > +               dev_err(dev, "Could not register to interrupt %d\n", irq);
> > > +               goto post_clk_err;
> > > +       }
> > > +       dev_dbg(dev, "Registered to IRQ: %d\n", irq);
> > > +
> > > +       /* Clear all pending interrupts */
> > > +       val = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
> > > +       dev_dbg(dev, "IRR=0x%08X\n", val);
> > > +       cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, val);
> >
> > The above accesses the engine's registers...
>
> That is right.
>
> > > +
> > > +       /* unmask HOST RNG interrupt */
> > > +       cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
> > > +                  cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
> > > +                  ~CC_HOST_RNG_IRQ_MASK);
> > > +
> > > +       /* init PM */
> > > +       rc = cc_trng_pm_init(drvdata);
> > > +       if (rc) {
> > > +               dev_err(dev, "cc_trng_pm_init failed\n");
> > > +               goto post_clk_err;
> > > +       }
> >
> > > +
> > > +       /* increment device's usage counter */
> > > +       rc = cc_trng_pm_get(dev);
> >
> > ... but only here is Runtime PM initialized, and the device guaranteed to be
> > powered.  If a device is accessed while powered down, this may lead to an
> > asynchronous external abort, or a plain lockup.
>
> It is assumed that when the driver is probed it is already powered. Only then, the driver initializes and enables the runtime PM to allow power down of the HW when it is not in use.

Who guarantees it is powered up? Your driver has indeed enabled the
(optional) clock above, but if the hardware block is part of a power
domain, it may still be powered down. The only way to make sure a
hardware block in a power domain is powered, is by enabling Runtime
PM and calling pm_runtime_get_sync().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v7 0/3] hw_random: introduce Arm CryptoCell TRNG driver

[Geert Uytterhoeven]

Hi Hadar (and Gilad),

On Tue, Apr 21, 2020 at 3:13 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> To better explain the relationship between ccree and cctrng drivers, here an description of the underlying hardware and the relationship to the two drivers:
>
> Arm TrustZone CryptoCell is a hardware block that implements two separate and discreet, although related, interfaces: one for the Rich Execution Environment  (read: Linux) and the other for the Trusted Execution Environment (e.g. Trusty, Op-TEE).
>
> The ccree driver exposes the REE interface of CryptoCell to Linux. Where a SoC vendor implements both REE and TEE in their design, that is all that is needed.
>
> However, we have some customers that make use CryptoCell but never implement a Trusted Execution Environment. This is a design decision taken when the SoC hardware is being designed and not a software controlled configuration, as it involves how the buses are laid out. Some of these customers have requested from us to allow making use in Linux of the TRNG resources which are normally associated with the TEE side when it is not in use. For these customers, the cctrng driver allows making use in Linux the TRNG which is normally part of the TEE side of CryptoCell.

Thank you, that is the part I was missing.

BTW, there seems to be no mention of CryptoCell 630 on arm.com; it
covers only CC-300 and CC-700.
But from the (very limited) information about the crypto engine on R-Car
Gen3 SoCs, it looks like the RNG is indeed only present in the secure
(trusted) part.

> > -----Original Message-----
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Mon, Apr 20, 2020 at 2:27 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Sent: Monday, 20 April 2020 12:35
> > > >
> > > > On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > > > > The Arm CryptoCell is a hardware security engine.
> > > > > This patch introduces driver for its TRNG (True Random Number
> > > > > Generator) engine.
> > > >
> > > > Thanks for your series!
> > > >
> > > > I am wondering what is the relation between this and
> > > > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt?
> > >
> > > Arm TrustZone CryptoCell hardware contains both cryptographic engine
> > (ccree) and true random number generator engine (cctrng).
> >
> > OK.
> >
> > > These are separate engines with some sharing in logic and interface.
> >
> > Do they share the same register block?
> >
> > > cctrng engine may not always be present.
> >
> > I assume that applies to e.g. the older 630p?
> >
> > > The devicetree documentation is in:
> > > For ccree -
> > > Documentation/devicetree/bindings/crypto/arm-cryptocell.txt
> > > For cctrng - Documentation/devicetree/bindings/rng/arm-cctrng.yaml
> >
> > Thank you, I had already read both documents.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Hadar Gat]

> From: Geert Uytterhoeven <geert@linux-m68k.org>
> 
> Hi Hadar,
> 
> On Tue, Apr 21, 2020 at 3:16 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Sent: Monday, 20 April 2020 16:45
> > >
> > > On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > > > Introduce low level Arm CryptoCell TRNG HW support.
> > > >
> > > > Signed-off-by: Hadar Gat <hadar.gat@arm.com>
> > >
> > > Thanks for your patch!
> > >
> > > > --- /dev/null
> > > > +++ b/drivers/char/hw_random/cctrng.c
> > >
> > > > +static int cctrng_probe(struct platform_device *pdev) {
> > > > +       struct resource *req_mem_cc_regs = NULL;
> > > > +       struct cctrng_drvdata *drvdata;
> > > > +       struct device *dev = &pdev->dev;
> > > > +       int rc = 0;
> > > > +       u32 val;
> > > > +       int irq;
> > > > +
> > > > +       drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > > > +       if (!drvdata)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       drvdata->rng.name = devm_kstrdup(dev, dev_name(dev),
> > > GFP_KERNEL);
> > > > +       if (!drvdata->rng.name)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       drvdata->rng.read = cctrng_read;
> > > > +       drvdata->rng.priv = (unsigned long)drvdata;
> > > > +       drvdata->rng.quality = CC_TRNG_QUALITY;
> > > > +
> > > > +       platform_set_drvdata(pdev, drvdata);
> > > > +       drvdata->pdev = pdev;
> > > > +
> > > > +       drvdata->circ.buf = (char *)drvdata->data_buf;
> > > > +
> > > > +       /* Get device resources */
> > > > +       /* First CC registers space */
> > > > +       req_mem_cc_regs = platform_get_resource(pdev,
> > > IORESOURCE_MEM, 0);
> > > > +       /* Map registers space */
> > > > +       drvdata->cc_base = devm_ioremap_resource(dev,
> > > req_mem_cc_regs);
> > > > +       if (IS_ERR(drvdata->cc_base)) {
> > > > +               dev_err(dev, "Failed to ioremap registers");
> > > > +               return PTR_ERR(drvdata->cc_base);
> > > > +       }
> > > > +
> > > > +       dev_dbg(dev, "Got MEM resource (%s): %pR\n",
> > > > + req_mem_cc_regs-
> > > >name,
> > > > +               req_mem_cc_regs);
> > > > +       dev_dbg(dev, "CC registers mapped from %pa to 0x%p\n",
> > > > +               &req_mem_cc_regs->start, drvdata->cc_base);
> > > > +
> > > > +       /* Then IRQ */
> > > > +       irq = platform_get_irq(pdev, 0);
> > > > +       if (irq < 0) {
> > > > +               dev_err(dev, "Failed getting IRQ resource\n");
> > > > +               return irq;
> > > > +       }
> > > > +
> > > > +       /* parse sampling rate from device tree */
> > > > +       rc = cc_trng_parse_sampling_ratio(drvdata);
> > > > +       if (rc) {
> > > > +               dev_err(dev, "Failed to get legal sampling ratio for rosc\n");
> > > > +               return rc;
> > > > +       }
> > > > +
> > > > +       rc = cc_trng_clk_init(drvdata);
> > > > +       if (rc) {
> > > > +               dev_err(dev, "cc_trng_clk_init failed\n");
> > > > +               return rc;
> > > > +       }
> > > > +
> > > > +       INIT_WORK(&drvdata->compwork, cc_trng_compwork_handler);
> > > > +       INIT_WORK(&drvdata->startwork, cc_trng_startwork_handler);
> > > > +       spin_lock_init(&drvdata->read_lock);
> > > > +
> > > > +       /* register the driver isr function */
> > > > +       rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED,
> > > > + "cctrng", drvdata);
> > >
> > > Shoudn't this be done after clearing the pending interrupts below?
> >
> > I'm not sure what do you mean in your question...
> > I assume you're suggesting that the registration of the driver ISR function
> should be done only after clearing the pending interrupts?!
> 
> Indeed.
> 
> > Anyway, any pending interrupt that might exist is irrelevant to the
> > current cctrng driver which just started (we're in the probe function)
> 
> If there is a pending interrupt, your interrupt handler (which returns
> IRQ_NONE in this case) will be called repeatedly, until the driver gets to
> clearing the pending interrupts below, or until the interrupt core decides to
> give up, and disable it for good.

Ok, I get your point now.
But note that when the cctrng HW boots, the default is that all interrupts are masked, hence the interrupt handler will not be called.
The unmask of the RNG interrupts is done afterwards and only then ISR may potentially be called.

> > > > +       if (rc) {
> > > > +               dev_err(dev, "Could not register to interrupt %d\n", irq);
> > > > +               goto post_clk_err;
> > > > +       }
> > > > +       dev_dbg(dev, "Registered to IRQ: %d\n", irq);
> > > > +
> > > > +       /* Clear all pending interrupts */
> > > > +       val = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
> > > > +       dev_dbg(dev, "IRR=0x%08X\n", val);
> > > > +       cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, val);
> > >
> > > The above accesses the engine's registers...
> >
> > That is right.
> >
> > > > +
> > > > +       /* unmask HOST RNG interrupt */
> > > > +       cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
> > > > +                  cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
> > > > +                  ~CC_HOST_RNG_IRQ_MASK);

The above unmask the RNG interrupt.

> > > > +
> > > > +       /* init PM */
> > > > +       rc = cc_trng_pm_init(drvdata);
> > > > +       if (rc) {
> > > > +               dev_err(dev, "cc_trng_pm_init failed\n");
> > > > +               goto post_clk_err;
> > > > +       }
> > >
> > > > +
> > > > +       /* increment device's usage counter */
> > > > +       rc = cc_trng_pm_get(dev);
> > >
> > > ... but only here is Runtime PM initialized, and the device
> > > guaranteed to be powered.  If a device is accessed while powered
> > > down, this may lead to an asynchronous external abort, or a plain lockup.
> >
> > It is assumed that when the driver is probed it is already powered. Only
> then, the driver initializes and enables the runtime PM to allow power down
> of the HW when it is not in use.
> 
> Who guarantees it is powered up? Your driver has indeed enabled the
> (optional) clock above, but if the hardware block is part of a power domain, it
> may still be powered down. The only way to make sure a hardware block in a
> power domain is powered, is by enabling Runtime PM and calling
> pm_runtime_get_sync().

Got it now and indeed this need to be fixed.
I reviewed a similar fix that you did in ccree, commit 8c7849a30255cfd9a9ba412b1517e20a8572448b.
Now I understand that the initialization of runtime-PM should be done before any register access.
I will fix this in another patch.

Thanks a lot for your review and valuable inputs.
Hadar

Re: [PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Geert Uytterhoeven]

Hi Hadar,

On Tue, Apr 21, 2020 at 5:13 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Tue, Apr 21, 2020 at 3:16 PM Hadar Gat <Hadar.Gat@arm.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Sent: Monday, 20 April 2020 16:45
> > > > On Fri, Mar 27, 2020 at 7:11 AM Hadar Gat <hadar.gat@arm.com> wrote:
> > > > > Introduce low level Arm CryptoCell TRNG HW support.
> > > > > --- /dev/null
> > > > > +++ b/drivers/char/hw_random/cctrng.c
> > > >
> > > > > +static int cctrng_probe(struct platform_device *pdev) {

> > > > > +       /* register the driver isr function */
> > > > > +       rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED,
> > > > > + "cctrng", drvdata);
> > > >
> > > > Shoudn't this be done after clearing the pending interrupts below?
> > >
> > > I'm not sure what do you mean in your question...
> > > I assume you're suggesting that the registration of the driver ISR function
> > should be done only after clearing the pending interrupts?!
> >
> > Indeed.
> >
> > > Anyway, any pending interrupt that might exist is irrelevant to the
> > > current cctrng driver which just started (we're in the probe function)
> >
> > If there is a pending interrupt, your interrupt handler (which returns
> > IRQ_NONE in this case) will be called repeatedly, until the driver gets to
> > clearing the pending interrupts below, or until the interrupt core decides to
> > give up, and disable it for good.
>
> Ok, I get your point now.
> But note that when the cctrng HW boots, the default is that all interrupts are masked, hence the interrupt handler will not be called.

Is that also the case when booting into a new kernel using kexec?

> The unmask of the RNG interrupts is done afterwards and only then ISR may potentially be called.

> > > > > +       if (rc) {
> > > > > +               dev_err(dev, "Could not register to interrupt %d\n", irq);
> > > > > +               goto post_clk_err;
> > > > > +       }
> > > > > +       dev_dbg(dev, "Registered to IRQ: %d\n", irq);
> > > > > +
> > > > > +       /* Clear all pending interrupts */
> > > > > +       val = cc_ioread(drvdata, CC_HOST_RGF_IRR_REG_OFFSET);
> > > > > +       dev_dbg(dev, "IRR=0x%08X\n", val);
> > > > > +       cc_iowrite(drvdata, CC_HOST_RGF_ICR_REG_OFFSET, val);
> > > >
> > > > The above accesses the engine's registers...
> > >
> > > That is right.
> > >
> > > > > +
> > > > > +       /* unmask HOST RNG interrupt */
> > > > > +       cc_iowrite(drvdata, CC_HOST_RGF_IMR_REG_OFFSET,
> > > > > +                  cc_ioread(drvdata, CC_HOST_RGF_IMR_REG_OFFSET) &
> > > > > +                  ~CC_HOST_RNG_IRQ_MASK);
>
> The above unmask the RNG interrupt.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v7 2/3] hw_random: cctrng: introduce Arm CryptoCell driver

[Hadar Gat]

> > > > > > +static int cctrng_probe(struct platform_device *pdev) {
> 
> > > > > > +       /* register the driver isr function */
> > > > > > +       rc = devm_request_irq(dev, irq, cc_isr, IRQF_SHARED,
> > > > > > + "cctrng", drvdata);
> > > > >
> > > > > Shoudn't this be done after clearing the pending interrupts below?
> > > >
> > > > I'm not sure what do you mean in your question...
> > > > I assume you're suggesting that the registration of the driver ISR
> > > > function
> > > should be done only after clearing the pending interrupts?!
> > >
> > > Indeed.
> > >
> > > > Anyway, any pending interrupt that might exist is irrelevant to
> > > > the current cctrng driver which just started (we're in the probe
> > > > function)
> > >
> > > If there is a pending interrupt, your interrupt handler (which
> > > returns IRQ_NONE in this case) will be called repeatedly, until the
> > > driver gets to clearing the pending interrupts below, or until the
> > > interrupt core decides to give up, and disable it for good.
> >
> > Ok, I get your point now.
> > But note that when the cctrng HW boots, the default is that all interrupts
> are masked, hence the interrupt handler will not be called.
> 
> Is that also the case when booting into a new kernel using kexec?

Well.. no. ☹
A fix is needed here to consider also the case of kexec.
I'll fix that in another patch.
Thanks!