[v2,07/12] hwrng: bcm2835-rng: Manage an optional clock
diff mbox

Message ID 20171108004449.32730-8-f.fainelli@gmail.com
State Accepted
Delegated to: Herbert Xu
Headers show

Commit Message

Florian Fainelli Nov. 8, 2017, 12:44 a.m. UTC
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.fainelli@gmail.com>
---
 drivers/char/hw_random/bcm2835-rng.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Eric Anholt Nov. 8, 2017, 6:31 p.m. UTC | #1
Florian Fainelli <f.fainelli@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.fainelli@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 <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
> +#include <linux/clk.h>
>  
>  #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 <eric@anholt.net>
Stefan Wahren Nov. 8, 2017, 7:19 p.m. UTC | #2
Hi Florian,

> Florian Fainelli <f.fainelli@gmail.com> hat am 8. November 2017 um 01:44 geschrieben:
> 
> 
> 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.fainelli@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 <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/printk.h>
> +#include <linux/clk.h>
>  
>  #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;
> +	}
> +

the clocks are optional to the binding, but not for the proper function of all RNG. So shouldn't we catch the case that we cannot get the clock during probe, but the clock is required for a specific SoC?

>  	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);
>  }
>  
>  struct bcm2835_rng_of_data {
> @@ -130,6 +142,9 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
>  		return PTR_ERR(priv->base);
>  	}
>  
> +	/* Clock is optional on most platforms */
> +	priv->clk = devm_clk_get(dev, NULL);
> +

At least EPROBE_DEFER should be handled here:

if (IS_ERR(priv->clk) && PTR_ERR(priv->clk) == -EPROBE_DEFER)
	return -EPROBE_DEFER;

Regards
Stefan
Russell King - ARM Linux Nov. 8, 2017, 7:23 p.m. UTC | #3
On Wed, Nov 08, 2017 at 08:19:57PM +0100, Stefan Wahren wrote:
> Hi Florian,
> > +	/* Clock is optional on most platforms */
> > +	priv->clk = devm_clk_get(dev, NULL);
> > +
> 
> At least EPROBE_DEFER should be handled here:
> 
> if (IS_ERR(priv->clk) && PTR_ERR(priv->clk) == -EPROBE_DEFER)
> 	return -EPROBE_DEFER;

	if (priv->clk == ERR_PTR(-EPROBE_DEFER))
		return -EPROBE_DEFER;

is a simpler test for one particular error-pointer value.

Patch
diff mbox

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 <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/printk.h>
+#include <linux/clk.h>
 
 #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;
+	}
+
 	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);
 }
 
 struct bcm2835_rng_of_data {
@@ -130,6 +142,9 @@  static int bcm2835_rng_probe(struct platform_device *pdev)
 		return PTR_ERR(priv->base);
 	}
 
+	/* Clock is optional on most platforms */
+	priv->clk = devm_clk_get(dev, NULL);
+
 	priv->rng.name = "bcm2835-rng";
 	priv->rng.init = bcm2835_rng_init;
 	priv->rng.read = bcm2835_rng_read;