[PATCH v2] Exynos : Add support for Exynos random number generator

[PATCH v2] Exynos : Add support for Exynos random number generator

[Jonghwa Lee]

This patch supports Exynos SOC's PRNG driver. Exynos's PRNG has 5 seeds and
5 random number outputs. Module is excuted under runtime power management control,
so it activates only while it's in use. Otherwise it will be suspended generally.
It was tested on PQ board by rngtest program.

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
v2
 - Add HAS_IOMEM and PM_RUNTIME to config condition in Kconfig.
 - Modify parameter of exynos_rng_readl/writel fuction from base register address
   to struct exynos_rng.
 - Fix obscure using of pm_runtime API in exynos_init.
 - Register suspend/resume function with UNIVERSAL_DEV_PM_OPS macro to support
   system suspend/resume and runtime suspend/resume.

 drivers/char/hw_random/Kconfig      |   12 ++
 drivers/char/hw_random/Makefile     |    1 +
 drivers/char/hw_random/exynos-rng.c |  191 +++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+), 0 deletions(-)
 create mode 100644 drivers/char/hw_random/exynos-rng.c

diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index f45dad3..a9e3806 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -263,3 +263,15 @@ config HW_RANDOM_PSERIES
 	  module will be called pseries-rng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_EXYNOS
+	tristate "EXYNOS HW random number generator support"
+	depends on HW_RANDOM && ARCH_EXYNOS && HAS_IOMEM && PM_RUNTIME
+	---help---
+	  This driver provides kernel-side support for the Random Number
+	  Generator hardware found on EXYNOS SOCs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called exynos-rng.
+
+	  If unsure, say Y.
diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
index d901dfa..8d6d173 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_HW_RANDOM_NOMADIK) += nomadik-rng.o
 obj-$(CONFIG_HW_RANDOM_PICOXCELL) += picoxcell-rng.o
 obj-$(CONFIG_HW_RANDOM_PPC4XX) += ppc4xx-rng.o
 obj-$(CONFIG_HW_RANDOM_PSERIES) += pseries-rng.o
