| Message ID | 20180619095430.26358-3-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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
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
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,
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.
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
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.
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);
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(-)