Messages in this thread |  | | | Date | Thu, 21 Jun 2012 11:39:56 +0900 | | From | jonghwa3.lee@samsung ... | | Subject | Re: [PATCH] Exynos : Add support for Exynos random number generator |
| |
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.
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|  |