[PATCH v2 0/2] hwrng: Add support for qcpm v2 hwrng

[PATCH v2 0/2] hwrng: Add support for qcpm v2 hwrng

[Vinod Koul]

This series adds support for newer version of hwrng as found in
Qualcomm SoCs. To do that add new v2 compatible string and driver_data
values for v2.

Changes in v2:
 - remove table based approach and set the init/cleanup routine based on
   driver data value

Vinod Koul (2):
  dt-bindings: rng: Add new compatible qcom,prng-v2
  hwrng: msm - Add support for prng v2

 Documentation/devicetree/bindings/rng/qcom,prng.txt |  3 ++-
 drivers/char/hw_random/msm-rng.c                    | 14 ++++++++++----
 2 files changed, 12 insertions(+), 5 deletions(-)

-- 
2.14.4

[PATCH v2 1/2] dt-bindings: rng: Add new compatible qcom,prng-v2

[Vinod Koul]

Later qcom chips support v2 of the prng, so add new compatible
qcom,prng-v2 for this.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---

Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
 Documentation/devicetree/bindings/rng/qcom,prng.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rng/qcom,prng.txt b/Documentation/devicetree/bindings/rng/qcom,prng.txt
index 8e5853c2879b..03fd218bd21a 100644
--- a/Documentation/devicetree/bindings/rng/qcom,prng.txt
+++ b/Documentation/devicetree/bindings/rng/qcom,prng.txt
@@ -2,7 +2,8 @@ Qualcomm MSM pseudo random number generator.
 
 Required properties:
 
-- compatible  : should be "qcom,prng"
+- compatible  : should be "qcom,prng" for 8916 etc
+              : should be "qcom,prng-v2" for 8996 and later
 - reg         : specifies base physical address and size of the registers map
 - clocks      : phandle to clock-controller plus clock-specifier pair
 - clock-names : "core" clocks all registers, FIFO and circuits in PRNG IP block
-- 
2.14.4

Re: [PATCH v2 1/2] dt-bindings: rng: Add new compatible qcom,prng-v2

[Rob Herring]

On Tue, Jun 19, 2018 at 03:24:29PM +0530, Vinod Koul wrote:
> Later qcom chips support v2 of the prng, so add new compatible
> qcom,prng-v2 for this.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
>  Documentation/devicetree/bindings/rng/qcom,prng.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/rng/qcom,prng.txt b/Documentation/devicetree/bindings/rng/qcom,prng.txt
> index 8e5853c2879b..03fd218bd21a 100644
> --- a/Documentation/devicetree/bindings/rng/qcom,prng.txt
> +++ b/Documentation/devicetree/bindings/rng/qcom,prng.txt
> @@ -2,7 +2,8 @@ Qualcomm MSM pseudo random number generator.
>  
>  Required properties:
>  
> -- compatible  : should be "qcom,prng"
> +- compatible  : should be "qcom,prng" for 8916 etc
> +              : should be "qcom,prng-v2" for 8996 and later

Either use SoC specific compatibles or convince me 'v2' is a meaningful 
and specific enough version. Some other QC IP has x.y.z versioning for 
example.

>  - reg         : specifies base physical address and size of the registers map
>  - clocks      : phandle to clock-controller plus clock-specifier pair
>  - clock-names : "core" clocks all registers, FIFO and circuits in PRNG IP block
> -- 
> 2.14.4
> 

Re: [PATCH v2 1/2] dt-bindings: rng: Add new compatible qcom,prng-v2

[Vinod]

On 25-06-18, 13:06, Rob Herring wrote:
> On Tue, Jun 19, 2018 at 03:24:29PM +0530, Vinod Koul wrote:
> > Later qcom chips support v2 of the prng, so add new compatible
> > qcom,prng-v2 for this.
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> >  Documentation/devicetree/bindings/rng/qcom,prng.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/rng/qcom,prng.txt b/Documentation/devicetree/bindings/rng/qcom,prng.txt
> > index 8e5853c2879b..03fd218bd21a 100644
> > --- a/Documentation/devicetree/bindings/rng/qcom,prng.txt
> > +++ b/Documentation/devicetree/bindings/rng/qcom,prng.txt
> > @@ -2,7 +2,8 @@ Qualcomm MSM pseudo random number generator.
> >  
> >  Required properties:
> >  
> > -- compatible  : should be "qcom,prng"
> > +- compatible  : should be "qcom,prng" for 8916 etc
> > +              : should be "qcom,prng-v2" for 8996 and later
> 
> Either use SoC specific compatibles or convince me 'v2' is a meaningful 
> and specific enough version. Some other QC IP has x.y.z versioning for 
> example.

