mbox series

[v2,0/3] hwrng: starfive - Add driver for TRNG module

Message ID 20221228071103.91797-1-jiajie.ho@starfivetech.com
Headers show
Series hwrng: starfive - Add driver for TRNG module | expand

Message

JiaJie Ho Dec. 28, 2022, 7:11 a.m. UTC 
This patch series adds kernel support for StarFive hardware random
number generator. First 2 patches add bindings documentation and driver
for this module. Patch 3 adds devicetree entry for VisionFive v2 SoC.

Patch 3 needs to be applied on top of:
https://patchwork.kernel.org/project/linux-riscv/patch/20221220011247.35560-7-hal.feng@starfivetech.com/

Changes since v1:
- updated of_match_ptr and added pm_sleep_ptr in Patch 2. (by Krzysztof)
- drop "status" in dts as module is always on in Patch 3. (by Krzysztof)

Jia Jie Ho (3):
  dt-bindings: rng: Add StarFive TRNG module
  hwrng: starfive - Add TRNG driver for StarFive SoC
  riscv: dts: starfive: Add TRNG node for VisionFive 2

 .../bindings/rng/starfive,jh7110-trng.yaml    |  55 +++
 MAINTAINERS                                   |   6 +
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  10 +
 drivers/char/hw_random/Kconfig                |  11 +
 drivers/char/hw_random/Makefile               |   1 +
 drivers/char/hw_random/starfive-trng.c        | 388 ++++++++++++++++++
 6 files changed, 471 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/rng/starfive,jh7110-trng.yaml
 create mode 100644 drivers/char/hw_random/starfive-trng.c

Comments

