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

[PATCH] 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>
---
 drivers/char/hw_random/Kconfig      |   12 ++
 drivers/char/hw_random/Makefile     |    1 +
 drivers/char/hw_random/exynos-rng.c |  204 +++++++++++++++++++++++++++++++++++
 3 files changed, 217 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..8220026 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 Random Number Generator support"
+	depends on HW_RANDOM && ARCH_EXYNOS4
+	---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..b58a28b
--- /dev/null
+++ b/drivers/char/hw_random/exynos-rng.c
@@ -0,0 +1,204 @@
+/*
+ * 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>
+
+#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(void __iomem *base, u32 offset)
+{
+	return	__raw_readl(base + offset);
+}
+
+static void exynos_rng_writel(u32 val, void __iomem *base, u32 offset)
+{
+	__raw_writel(val, base + 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;
+	u32 PRND_SEED[5];
+
+	pm_runtime_put_noidle(exynos_rng->dev);
+	pm_runtime_get_sync(exynos_rng->dev);
+
+	for (i = 0 ; i < 5 ; i++) {
+		PRND_SEED[i] = i;
+		exynos_rng_writel(PRND_SEED[i], exynos_rng->mem,
+					EXYNOS_PRNG_SEED_OFFSET + 4*i);
+	}
+
+	if (!(exynos_rng_readl(exynos_rng->mem, EXYNOS_PRNG_STATUS_OFFSET)
+						 & SEED_SETTING_DONE))
+		ret = -EIO;
+
+	pm_runtime_put(exynos_rng->dev);
+	pm_runtime_get_noresume(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;
+	u32 status = 0;
+
+	pm_runtime_get_sync(exynos_rng->dev);
+	exynos_rng_writel(PRNG_START, exynos_rng->mem, 0);
+
+	while (!status) {
+		status = exynos_rng_readl(exynos_rng->mem,
+					EXYNOS_PRNG_STATUS_OFFSET);
+		status &= PRNG_DONE;
+	}
+
+	exynos_rng_writel(PRNG_DONE, exynos_rng->mem,
+					EXYNOS_PRNG_STATUS_OFFSET);
+
+	*data = exynos_rng_readl(exynos_rng->mem,
+					EXYNOS_PRNG_OUT1_OFFSET);
+
+	pm_runtime_put(exynos_rng->dev);
+	return 4;
+}
+
+static int __init 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 = clk_get(NULL, "secss");
+	if (!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_ioremap(&pdev->dev, res->start,
+						 resource_size(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);
+	pm_runtime_irq_safe(&pdev->dev);
+
+	ret = hwrng_register(&exynos_rng->rng);
+	if (ret) {
+		clk_put(exynos_rng->clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __exit exynos_rng_remove(struct platform_device *pdev)
+{
+	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
+
+	hwrng_unregister(&exynos_rng->rng);
+	clk_put(exynos_rng->clk);
+	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(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_enable(exynos_rng->clk);
+	return 0;
+}
+
+static const struct dev_pm_ops exynos_rng_pm_ops = {
+	.runtime_suspend = exynos_rng_runtime_suspend,
+	.runtime_resume = exynos_rng_runtime_resume,
+};
+
+static struct platform_driver exynos_rng_driver = {
+	.driver		= {
+		.name	= "exynos-rng",
+		.owner	= THIS_MODULE,
+		.pm	= &exynos_rng_pm_ops,
+	},
+	.probe		= exynos_rng_probe,
+	.remove		= 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] Exynos : Add support for Exynos random number generator

[Stephen Boyd]

On 06/20/12 01:22, Jonghwa Lee wrote:
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index f45dad3..8220026 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 Random Number Generator support"
> +	depends on HW_RANDOM && ARCH_EXYNOS4

I don't see how this actually depends on ARCH_EXYNOS4 to be compiled. I
obviously wouldn't want to compile in this driver if I didn't have the
hardware but the driver seems generic enough to be compiled anywhere
(e.g. in an x86 allmodconfig). I suppose you need to add HAS_IOMEM though.

> +	---help---
> +	  This driver provides kernel-side support for the Random Number
> +	  Generator hardware found on EXYNOS SOCs.

Why is 'random number generator' capitalized?

> diff --git a/drivers/char/hw_random/exynos-rng.c b/drivers/char/hw_random/exynos-rng.c
> new file mode 100644
> index 0000000..b58a28b
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-rng.c
> @@ -0,0 +1,204 @@
[snip]
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.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)

Please consistently use tabs or spaces here between the '#define' and
the name.

> +
> +struct exynos_rng {
> +	struct device *dev;
> +	struct hwrng rng;
> +	void __iomem *mem;
> +	struct clk *clk;
> +};
> +
> +static u32 exynos_rng_readl(void __iomem *base, u32 offset)
> +{
> +	return	__raw_readl(base + offset);
> +}

There seems to be a tab here? Also, why don't these read/write functions
take the exynos_rng struct so that you don't have to pass the base
pointer. That would make these functions more useful than just being a
wrapper around __raw_{readl,writel}()

    u32 exynos_rng_readl(struct exynos_rng *rng, u32 offset)
    void exynos_rng_writel(struct exynos_rng *rng, u32 val, u32 offset)

> +
> +static void exynos_rng_writel(u32 val, void __iomem *base, u32 offset)
> +{
> +	__raw_writel(val, base + 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;
> +	u32 PRND_SEED[5];
> +
> +	pm_runtime_put_noidle(exynos_rng->dev);
> +	pm_runtime_get_sync(exynos_rng->dev);

This looks very odd. Why are you calling pm_runtime_put_noidle()?

> +
> +	for (i = 0 ; i < 5 ; i++) {
> +		PRND_SEED[i] = i;
> +		exynos_rng_writel(PRND_SEED[i], exynos_rng->mem,
> +					EXYNOS_PRNG_SEED_OFFSET + 4*i);
> +	}

Is this just writing 0,1,2,3,4 to registers? What is the array for?

> +
> +	if (!(exynos_rng_readl(exynos_rng->mem, EXYNOS_PRNG_STATUS_OFFSET)
> +						 & SEED_SETTING_DONE))
> +		ret = -EIO;
> +
> +	pm_runtime_put(exynos_rng->dev);
> +	pm_runtime_get_noresume(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;
> +	u32 status = 0;

Drop this assignment here.

> +
> +	pm_runtime_get_sync(exynos_rng->dev);
> +	exynos_rng_writel(PRNG_START, exynos_rng->mem, 0);
> +
> +	while (!status) {
> +		status = exynos_rng_readl(exynos_rng->mem,
> +					EXYNOS_PRNG_STATUS_OFFSET);
> +		status &= PRNG_DONE;
> +	}

And make this into a do while with a cpu_relax() thrown in there.

> +
> +	exynos_rng_writel(PRNG_DONE, exynos_rng->mem,
> +					EXYNOS_PRNG_STATUS_OFFSET);
> +
> +	*data = exynos_rng_readl(exynos_rng->mem,
> +					EXYNOS_PRNG_OUT1_OFFSET);
> +
> +	pm_runtime_put(exynos_rng->dev);
> +	return 4;
> +}
> +
> +static int __init exynos_rng_probe(struct platform_device *pdev)

__devinit

> +{
> +	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 = clk_get(NULL, "secss");

Can you please pass &pdev->dev to clk_get()?

> +	if (!exynos_rng->clk) {

NULL is a valid clock. Please check for IS_ERR() only. Also you may want
to use devm_clk_get().

> +		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_ioremap(&pdev->dev, res->start,
> +						 resource_size(res));

It might be a good idea to use devm_request_and_ioremap() here instead.

> +	if (!exynos_rng->mem) {
> +		dev_err(&pdev->dev, "Ioremap failed.\n");
> +		return -EBUSY;
> +	}
> +
> +	platform_set_drvdata(pdev, exynos_rng);
> +
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_irq_safe(&pdev->dev);

It doesn't seem like you need to run runtime PM calls in irq context.
Why is this here?

> +
> +	ret = hwrng_register(&exynos_rng->rng);
> +	if (ret) {
> +		clk_put(exynos_rng->clk);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __exit exynos_rng_remove(struct platform_device *pdev)

__devexit

> +{
> +	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
> +
> +	hwrng_unregister(&exynos_rng->rng);
> +	clk_put(exynos_rng->clk);
> +	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(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_enable(exynos_rng->clk);

Please use clk_prepare_enable()/clk_disable_unprepare() so we don't have
to convert this driver later.

> +
> +static const struct dev_pm_ops exynos_rng_pm_ops = {
> +	.runtime_suspend = exynos_rng_runtime_suspend,
> +	.runtime_resume = exynos_rng_runtime_resume,
> +};

You should use something like UNIVERSAL_DEV_PM_OPS here so that you can
#ifdef CONFIG_PM the runtime suspend/resume functions. If CONFIG_PM=n
does this driver work? I wonder if the clocks are assumed to be on in
that case?

> +
> +static struct platform_driver exynos_rng_driver = {
> +	.driver		= {
> +		.name	= "exynos-rng",
> +		.owner	= THIS_MODULE,
> +		.pm	= &exynos_rng_pm_ops,
> +	},
> +	.probe		= exynos_rng_probe,
> +	.remove		= exynos_rng_remove,

__devexit_p()

-- 
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] Exynos : Add support for Exynos random number generator

[jonghwa3.lee]

Hi, Stephen.
Thanks for quick reviewing.

On 2012년 06월 21일 09:47, Stephen Boyd wrote:

> On 06/20/12 01:22, Jonghwa Lee wrote:
>> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
>> index f45dad3..8220026 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 Random Number Generator support"
>> +	depends on HW_RANDOM && ARCH_EXYNOS4
> 
> I don't see how this actually depends on ARCH_EXYNOS4 to be compiled. I
> obviously wouldn't want to compile in this driver if I didn't have the
> hardware but the driver seems generic enough to be compiled anywhere
> (e.g. in an x86 allmodconfig). I suppose you need to add HAS_IOMEM though.
> 


I just add this condition only because EXYNOS series has PRNG in it
generally. But, I'll fix it as you mentioned.

>> +	---help---
>> +	  This driver provides kernel-side support for the Random Number
>> +	  Generator hardware found on EXYNOS SOCs.
> 
> Why is 'random number generator' capitalized?
> 


I'll modify it.

>> diff --git a/drivers/char/hw_random/exynos-rng.c b/drivers/char/hw_random/exynos-rng.c
>> new file mode 100644
>> index 0000000..b58a28b
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-rng.c
>> @@ -0,0 +1,204 @@
> [snip]
>> +#include <linux/clk.h>
>> +#include <linux/pm_runtime.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)
> 
> Please consistently use tabs or spaces here between the '#define' and
> the name.
> 


Okay,

>> +
>> +struct exynos_rng {
>> +	struct device *dev;
>> +	struct hwrng rng;
>> +	void __iomem *mem;
>> +	struct clk *clk;
>> +};
>> +
>> +static u32 exynos_rng_readl(void __iomem *base, u32 offset)
>> +{
>> +	return	__raw_readl(base + offset);
>> +}
> 
> There seems to be a tab here? Also, why don't these read/write functions
> take the exynos_rng struct so that you don't have to pass the base
> pointer. That would make these functions more useful than just being a
> wrapper around __raw_{readl,writel}()
> 
>     u32 exynos_rng_readl(struct exynos_rng *rng, u32 offset)
>     void exynos_rng_writel(struct exynos_rng *rng, u32 val, u32 offset)
> 


Okay, I'll apply it.

>> +
>> +static void exynos_rng_writel(u32 val, void __iomem *base, u32 offset)
>> +{
>> +	__raw_writel(val, base + 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;
>> +	u32 PRND_SEED[5];
>> +
>> +	pm_runtime_put_noidle(exynos_rng->dev);
>> +	pm_runtime_get_sync(exynos_rng->dev);
> 
> This looks very odd. Why are you calling pm_runtime_put_noidle()?
> 


When this callback function is called, the status of power state is
'suspended' and use_count is 1. To perform pm_runtime_get_sync()
correctly, it requires to have 'suspended' status and use_count is 0.
Thus i force to decrease use_count only with using
pm_runtime_put_noidle. I know it looks odd, but i couldn't find better
way. Otherwise it can use clk_enable() directly, but i think that it
isn't good neither.

>> +
>> +	for (i = 0 ; i < 5 ; i++) {
>> +		PRND_SEED[i] = i;
>> +		exynos_rng_writel(PRND_SEED[i], exynos_rng->mem,
>> +					EXYNOS_PRNG_SEED_OFFSET + 4*i);
>> +	}
> 
> Is this just writing 0,1,2,3,4 to registers? What is the array for?
> 


I'll modify it.

>> +
>> +	if (!(exynos_rng_readl(exynos_rng->mem, EXYNOS_PRNG_STATUS_OFFSET)
>> +						 & SEED_SETTING_DONE))
>> +		ret = -EIO;
>> +
>> +	pm_runtime_put(exynos_rng->dev);
>> +	pm_runtime_get_noresume(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;
>> +	u32 status = 0;
> 
> Drop this assignment here.
> 


Okay,

>> +
>> +	pm_runtime_get_sync(exynos_rng->dev);
>> +	exynos_rng_writel(PRNG_START, exynos_rng->mem, 0);
>> +
>> +	while (!status) {
>> +		status = exynos_rng_readl(exynos_rng->mem,
>> +					EXYNOS_PRNG_STATUS_OFFSET);
>> +		status &= PRNG_DONE;
>> +	}
> 
> And make this into a do while with a cpu_relax() thrown in there.
> 


Okay

>> +
>> +	exynos_rng_writel(PRNG_DONE, exynos_rng->mem,
>> +					EXYNOS_PRNG_STATUS_OFFSET);
>> +
>> +	*data = exynos_rng_readl(exynos_rng->mem,
>> +					EXYNOS_PRNG_OUT1_OFFSET);
>> +
>> +	pm_runtime_put(exynos_rng->dev);
>> +	return 4;
>> +}
>> +
>> +static int __init exynos_rng_probe(struct platform_device *pdev)
> 
> __devinit
> 


Okay,

>> +{
>> +	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 = clk_get(NULL, "secss");
> 
> Can you please pass &pdev->dev to clk_get()?
>


But, this clock is not only used in PRNG. Should i put it in?

 
>> +	if (!exynos_rng->clk) {
> 
> NULL is a valid clock. Please check for IS_ERR() only. Also you may want
> to use devm_clk_get().
> 


Okay

>> +		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_ioremap(&pdev->dev, res->start,
>> +						 resource_size(res));
> 
> It might be a good idea to use devm_request_and_ioremap() here instead.
> 


Okay,

>> +	if (!exynos_rng->mem) {
>> +		dev_err(&pdev->dev, "Ioremap failed.\n");
>> +		return -EBUSY;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, exynos_rng);
>> +
>> +	pm_runtime_enable(&pdev->dev);
>> +	pm_runtime_irq_safe(&pdev->dev);
> 
> It doesn't seem like you need to run runtime PM calls in irq context.
> Why is this here?
> 


I think i misunderstood to use pm_runtime_sync(). I understood it is
needed for using pm_runtime_sync() for any condition. I'll remove it.

>> +
>> +	ret = hwrng_register(&exynos_rng->rng);
>> +	if (ret) {
>> +		clk_put(exynos_rng->clk);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int __exit exynos_rng_remove(struct platform_device *pdev)
> 
> __devexit
> 


Okay,

>> +{
>> +	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
>> +
>> +	hwrng_unregister(&exynos_rng->rng);
>> +	clk_put(exynos_rng->clk);
>> +	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(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_enable(exynos_rng->clk);
> 
> Please use clk_prepare_enable()/clk_disable_unprepare() so we don't have
> to convert this driver later.
> 


Okay,

>> +
>> +static const struct dev_pm_ops exynos_rng_pm_ops = {
>> +	.runtime_suspend = exynos_rng_runtime_suspend,
>> +	.runtime_resume = exynos_rng_runtime_resume,
>> +};
> 
> You should use something like UNIVERSAL_DEV_PM_OPS here so that you can
> #ifdef CONFIG_PM the runtime suspend/resume functions. If CONFIG_PM=n
> does this driver work? I wonder if the clocks are assumed to be on in
> that case?
> 


Okay, I'll fix it.

>> +
>> +static struct platform_driver exynos_rng_driver = {
>> +	.driver		= {
>> +		.name	= "exynos-rng",
>> +		.owner	= THIS_MODULE,
>> +		.pm	= &exynos_rng_pm_ops,
>> +	},
>> +	.probe		= exynos_rng_probe,
>> +	.remove		= exynos_rng_remove,
> 
> __devexit_p()
> 

Okay


Regards.

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

[Stephen Boyd]

On 06/20/12 19:39, jonghwa3.lee@samsung.com wrote:
>
>>> +static int exynos_init(struct hwrng *rng)
>>> +{
>>> +	struct exynos_rng *exynos_rng = container_of(rng,
>>> +						struct exynos_rng, rng);
>>> +	int i;
>>> +	int ret = 0;
>>> +	u32 PRND_SEED[5];
>>> +
>>> +	pm_runtime_put_noidle(exynos_rng->dev);
>>> +	pm_runtime_get_sync(exynos_rng->dev);
>> This looks very odd. Why are you calling pm_runtime_put_noidle()?
>>
>
> When this callback function is called, the status of power state is
> 'suspended' and use_count is 1. To perform pm_runtime_get_sync()
> correctly, it requires to have 'suspended' status and use_count is 0.
> Thus i force to decrease use_count only with using
> pm_runtime_put_noidle. I know it looks odd, but i couldn't find better
> way. Otherwise it can use clk_enable() directly, but i think that it
> isn't good neither.

Is the device suspended initially at probe? If so can you set the state
of the device to suspended?

>
>>> +{
>>> +	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 = clk_get(NULL, "secss");
>> Can you please pass &pdev->dev to clk_get()?
>>
>
> But, this clock is not only used in PRNG. Should i put it in?

Why would that matter? I'm not familiar with the samsung clock
implementation but I would assume that something like clkdev is being
used. Ideally you would have one lookup entry per struct device that
uses this clock and then tie each lookup entry to the same clock. This
allows you to support multiple instances of the same device in a generic
way (e.g. two rng devices that use different clocks but from the driver
perspective they have the same name 'secss').

-- 
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] Exynos : Add support for Exynos random number generator

[jonghwa3.lee]

On 2012년 06월 21일 12:12, Stephen Boyd wrote:

> On 06/20/12 19:39, jonghwa3.lee@samsung.com wrote:
>>
>>>> +static int exynos_init(struct hwrng *rng)
>>>> +{
>>>> +	struct exynos_rng *exynos_rng = container_of(rng,
>>>> +						struct exynos_rng, rng);
>>>> +	int i;
>>>> +	int ret = 0;
>>>> +	u32 PRND_SEED[5];
>>>> +
>>>> +	pm_runtime_put_noidle(exynos_rng->dev);
>>>> +	pm_runtime_get_sync(exynos_rng->dev);
>>> This looks very odd. Why are you calling pm_runtime_put_noidle()?
>>>
>>
>> When this callback function is called, the status of power state is
>> 'suspended' and use_count is 1. To perform pm_runtime_get_sync()
>> correctly, it requires to have 'suspended' status and use_count is 0.
>> Thus i force to decrease use_count only with using
>> pm_runtime_put_noidle. I know it looks odd, but i couldn't find better
>> way. Otherwise it can use clk_enable() directly, but i think that it
>> isn't good neither.
> 
> Is the device suspended initially at probe? If so can you set the state
> of the device to suspended?
> 


Yes, it suspended already at the initial of probing. But i can't get
your point, you want to me to make device suspended?

>>
>>>> +{
>>>> +	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 = clk_get(NULL, "secss");
>>> Can you please pass &pdev->dev to clk_get()?
>>>
>>
>> But, this clock is not only used in PRNG. Should i put it in?
> 
> Why would that matter? I'm not familiar with the samsung clock
> implementation but I would assume that something like clkdev is being
> used. Ideally you would have one lookup entry per struct device that
> uses this clock and then tie each lookup entry to the same clock. This
> allows you to support multiple instances of the same device in a generic
> way (e.g. two rng devices that use different clocks but from the driver
> perspective they have the same name 'secss').
> 


Okay, I see. I'll apply it.

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

[Stephen Boyd]

On 06/21/12 18:39, jonghwa3.lee@samsung.com wrote:
> On 2012년 06월 21일 12:12, Stephen Boyd wrote:
>
>> On 06/20/12 19:39, jonghwa3.lee@samsung.com wrote:
>>>>> +static int exynos_init(struct hwrng *rng)
>>>>> +{
>>>>> +	struct exynos_rng *exynos_rng = container_of(rng,
>>>>> +						struct exynos_rng, rng);
>>>>> +	int i;
>>>>> +	int ret = 0;
>>>>> +	u32 PRND_SEED[5];
>>>>> +
>>>>> +	pm_runtime_put_noidle(exynos_rng->dev);
>>>>> +	pm_runtime_get_sync(exynos_rng->dev);
>>>> This looks very odd. Why are you calling pm_runtime_put_noidle()?
>>>>
>>> When this callback function is called, the status of power state is
>>> 'suspended' and use_count is 1. To perform pm_runtime_get_sync()
>>> correctly, it requires to have 'suspended' status and use_count is 0.
>>> Thus i force to decrease use_count only with using
>>> pm_runtime_put_noidle. I know it looks odd, but i couldn't find better
>>> way. Otherwise it can use clk_enable() directly, but i think that it
>>> isn't good neither.
>> Is the device suspended initially at probe? If so can you set the state
>> of the device to suspended?
>>
>
> Yes, it suspended already at the initial of probing. But i can't get
> your point, you want to me to make device suspended?

I mean to say you should probably call pm_runtime_set_suspended() during
probe. But I'm not sure and I would defer to people more familiar with
runtime pm.

-- 
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] Exynos : Add support for Exynos random number generator

[Alan Stern]

On Tue, 26 Jun 2012, Stephen Boyd wrote:

> On 06/21/12 18:39, jonghwa3.lee@samsung.com wrote:
> > On 2012년 06월 21일 12:12, Stephen Boyd wrote:
> >
> >> On 06/20/12 19:39, jonghwa3.lee@samsung.com wrote:
> >>>>> +static int exynos_init(struct hwrng *rng)
> >>>>> +{
> >>>>> +	struct exynos_rng *exynos_rng = container_of(rng,
> >>>>> +						struct exynos_rng, rng);
> >>>>> +	int i;
> >>>>> +	int ret = 0;
> >>>>> +	u32 PRND_SEED[5];
> >>>>> +
> >>>>> +	pm_runtime_put_noidle(exynos_rng->dev);
> >>>>> +	pm_runtime_get_sync(exynos_rng->dev);
> >>>> This looks very odd. Why are you calling pm_runtime_put_noidle()?
> >>>>
> >>> When this callback function is called, the status of power state is
> >>> 'suspended' and use_count is 1.

That's strange to begin with.  If the usage counter is > 0 then the
device shouldn't be suspended.  How does this happen?

>  To perform pm_runtime_get_sync()
> >>> correctly, it requires to have 'suspended' status and use_count is 0.

That's not true.  pm_runtime_get_sync works perfectly well when the 
status is "suspended" and the usage counter is > 0.

> >>> Thus i force to decrease use_count only with using
> >>> pm_runtime_put_noidle. I know it looks odd, but i couldn't find better
> >>> way. Otherwise it can use clk_enable() directly, but i think that it
> >>> isn't good neither.
> >> Is the device suspended initially at probe? If so can you set the state
> >> of the device to suspended?
> >>
> >
> > Yes, it suspended already at the initial of probing. But i can't get
> > your point, you want to me to make device suspended?
> 
> I mean to say you should probably call pm_runtime_set_suspended() during
> probe. But I'm not sure and I would defer to people more familiar with
> runtime pm.

If the device starts out in a suspended state then the subsystem or 
driver should call pm_runtime_set_suspended before calling 
pm_runtime_enable.

Alan Stern

[PATCH] 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>
---
 drivers/char/hw_random/Kconfig      |   12 ++
 drivers/char/hw_random/Makefile     |    1 +
 drivers/char/hw_random/exynos-rng.c |  204 +++++++++++++++++++++++++++++++++++
 3 files changed, 217 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..8220026 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 Random Number Generator support"
+	depends on HW_RANDOM && ARCH_EXYNOS4
+	---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..b58a28b
--- /dev/null
+++ b/drivers/char/hw_random/exynos-rng.c
@@ -0,0 +1,204 @@
+/*
+ * 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>
+
+#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(void __iomem *base, u32 offset)
+{
+	return	__raw_readl(base + offset);
+}
+
+static void exynos_rng_writel(u32 val, void __iomem *base, u32 offset)
+{
+	__raw_writel(val, base + 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;
+	u32 PRND_SEED[5];
+
+	pm_runtime_put_noidle(exynos_rng->dev);
+	pm_runtime_get_sync(exynos_rng->dev);
+
+	for (i = 0 ; i < 5 ; i++) {
+		PRND_SEED[i] = i;
+		exynos_rng_writel(PRND_SEED[i], exynos_rng->mem,
+					EXYNOS_PRNG_SEED_OFFSET + 4*i);
+	}
+
+	if (!(exynos_rng_readl(exynos_rng->mem, EXYNOS_PRNG_STATUS_OFFSET)
+						 & SEED_SETTING_DONE))
+		ret = -EIO;
+
+	pm_runtime_put(exynos_rng->dev);
+	pm_runtime_get_noresume(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;
+	u32 status = 0;
+
+	pm_runtime_get_sync(exynos_rng->dev);
+	exynos_rng_writel(PRNG_START, exynos_rng->mem, 0);
+
+	while (!status) {
+		status = exynos_rng_readl(exynos_rng->mem,
+					EXYNOS_PRNG_STATUS_OFFSET);
+		status &= PRNG_DONE;
+	}
+
+	exynos_rng_writel(PRNG_DONE, exynos_rng->mem,
+					EXYNOS_PRNG_STATUS_OFFSET);
+
+	*data = exynos_rng_readl(exynos_rng->mem,
+					EXYNOS_PRNG_OUT1_OFFSET);
+
+	pm_runtime_put(exynos_rng->dev);
+	return 4;
+}
+
+static int __init 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 = clk_get(NULL, "secss");
+	if (!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_ioremap(&pdev->dev, res->start,
+						 resource_size(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);
+	pm_runtime_irq_safe(&pdev->dev);
+
+	ret = hwrng_register(&exynos_rng->rng);
+	if (ret) {
+		clk_put(exynos_rng->clk);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __exit exynos_rng_remove(struct platform_device *pdev)
+{
+	struct exynos_rng *exynos_rng = platform_get_drvdata(pdev);
+
+	hwrng_unregister(&exynos_rng->rng);
+	clk_put(exynos_rng->clk);
+	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(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_enable(exynos_rng->clk);
+	return 0;
+}
+
+static const struct dev_pm_ops exynos_rng_pm_ops = {
+	.runtime_suspend = exynos_rng_runtime_suspend,
+	.runtime_resume = exynos_rng_runtime_resume,
+};
+
+static struct platform_driver exynos_rng_driver = {
+	.driver		= {
+		.name	= "exynos-rng",
+		.owner	= THIS_MODULE,
+		.pm	= &exynos_rng_pm_ops,
+	},
+	.probe		= exynos_rng_probe,
+	.remove		= 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