[PATCH v2 3/3] hwrng: mxc-fsl - add support for Freescale RNGC

Dear all,

looking for a Freescale RNGB/C driver, I came across this old mail
thread. It seems the review got stuck and the driver was never merged.
This mail is the latest conversation I could find.

I would like to pick up this work and prepare the RNGC driver for
merging into the mailine kernel.

Thus wrote Vladimir Zapolskiy (vladimir_zapolskiy at mentor.com):

quoted

+#define RNGC_VERID_VERSION_MAJOR_MASK		0x0000ff00
+#define RNGC_VERID_VERSION_MAJOR_SHIFT		8
+#define RNGC_VERID_VERSION_MINOR_MASK		0x000000ff
+#define RNGC_VERID_VERSION_MINOR_SHIFT		0

All RNGC_VERID_* are not used. And actually quite many other
defined values are not used, e.g. all *_ZEROS_MASK etc.

I removed unused defines.

quoted

+	struct mxc_rngc *rngc = (struct mxc_rngc *)priv;
+	int handled = IRQ_NONE;
+
+	/* is the seed creation done? */
+	if (__raw_readl(rngc->base + RNGC_STATUS) & RNGC_STATUS_SEED_DONE) {
+		complete(&rngc->rng_seed_done);
+		__raw_writel(RNGC_CMD_CLR_INT | RNGC_CMD_CLR_ERR,
+			     rngc->base + RNGC_COMMAND);
+		handled = IRQ_HANDLED;
+	}
+
+	/* is the self test done? */
+	if (__raw_readl(rngc->base + RNGC_STATUS) & RNGC_STATUS_ST_DONE) {
+		complete(&rngc->rng_self_testing);
+		__raw_writel(RNGC_CMD_CLR_INT | RNGC_CMD_CLR_ERR,
+			     rngc->base + RNGC_COMMAND);
+		handled = IRQ_HANDLED;
+	}
+
+	/* is there any error? */
+	if (__raw_readl(rngc->base + RNGC_STATUS) & RNGC_STATUS_ERROR) {
+		/* clear interrupt */
+		__raw_writel(RNGC_CMD_CLR_INT | RNGC_CMD_CLR_ERR,
+			     rngc->base + RNGC_COMMAND);
+		handled = IRQ_HANDLED;

For the code errors seem to be harmless, is it so? Does it make
sense to inform a user about errors?

Either one of the completions is triggered or we time out when we wait
on a wait queue, see below. In any case, we read the error register and
abort the operation if there's an error.

quoted

+		wait_for_completion(&rngc->rng_self_testing);

I would suggest to use wait_for_completion_timeout().

I fixed this.

quoted

+
+	} while (__raw_readl(rngc->base + RNGC_ERROR) &
+		 RNGC_ERROR_STATUS_ST_ERR);

Logic of running a self test until error condition is gone looks strange.

Agreed. I don't see why the self test should fail in the first run and
complete successfully when we retry without a reset. I changed the code
to run the self test only once.

quoted

+
+	/* clear interrupt. Is it really necessary here? */
+	__raw_writel(RNGC_CMD_CLR_INT | RNGC_CMD_CLR_ERR,
+		     rngc->base + RNGC_COMMAND);

Answering the question above I believe it is not needed here.

True. The irq handler clears the error status and interrupt bits.

quoted

+		wait_for_completion(&rngc->rng_seed_done);

I would suggest to use wait_for_completion_timeout().

Done.

quoted

+
+	} while (__raw_readl(rngc->base + RNGC_ERROR) &
+		 RNGC_ERROR_STATUS_STAT_ERR);

Any chance to loop forever?

I exit the loop now for all errors except the "statistical error". This
+ the timeout on the wait queue should prevent us from looping forever.

quoted

+	if (ret) {
+		dev_err(rngc->dev, "Can't get interrupt working.\n");
+		return -EIO;

Leaked enabled rngc->clk clock on error path.

I restructured the probe function such that the clock is disabled when
there's an error after it was enabled.


Best regards,

   Martin