On Mon, 05 Oct 2015, Daniel Thompson wrote:

On 05/10/15 13:11, Lee Jones wrote:

quoted

On Mon, 05 Oct 2015, Daniel Thompson wrote:

quoted

Late but...

That's okay.  Fixup patches can always be submitted.

We have forever. :)

quoted

On 17/09/15 14:45, Lee Jones wrote:

quoted

diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index 055bb01..8bcfb45 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile

@@ -30,4 +30,5 @@ obj-$(CONFIG_HW_RANDOM_TPM) += tpm-rng.o
 obj-$(CONFIG_HW_RANDOM_BCM2835) += bcm2835-rng.o
 obj-$(CONFIG_HW_RANDOM_IPROC_RNG200) += iproc-rng200.o
 obj-$(CONFIG_HW_RANDOM_MSM) += msm-rng.o
+obj-$(CONFIG_HW_RANDOM_ST) += st-rng.o
 obj-$(CONFIG_HW_RANDOM_XGENE) += xgene-rng.o

diff --git a/drivers/char/hw_random/st-rng.c b/drivers/char/hw_random/st-rng.c
new file mode 100644
index 0000000..8c8a435
--- /dev/null
+++ b/drivers/char/hw_random/st-rng.c

@@ -0,0 +1,144 @@
+/*
+ * ST Random Number Generator Driver ST's Platforms
+ *
+ * Author: Pankaj Dev: <pankaj.dev@st.com>
+ *         Lee Jones <lee.jones@linaro.org>
+ *
+ * Copyright (C) 2015 STMicroelectronics (R&D) Limited
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/hw_random.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+/* Registers */
+#define ST_RNG_STATUS_REG		0x20
+#define ST_RNG_DATA_REG			0x24
+
+/* Registers fields */
+#define ST_RNG_STATUS_BAD_SEQUENCE	BIT(0)
+#define ST_RNG_STATUS_BAD_ALTERNANCE	BIT(1)
+#define ST_RNG_STATUS_FIFO_FULL		BIT(5)
+
+#define ST_RNG_FIFO_SIZE		8
+#define ST_RNG_SAMPLE_SIZE		2 /* 2 Byte (16bit) samples */
+
+/* Samples are available every 0.667us, which we round to 1us */
+#define ST_RNG_FILL_FIFO_TIMEOUT   (1 * (ST_RNG_FIFO_SIZE / ST_RNG_SAMPLE_SIZE))
+
+struct st_rng_data {
+	void __iomem	*base;
+	struct clk	*clk;
+	struct hwrng	ops;
+};
+
+static int st_rng_read(struct hwrng *rng, void *data, size_t max, bool wait)
+{
+	struct st_rng_data *ddata = (struct st_rng_data *)rng->priv;
+	u32 status;
+	int i;
+
+	if (max < sizeof(u16))
+		return -EINVAL;
+
+	/* Wait until FIFO is full - max 4uS*/
+	for (i = 0; i < ST_RNG_FILL_FIFO_TIMEOUT; i++) {
+		status = readl_relaxed(ddata->base + ST_RNG_STATUS_REG);
+		if (status & ST_RNG_STATUS_FIFO_FULL)
+			break;
+		udelay(1);

How much bandwidth does using udelay() cost? I think it could be

quoted

10% compared to a tighter polling loop.

Samples are only available every 0.7uS and we only do this for every
4.  The maximum it could 'cost' is <1uS.  Do we really want to fuss
over that tiny amount of time?  It's an understandable point if we
were talking about milliseconds, but a single microsecond?

This code is called in a tight loop so we're not talking about a
*single* microsecond! We are are talking about about one microsecond
in every five[1] and yes, I think we should care about that.

It takes 2.67uS for the FIFO to come ready so with a polling
interval of 1uS + loop overhead so I would fully expect on average
to "waste" 0.5uS each time st_rng_read() is called (in a tight
loop).

[1]... point three recurring  ;-)

`time dd if=/dev/hwrng of=/dev/null bs=1 count=10M`

/* Current code, inc delay */
Run 1: real 0m17.996s
Run 2: real 0m17.991s
Run 3: real 0m17.996s
Run 4: real 0m18.000s
Run 5: real 0m18.000s
Total       0m89.983s

/* Tight loop, no delay */
Run 1: real 0m18.011s
Run 2: real 0m17.995s
Run 3: real 0m18.005s
Run 4: real 0m18.020s
Run 5: real 0m17.990s
Total       0m90.021s

(89.983 / 90.021) * 100 = 99.958%

0.042% saving.

Not quite the >10% predicted.  I'd say that's negligible. 

quoted

quoted

quoted

+	}
+
+	if (i == ST_RNG_FILL_FIFO_TIMEOUT)
+		return 0;

Isn't a timeout an error condition?

Yes, which is why we're returning 0.  In this context 0 == 'no data'.
This will be converted to -EAGAIN if the caller did not request
NONBLOCKING.

I took the view that hitting the timeout means the hardware is
broken. Is it likely that the next time we call it there will be
data ready? If it is should it be trusted?

From experience gained whilst testing, I can say that when returning

an error the HNG breaks and the user receives an error.  If instead we
return 0, we get to have another go and random numbers again pour
out.  Perhaps we're just not waiting long enough?  Either way, the
current implementation works real well.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog