Re: [PATCH 03/17] hw_random: bcm2835-rng: Switch to SPDX identifier

[Eric Anholt]

Stefan Wahren  writes:

> Adopt the SPDX license identifier headers to ease license compliance
> management. While we are at this fix the comment style, too.

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature

Re: [PATCH -next] hwrng: bcm2835 - Remove redundant dev_err call in bcm2835_rng_probe()

[Eric Anholt]

Wei Yongjun <weiyongj...@huawei.com> writes:

> There is a error message within devm_ioremap_resource
> already, so remove the dev_err call to avoid redundant
> error message.

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH v2 06/12] hwrng: bcm2835-rng: Rework interrupt masking

[Eric Anholt]

Stefan Wahren  writes:

> Hi Florian,
>
>> Florian Fainelli  hat am 8. November 2017 um 01:44 
>> geschrieben:
>> 
>> 
>> The interrupt masking done for Northstart Plus and Northstar (BCM5301X)
>> is moved from being a function pointer mapped to of_device_id::data into
>> a proper part of the hwrng::init callback. While at it, we also make the
>> of_data be a proper structure indicating the platform specifics, since
>> the day we need to add a second type of platform information, we would
>> have to do that anyway.
>
> in the lack of proper documentation for bcm2835 rng, is it possible
> that we should mask the interrupts for bcm2835 as well?

I don't have the RTL nearby and the RNG block doesn't have the usual
#defines for power-on-reset values, so I'm not sure.  I don't see any
use of the RNG by the firmware that should impact Linux -- a driver
exists and it uses IRQs, but it shouldn't have been active, and even if
it was then its teardown process masks the interrupt off again.

However, masking it off ourselves should be harmless at worst.


signature.asc
Description: PGP signature

Re: [PATCH v2 12/12] hwrng: bcm63xx-rng: Remove since bcm2835-rng takes over

[Eric Anholt]

Florian Fainelli <f.faine...@gmail.com> writes:

> bcm2835-rng is now capable of supporting the BCM63xx hardware, so remove
> the driver which duplicates the same functionality.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>

Assuming Stefan's testing says that the .name handling you settled on
works, patches 9, 11, 12 are:

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH v2 08/12] hwrng: bcm2835-rng: Abstract I/O accessors

[Eric Anholt]

Florian Fainelli <f.faine...@gmail.com> writes:

> In preparation for allowing BCM63xx to use this driver, we abstract I/O
> accessors such that we can easily change those later on.

This may even be a fix on rpi, since i/o from different devices can get
reordered and you're using the barrier variants now!

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH v2 07/12] hwrng: bcm2835-rng: Manage an optional clock

[Eric Anholt]

Florian Fainelli <f.faine...@gmail.com> writes:

> One of the last steps before bcm63xx-rng can be eliminated is to manage
> a clock during hwrng::init and hwrng::cleanup, so fetch it in the probe
> function, and manage it during these two steps when valid.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> ---
>  drivers/char/hw_random/bcm2835-rng.c | 17 -
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/bcm2835-rng.c 
> b/drivers/char/hw_random/bcm2835-rng.c
> index ed20e0b6b7ae..99b56fd5482c 100644
> --- a/drivers/char/hw_random/bcm2835-rng.c
> +++ b/drivers/char/hw_random/bcm2835-rng.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define RNG_CTRL 0x0
>  #define RNG_STATUS   0x4
> @@ -33,6 +34,7 @@ struct bcm2835_rng_priv {
>   struct hwrng rng;
>   void __iomem *base;
>   bool mask_interrupts;
> + struct clk *clk;
>  };
>  
>  static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng)
> @@ -66,8 +68,15 @@ static int bcm2835_rng_read(struct hwrng *rng, void *buf, 
> size_t max,
>  static int bcm2835_rng_init(struct hwrng *rng)
>  {
>   struct bcm2835_rng_priv *priv = to_rng_priv(rng);
> + int ret = 0;
>   u32 val;
>  
> + if (!IS_ERR(priv->clk)) {
> + ret = clk_prepare_enable(priv->clk);
> + if (ret)
> + return ret;
> + }

clk_prepare_enable() is protected by IS_ERR_OR_NULL() checks and will
return 0.

> +
>   if (priv->mask_interrupts) {
>   /* mask the interrupt */
>   val = readl(priv->base + RNG_INT_MASK);
> @@ -79,7 +88,7 @@ static int bcm2835_rng_init(struct hwrng *rng)
>   __raw_writel(RNG_WARMUP_COUNT, priv->base + RNG_STATUS);
>   __raw_writel(RNG_RBGEN, priv->base + RNG_CTRL);
>  
> - return 0;
> + return ret;
>  }
>  
>  static void bcm2835_rng_cleanup(struct hwrng *rng)
> @@ -88,6 +97,9 @@ static void bcm2835_rng_cleanup(struct hwrng *rng)
>  
>   /* disable rng hardware */
>   __raw_writel(0, priv->base + RNG_CTRL);
> +
> + if (!IS_ERR(priv->clk))
> + clk_disable_unprepare(priv->clk);

Same.  With those conditionals dropped,

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH v2 06/12] hwrng: bcm2835-rng: Rework interrupt masking

[Eric Anholt]

Florian Fainelli <f.faine...@gmail.com> writes:

> The interrupt masking done for Northstart Plus and Northstar (BCM5301X)
> is moved from being a function pointer mapped to of_device_id::data into
> a proper part of the hwrng::init callback. While at it, we also make the
> of_data be a proper structure indicating the platform specifics, since
> the day we need to add a second type of platform information, we would
> have to do that anyway.

I still think we should just unconditionally mask off the interrupt
regardless of platform if we're not going to use it in the driver and
some platforms need it.  Looks like a fine refactor, though:

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH v2 05/12] hwrng: bcm2835-rng: Use device managed helpers

[Eric Anholt]

Florian Fainelli <f.faine...@gmail.com> writes:

> Now that we have moved the RNG disabling into a hwrng::cleanup callback,
> we can use the device managed registration operation and remove our
> remove callback since it won't do anything necessary.

3-5 are:

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH v2 02/12] hwrng: bcm2835-rng: Define a driver private context

[Eric Anholt]

Florian Fainelli <f.faine...@gmail.com> writes:

> Instead of making hwrng::priv host the base register address, define a
> driver private context, make it per platform device instance and pass it
> down the different functions.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> ---
>  drivers/char/hw_random/bcm2835-rng.c | 55 
> ++--
>  1 file changed, 34 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/hw_random/bcm2835-rng.c 
> b/drivers/char/hw_random/bcm2835-rng.c
> index a818418a7e4c..0d72147ab45b 100644
> --- a/drivers/char/hw_random/bcm2835-rng.c
> +++ b/drivers/char/hw_random/bcm2835-rng.c

> -static struct hwrng bcm2835_rng_ops = {
> - .name   = "bcm2835",
> - .read   = bcm2835_rng_read,
> -};
> -

>   /* map peripheral */
> - rng_base = devm_ioremap_resource(dev, r);
> - if (IS_ERR(rng_base)) {
> + priv->base = devm_ioremap_resource(dev, r);
> + if (IS_ERR(priv->base)) {
>   dev_err(dev, "failed to remap rng regs");
> - return PTR_ERR(rng_base);
> + return PTR_ERR(priv->base);
>   }
> - bcm2835_rng_ops.priv = (unsigned long)rng_base;
> +
> + priv->rng.name = "bcm2835-rng";

Looks like an unintentional change from the previous "bcm2835" here?

Other than that, 1-2 are:

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH 08/12] hwrng: bcm2835-rng: Abstract I/O accessors

[Eric Anholt]

Florian Fainelli  writes:

> In preparation for allowing BCM63xx to use this driver, we abstract I/O
> accessors such that we can easily change those later on.
>
> Signed-off-by: Florian Fainelli 
> ---
>  drivers/char/hw_random/bcm2835-rng.c | 27 +++
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/char/hw_random/bcm2835-rng.c 
> b/drivers/char/hw_random/bcm2835-rng.c
> index 35928efb52e7..500275d55044 100644
> --- a/drivers/char/hw_random/bcm2835-rng.c
> +++ b/drivers/char/hw_random/bcm2835-rng.c
> @@ -42,6 +42,17 @@ static inline struct bcm2835_rng_priv *to_rng_priv(struct 
> hwrng *rng)
>   return container_of(rng, struct bcm2835_rng_priv, rng);
>  }
>  
> +static inline u32 rng_readl(struct bcm2835_rng_priv *priv, u32 offset)
> +{
> + return readl(priv->base + offset);
> +}
> +
> +static inline void rng_writel(struct bcm2835_rng_priv *priv, u32 val,
> +   u32 offset)
> +{
> + writel(val, priv->base + offset);
> +}
> +
>  static int bcm2835_rng_read(struct hwrng *rng, void *buf, size_t max,
>  bool wait)
>  {
> @@ -49,18 +60,18 @@ static int bcm2835_rng_read(struct hwrng *rng, void *buf, 
> size_t max,
>   u32 max_words = max / sizeof(u32);
>   u32 num_words, count;
>  
> - while ((__raw_readl(priv->base + RNG_STATUS) >> 24) == 0) {
> + while ((rng_readl(priv, RNG_STATUS) >> 24) == 0) {
>   if (!wait)
>   return 0;
>   cpu_relax();
>   }

What was the difference between the __raw_readl and readl that's now
being done in the new call?  Is it important?

>   /* set warm-up count & enable */
> - __raw_writel(RNG_WARMUP_COUNT, priv->base + RNG_STATUS);
> - __raw_writel(RNG_RBGEN, priv->base + RNG_CTRL);
> + rng_writel(priv, RNG_WARMUP_COUNT, RNG_STATUS);
> + rng_writel(priv, RNG_RBGEN, RNG_CTRL);

Similar question.


signature.asc
Description: PGP signature

Re: [PATCH 12/12] hwrng: bcm63xx-rng: Remove since bcm2835-rng takes over

[Eric Anholt]

Florian Fainelli  writes:

> bcm2835-rng is now capable of supporting the BCM63xx hardware, so remove
> the driver which duplicates the same functionality.
>
> Signed-off-by: Florian Fainelli 
> ---
>  drivers/char/hw_random/Kconfig   |  13 ---
>  drivers/char/hw_random/Makefile  |   1 -
>  drivers/char/hw_random/bcm63xx-rng.c | 154 
> ---
>  3 files changed, 168 deletions(-)
>  delete mode 100644 drivers/char/hw_random/bcm63xx-rng.c
>

> diff --git a/drivers/char/hw_random/bcm63xx-rng.c 
> b/drivers/char/hw_random/bcm63xx-rng.c
> deleted file mode 100644
> index 5132c9cde50d..
> --- a/drivers/char/hw_random/bcm63xx-rng.c
> +++ /dev/null

> -static int bcm63xx_rng_data_present(struct hwrng *rng, int wait)
> -{
> - struct bcm63xx_rng_priv *priv = to_rng_priv(rng);
> -
> - return __raw_readl(priv->regs + RNG_STAT) & RNG_AVAIL_MASK;
> -}

It looks like this method isn't in the 2835 implementation.  Should it
get ported over?


signature.asc
Description: PGP signature

Re: [PATCH] char: hw_random: bcm2835: handle of_iomap failures in bcm2835 driver

[Eric Anholt]

Arvind Yadav <arvind.yadav...@gmail.com> writes:

> Check return value of of_iomap and handle errors correctly.
>
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>

Acked-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH 1/4] dt-bindings: rng: Northstar Plus SoC rng bindings

[Eric Anholt]

Yendapally Reddy Dhananjaya Reddy <yendapally.re...@broadcom.com>
writes:

> Document the bindings used by Northstar Plus(NSP) SoC random number
> generator.
>
> Signed-off-by: Yendapally Reddy Dhananjaya Reddy 
> <yendapally.re...@broadcom.com>

Acked-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature

Re: [PATCH 4/4] hwrng: bcm2835: Read as much data as available

[Eric Anholt]

Yendapally Reddy Dhananjaya Reddy <yendapally.re...@broadcom.com>
writes:

> Read the requested number of data from the fifo
>
> Signed-off-by: Yendapally Reddy Dhananjaya Reddy 
> <yendapally.re...@broadcom.com>
> ---
>  drivers/char/hw_random/bcm2835-rng.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/bcm2835-rng.c 
> b/drivers/char/hw_random/bcm2835-rng.c
> index b1e8b78..9bbdc07 100644
> --- a/drivers/char/hw_random/bcm2835-rng.c
> +++ b/drivers/char/hw_random/bcm2835-rng.c
> @@ -43,6 +43,8 @@ static int bcm2835_rng_read(struct hwrng *rng, void *buf, 
> size_t max,
>  bool wait)
>  {
>   void __iomem *rng_base = (void __iomem *)rng->priv;
> + u32 max_words = max/sizeof(u32);

Style fix: Binary operators get a space on each side, so
"max / sizeof(u32);"

> @@ -50,8 +52,14 @@ static int bcm2835_rng_read(struct hwrng *rng, void *buf, 
> size_t max,
>   cpu_relax();
>   }
>  
> - *(u32 *)buf = __raw_readl(rng_base + RNG_DATA);
> - return sizeof(u32);
> + num_words = (readl(rng_base + RNG_STATUS) >> 24);

Optional cleanup: here and in the return statement, drop the extra
parenthesis.

Functionality-wise, this patch looks great to me, and should make the
driver more efficient.  With at least the binary operators change done,
it will be:

Reviewed-by: Eric Anholt <e...@anholt.net>

Thanks!


signature.asc
Description: PGP signature