+obj-$(CONFIG_HW_RANDOM_EXYNOS)	+= exynos-rng.o
diff --git a/drivers/char/hw_random/exynos-rng.c b/drivers/char/hw_random/exynos-rng.c
new file mode 100644
index 0000000..9c8d690
--- /dev/null
+++ b/drivers/char/hw_random/exynos-rng.c
@@ -0,0 +1,191 @@
+/*
+ * exynos-rng.c - Random Number Generator driver for the exynos
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * Jonghwa Lee <jonghwa3.lee@smasung.com>
+ *
+ * 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;
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ *
+ */
+
+#include <linux/hw_random.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+#include <linux/err.h>
+
+#define EXYNOS_PRNG_STATUS_OFFSET	0x10
+#define EXYNOS_PRNG_SEED_OFFSET		0x140
+#define EXYNOS_PRNG_OUT1_OFFSET		0x160
+#define SEED_SETTING_DONE		BIT(1)
+#define PRNG_START			0x18
+#define PRNG_DONE			BIT(5)
+
+struct exynos_rng {
+	struct device *dev;
+	struct hwrng rng;
+	void __iomem *mem;
+	struct clk *clk;
+};
+
+static u32 exynos_rng_readl(struct exynos_rng *rng, u32 offset)
+{
+	return	__raw_readl(rng->mem + offset);
+}
+
+static void exynos_rng_writel(struct exynos_rng *rng, u32 val, u32 offset)
+{
+	__raw_writel(val, rng->mem + offset);
+}
+
+static int exynos_init(struct hwrng *rng)
+{
+	struct exynos_rng *exynos_rng = container_of(rng,
+						struct exynos_rng, rng);
+	int i;
+	int ret = 0;
+
+	pm_runtime_get_sync(exynos_rng->dev);
+
+	for (i = 0 ; i < 5 ; i++)
+		exynos_rng_writel(exynos_rng, i, EXYNOS_PRNG_SEED_OFFSET + 4*i);
+
+	if (!(exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET)
+						 & SEED_SETTING_DONE))
+		ret = -EIO;
+
+	pm_runtime_put_noidle(exynos_rng->dev);
+
+	return ret;
+}
+
+static int exynos_read(struct hwrng *rng, void *buf,
+					size_t max, bool wait)
+{
+	struct exynos_rng *exynos_rng = container_of(rng,
+						struct exynos_rng, rng);
+	u32 *data = buf;
+
+	pm_runtime_get_sync(exynos_rng->dev);
+
+	exynos_rng_writel(exynos_rng, PRNG_START, 0);
+
+	do {
+		cpu_relax();
+	} while (!(exynos_rng_readl(exynos_rng,
+			EXYNOS_PRNG_STATUS_OFFSET) & PRNG_DONE));
+
+	exynos_rng_writel(exynos_rng, PRNG_DONE, EXYNOS_PRNG_STATUS_OFFSET);
+
+	*data = exynos_rng_readl(exynos_rng, EXYNOS_PRNG_OUT1_OFFSET);
+
+	pm_runtime_put(exynos_rng->dev);
+	return 4;
+}
+
+static int __devinit exynos_rng_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct exynos_rng *exynos_rng;
+	struct resource *res;
+
+	exynos_rng = devm_kzalloc(&pdev->dev, sizeof(struct exynos_rng),
+					GFP_KERNEL);
+	if (!exynos_rng)
+		return -ENOMEM;
+
+	exynos_rng->dev = &pdev->dev;
+	exynos_rng->rng.name = "exynos";
+	exynos_rng->rng.init =	exynos_init;
+	exynos_rng->rng.read = exynos_read;
+	exynos_rng->clk = devm_clk_get(&pdev->dev, "secss");
+	if (IS_ERR(exynos_rng->clk)) {
+		dev_err(&pdev->dev, "Couldn't get clock.\n");
+		return -ENOENT;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		clk_put(exynos_rng->clk);
+		return -ENODEV;
+	}
+
+	exynos_rng->mem = devm_request_and_ioremap(&pdev->dev, res);
+	if (!exynos_rng->mem) {
+		dev_err(&pdev->dev, "Ioremap failed.\n");
+		return -EBUSY;
+	}
+
+	platform_set_drvdata(pdev, exynos_rng);
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = hwrng_register(&exynos_rng->rng);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int __devexit exynos_rng_remove(struct platform_device *pdev)
+{
+	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
+
+	hwrng_unregister(&exynos_rng->rng);
+
+	return 0;
+}
+
+static int exynos_rng_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(exynos_rng->clk);
+	return 0;
+}
+
+static int exynos_rng_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
+
+	clk_prepare_enable(exynos_rng->clk);
+	return 0;
+}
+
+
+UNIVERSAL_DEV_PM_OPS(exynos_rng_pm_ops, exynos_rng_runtime_suspend,
+					exynos_rng_runtime_resume, NULL);
+
+static struct platform_driver exynos_rng_driver = {
+	.driver		= {
+		.name	= "exynos-rng",
+		.owner	= THIS_MODULE,
+		.pm	= &exynos_rng_pm_ops,
+	},
+	.probe		= exynos_rng_probe,
+	.remove		= __devexit_p(exynos_rng_remove),
+};
+
+module_platform_driver(exynos_rng_driver);
+
+MODULE_DESCRIPTION("EXYNOS 4 H/W Random Number Generator driver");
+MODULE_AUTHOR("Jonghwa Lee <jonghwa3.lee@samsung.com>");
+MODULE_LICENSE("GPL");
-- 
1.7.4.1

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[Jamie Iles]

Hi Jongwha,

Just minor nits.

