Re: [PATCH 2/2] hwrng: mtk: Add driver for hardware random generator on MT7623 SoC

[PrasannaKumar Muralidharan]

On 14 April 2017 at 09:28, Sean Wang  wrote:
>
> Hi PrasannaKumar,
>
> Add my comments inline
>
>>
>> Use readl_poll_timeout_atomic's return value or -EIO instead of
>> !!ready. This will simplify mtk_rng_read.
>>
>
> !!ready provided is in order to let blocking/non-blocking case could
> share same code path. And readl_poll_timeout_atomic only handles
> blocking case.

Missed this point. Makes sense. My previous comment about return value
in mtk_rng_read is invalid as I based it on a wrong assumption.

>
>> > +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool 
>> > wait)
>> > +{
>> > +   struct mtk_rng *priv = to_mtk_rng(rng);
>> > +   int retval = 0;
>> > +
>> > +   while (max >= sizeof(u32)) {
>> > +   if (!mtk_rng_wait_ready(rng, wait))
>> > +   break;
>> > +
>> > +   *(u32 *)buf = readl(priv->base + RNG_DATA);
>> > +   retval += sizeof(u32);
>> > +   buf += sizeof(u32);
>> > +   max -= sizeof(u32);
>> > +   }
>> > +
>> > +   if (unlikely(wait && max))
>> > +   dev_warn(priv->dev, "timeout might be not properly set\n");
>>
>> Is this really necessary? Better to choose proper timeout than
>> providing this warning message. In rare cases if the timeout could
>> occur due to some reason (may be a hardware fault) print appropriate
>> warning message.
>
> It is good, I will choose the proper timeout and remove the log in the
> next one.
>
>>
>> > +   return retval || !wait ? retval : -EIO;
>> > +}
>>
>> Set retavl to mtk_rng_wait_ready and return retval.
>>
>
> Maybe i didn't get your points exactly. Adding some explanation about
> thoughts here.
>
> "return retval || !wait ? retval : -EIO;" I use can also help handling
> the both cases in one line which i think is elegant enough.
>
> And retval is accumulated with each round if some data's existing in
> hardware, so we don't return the value from mtk_rng_wait_ready().

retval can be 0 only when mkt_rng_wait_ready fails, returning 0 when
wait is true is confusing. Expected return value when 0 bytes is read
from device and wait is true is not clearly documented.

"return retval || !wait ? retval : -EIO;" is also fine.

Overall the code looks good to me. You can add:
Reviewed-by: PrasannaKumar Muralidharan .

Regards,
PrasannaKumar

Re: [PATCH 2/2] hwrng: mtk: Add driver for hardware random generator on MT7623 SoC

[Sean Wang]

Hi PrasannaKumar,

Add my comments inline


On Thu, 2017-04-13 at 14:09 +0530, PrasannaKumar Muralidharan wrote:
> Hi Sean,
> 
> Mostly looks good, have few minor comments.
> 
> On 13 April 2017 at 12:35,   wrote:
> > +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> > +{
> > +   struct mtk_rng *priv = to_mtk_rng(rng);
> > +   int ready;
> > +
> > +   ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> > +   if (!ready && wait)
> > +   readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
> > + ready & RNG_READY, USEC_POLL,
> > + TIMEOUT_POLL);
> > +   return !!ready;
> > +}
> 
> Use readl_poll_timeout_atomic's return value or -EIO instead of
> !!ready. This will simplify mtk_rng_read.
> 

!!ready provided is in order to let blocking/non-blocking case could
share same code path. And readl_poll_timeout_atomic only handles
blocking case.



> > +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool 
> > wait)
> > +{
> > +   struct mtk_rng *priv = to_mtk_rng(rng);
> > +   int retval = 0;
> > +
> > +   while (max >= sizeof(u32)) {
> > +   if (!mtk_rng_wait_ready(rng, wait))
> > +   break;
> > +
> > +   *(u32 *)buf = readl(priv->base + RNG_DATA);
> > +   retval += sizeof(u32);
> > +   buf += sizeof(u32);
> > +   max -= sizeof(u32);
> > +   }
> > +
> > +   if (unlikely(wait && max))
> > +   dev_warn(priv->dev, "timeout might be not properly set\n");
> 
> Is this really necessary? Better to choose proper timeout than
> providing this warning message. In rare cases if the timeout could
> occur due to some reason (may be a hardware fault) print appropriate
> warning message.

It is good, I will choose the proper timeout and remove the log in the
next one.

> 
> > +   return retval || !wait ? retval : -EIO;
> > +}
> 
> Set retavl to mtk_rng_wait_ready and return retval.
> 

Maybe i didn't get your points exactly. Adding some explanation about
thoughts here.

"return retval || !wait ? retval : -EIO;" I use can also help handling
the both cases in one line which i think is elegant enough. 

And retval is accumulated with each round if some data's existing in
hardware, so we don't return the value from mtk_rng_wait_ready().


> Regards,
> Prasanna

thanks for all your reviewing and suggestion

Sean

Re: [PATCH 2/2] hwrng: mtk: Add driver for hardware random generator on MT7623 SoC

[Sean Wang]

Hi Corentin,

I all agree and appreciate your careful reviewing. 

They will be added into the next one.

Sean