The specs says QC RNG V2 :)

Said that Stephen suggested to make it qcom,prng-ee to point out that we
are pointing to an execution environment in the IP, so will do that
instead unless you have better idea

-- 
~Vinod

[PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Vinod Koul]

Qcom 8996 and later chips support prng v2 which requires to
implement only .read callback for hwrng.

This version of chip has multiple Execution Environments (EE) and
secure world is typically responsible for configuring the prng.

Add driver data for qcom,prng as 0 and qcom,prng-v2 as 1 and use
that to skip initialization and cleanup routines.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/char/hw_random/msm-rng.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
index 841fee845ec9..4676520e1f16 100644
--- a/drivers/char/hw_random/msm-rng.c
+++ b/drivers/char/hw_random/msm-rng.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 
 /* Device specific register offsets */
@@ -131,6 +132,7 @@ static int msm_rng_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct msm_rng *rng;
+	unsigned int skip_init;
 	int ret;
 
 	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
@@ -149,9 +151,12 @@ static int msm_rng_probe(struct platform_device *pdev)
 		return PTR_ERR(rng->clk);
 
 	rng->hwrng.name = KBUILD_MODNAME,
-	rng->hwrng.init = msm_rng_init,
-	rng->hwrng.cleanup = msm_rng_cleanup,
-	rng->hwrng.read = msm_rng_read,
+	rng->hwrng.read = msm_rng_read;
+	skip_init = (unsigned long)of_device_get_match_data(&pdev->dev);
+	if (!skip_init) {
+		rng->hwrng.init = msm_rng_init;
+		rng->hwrng.cleanup = msm_rng_cleanup;
+	}
 
 	ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
 	if (ret) {
@@ -163,7 +168,8 @@ static int msm_rng_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id msm_rng_of_match[] = {
-	{ .compatible = "qcom,prng", },
+	{ .compatible = "qcom,prng", .data = (void *)0},
+	{ .compatible = "qcom,prng-v2", .data = (void *)1},
 	{}
 };
 MODULE_DEVICE_TABLE(of, msm_rng_of_match);
-- 
2.14.4

Re: [PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Stanimir Varbanov]

Hi Vinod,

On 06/19/2018 12:54 PM, Vinod Koul wrote:
> Qcom 8996 and later chips support prng v2 which requires to
> implement only .read callback for hwrng.
> 
> This version of chip has multiple Execution Environments (EE) and
> secure world is typically responsible for configuring the prng.
> 
> Add driver data for qcom,prng as 0 and qcom,prng-v2 as 1 and use
> that to skip initialization and cleanup routines.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/char/hw_random/msm-rng.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/hw_random/msm-rng.c b/drivers/char/hw_random/msm-rng.c
> index 841fee845ec9..4676520e1f16 100644
> --- a/drivers/char/hw_random/msm-rng.c
> +++ b/drivers/char/hw_random/msm-rng.c
> @@ -17,6 +17,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  
>  /* Device specific register offsets */
> @@ -131,6 +132,7 @@ static int msm_rng_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
>  	struct msm_rng *rng;
> +	unsigned int skip_init;
>  	int ret;
>  
>  	rng = devm_kzalloc(&pdev->dev, sizeof(*rng), GFP_KERNEL);
> @@ -149,9 +151,12 @@ static int msm_rng_probe(struct platform_device *pdev)
>  		return PTR_ERR(rng->clk);
>  
>  	rng->hwrng.name = KBUILD_MODNAME,
> -	rng->hwrng.init = msm_rng_init,
> -	rng->hwrng.cleanup = msm_rng_cleanup,
> -	rng->hwrng.read = msm_rng_read,
> +	rng->hwrng.read = msm_rng_read;
> +	skip_init = (unsigned long)of_device_get_match_data(&pdev->dev);

skip_init is unsigned int, despite I think you don't need to cast it.

> +	if (!skip_init) {
> +		rng->hwrng.init = msm_rng_init;
> +		rng->hwrng.cleanup = msm_rng_cleanup;
> +	}
>  
>  	ret = devm_hwrng_register(&pdev->dev, &rng->hwrng);
>  	if (ret) {
> @@ -163,7 +168,8 @@ static int msm_rng_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id msm_rng_of_match[] = {
> -	{ .compatible = "qcom,prng", },
> +	{ .compatible = "qcom,prng", .data = (void *)0},
> +	{ .compatible = "qcom,prng-v2", .data = (void *)1},

No need to cast. IMO it'd be better to create defines for these numbers:

#define PRNG_V1	0
#define PRNG_V2	1


-- 
regards,
Stan

Re: [PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Vinod]

On 19-06-18, 15:11, Stanimir Varbanov wrote:

Hi Stan,

> On 06/19/2018 12:54 PM, Vinod Koul wrote:

> >  	rng->hwrng.name = KBUILD_MODNAME,
> > -	rng->hwrng.init = msm_rng_init,
> > -	rng->hwrng.cleanup = msm_rng_cleanup,
> > -	rng->hwrng.read = msm_rng_read,
> > +	rng->hwrng.read = msm_rng_read;
> > +	skip_init = (unsigned long)of_device_get_match_data(&pdev->dev);
> 
> skip_init is unsigned int, despite I think you don't need to cast it.

This is needed. of_device_get_match_data returns void *, so an explicit
cast is required for integer warning, otherwise it shows up as
-Wint-conversion warning

> >  static const struct of_device_id msm_rng_of_match[] = {
> > -	{ .compatible = "qcom,prng", },
> > +	{ .compatible = "qcom,prng", .data = (void *)0},
> > +	{ .compatible = "qcom,prng-v2", .data = (void *)1},
> 
> No need to cast. IMO it'd be better to create defines for these numbers:

again driver_data is void * and we need a cast

> 
> #define PRNG_V1	0
> #define PRNG_V2	1

That can be done. Since it is more used as flag and not version number
here to skip init, it doesn't really mean versions now

Thanks
-- 
~Vinod

Re: [PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Herbert Xu]

On Tue, Jun 19, 2018 at 03:24:30PM +0530, Vinod Koul wrote:
> Qcom 8996 and later chips support prng v2 which requires to
> implement only .read callback for hwrng.
> 
> This version of chip has multiple Execution Environments (EE) and
> secure world is typically responsible for configuring the prng.
> 
> Add driver data for qcom,prng as 0 and qcom,prng-v2 as 1 and use
> that to skip initialization and cleanup routines.
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>

Is this a pseudo RNG?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Stephen Boyd]

Quoting Vinod Koul (2018-06-19 02:54:30)
> Qcom 8996 and later chips support prng v2 which requires to
> implement only .read callback for hwrng.
> 
> This version of chip has multiple Execution Environments (EE) and
> secure world is typically responsible for configuring the prng.

Sometimes secure world is not configuring the rng though. I prefer we
have a DT flag for this to indicate if secure world has configured it or
not and then skip the init logic when the bool property is present in
DT. Then the DT property can be set on firmwares that are making things
blow up when we try to read the 'configured' register. I'd also file a
bug to qcom to tell them to unlock that config register for reads so
that things can work simpler, but who knows how that will work out.

It really sounds like the hardware isn't actually different, just the
firmware has decided to be more strict about making reads fail now.

Re: [PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Vinod]

On 19-06-18, 22:58, Stephen Boyd wrote:
> Quoting Vinod Koul (2018-06-19 02:54:30)
> > Qcom 8996 and later chips support prng v2 which requires to
> > implement only .read callback for hwrng.
> > 
> > This version of chip has multiple Execution Environments (EE) and
> > secure world is typically responsible for configuring the prng.
> 
> Sometimes secure world is not configuring the rng though. I prefer we
> have a DT flag for this to indicate if secure world has configured it or
> not and then skip the init logic when the bool property is present in
> DT. Then the DT property can be set on firmwares that are making things
> blow up when we try to read the 'configured' register. I'd also file a
> bug to qcom to tell them to unlock that config register for reads so
> that things can work simpler, but who knows how that will work out.

I dont feel that would be required. See below..

> It really sounds like the hardware isn't actually different, just the
> firmware has decided to be more strict about making reads fail now.

So in this case base hw block seems similar but consists of multiple
Execution Environment (EEs) and all of these contain only data
registers. Only secure environment has configuration. Each one has its
own register space.

In a case where we dont have secure world, we can point to secure
environment with v1 ops, so driver shall configure and run.

Thanks
-- 
~Vinod

Re: [PATCH v2 2/2] hwrng: msm - Add support for prng v2

[Stephen Boyd]

Quoting Vinod (2018-06-20 06:37:25)
> On 19-06-18, 22:58, Stephen Boyd wrote:
> > Quoting Vinod Koul (2018-06-19 02:54:30)
> > > Qcom 8996 and later chips support prng v2 which requires to
> > > implement only .read callback for hwrng.
> > > 
> > > This version of chip has multiple Execution Environments (EE) and
> > > secure world is typically responsible for configuring the prng.
> > 
> > Sometimes secure world is not configuring the rng though. I prefer we
> > have a DT flag for this to indicate if secure world has configured it or
> > not and then skip the init logic when the bool property is present in
> > DT. Then the DT property can be set on firmwares that are making things
> > blow up when we try to read the 'configured' register. I'd also file a
> > bug to qcom to tell them to unlock that config register for reads so
> > that things can work simpler, but who knows how that will work out.
> 
> I dont feel that would be required. See below..
> 
> > It really sounds like the hardware isn't actually different, just the
> > firmware has decided to be more strict about making reads fail now.
> 
> So in this case base hw block seems similar but consists of multiple
> Execution Environment (EEs) and all of these contain only data
> registers. Only secure environment has configuration. Each one has its
> own register space.
> 
> In a case where we dont have secure world, we can point to secure
> environment with v1 ops, so driver shall configure and run.

Are you saying that there are multiple "frames" that each EE can read
from? And then only one of those frames is the "real" one that can also
configure the hardware to actually give us random data?

That sounds like it would work, but then the compatible string should
probably be more like qcom,prng-ee-frame or something like that to
indicate that this is a window into the real hardware, instead of
tacking on a -v2 and making everyone think it's new hardware.

Re: [PATCH v2 0/2] hwrng: Add support for qcpm v2 hwrng

[Timur Tabi]

On Tue, Jun 19, 2018 at 4:54 AM, Vinod Koul <vinod.koul@linaro.org> wrote:
> This series adds support for newer version of hwrng as found in
> Qualcomm SoCs. To do that add new v2 compatible string and driver_data
> values for v2.

It's amazing you're sending this now, because I also have a PRNG
patchset that I'm sending out today that adds support for v2 chips on
an ACPI platform.

I'll send my patches as-is, and we'll just have to work to resolve the
merge conflicts.

I will probably need you to test my patches as my last day at Qualcomm
is the 26th.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 0/2] hwrng: Add support for qcpm v2 hwrng

[Vinod]

On 21-06-18, 10:17, Timur Tabi wrote:
> On Tue, Jun 19, 2018 at 4:54 AM, Vinod Koul <vinod.koul@linaro.org> wrote:
> > This series adds support for newer version of hwrng as found in
> > Qualcomm SoCs. To do that add new v2 compatible string and driver_data
> > values for v2.
> 
> It's amazing you're sending this now, because I also have a PRNG
> patchset that I'm sending out today that adds support for v2 chips on
> an ACPI platform.
> 
> I'll send my patches as-is, and we'll just have to work to resolve the
> merge conflicts.

I have checked the series.

I am not sure I follow that this is v2 and you read CONFIG register.
Apart from that other changes look mostly okay with few nitpicks :)

But there is a bigger concern raised by Herbert on this not being a true
RNG and thus should be moved to crypto/

I have the changes ready for that, I will test them later today and
share with you, ACPI addition should be easy on that driver

> I will probably need you to test my patches as my last day at Qualcomm
> is the 26th.

Sad to hear that and good luck in your new adventure

-- 
~Vinod

Re: [PATCH v2 0/2] hwrng: Add support for qcpm v2 hwrng

[Timur Tabi]

On 6/21/18 11:27 PM, Vinod wrote:
> I have checked the series.
> 
> I am not sure I follow that this is v2 and you read CONFIG register.
> Apart from that other changes look mostly okay with few nitpicks:)