On Wed, Jun 27, 2012 at 07:31:01PM +0900, Jonghwa Lee wrote:
> diff --git a/drivers/char/hw_random/Kconfig 
> b/drivers/char/hw_random/Kconfig
> index f45dad3..a9e3806 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -263,3 +263,15 @@ config HW_RANDOM_PSERIES
>  	  module will be called pseries-rng.
>  
>  	  If unsure, say Y.
> +
> +config HW_RANDOM_EXYNOS
> +	tristate "EXYNOS HW random number generator support"
> +	depends on HW_RANDOM && ARCH_EXYNOS && HAS_IOMEM && PM_RUNTIME

I don't think this needs an ARCH_EXYNOS dependency does it?  I think you 
do need a dependency on HAVE_CLK though then it can be built for other 
platforms.

> diff --git a/drivers/char/hw_random/exynos-rng.c 
> b/drivers/char/hw_random/exynos-rng.c
> new file mode 100644
> index 0000000..9c8d690
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-rng.c
> @@ -0,0 +1,191 @@
> +/*
> + * exynos-rng.c - Random Number Generator driver for the exynos
> + *
> + * Copyright (C) 2012 Samsung Electronics
> + * Jonghwa Lee <jonghwa3.lee@smasung.com>
> + *
> + * 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;
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + */
> +
> +#include <linux/hw_random.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/err.h>
> +
> +#define EXYNOS_PRNG_STATUS_OFFSET	0x10
> +#define EXYNOS_PRNG_SEED_OFFSET		0x140
> +#define EXYNOS_PRNG_OUT1_OFFSET		0x160
> +#define SEED_SETTING_DONE		BIT(1)
> +#define PRNG_START			0x18
> +#define PRNG_DONE			BIT(5)
> +
> +struct exynos_rng {
> +	struct device *dev;
> +	struct hwrng rng;
> +	void __iomem *mem;
> +	struct clk *clk;
> +};
> +
> +static u32 exynos_rng_readl(struct exynos_rng *rng, u32 offset)
> +{
> +	return	__raw_readl(rng->mem + offset);
> +}
> +
> +static void exynos_rng_writel(struct exynos_rng *rng, u32 val, u32 offset)
> +{
> +	__raw_writel(val, rng->mem + offset);
> +}
> +
> +static int exynos_init(struct hwrng *rng)
> +{
> +	struct exynos_rng *exynos_rng = container_of(rng,
> +						struct exynos_rng, rng);
> +	int i;
> +	int ret = 0;
> +
> +	pm_runtime_get_sync(exynos_rng->dev);
> +
> +	for (i = 0 ; i < 5 ; i++)
> +		exynos_rng_writel(exynos_rng, i, EXYNOS_PRNG_SEED_OFFSET + 4*i);
> +
> +	if (!(exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET)
> +						 & SEED_SETTING_DONE))
> +		ret = -EIO;
> +
> +	pm_runtime_put_noidle(exynos_rng->dev);
> +
> +	return ret;
> +}
> +
> +static int exynos_read(struct hwrng *rng, void *buf,
> +					size_t max, bool wait)
> +{
> +	struct exynos_rng *exynos_rng = container_of(rng,
> +						struct exynos_rng, rng);
> +	u32 *data = buf;
> +
> +	pm_runtime_get_sync(exynos_rng->dev);
> +
> +	exynos_rng_writel(exynos_rng, PRNG_START, 0);
> +
> +	do {
> +		cpu_relax();
> +	} while (!(exynos_rng_readl(exynos_rng,
> +			EXYNOS_PRNG_STATUS_OFFSET) & PRNG_DONE));

A minor nit, but

	while (!(exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET) & 
				  PRNG_DONE))
		cpu_relax();

would be a bit more conventional.