On Thu, 2017-04-13 at 13:06 +0200, Corentin Labbe wrote:
> Hello
> 
> I have some minor comment below:
> 
> On Thu, Apr 13, 2017 at 03:05:08PM +0800, sean.w...@mediatek.com wrote:
> > From: Sean Wang 
> > 
> > This patch adds support for hardware random generator on MT7623 SoC
> > and should also work on other similar Mediatek SoCs. Currently,
> > the driver is already tested successfully with rng-tools.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> >  drivers/char/hw_random/Kconfig   |  16 +++-
> >  drivers/char/hw_random/Makefile  |   2 +-
> >  drivers/char/hw_random/mtk-rng.c | 174 
> > +++
> >  3 files changed, 190 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/char/hw_random/mtk-rng.c
> > 
> > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> > index 0cafe08..af782ce 100644
> > --- a/drivers/char/hw_random/Kconfig
> > +++ b/drivers/char/hw_random/Kconfig
> > @@ -419,10 +419,24 @@ config HW_RANDOM_CAVIUM
> >   Generator hardware found on Cavium SoCs.
> >  
> >   To compile this driver as a module, choose M here: the
> > - module will be called cavium_rng.
> > + module will be called mtk-rng.
> 
> Unwanted change
> 
> >  
> >   If unsure, say Y.
> >  
> > +config HW_RANDOM_MTK
> > +   tristate "Mediatek Random Number Generator support"
> > +   depends on HW_RANDOM
> > +   depends on ARCH_MEDIATEK || COMPILE_TEST
> > +   default y
> > +   ---help---
> > + This driver provides kernel-side support for the Random Number
> > + Generator hardware found on Mediatek SoCs.
> > +
> > + To compile this driver as a module, choose M here. the
> > + module will be called mtk-rng.
> > +
> > + 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 5f52b1e..68be716 100644
> > --- a/drivers/char/hw_random/Makefile
> > +++ b/drivers/char/hw_random/Makefile
> > @@ -1,7 +1,6 @@
> >  #
> >  # Makefile for HW Random Number Generator (RNG) device drivers.
> >  #
> > -
> 
> Another unwanted change
> 
> >  obj-$(CONFIG_HW_RANDOM) += rng-core.o
> >  rng-core-y := core.o
> >  obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
> > @@ -36,3 +35,4 @@ obj-$(CONFIG_HW_RANDOM_STM32) += stm32-rng.o
> >  obj-$(CONFIG_HW_RANDOM_PIC32) += pic32-rng.o
> >  obj-$(CONFIG_HW_RANDOM_MESON) += meson-rng.o
> >  obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o
> > +obj-$(CONFIG_HW_RANDOM_MTK)+= mtk-rng.o
> > diff --git a/drivers/char/hw_random/mtk-rng.c 
> > b/drivers/char/hw_random/mtk-rng.c
> > new file mode 100644
> > index 000..6561ee0
> > --- /dev/null
> > +++ b/drivers/char/hw_random/mtk-rng.c
> > @@ -0,0 +1,174 @@
> > +/*
> > + * Driver for Mediatek Hardware Random Number Generator
> > + *
> > + * Copyright (C) 2017 Sean Wang 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +#define MTK_RNG_DEV KBUILD_MODNAME
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#define USEC_POLL  2
> > +#define TIMEOUT_POLL   20
> > +
> > +#define RNG_CTRL   0x00
> > +#define  RNG_ENBIT(0)
> > +#define  RNG_READY BIT(31)
> 
> Keep only one space between define and name
> 
> > +
> > +#define RNG_DATA   0x08
> > +
> > +#define to_mtk_rng(p)  container_of(p, struct mtk_rng, rng)
> > +
> > +struct mtk_rng {
> > +   struct device   *dev;
> > +   void __iomem *base;
> > +   struct clk *clk;
> > +   struct hwrng rng;
> > +};
> > +
> > +static int mtk_rng_init(struct hwrng *rng)
> > +{
> > +   struct mtk_rng *priv = to_mtk_rng(rng);
> > +   u32 val;
> > +   int err;
> > +
> > +   err = clk_prepare_enable(priv->clk);
> > +   if (err)
> > +   return err;
> > +
> > +   val = readl(priv->base + RNG_CTRL);
> > +   val |= RNG_EN;
> > +   writel(val, priv->base + RNG_CTRL);
> > +
> > +   return 0;
> > +}
> > +
> > +static void mtk_rng_cleanup(struct hwrng *rng)
> > +{
> > +   struct mtk_rng *priv = to_mtk_rng(rng);
> > +   u32 val;
> > +
> > +   val = 

Re: [PATCH 2/2] hwrng: mtk: Add driver for hardware random generator on MT7623 SoC

[Corentin Labbe]

Hello

I have some minor comment below:

On Thu, Apr 13, 2017 at 03:05:08PM +0800, sean.w...@mediatek.com wrote:
> From: Sean Wang 
> 
> This patch adds support for hardware random generator on MT7623 SoC
> and should also work on other similar Mediatek SoCs. Currently,
> the driver is already tested successfully with rng-tools.
> 
> Signed-off-by: Sean Wang 
> ---
>  drivers/char/hw_random/Kconfig   |  16 +++-
>  drivers/char/hw_random/Makefile  |   2 +-
>  drivers/char/hw_random/mtk-rng.c | 174 
> +++
>  3 files changed, 190 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/char/hw_random/mtk-rng.c
> 
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 0cafe08..af782ce 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -419,10 +419,24 @@ config HW_RANDOM_CAVIUM
>   Generator hardware found on Cavium SoCs.
>  
>   To compile this driver as a module, choose M here: the
> - module will be called cavium_rng.
> + module will be called mtk-rng.

Unwanted change

>  
>   If unsure, say Y.
>  
> +config HW_RANDOM_MTK
> + tristate "Mediatek Random Number Generator support"
> + depends on HW_RANDOM
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + default y
> + ---help---
> +   This driver provides kernel-side support for the Random Number
> +   Generator hardware found on Mediatek SoCs.
> +
> +   To compile this driver as a module, choose M here. the
> +   module will be called mtk-rng.
> +
> +   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 5f52b1e..68be716 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -1,7 +1,6 @@
>  #
>  # Makefile for HW Random Number Generator (RNG) device drivers.
>  #
> -

Another unwanted change

>  obj-$(CONFIG_HW_RANDOM) += rng-core.o
>  rng-core-y := core.o
>  obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
> @@ -36,3 +35,4 @@ obj-$(CONFIG_HW_RANDOM_STM32) += stm32-rng.o
>  obj-$(CONFIG_HW_RANDOM_PIC32) += pic32-rng.o
>  obj-$(CONFIG_HW_RANDOM_MESON) += meson-rng.o
>  obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o
> +obj-$(CONFIG_HW_RANDOM_MTK)  += mtk-rng.o
> diff --git a/drivers/char/hw_random/mtk-rng.c 
> b/drivers/char/hw_random/mtk-rng.c
> new file mode 100644
> index 000..6561ee0
> --- /dev/null
> +++ b/drivers/char/hw_random/mtk-rng.c
> @@ -0,0 +1,174 @@
> +/*
> + * Driver for Mediatek Hardware Random Number Generator
> + *
> + * Copyright (C) 2017 Sean Wang 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +#define MTK_RNG_DEV KBUILD_MODNAME
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define USEC_POLL2
> +#define TIMEOUT_POLL 20
> +
> +#define RNG_CTRL 0x00
> +#define  RNG_EN  BIT(0)
> +#define  RNG_READY   BIT(31)

Keep only one space between define and name

> +
> +#define RNG_DATA 0x08
> +
> +#define to_mtk_rng(p)container_of(p, struct mtk_rng, rng)
> +
> +struct mtk_rng {
> + struct device   *dev;
> + void __iomem *base;
> + struct clk *clk;
> + struct hwrng rng;
> +};
> +
> +static int mtk_rng_init(struct hwrng *rng)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + u32 val;
> + int err;
> +
> + err = clk_prepare_enable(priv->clk);
> + if (err)
> + return err;
> +
> + val = readl(priv->base + RNG_CTRL);
> + val |= RNG_EN;
> + writel(val, priv->base + RNG_CTRL);
> +
> + return 0;
> +}
> +
> +static void mtk_rng_cleanup(struct hwrng *rng)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + u32 val;
> +
> + val = readl(priv->base + RNG_CTRL);
> + val &= ~RNG_EN;
> + writel(val, priv->base + RNG_CTRL);
> +
> + clk_disable_unprepare(priv->clk);
> +}
> +
> +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + int ready;
> +
> + ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> + if (!ready && wait)
> + readl_poll_timeout_atomic(priv->base + 

Re: [PATCH 2/2] hwrng: mtk: Add driver for hardware random generator on MT7623 SoC

[PrasannaKumar Muralidharan]

Hi Sean,

Mostly looks good, have few minor comments.

On 13 April 2017 at 12:35,   wrote:
> +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> +{
> +   struct mtk_rng *priv = to_mtk_rng(rng);
> +   int ready;
> +
> +   ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> +   if (!ready && wait)
> +   readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
> + ready & RNG_READY, USEC_POLL,
> + TIMEOUT_POLL);
> +   return !!ready;
> +}

Use readl_poll_timeout_atomic's return value or -EIO instead of
!!ready. This will simplify mtk_rng_read.

> +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> +   struct mtk_rng *priv = to_mtk_rng(rng);
> +   int retval = 0;
> +
> +   while (max >= sizeof(u32)) {
> +   if (!mtk_rng_wait_ready(rng, wait))
> +   break;
> +
> +   *(u32 *)buf = readl(priv->base + RNG_DATA);
> +   retval += sizeof(u32);
> +   buf += sizeof(u32);
> +   max -= sizeof(u32);
> +   }
> +
> +   if (unlikely(wait && max))
> +   dev_warn(priv->dev, "timeout might be not properly set\n");

Is this really necessary? Better to choose proper timeout than
providing this warning message. In rare cases if the timeout could
occur due to some reason (may be a hardware fault) print appropriate
warning message.

> +   return retval || !wait ? retval : -EIO;
> +}

Set retavl to mtk_rng_wait_ready and return retval.

Regards,
Prasanna