It seems that there are two registers sets for v2 hardware, one with 
CONFIG and one without.  I just chose to support the one with.

> But there is a bigger concern raised by Herbert on this not being a true
> RNG and thus should be moved to crypto/

I've asked some of the crypto experts in Q to answer that question.

> I have the changes ready for that, I will test them later today and
> share with you, ACPI addition should be easy on that driver

Ok.  Feel free to just merge my ACPI changes into yours.  Technically, 
it's only a couple lines that are really ACPI-specific.

>> I will probably need you to test my patches as my last day at Qualcomm
>> is the 26th.
> Sad to hear that and good luck in your new adventure

Thanks.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v2 0/2] hwrng: Add support for qcpm v2 hwrng

[Vinod]

On 21-06-18, 23:30, Timur Tabi wrote:

> > But there is a bigger concern raised by Herbert on this not being a true
> > RNG and thus should be moved to crypto/
> 
> I've asked some of the crypto experts in Q to answer that question.

That will be very helpful, I have not got any info on this. Although
specs mention PRNG everywhere..

> > I have the changes ready for that, I will test them later today and
> > share with you, ACPI addition should be easy on that driver
> 
> Ok.  Feel free to just merge my ACPI changes into yours.  Technically, it's
> only a couple lines that are really ACPI-specific.

Sure I will add/port you patch on top.

-- 
~Vinod