> +
> +	exynos_rng_writel(exynos_rng, PRNG_DONE, EXYNOS_PRNG_STATUS_OFFSET);
> +
> +	*data = exynos_rng_readl(exynos_rng, EXYNOS_PRNG_OUT1_OFFSET);
> +
> +	pm_runtime_put(exynos_rng->dev);
> +	return 4;
> +}
> +
> +static int __devinit exynos_rng_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct exynos_rng *exynos_rng;
> +	struct resource *res;
> +
> +	exynos_rng = devm_kzalloc(&pdev->dev, sizeof(struct exynos_rng),
> +					GFP_KERNEL);
> +	if (!exynos_rng)
> +		return -ENOMEM;
> +
> +	exynos_rng->dev = &pdev->dev;
> +	exynos_rng->rng.name = "exynos";
> +	exynos_rng->rng.init =	exynos_init;
> +	exynos_rng->rng.read = exynos_read;
> +	exynos_rng->clk = devm_clk_get(&pdev->dev, "secss");
> +	if (IS_ERR(exynos_rng->clk)) {
> +		dev_err(&pdev->dev, "Couldn't get clock.\n");
> +		return -ENOENT;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		clk_put(exynos_rng->clk);

Doesn't the devm_clk_get() above handle this for you?

Otherwise looks good to me!

Jamie

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[jonghwa3.lee]

On 2012년 06월 27일 20:07, Jamie Iles wrote:

> Hi Jongwha,
> 
> Just minor nits.
> 
> On Wed, Jun 27, 2012 at 07:31:01PM +0900, Jonghwa Lee wrote:
>> diff --git a/drivers/char/hw_random/Kconfig 
>> b/drivers/char/hw_random/Kconfig
>> index f45dad3..a9e3806 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -263,3 +263,15 @@ config HW_RANDOM_PSERIES
>>  	  module will be called pseries-rng.
>>  
>>  	  If unsure, say Y.
>> +
>> +config HW_RANDOM_EXYNOS
>> +	tristate "EXYNOS HW random number generator support"
>> +	depends on HW_RANDOM && ARCH_EXYNOS && HAS_IOMEM && PM_RUNTIME
> 
> I don't think this needs an ARCH_EXYNOS dependency does it?  I think you 
> do need a dependency on HAVE_CLK though then it can be built for other 
> platforms.
> 


This random number generator driver is only for Exynos SOC. And why
should I add HAVE_CLK dependency since there is no relation with?

>> diff --git a/drivers/char/hw_random/exynos-rng.c 
>> b/drivers/char/hw_random/exynos-rng.c
>> new file mode 100644
>> index 0000000..9c8d690
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-rng.c
>> @@ -0,0 +1,191 @@
>> +/*
>> + * exynos-rng.c - Random Number Generator driver for the exynos
>> + *
>> + * Copyright (C) 2012 Samsung Electronics
>> + * Jonghwa Lee <jonghwa3.lee@smasung.com>
>> + *
>> + * 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;
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + *
>> + */
>> +
>> +#include <linux/hw_random.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/clk.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/err.h>
>> +
>> +#define EXYNOS_PRNG_STATUS_OFFSET	0x10
>> +#define EXYNOS_PRNG_SEED_OFFSET		0x140
>> +#define EXYNOS_PRNG_OUT1_OFFSET		0x160
>> +#define SEED_SETTING_DONE		BIT(1)
>> +#define PRNG_START			0x18
>> +#define PRNG_DONE			BIT(5)
>> +
>> +struct exynos_rng {
>> +	struct device *dev;
>> +	struct hwrng rng;
>> +	void __iomem *mem;
>> +	struct clk *clk;
>> +};
>> +
>> +static u32 exynos_rng_readl(struct exynos_rng *rng, u32 offset)
>> +{
>> +	return	__raw_readl(rng->mem + offset);
>> +}
>> +
>> +static void exynos_rng_writel(struct exynos_rng *rng, u32 val, u32 offset)
>> +{
>> +	__raw_writel(val, rng->mem + offset);
>> +}
>> +
>> +static int exynos_init(struct hwrng *rng)
>> +{
>> +	struct exynos_rng *exynos_rng = container_of(rng,
>> +						struct exynos_rng, rng);
>> +	int i;
>> +	int ret = 0;
>> +
>> +	pm_runtime_get_sync(exynos_rng->dev);
>> +
>> +	for (i = 0 ; i < 5 ; i++)
>> +		exynos_rng_writel(exynos_rng, i, EXYNOS_PRNG_SEED_OFFSET + 4*i);
>> +
>> +	if (!(exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET)
>> +						 & SEED_SETTING_DONE))
>> +		ret = -EIO;
>> +
>> +	pm_runtime_put_noidle(exynos_rng->dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int exynos_read(struct hwrng *rng, void *buf,
>> +					size_t max, bool wait)
>> +{
>> +	struct exynos_rng *exynos_rng = container_of(rng,
>> +						struct exynos_rng, rng);
>> +	u32 *data = buf;
>> +
>> +	pm_runtime_get_sync(exynos_rng->dev);
>> +
>> +	exynos_rng_writel(exynos_rng, PRNG_START, 0);
>> +
>> +	do {
>> +		cpu_relax();
>> +	} while (!(exynos_rng_readl(exynos_rng,
>> +			EXYNOS_PRNG_STATUS_OFFSET) & PRNG_DONE));
> 
> A minor nit, but
> 
> 	while (!(exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET) & 
> 				  PRNG_DONE))
> 		cpu_relax();
> 
> would be a bit more conventional.
> 


Stephen Boyd suggested to me that way. Anyway I'll follow with
conventional way.

>> +
>> +	exynos_rng_writel(exynos_rng, PRNG_DONE, EXYNOS_PRNG_STATUS_OFFSET);
>> +
>> +	*data = exynos_rng_readl(exynos_rng, EXYNOS_PRNG_OUT1_OFFSET);
>> +
>> +	pm_runtime_put(exynos_rng->dev);
>> +	return 4;
>> +}
>> +
>> +static int __devinit exynos_rng_probe(struct platform_device *pdev)
>> +{
>> +	int ret;
>> +	struct exynos_rng *exynos_rng;
>> +	struct resource *res;
>> +
>> +	exynos_rng = devm_kzalloc(&pdev->dev, sizeof(struct exynos_rng),
>> +					GFP_KERNEL);
>> +	if (!exynos_rng)
>> +		return -ENOMEM;
>> +
>> +	exynos_rng->dev = &pdev->dev;
>> +	exynos_rng->rng.name = "exynos";
>> +	exynos_rng->rng.init =	exynos_init;
>> +	exynos_rng->rng.read = exynos_read;
>> +	exynos_rng->clk = devm_clk_get(&pdev->dev, "secss");
>> +	if (IS_ERR(exynos_rng->clk)) {
>> +		dev_err(&pdev->dev, "Couldn't get clock.\n");
>> +		return -ENOENT;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		clk_put(exynos_rng->clk);
> 
> Doesn't the devm_clk_get() above handle this for you?
> 


Yes, it's my mistake, I'll remove it.

Thanks.

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[Jamie Iles]

On Wed, Jun 27, 2012 at 08:21:50PM +0900, jonghwa3.lee@samsung.com wrote:
> On 2012년 06월 27일 20:07, Jamie Iles wrote:
> > On Wed, Jun 27, 2012 at 07:31:01PM +0900, Jonghwa Lee wrote:
> >> diff --git a/drivers/char/hw_random/Kconfig 
> >> b/drivers/char/hw_random/Kconfig
> >> index f45dad3..a9e3806 100644
> >> --- a/drivers/char/hw_random/Kconfig
> >> +++ b/drivers/char/hw_random/Kconfig
> >> @@ -263,3 +263,15 @@ config HW_RANDOM_PSERIES
> >>  	  module will be called pseries-rng.
> >>  
> >>  	  If unsure, say Y.
> >> +
> >> +config HW_RANDOM_EXYNOS
> >> +	tristate "EXYNOS HW random number generator support"
> >> +	depends on HW_RANDOM && ARCH_EXYNOS && HAS_IOMEM && PM_RUNTIME
> > 
> > I don't think this needs an ARCH_EXYNOS dependency does it?  I think you 
> > do need a dependency on HAVE_CLK though then it can be built for other 
> > platforms.
> > 
> This random number generator driver is only for Exynos SOC. And why
> should I add HAVE_CLK dependency since there is no relation with?

Well if you remove the ARCH_EXYNOS dependency then you should get higher 
build coverage which is a nice thing to have.

Anything using the clk API should have a dependency on HAVE_CLK as 
clk_*() doesn't have stubs for platforms that don't implement the API.

Jamie

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[Stephen Boyd]

On 06/27/12 04:21, jonghwa3.lee@samsung.com wrote:
> On 2012년 06월 27일 20:07, Jamie Iles wrote:
>> A minor nit, but
>>
>> 	while (!(exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET) & 
>> 				  PRNG_DONE))
>> 		cpu_relax();
>>
>> would be a bit more conventional.
>>
>
> Stephen Boyd suggested to me that way. Anyway I'll follow with
> conventional way.

I was thinking

	do {
		status = exynos_rng_readl(exynos_rng, EXYNOS_PRNG_STATUS_OFFSET);
	} while (!(status & PRNG_DONE));


But that works too.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[Stephen Boyd]

Some minor comments, otherwise this looks much better than the previous
patch.

On 06/27/12 03:31, Jonghwa Lee wrote:
> This patch supports Exynos SOC's PRNG driver. Exynos's PRNG has 5 seeds and
> 5 random number outputs. Module is excuted under runtime power management control,
> so it activates only while it's in use. Otherwise it will be suspended generally.
> It was tested on PQ board by rngtest program.
>
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

This is an incorrect signoff chain. Kyungmin is not sending this so why
are you not the last one to sign off? Who is the author, Kyungmin or
yourself?

> +
> +config HW_RANDOM_EXYNOS
> +	tristate "EXYNOS HW random number generator support"
> +	depends on HW_RANDOM && ARCH_EXYNOS && HAS_IOMEM && PM_RUNTIME

There is no need to depend on PM_RUNTIME or ARCH_EXYNOS.

> +
> +static int exynos_read(struct hwrng *rng, void *buf,
> +					size_t max, bool wait)
> +{
> +	struct exynos_rng *exynos_rng = container_of(rng,
> +						struct exynos_rng, rng);
> +	u32 *data = buf;
> +
> +	pm_runtime_get_sync(exynos_rng->dev);
> +
> +	exynos_rng_writel(exynos_rng, PRNG_START, 0);
> +
> +	do {
> +		cpu_relax();
> +	} while (!(exynos_rng_readl(exynos_rng,
> +			EXYNOS_PRNG_STATUS_OFFSET) & PRNG_DONE));
> +
> +	exynos_rng_writel(exynos_rng, PRNG_DONE, EXYNOS_PRNG_STATUS_OFFSET);

Curious, is this actually required? You poll for the status to say done
and the hardware requires you to write back the done bit after it
signals done?

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		clk_put(exynos_rng->clk);
> +		return -ENODEV;
> +	}

Pass this through directly to devm_request_and_ioremap() without
checking the return value to save some lines.

> +
> +	exynos_rng->mem = devm_request_and_ioremap(&pdev->dev, res);
> +	if (!exynos_rng->mem) {
> +		dev_err(&pdev->dev, "Ioremap failed.\n");

devm_request_and_ioremap() already prints a message on failure to remap
so this is unnecessary printk.

> +		return -EBUSY;
> +	}
> +
> +	platform_set_drvdata(pdev, exynos_rng);
> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = hwrng_register(&exynos_rng->rng);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

Why not just 'return hwrng_register()'?

> +
> +static int exynos_rng_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
> +
> +	clk_prepare_enable(exynos_rng->clk);
> +	return 0;

Perhaps return the value of clk_prepare_enable() in case it fails for
some reason?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[jonghwa3.lee]

On 2012년 06월 28일 02:52, Stephen Boyd wrote:

> Some minor comments, otherwise this looks much better than the previous
> patch.
> 
> On 06/27/12 03:31, Jonghwa Lee wrote:
>> This patch supports Exynos SOC's PRNG driver. Exynos's PRNG has 5 seeds and
>> 5 random number outputs. Module is excuted under runtime power management control,
>> so it activates only while it's in use. Otherwise it will be suspended generally.
>> It was tested on PQ board by rngtest program.
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> This is an incorrect signoff chain. Kyungmin is not sending this so why
> are you not the last one to sign off? Who is the author, Kyungmin or
> yourself?
> 


I'm author. Okay, I'll leave my name only.

>> +
>> +config HW_RANDOM_EXYNOS
>> +	tristate "EXYNOS HW random number generator support"
>> +	depends on HW_RANDOM && ARCH_EXYNOS && HAS_IOMEM && PM_RUNTIME
> 
> There is no need to depend on PM_RUNTIME or ARCH_EXYNOS.
> 
>> +
>> +static int exynos_read(struct hwrng *rng, void *buf,
>> +					size_t max, bool wait)
>> +{
>> +	struct exynos_rng *exynos_rng = container_of(rng,
>> +						struct exynos_rng, rng);
>> +	u32 *data = buf;
>> +
>> +	pm_runtime_get_sync(exynos_rng->dev);
>> +
>> +	exynos_rng_writel(exynos_rng, PRNG_START, 0);
>> +
>> +	do {
>> +		cpu_relax();
>> +	} while (!(exynos_rng_readl(exynos_rng,
>> +			EXYNOS_PRNG_STATUS_OFFSET) & PRNG_DONE));
>> +
>> +	exynos_rng_writel(exynos_rng, PRNG_DONE, EXYNOS_PRNG_STATUS_OFFSET);
> 
> Curious, is this actually required? You poll for the status to say done
> and the hardware requires you to write back the done bit after it
> signals done?
> 


Yes, It's hardware's own characteristic. It needs to be written 1 on
status register to clear it.

>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		clk_put(exynos_rng->clk);
>> +		return -ENODEV;
>> +	}
> 
> Pass this through directly to devm_request_and_ioremap() without
> checking the return value to save some lines.
> 

>> +

>> +	exynos_rng->mem = devm_request_and_ioremap(&pdev->dev, res);
>> +	if (!exynos_rng->mem) {
>> +		dev_err(&pdev->dev, "Ioremap failed.\n");
> 
> devm_request_and_ioremap() already prints a message on failure to remap
> so this is unnecessary printk.
> 

>> +		return -EBUSY;

>> +	}
>> +
>> +	platform_set_drvdata(pdev, exynos_rng);
>> +
>> +	pm_runtime_enable(&pdev->dev);
>> +
>> +	ret = hwrng_register(&exynos_rng->rng);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
> 
> Why not just 'return hwrng_register()'?
> 

>> +

>> +static int exynos_rng_runtime_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
>> +
>> +	clk_prepare_enable(exynos_rng->clk);
>> +	return 0;
> 
> Perhaps return the value of clk_prepare_enable() in case it fails for
> some reason?
> 


Okay, I agree with your opinion all.

Thanks.

Re: [PATCH v2] Exynos : Add support for Exynos random number generator

[jonghwa3.lee]

On 2012년 06월 28일 09:20, jonghwa3.lee@samsung.com wrote:
> On 2012년 06월 28일 02:52, Stephen Boyd wrote:
>> On 06/27/12 03:31, Jonghwa Lee wrote:
>>> This patch supports Exynos SOC's PRNG driver. Exynos's PRNG has 5
seeds and
>>> 5 random number outputs. Module is excuted under runtime power
management control,
>>> so it activates only while it's in use. Otherwise it will be
suspended generally.
>>> It was tested on PQ board by rngtest program.
>>>
>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> This is an incorrect signoff chain. Kyungmin is not sending this so why
>> are you not the last one to sign off? Who is the author, Kyungmin or
>> yourself?
>>
>
>
> I'm author. Okay, I'll leave my name only.
>

Sorry I have to change my word, I can't do that. Actually, we work in
same place and he is supervisor of me. So due to our internal
regulations, we have put the supervisor's name bottom of signed-off list
conventionally. Do you want me to reverse the signed-off list?