Herbert Xu Jan. 6, 2023, 8:39 a.m. UTC | #1 
On Wed, Dec 28, 2022 at 03:11:02PM +0800, Jia Jie Ho wrote:
>
> +static int starfive_trng_cmd(struct starfive_trng *trng, u32 cmd)
> +{
> +	int ret;
> +
> +	ret = starfive_trng_wait_idle(trng);
> +	if (ret)
> +		return -ETIMEDOUT;
> +
> +	switch (cmd) {
> +	case STARFIVE_CTRL_EXEC_NOP:
> +		writel(cmd, trng->base + STARFIVE_CTRL);
> +		break;
> +	case STARFIVE_CTRL_GENE_RANDNUM:
> +		reinit_completion(&trng->random_done);
> +		writel(cmd, trng->base + STARFIVE_CTRL);
> +		ret = wait_for_completion_timeout(&trng->random_done, 3000);

Please don't use a constant jiffies value, because it may vary
in length.  Instead use a constant millisecond value and convert
it to jiffies.

> +static irqreturn_t starfive_trng_irq(int irq, void *priv)
> +{
> +	u32 status;
> +	struct starfive_trng *trng = (struct starfive_trng *)priv;
> +
> +	status = readl(trng->base + STARFIVE_ISTAT);
> +	if (status & STARFIVE_ISTAT_RAND_RDY) {
> +		writel(STARFIVE_ISTAT_RAND_RDY, trng->base + STARFIVE_ISTAT);
> +		complete(&trng->random_done);
> +	}
> +
> +	if (status & STARFIVE_ISTAT_SEED_DONE) {
> +		writel(STARFIVE_ISTAT_SEED_DONE, trng->base + STARFIVE_ISTAT);
> +		complete(&trng->reseed_done);
> +	}
> +
> +	if (status & STARFIVE_ISTAT_LFSR_LOCKUP) {
> +		writel(STARFIVE_ISTAT_LFSR_LOCKUP, trng->base + STARFIVE_ISTAT);
> +		starfive_trng_cmd(trng, STARFIVE_CTRL_EXEC_RANDRESEED);

You should not sleep in an IRQ handler.

> +static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wait)

You should respect the wait argument and not do polling/sleeping
if it is false.

Cheers,
JiaJie Ho Jan. 8, 2023, 2:13 p.m. UTC | #2 
> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 6 January, 2023 4:39 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Wed, Dec 28, 2022 at 03:11:02PM +0800, Jia Jie Ho wrote:
> >
> > +static int starfive_trng_cmd(struct starfive_trng *trng, u32 cmd) {
> > +	int ret;
> > +
> > +	ret = starfive_trng_wait_idle(trng);
> > +	if (ret)
> > +		return -ETIMEDOUT;
> > +
> > +	switch (cmd) {
> > +	case STARFIVE_CTRL_EXEC_NOP:
> > +		writel(cmd, trng->base + STARFIVE_CTRL);
> > +		break;
> > +	case STARFIVE_CTRL_GENE_RANDNUM:
> > +		reinit_completion(&trng->random_done);
> > +		writel(cmd, trng->base + STARFIVE_CTRL);
> > +		ret = wait_for_completion_timeout(&trng->random_done,
> 3000);
> 
> Please don't use a constant jiffies value, because it may vary in length.
> Instead use a constant millisecond value and convert it to jiffies.
> 

I'll fix this in next version.

> > +static irqreturn_t starfive_trng_irq(int irq, void *priv) {
> > +	u32 status;
> > +	struct starfive_trng *trng = (struct starfive_trng *)priv;
> > +
> > +	status = readl(trng->base + STARFIVE_ISTAT);
> > +	if (status & STARFIVE_ISTAT_RAND_RDY) {
> > +		writel(STARFIVE_ISTAT_RAND_RDY, trng->base +
> STARFIVE_ISTAT);
> > +		complete(&trng->random_done);
> > +	}
> > +
> > +	if (status & STARFIVE_ISTAT_SEED_DONE) {
> > +		writel(STARFIVE_ISTAT_SEED_DONE, trng->base +
> STARFIVE_ISTAT);
> > +		complete(&trng->reseed_done);
> > +	}
> > +
> > +	if (status & STARFIVE_ISTAT_LFSR_LOCKUP) {
> > +		writel(STARFIVE_ISTAT_LFSR_LOCKUP, trng->base +
> STARFIVE_ISTAT);
> > +		starfive_trng_cmd(trng,
> STARFIVE_CTRL_EXEC_RANDRESEED);
> 
> You should not sleep in an IRQ handler.
> 

Will fix this too.

> > +static int starfive_trng_read(struct hwrng *rng, void *buf, size_t
> > +max, bool wait)
> 
> You should respect the wait argument and not do polling/sleeping if it is false.

I'll add this in the next version.

Thanks for reviewing this patch.

Best regards,
Jia Jie
JiaJie Ho Jan. 9, 2023, 2:58 a.m. UTC | #3 
> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 6 January, 2023 4:39 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Wed, Dec 28, 2022 at 03:11:02PM +0800, Jia Jie Ho wrote:
> > +static int starfive_trng_read(struct hwrng *rng, void *buf, size_t
> > +max, bool wait)
> 
> You should respect the wait argument and not do polling/sleeping if it is false.
> 

Hi Herbert, 

My trng device requires sending a generate new number cmd before each read.
It then only populates the data registers with new random number and raise an interrupt when ready.
If user choose to not wait, they will always get stale bits. 
Is it okay to always return error if wait=false ?

Thanks
Jia Jie
Herbert Xu Jan. 9, 2023, 3:03 a.m. UTC | #4 
On Mon, Jan 09, 2023 at 02:58:14AM +0000, JiaJie Ho wrote:
>
> My trng device requires sending a generate new number cmd before each read.
> It then only populates the data registers with new random number and raise an interrupt when ready.
> If user choose to not wait, they will always get stale bits. 
> Is it okay to always return error if wait=false ?

What is the length of the wait time? Is there an upper bound?
What is the average wait time?

Thanks,
JiaJie Ho Jan. 9, 2023, 3:41 a.m. UTC | #5 
> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 9 January, 2023 11:04 AM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Mon, Jan 09, 2023 at 02:58:14AM +0000, JiaJie Ho wrote:
> >
> > My trng device requires sending a generate new number cmd before each
> read.
> > It then only populates the data registers with new random number and
> raise an interrupt when ready.
> > If user choose to not wait, they will always get stale bits.
> > Is it okay to always return error if wait=false ?
> 
> What is the length of the wait time? Is there an upper bound?
> What is the average wait time?
> 

The average wait time is around 20 microseconds.
I measured from writel cmd to wait_for_completion done.

Thanks
Jia Jie
Herbert Xu Jan. 9, 2023, 4 a.m. UTC | #6 
On Mon, Jan 09, 2023 at 03:41:14AM +0000, JiaJie Ho wrote:
>
> The average wait time is around 20 microseconds.
> I measured from writel cmd to wait_for_completion done.

Do you know of an upper bound? E.g., if we limit it to 40us how
many requests would fail on average?

Having a maximum delay of 40us would be OK with wait == 0.  So
you could implement it in a way such that if the wait time exceeded
40us then you return if wait == 0, otherwise you can wait longer.

Cheers,
JiaJie Ho Jan. 9, 2023, 4:33 a.m. UTC | #7 
> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: 9 January, 2023 12:00 PM
> To: JiaJie Ho <jiajie.ho@starfivetech.com>
> Cc: Olivia Mackall <olivia@selenic.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner
> Berthing <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>;
> linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-riscv@lists.infradead.org
> Subject: Re: [PATCH v2 2/3] hwrng: starfive - Add TRNG driver for StarFive
> SoC
> 
> On Mon, Jan 09, 2023 at 03:41:14AM +0000, JiaJie Ho wrote:
> >
> > The average wait time is around 20 microseconds.
> > I measured from writel cmd to wait_for_completion done.
> 
> Do you know of an upper bound? E.g., if we limit it to 40us how many
> requests would fail on average?
> 

I don't have the upper bound. 
I ran the rngtest with 1000 blocks, none of the wait time exceeded 30us.

> Having a maximum delay of 40us would be OK with wait == 0.  So you could
> implement it in a way such that if the wait time exceeded 40us then you
> return if wait == 0, otherwise you can wait longer.
> 

I'll do this then.

Thanks
Jia Jie