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@xxxxxxxxxx):
> > +#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.
> > + 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.
> > + wait_for_completion(&rngc->rng_self_testing);
> I would suggest to use wait_for_completion_timeout().
I fixed this.
> > +
> > + } 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.
> > +
> > + /* 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.
> > + wait_for_completion(&rngc->rng_seed_done);
> I would suggest to use wait_for_completion_timeout().
Done.
> > +
> > + } 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.
> > + 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
