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

[jonghwa3.lee@samsung.com]

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.