* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
2017-11-23 15:09 ` [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver Łukasz Stelmach
@ 2017-11-23 15:45 ` Andrew F. Davis
2017-11-23 16:31 ` Krzysztof Kozlowski
2017-11-24 15:25 ` PrasannaKumar Muralidharan
2 siblings, 0 replies; 20+ messages in thread
From: Andrew F. Davis @ 2017-11-23 15:45 UTC (permalink / raw)
To: Łukasz Stelmach, Rob Herring, Matt Mackall, Herbert Xu,
Krzysztof Kozlowski, devicetree, linux-crypto, linux-samsung-soc,
linux-kernel
Cc: Marek Szyprowski, Bartlomiej Zolnierkiewicz
On 11/23/2017 09:09 AM, Łukasz Stelmach wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
> MAINTAINERS | 7 +
> drivers/char/hw_random/Kconfig | 12 ++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
> 4 files changed, 276 insertions(+)
> create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
> F: drivers/crypto/exynos-rng.c
> F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach <l.stelmach@samsung.com>
> +L: linux-samsung-soc@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
> SAMSUNG FRAMEBUFFER DRIVER
> M: Jingoo Han <jingoohan1@gmail.com>
> L: linux-fbdev@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
> If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> + tristate "Samsung Exynos True Random Number Generator support"
> + depends on ARCH_EXYNOS || COMPILE_TEST
> + default HW_RANDOM
> + ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
Some whitespace strangness here.
> + To compile this driver as a module, choose M here: the module
> + will be called exynos-trng.
> +
> + If unsure, say Y.
> endif # HW_RANDOM
>
> config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
> obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
> n2-rng-y := n2-drv.o n2-asm.o
> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index 000000000000..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski <krzk@kernel.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/crypto.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL (0x20)
> +#define EXYNOS_TRNG_POST_CTRL (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL (0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT (0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (500000)
> +
> +struct exynos_trng_dev {
> + struct device *dev;
> + void __iomem *mem;
> + struct clk *clk;
> + struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
This only seems to be used as a flag to block this driver from probing
twice, but it doesn't even do that as it is not ever set to anything.
Even if it was, why block a system from having two?
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> + u32 val)
> +{
> + /* Check range of reg? */
> + __raw_writel(val, trng->mem + reg);
If you are not checking, then just drop this one line function, it adds
nothing.
> +}
> +
> +static inline u32 exynos_trng_get_reg(struct exynos_trng_dev *trng, u16 reg)
> +{
> + /* Check range of reg? */
> + return __raw_readl(trng->mem + reg);
Same
> +}
> +
> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
> + bool wait)
> +{
> + struct exynos_trng_dev *trng;
> + u32 val;
> +
> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
min()
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> +
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_FIFO_CTRL, max * 8);
> + val = readl_poll_timeout(trng->mem + EXYNOS_TRNG_FIFO_CTRL, val,
> + val == 0, 200, 1000000);
> + if (val < 0)
> + return val;
> +
> + memcpy_fromio(data, trng->mem + EXYNOS_TRNG_FIFO_0, max);
> +
> + return max;
> +}
> +
> +static int exynos_trng_init(struct hwrng *rng)
> +{
> + struct exynos_trng_dev *trng;
> + unsigned long sss_rate;
> + u32 val;
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
Move up to initializer.
> + sss_rate = clk_get_rate(trng->clk);
> +
> + /* For most TRNG circuits the clock frequency of under 500 kHz
> + * is safe.
> + */
> + val = sss_rate / (EXYNOS_TRNG_CLOCK_RATE * 2);
> + if (val > 0x7fff) {
> + dev_err(trng->dev, "clock divider too large: %d", val);
> + return -ERANGE;
> + }
> + val = val << 1;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CLKDIV, val);
> +
> + /* Enable the generator. */
> + val = 1 << 31;
#define TRNG_CTRL_ENABLE BIT(31)
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CTRL, val);
> +
> + /* Disable post processing. /dev/hwrng is supposed to deliver
> + * unprocessed data.
> + */
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_POST_CTRL, 0);
> +
> + return 0;
> +}
> +
> +static int exynos_trng_probe(struct platform_device *pdev)
> +{
> + struct exynos_trng_dev *trng;
> + struct resource *res;
> + int ret = -ENOMEM;
> +
> + if (exynos_trng_dev)
> + return -EEXIST;
> +
> + trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
> + if (!trng)
> + goto err_ioremap;
> +
> + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev),
> + GFP_KERNEL);
> + if (!trng->rng.name)
> + goto err_ioremap;
> +
> + trng->rng.init = exynos_trng_init;
> + trng->rng.read = exynos_trng_do_read;
> + trng->rng.priv = (unsigned long) trng;
> +
> + platform_set_drvdata(pdev, trng);
> + trng->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + trng->mem = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(trng->mem)) {
> + dev_err(&pdev->dev, "Couldn't map IO resources.\n");
> + ret = PTR_ERR(trng->mem);
> + goto err_ioremap;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
"Could not get runtime PM",
Be descriptive an consistent, the other dev_errs don't print ret, it
will get printed as part of the probe fail message.
> + goto err_ioremap;
> + }
> +
> + trng->clk = devm_clk_get(&pdev->dev, "secss");
> + if (IS_ERR(trng->clk)) {
> + /* XXX: EPROBE_DEFER ? */
Are you asking? If it's posible the clock is not available then yes, you
should not print the below message and exit gracefully.
> + dev_err(&pdev->dev, "Couldn't get clock.\n");
> + ret = PTR_ERR(trng->clk);
> + goto err_clock;
> + }
> +
> + ret = clk_prepare_enable(trng->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
> + goto err_clock;
> + }
> +
> + ret = hwrng_register(&trng->rng);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
> + goto err_register;
> + }
> +
> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
> +
> + return 0;
> +
> +err_register:
> + clk_disable_unprepare(trng->clk);
> +
> +err_clock:
> + trng->mem = NULL;
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +
> +err_ioremap:
> + dev_err(&pdev->dev, "Initialisation failed.\n");
> + return ret;
> +}
> +
> +static int exynos_trng_remove(struct platform_device *pdev)
> +{
pm_runtime_*, or just use devm_ versions and drop this function.
> + exynos_trng_dev = NULL;
> +
> + return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
> +{
> + pm_runtime_put_sync(dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_resume(struct device *dev)
> +{
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {
> + dev_err(dev, "pm_runtime_get_sync() failed: %d.\n", ret);
> + pm_runtime_put_noidle(dev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
> + exynos_trng_resume);
> +
> +static const struct of_device_id exynos_trng_dt_match[] = {
> + {
> + .compatible = "samsung,exynos5250-trng",
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
> +
> +static struct platform_driver exynos_trng_driver = {
> + .driver = {
> + .name = "exynos-trng",
> + .pm = &exynos_trng_pm_ops,
> + .of_match_table = exynos_trng_dt_match,
> + },
> + .probe = exynos_trng_probe,
> + .remove = exynos_trng_remove,
> +};
> +
> +module_platform_driver(exynos_trng_driver);
> +MODULE_AUTHOR("Łukasz Stelmach");
> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
2017-11-23 15:09 ` [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver Łukasz Stelmach
2017-11-23 15:45 ` Andrew F. Davis
@ 2017-11-23 16:31 ` Krzysztof Kozlowski
[not found] ` <CGME20171123184736eucas1p15d4e73d5a6596a6cba1f940dcc473c9a@eucas1p1.samsung.com>
2017-11-24 15:25 ` PrasannaKumar Muralidharan
2 siblings, 1 reply; 20+ messages in thread
From: Krzysztof Kozlowski @ 2017-11-23 16:31 UTC (permalink / raw)
To: Łukasz Stelmach
Cc: Rob Herring, Matt Mackall, Herbert Xu, devicetree, linux-crypto,
linux-samsung-soc, linux-kernel, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On Thu, Nov 23, 2017 at 4:09 PM, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
> MAINTAINERS | 7 +
> drivers/char/hw_random/Kconfig | 12 ++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
> 4 files changed, 276 insertions(+)
> create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
> F: drivers/crypto/exynos-rng.c
> F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach <l.stelmach@samsung.com>
> +L: linux-samsung-soc@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
> SAMSUNG FRAMEBUFFER DRIVER
> M: Jingoo Han <jingoohan1@gmail.com>
> L: linux-fbdev@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
> If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> + tristate "Samsung Exynos True Random Number Generator support"
> + depends on ARCH_EXYNOS || COMPILE_TEST
> + default HW_RANDOM
> + ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called exynos-trng.
> +
> + If unsure, say Y.
> endif # HW_RANDOM
Thanks for working on TRNG.
1. I was rather thinking about extending existing exynos-rng.c [1] so
it would be using TRNG as seed for PRNG as this gives you much more
random data. Instead you developed totally separate driver which has
its own benefits - one can choose which interface he wants. Although
it is a little bit duplication.
2. I understand that in your current version of TRNG driver there is
no conflict with PRNG and they can work both at the same time?
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/exynos-rng.c
>
> config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
> obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
> n2-rng-y := n2-drv.o n2-asm.o
> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index 000000000000..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski <krzk@kernel.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/crypto.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL (0x20)
> +#define EXYNOS_TRNG_POST_CTRL (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL (0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT (0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (500000)
> +
> +struct exynos_trng_dev {
> + struct device *dev;
> + void __iomem *mem;
> + struct clk *clk;
> + struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
This definitely should not be globally linked. Also I think this is
not even needed to be file-scope.
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> + u32 val)
> +{
> + /* Check range of reg? */
Is this is a FIXME/TODO note?
> + __raw_writel(val, trng->mem + reg);
> +}
> +
> +static inline u32 exynos_trng_get_reg(struct exynos_trng_dev *trng, u16 reg)
> +{
> + /* Check range of reg? */
Ditto
> + return __raw_readl(trng->mem + reg);
> +}
> +
> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
> + bool wait)
> +{
> + struct exynos_trng_dev *trng;
> + u32 val;
> +
> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
Maybe use macro min()? This will also nicely point that variable name
"max" is misleading. Max what? Isn't it the size of buffer to read?
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> +
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_FIFO_CTRL, max * 8);
> + val = readl_poll_timeout(trng->mem + EXYNOS_TRNG_FIFO_CTRL, val,
> + val == 0, 200, 1000000);
> + if (val < 0)
> + return val;
> +
> + memcpy_fromio(data, trng->mem + EXYNOS_TRNG_FIFO_0, max);
> +
> + return max;
> +}
> +
> +static int exynos_trng_init(struct hwrng *rng)
> +{
> + struct exynos_trng_dev *trng;
> + unsigned long sss_rate;
> + u32 val;
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> + sss_rate = clk_get_rate(trng->clk);
> +
> + /* For most TRNG circuits the clock frequency of under 500 kHz
/* Before actual comment
> + * is safe.
> + */
> + val = sss_rate / (EXYNOS_TRNG_CLOCK_RATE * 2);
> + if (val > 0x7fff) {
> + dev_err(trng->dev, "clock divider too large: %d", val);
> + return -ERANGE;
> + }
> + val = val << 1;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CLKDIV, val);
> +
> + /* Enable the generator. */
> + val = 1 << 31;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CTRL, val);
> +
> + /* Disable post processing. /dev/hwrng is supposed to deliver
> + * unprocessed data.
> + */
Ditto
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_POST_CTRL, 0);
> +
> + return 0;
> +}
> +
> +static int exynos_trng_probe(struct platform_device *pdev)
> +{
> + struct exynos_trng_dev *trng;
> + struct resource *res;
> + int ret = -ENOMEM;
> +
> + if (exynos_trng_dev)
> + return -EEXIST;
> +
> + trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
> + if (!trng)
> + goto err_ioremap;
No, this should just return with -ENOMEM. Do not print errors on
memory alloc failures.
> +
> + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev),
> + GFP_KERNEL);
> + if (!trng->rng.name)
> + goto err_ioremap;
Ditto
> +
> + trng->rng.init = exynos_trng_init;
> + trng->rng.read = exynos_trng_do_read;
> + trng->rng.priv = (unsigned long) trng;
> +
> + platform_set_drvdata(pdev, trng);
> + trng->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + trng->mem = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(trng->mem)) {
> + dev_err(&pdev->dev, "Couldn't map IO resources.\n");
> + ret = PTR_ERR(trng->mem);
> + goto err_ioremap;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
> + goto err_ioremap;
Missing pm_runtime_disable?
> + }
> +
> + trng->clk = devm_clk_get(&pdev->dev, "secss");
> + if (IS_ERR(trng->clk)) {
> + /* XXX: EPROBE_DEFER ? */
TODO note?
If you have questions how to deal with something, ask them. Do not
force reader to rediscover problems you are aware of.
> + dev_err(&pdev->dev, "Couldn't get clock.\n");
> + ret = PTR_ERR(trng->clk);
> + goto err_clock;
> + }
> +
> + ret = clk_prepare_enable(trng->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
> + goto err_clock;
> + }
> +
> + ret = hwrng_register(&trng->rng);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
> + goto err_register;
> + }
> +
> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
> +
> + return 0;
> +
> +err_register:
> + clk_disable_unprepare(trng->clk);
> +
> +err_clock:
> + trng->mem = NULL;
Why this has to be null-ified?
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +
> +err_ioremap:
> + dev_err(&pdev->dev, "Initialisation failed.\n");
Double error msg, you already print one before.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
2017-11-23 15:09 ` [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver Łukasz Stelmach
@ 2017-11-24 15:25 ` PrasannaKumar Muralidharan
2017-11-23 16:31 ` Krzysztof Kozlowski
2017-11-24 15:25 ` PrasannaKumar Muralidharan
2 siblings, 0 replies; 20+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-24 15:25 UTC (permalink / raw)
To: Łukasz Stelmach
Cc: Rob Herring, Matt Mackall, Herbert Xu, Krzysztof Kozlowski,
devicetree, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-samsung-soc, open list, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
Hi Lukasz,
Some minor comments below.
On 23 November 2017 at 20:39, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
> MAINTAINERS | 7 +
> drivers/char/hw_random/Kconfig | 12 ++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
> 4 files changed, 276 insertions(+)
> create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
> F: drivers/crypto/exynos-rng.c
> F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach <l.stelmach@samsung.com>
> +L: linux-samsung-soc@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
> SAMSUNG FRAMEBUFFER DRIVER
> M: Jingoo Han <jingoohan1@gmail.com>
> L: linux-fbdev@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
> If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> + tristate "Samsung Exynos True Random Number Generator support"
> + depends on ARCH_EXYNOS || COMPILE_TEST
> + default HW_RANDOM
> + ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called exynos-trng.
> +
> + If unsure, say Y.
> endif # HW_RANDOM
>
> config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
> obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
> n2-rng-y := n2-drv.o n2-asm.o
> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index 000000000000..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski <krzk@kernel.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/crypto.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL (0x20)
> +#define EXYNOS_TRNG_POST_CTRL (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL (0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT (0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (500000)
> +
> +struct exynos_trng_dev {
> + struct device *dev;
> + void __iomem *mem;
> + struct clk *clk;
> + struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> + u32 val)
> +{
> + /* Check range of reg? */
> + __raw_writel(val, trng->mem + reg);
Any specific reason to use __raw_writel? Why not just writel?
> +}
> +
> +static inline u32 exynos_trng_get_reg(struct exynos_trng_dev *trng, u16 reg)
> +{
> + /* Check range of reg? */
> + return __raw_readl(trng->mem + reg);
Same as above.
> +}
> +
> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
> + bool wait)
> +{
> + struct exynos_trng_dev *trng;
> + u32 val;
> +
> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
max is always > 32. This condition is not required.
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> +
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_FIFO_CTRL, max * 8);
> + val = readl_poll_timeout(trng->mem + EXYNOS_TRNG_FIFO_CTRL, val,
> + val == 0, 200, 1000000);
> + if (val < 0)
> + return val;
> +
> + memcpy_fromio(data, trng->mem + EXYNOS_TRNG_FIFO_0, max);
> +
> + return max;
> +}
> +
> +static int exynos_trng_init(struct hwrng *rng)
> +{
> + struct exynos_trng_dev *trng;
> + unsigned long sss_rate;
> + u32 val;
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> + sss_rate = clk_get_rate(trng->clk);
> +
> + /* For most TRNG circuits the clock frequency of under 500 kHz
> + * is safe.
> + */
> + val = sss_rate / (EXYNOS_TRNG_CLOCK_RATE * 2);
> + if (val > 0x7fff) {
> + dev_err(trng->dev, "clock divider too large: %d", val);
> + return -ERANGE;
> + }
> + val = val << 1;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CLKDIV, val);
> +
> + /* Enable the generator. */
> + val = 1 << 31;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CTRL, val);
> +
> + /* Disable post processing. /dev/hwrng is supposed to deliver
> + * unprocessed data.
> + */
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_POST_CTRL, 0);
> +
> + return 0;
> +}
> +
> +static int exynos_trng_probe(struct platform_device *pdev)
> +{
> + struct exynos_trng_dev *trng;
> + struct resource *res;
> + int ret = -ENOMEM;
> +
> + if (exynos_trng_dev)
> + return -EEXIST;
> +
> + trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
> + if (!trng)
> + goto err_ioremap;
> +
> + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev),
> + GFP_KERNEL);
> + if (!trng->rng.name)
> + goto err_ioremap;
> +
> + trng->rng.init = exynos_trng_init;
> + trng->rng.read = exynos_trng_do_read;
> + trng->rng.priv = (unsigned long) trng;
> +
> + platform_set_drvdata(pdev, trng);
> + trng->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + trng->mem = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(trng->mem)) {
> + dev_err(&pdev->dev, "Couldn't map IO resources.\n");
> + ret = PTR_ERR(trng->mem);
> + goto err_ioremap;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
> + goto err_ioremap;
> + }
> +
> + trng->clk = devm_clk_get(&pdev->dev, "secss");
> + if (IS_ERR(trng->clk)) {
> + /* XXX: EPROBE_DEFER ? */
> + dev_err(&pdev->dev, "Couldn't get clock.\n");
> + ret = PTR_ERR(trng->clk);
> + goto err_clock;
> + }
> +
> + ret = clk_prepare_enable(trng->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
> + goto err_clock;
> + }
Why not move clk_prepare_enable to init call? While at it please add a
cleanup callback as well.
> +
> + ret = hwrng_register(&trng->rng);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
> + goto err_register;
> + }
> +
> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
> +
> + return 0;
> +
> +err_register:
> + clk_disable_unprepare(trng->clk);
> +
> +err_clock:
> + trng->mem = NULL;
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +
> +err_ioremap:
> + dev_err(&pdev->dev, "Initialisation failed.\n");
> + return ret;
> +}
> +
> +static int exynos_trng_remove(struct platform_device *pdev)
> +{
> + exynos_trng_dev = NULL;
> +
> + return 0;
> +}
remove is not required as it does not do anything significant.
> +
> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
> +{
> + pm_runtime_put_sync(dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_resume(struct device *dev)
> +{
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {
> + dev_err(dev, "pm_runtime_get_sync() failed: %d.\n", ret);
> + pm_runtime_put_noidle(dev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
> + exynos_trng_resume);
> +
> +static const struct of_device_id exynos_trng_dt_match[] = {
> + {
> + .compatible = "samsung,exynos5250-trng",
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
> +
> +static struct platform_driver exynos_trng_driver = {
> + .driver = {
> + .name = "exynos-trng",
> + .pm = &exynos_trng_pm_ops,
> + .of_match_table = exynos_trng_dt_match,
> + },
> + .probe = exynos_trng_probe,
> + .remove = exynos_trng_remove,
> +};
> +
> +module_platform_driver(exynos_trng_driver);
> +MODULE_AUTHOR("Łukasz Stelmach");
> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>
Regards,
PrasannaKumar
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
@ 2017-11-24 15:25 ` PrasannaKumar Muralidharan
0 siblings, 0 replies; 20+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-24 15:25 UTC (permalink / raw)
To: Łukasz Stelmach
Cc: Rob Herring, Matt Mackall, Herbert Xu, Krzysztof Kozlowski,
devicetree, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-samsung-soc, open list, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
Hi Lukasz,
Some minor comments below.
On 23 November 2017 at 20:39, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
> Add support for True Random Number Generator found in Samsung Exynos
> 5250+ SoCs.
>
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
> MAINTAINERS | 7 +
> drivers/char/hw_random/Kconfig | 12 ++
> drivers/char/hw_random/Makefile | 1 +
> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
> 4 files changed, 276 insertions(+)
> create mode 100644 drivers/char/hw_random/exynos-trng.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2811a211632c..992074cca612 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11780,6 +11780,13 @@ S: Maintained
> F: drivers/crypto/exynos-rng.c
> F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>
> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
> +M: Łukasz Stelmach <l.stelmach@samsung.com>
> +L: linux-samsung-soc@vger.kernel.org
> +S: Maintained
> +F: drivers/char/hw_random/exynos-trng.c
> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
> +
> SAMSUNG FRAMEBUFFER DRIVER
> M: Jingoo Han <jingoohan1@gmail.com>
> L: linux-fbdev@vger.kernel.org
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 95a031e9eced..a788ac07081b 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>
> If unsure, say Y.
>
> +config HW_RANDOM_EXYNOS
> + tristate "Samsung Exynos True Random Number Generator support"
> + depends on ARCH_EXYNOS || COMPILE_TEST
> + default HW_RANDOM
> + ---help---
> + This driver provides support for the True Random Number
> + Generator available in Exynos SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called exynos-trng.
> +
> + If unsure, say Y.
> endif # HW_RANDOM
>
> config UML_RANDOM
> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
> index f3728d008fff..5595df97da7a 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
> obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
> n2-rng-y := n2-drv.o n2-asm.o
> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
> diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
> new file mode 100644
> index 000000000000..340e106cae24
> --- /dev/null
> +++ b/drivers/char/hw_random/exynos-trng.c
> @@ -0,0 +1,256 @@
> +/*
> + * RNG driver for Exynos TRNGs
> + *
> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
> + *
> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
> + *
> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
> + * Krzysztof Kozłowski <krzk@kernel.org>
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/crypto.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hw_random.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define EXYNOS_TRNG_CLKDIV (0x0)
> +#define EXYNOS_TRNG_CTRL (0x20)
> +#define EXYNOS_TRNG_POST_CTRL (0x30)
> +#define EXYNOS_TRNG_ONLINE_CTRL (0x40)
> +#define EXYNOS_TRNG_ONLINE_STAT (0x44)
> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
> +#define EXYNOS_TRNG_FIFO_CTRL (0x50)
> +#define EXYNOS_TRNG_FIFO_0 (0x80)
> +#define EXYNOS_TRNG_FIFO_1 (0x84)
> +#define EXYNOS_TRNG_FIFO_2 (0x88)
> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
> +#define EXYNOS_TRNG_FIFO_4 (0x90)
> +#define EXYNOS_TRNG_FIFO_5 (0x94)
> +#define EXYNOS_TRNG_FIFO_6 (0x98)
> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
> +#define EXYNOS_TRNG_FIFO_LEN (8)
> +#define EXYNOS_TRNG_CLOCK_RATE (500000)
> +
> +struct exynos_trng_dev {
> + struct device *dev;
> + void __iomem *mem;
> + struct clk *clk;
> + struct hwrng rng;
> +};
> +
> +struct exynos_trng_dev *exynos_trng_dev;
> +
> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
> + u32 val)
> +{
> + /* Check range of reg? */
> + __raw_writel(val, trng->mem + reg);
Any specific reason to use __raw_writel? Why not just writel?
> +}
> +
> +static inline u32 exynos_trng_get_reg(struct exynos_trng_dev *trng, u16 reg)
> +{
> + /* Check range of reg? */
> + return __raw_readl(trng->mem + reg);
Same as above.
> +}
> +
> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
> + bool wait)
> +{
> + struct exynos_trng_dev *trng;
> + u32 val;
> +
> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
max is always > 32. This condition is not required.
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> +
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_FIFO_CTRL, max * 8);
> + val = readl_poll_timeout(trng->mem + EXYNOS_TRNG_FIFO_CTRL, val,
> + val == 0, 200, 1000000);
> + if (val < 0)
> + return val;
> +
> + memcpy_fromio(data, trng->mem + EXYNOS_TRNG_FIFO_0, max);
> +
> + return max;
> +}
> +
> +static int exynos_trng_init(struct hwrng *rng)
> +{
> + struct exynos_trng_dev *trng;
> + unsigned long sss_rate;
> + u32 val;
> +
> + trng = (struct exynos_trng_dev *)rng->priv;
> + sss_rate = clk_get_rate(trng->clk);
> +
> + /* For most TRNG circuits the clock frequency of under 500 kHz
> + * is safe.
> + */
> + val = sss_rate / (EXYNOS_TRNG_CLOCK_RATE * 2);
> + if (val > 0x7fff) {
> + dev_err(trng->dev, "clock divider too large: %d", val);
> + return -ERANGE;
> + }
> + val = val << 1;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CLKDIV, val);
> +
> + /* Enable the generator. */
> + val = 1 << 31;
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CTRL, val);
> +
> + /* Disable post processing. /dev/hwrng is supposed to deliver
> + * unprocessed data.
> + */
> + exynos_trng_set_reg(trng, EXYNOS_TRNG_POST_CTRL, 0);
> +
> + return 0;
> +}
> +
> +static int exynos_trng_probe(struct platform_device *pdev)
> +{
> + struct exynos_trng_dev *trng;
> + struct resource *res;
> + int ret = -ENOMEM;
> +
> + if (exynos_trng_dev)
> + return -EEXIST;
> +
> + trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
> + if (!trng)
> + goto err_ioremap;
> +
> + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev),
> + GFP_KERNEL);
> + if (!trng->rng.name)
> + goto err_ioremap;
> +
> + trng->rng.init = exynos_trng_init;
> + trng->rng.read = exynos_trng_do_read;
> + trng->rng.priv = (unsigned long) trng;
> +
> + platform_set_drvdata(pdev, trng);
> + trng->dev = &pdev->dev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + trng->mem = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(trng->mem)) {
> + dev_err(&pdev->dev, "Couldn't map IO resources.\n");
> + ret = PTR_ERR(trng->mem);
> + goto err_ioremap;
> + }
> +
> + pm_runtime_enable(&pdev->dev);
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
> + goto err_ioremap;
> + }
> +
> + trng->clk = devm_clk_get(&pdev->dev, "secss");
> + if (IS_ERR(trng->clk)) {
> + /* XXX: EPROBE_DEFER ? */
> + dev_err(&pdev->dev, "Couldn't get clock.\n");
> + ret = PTR_ERR(trng->clk);
> + goto err_clock;
> + }
> +
> + ret = clk_prepare_enable(trng->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
> + goto err_clock;
> + }
Why not move clk_prepare_enable to init call? While at it please add a
cleanup callback as well.
> +
> + ret = hwrng_register(&trng->rng);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
> + goto err_register;
> + }
> +
> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
> +
> + return 0;
> +
> +err_register:
> + clk_disable_unprepare(trng->clk);
> +
> +err_clock:
> + trng->mem = NULL;
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +
> +err_ioremap:
> + dev_err(&pdev->dev, "Initialisation failed.\n");
> + return ret;
> +}
> +
> +static int exynos_trng_remove(struct platform_device *pdev)
> +{
> + exynos_trng_dev = NULL;
> +
> + return 0;
> +}
remove is not required as it does not do anything significant.
> +
> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
> +{
> + pm_runtime_put_sync(dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused exynos_trng_resume(struct device *dev)
> +{
> + int ret;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0) {
> + dev_err(dev, "pm_runtime_get_sync() failed: %d.\n", ret);
> + pm_runtime_put_noidle(dev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
> + exynos_trng_resume);
> +
> +static const struct of_device_id exynos_trng_dt_match[] = {
> + {
> + .compatible = "samsung,exynos5250-trng",
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
> +
> +static struct platform_driver exynos_trng_driver = {
> + .driver = {
> + .name = "exynos-trng",
> + .pm = &exynos_trng_pm_ops,
> + .of_match_table = exynos_trng_dt_match,
> + },
> + .probe = exynos_trng_probe,
> + .remove = exynos_trng_remove,
> +};
> +
> +module_platform_driver(exynos_trng_driver);
> +MODULE_AUTHOR("Łukasz Stelmach");
> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
> +MODULE_LICENSE("GPL");
> --
> 2.11.0
>
Regards,
PrasannaKumar
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
2017-11-24 15:25 ` PrasannaKumar Muralidharan
@ 2017-11-24 15:54 ` PrasannaKumar Muralidharan
-1 siblings, 0 replies; 20+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-24 15:54 UTC (permalink / raw)
To: Łukasz Stelmach
Cc: Rob Herring, Matt Mackall, Herbert Xu, Krzysztof Kozlowski,
devicetree, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-samsung-soc, open list, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On 24 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> Hi Lukasz,
>
> Some minor comments below.
>
> On 23 November 2017 at 20:39, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>> MAINTAINERS | 7 +
>> drivers/char/hw_random/Kconfig | 12 ++
>> drivers/char/hw_random/Makefile | 1 +
>> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 276 insertions(+)
>> create mode 100644 drivers/char/hw_random/exynos-trng.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2811a211632c..992074cca612 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11780,6 +11780,13 @@ S: Maintained
>> F: drivers/crypto/exynos-rng.c
>> F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>>
>> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
>> +M: Łukasz Stelmach <l.stelmach@samsung.com>
>> +L: linux-samsung-soc@vger.kernel.org
>> +S: Maintained
>> +F: drivers/char/hw_random/exynos-trng.c
>> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>> +
>> SAMSUNG FRAMEBUFFER DRIVER
>> M: Jingoo Han <jingoohan1@gmail.com>
>> L: linux-fbdev@vger.kernel.org
>> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
>> index 95a031e9eced..a788ac07081b 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>>
>> If unsure, say Y.
>>
>> +config HW_RANDOM_EXYNOS
>> + tristate "Samsung Exynos True Random Number Generator support"
>> + depends on ARCH_EXYNOS || COMPILE_TEST
>> + default HW_RANDOM
>> + ---help---
>> + This driver provides support for the True Random Number
>> + Generator available in Exynos SoCs.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called exynos-trng.
>> +
>> + If unsure, say Y.
>> endif # HW_RANDOM
>>
>> config UML_RANDOM
>> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
>> index f3728d008fff..5595df97da7a 100644
>> --- a/drivers/char/hw_random/Makefile
>> +++ b/drivers/char/hw_random/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>> obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>> n2-rng-y := n2-drv.o n2-asm.o
>> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
>> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
>> diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
>> new file mode 100644
>> index 000000000000..340e106cae24
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-trng.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * RNG driver for Exynos TRNGs
>> + *
>> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
>> + *
>> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
>> + *
>> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
>> + * Krzysztof Kozłowski <krzk@kernel.org>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/crypto.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/hw_random.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#define EXYNOS_TRNG_CLKDIV (0x0)
>> +#define EXYNOS_TRNG_CTRL (0x20)
>> +#define EXYNOS_TRNG_POST_CTRL (0x30)
>> +#define EXYNOS_TRNG_ONLINE_CTRL (0x40)
>> +#define EXYNOS_TRNG_ONLINE_STAT (0x44)
>> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
>> +#define EXYNOS_TRNG_FIFO_CTRL (0x50)
>> +#define EXYNOS_TRNG_FIFO_0 (0x80)
>> +#define EXYNOS_TRNG_FIFO_1 (0x84)
>> +#define EXYNOS_TRNG_FIFO_2 (0x88)
>> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
>> +#define EXYNOS_TRNG_FIFO_4 (0x90)
>> +#define EXYNOS_TRNG_FIFO_5 (0x94)
>> +#define EXYNOS_TRNG_FIFO_6 (0x98)
>> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
>> +#define EXYNOS_TRNG_FIFO_LEN (8)
>> +#define EXYNOS_TRNG_CLOCK_RATE (500000)
>> +
>> +struct exynos_trng_dev {
>> + struct device *dev;
>> + void __iomem *mem;
>> + struct clk *clk;
>> + struct hwrng rng;
>> +};
>> +
>> +struct exynos_trng_dev *exynos_trng_dev;
>> +
>> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
>> + u32 val)
>> +{
>> + /* Check range of reg? */
>> + __raw_writel(val, trng->mem + reg);
>
> Any specific reason to use __raw_writel? Why not just writel?
>
>> +}
>> +
>> +static inline u32 exynos_trng_get_reg(struct exynos_trng_dev *trng, u16 reg)
>> +{
>> + /* Check range of reg? */
>> + return __raw_readl(trng->mem + reg);
>
> Same as above.
>
>> +}
>> +
>> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
>> + bool wait)
>> +{
>> + struct exynos_trng_dev *trng;
>> + u32 val;
>> +
>> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
>> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
>
> max is always > 32. This condition is not required.
>
>> +
>> + trng = (struct exynos_trng_dev *)rng->priv;
>> +
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_FIFO_CTRL, max * 8);
>> + val = readl_poll_timeout(trng->mem + EXYNOS_TRNG_FIFO_CTRL, val,
>> + val == 0, 200, 1000000);
>> + if (val < 0)
>> + return val;
>> +
>> + memcpy_fromio(data, trng->mem + EXYNOS_TRNG_FIFO_0, max);
>> +
>> + return max;
>> +}
>> +
>> +static int exynos_trng_init(struct hwrng *rng)
>> +{
>> + struct exynos_trng_dev *trng;
>> + unsigned long sss_rate;
>> + u32 val;
>> +
>> + trng = (struct exynos_trng_dev *)rng->priv;
>> + sss_rate = clk_get_rate(trng->clk);
>> +
>> + /* For most TRNG circuits the clock frequency of under 500 kHz
>> + * is safe.
>> + */
>> + val = sss_rate / (EXYNOS_TRNG_CLOCK_RATE * 2);
>> + if (val > 0x7fff) {
>> + dev_err(trng->dev, "clock divider too large: %d", val);
>> + return -ERANGE;
>> + }
>> + val = val << 1;
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CLKDIV, val);
>> +
>> + /* Enable the generator. */
>> + val = 1 << 31;
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CTRL, val);
>> +
>> + /* Disable post processing. /dev/hwrng is supposed to deliver
>> + * unprocessed data.
>> + */
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_POST_CTRL, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_trng_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_trng_dev *trng;
>> + struct resource *res;
>> + int ret = -ENOMEM;
>> +
>> + if (exynos_trng_dev)
>> + return -EEXIST;
>> +
>> + trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
>> + if (!trng)
>> + goto err_ioremap;
>> +
>> + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev),
>> + GFP_KERNEL);
>> + if (!trng->rng.name)
>> + goto err_ioremap;
>> +
>> + trng->rng.init = exynos_trng_init;
>> + trng->rng.read = exynos_trng_do_read;
>> + trng->rng.priv = (unsigned long) trng;
>> +
>> + platform_set_drvdata(pdev, trng);
>> + trng->dev = &pdev->dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + trng->mem = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(trng->mem)) {
>> + dev_err(&pdev->dev, "Couldn't map IO resources.\n");
>> + ret = PTR_ERR(trng->mem);
>> + goto err_ioremap;
>> + }
>> +
>> + pm_runtime_enable(&pdev->dev);
>> + ret = pm_runtime_get_sync(&pdev->dev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
>> + goto err_ioremap;
>> + }
>> +
>> + trng->clk = devm_clk_get(&pdev->dev, "secss");
>> + if (IS_ERR(trng->clk)) {
>> + /* XXX: EPROBE_DEFER ? */
>> + dev_err(&pdev->dev, "Couldn't get clock.\n");
>> + ret = PTR_ERR(trng->clk);
>> + goto err_clock;
>> + }
>> +
>> + ret = clk_prepare_enable(trng->clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
>> + goto err_clock;
>> + }
>
> Why not move clk_prepare_enable to init call? While at it please add a
> cleanup callback as well.
>
>> +
>> + ret = hwrng_register(&trng->rng);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
>> + goto err_register;
>> + }
>> +
>> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
>> +
>> + return 0;
>> +
>> +err_register:
>> + clk_disable_unprepare(trng->clk);
>> +
>> +err_clock:
>> + trng->mem = NULL;
>> + pm_runtime_put_sync(&pdev->dev);
>> + pm_runtime_disable(&pdev->dev);
>> +
>> +err_ioremap:
>> + dev_err(&pdev->dev, "Initialisation failed.\n");
>> + return ret;
>> +}
>> +
>> +static int exynos_trng_remove(struct platform_device *pdev)
>> +{
>> + exynos_trng_dev = NULL;
>> +
>> + return 0;
>> +}
>
> remove is not required as it does not do anything significant.
Should unregister from hwrng in remove callback.
>
>> +
>> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
>> +{
>> + pm_runtime_put_sync(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused exynos_trng_resume(struct device *dev)
>> +{
>> + int ret;
>> +
>> + ret = pm_runtime_get_sync(dev);
>> + if (ret < 0) {
>> + dev_err(dev, "pm_runtime_get_sync() failed: %d.\n", ret);
>> + pm_runtime_put_noidle(dev);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
>> + exynos_trng_resume);
>> +
>> +static const struct of_device_id exynos_trng_dt_match[] = {
>> + {
>> + .compatible = "samsung,exynos5250-trng",
>> + },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
>> +
>> +static struct platform_driver exynos_trng_driver = {
>> + .driver = {
>> + .name = "exynos-trng",
>> + .pm = &exynos_trng_pm_ops,
>> + .of_match_table = exynos_trng_dt_match,
>> + },
>> + .probe = exynos_trng_probe,
>> + .remove = exynos_trng_remove,
>> +};
>> +
>> +module_platform_driver(exynos_trng_driver);
>> +MODULE_AUTHOR("Łukasz Stelmach");
>> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.11.0
>>
>
> Regards,
> PrasannaKumar
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
@ 2017-11-24 15:54 ` PrasannaKumar Muralidharan
0 siblings, 0 replies; 20+ messages in thread
From: PrasannaKumar Muralidharan @ 2017-11-24 15:54 UTC (permalink / raw)
To: Łukasz Stelmach
Cc: Rob Herring, Matt Mackall, Herbert Xu, Krzysztof Kozlowski,
devicetree, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
linux-samsung-soc, open list, Marek Szyprowski,
Bartlomiej Zolnierkiewicz
On 24 November 2017 at 20:55, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> Hi Lukasz,
>
> Some minor comments below.
>
> On 23 November 2017 at 20:39, Łukasz Stelmach <l.stelmach@samsung.com> wrote:
>> Add support for True Random Number Generator found in Samsung Exynos
>> 5250+ SoCs.
>>
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>> MAINTAINERS | 7 +
>> drivers/char/hw_random/Kconfig | 12 ++
>> drivers/char/hw_random/Makefile | 1 +
>> drivers/char/hw_random/exynos-trng.c | 256 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 276 insertions(+)
>> create mode 100644 drivers/char/hw_random/exynos-trng.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 2811a211632c..992074cca612 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -11780,6 +11780,13 @@ S: Maintained
>> F: drivers/crypto/exynos-rng.c
>> F: Documentation/devicetree/bindings/rng/samsung,exynos-rng4.txt
>>
>> +SAMSUNG EXYNOS TRUE RANDOM NUMBER GENERATOR (TRNG) DRIVER
>> +M: Łukasz Stelmach <l.stelmach@samsung.com>
>> +L: linux-samsung-soc@vger.kernel.org
>> +S: Maintained
>> +F: drivers/char/hw_random/exynos-trng.c
>> +F: Documentation/devicetree/bindings/rng/samsung,exynos5250-trng.txt
>> +
>> SAMSUNG FRAMEBUFFER DRIVER
>> M: Jingoo Han <jingoohan1@gmail.com>
>> L: linux-fbdev@vger.kernel.org
>> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
>> index 95a031e9eced..a788ac07081b 100644
>> --- a/drivers/char/hw_random/Kconfig
>> +++ b/drivers/char/hw_random/Kconfig
>> @@ -449,6 +449,18 @@ config HW_RANDOM_S390
>>
>> If unsure, say Y.
>>
>> +config HW_RANDOM_EXYNOS
>> + tristate "Samsung Exynos True Random Number Generator support"
>> + depends on ARCH_EXYNOS || COMPILE_TEST
>> + default HW_RANDOM
>> + ---help---
>> + This driver provides support for the True Random Number
>> + Generator available in Exynos SoCs.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called exynos-trng.
>> +
>> + If unsure, say Y.
>> endif # HW_RANDOM
>>
>> config UML_RANDOM
>> diff --git a/drivers/char/hw_random/Makefile b/drivers/char/hw_random/Makefile
>> index f3728d008fff..5595df97da7a 100644
>> --- a/drivers/char/hw_random/Makefile
>> +++ b/drivers/char/hw_random/Makefile
>> @@ -14,6 +14,7 @@ obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
>> obj-$(CONFIG_HW_RANDOM_N2RNG) += n2-rng.o
>> n2-rng-y := n2-drv.o n2-asm.o
>> obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
>> +obj-$(CONFIG_HW_RANDOM_EXYNOS) += exynos-trng.o
>> obj-$(CONFIG_HW_RANDOM_IXP4XX) += ixp4xx-rng.o
>> obj-$(CONFIG_HW_RANDOM_OMAP) += omap-rng.o
>> obj-$(CONFIG_HW_RANDOM_OMAP3_ROM) += omap3-rom-rng.o
>> diff --git a/drivers/char/hw_random/exynos-trng.c b/drivers/char/hw_random/exynos-trng.c
>> new file mode 100644
>> index 000000000000..340e106cae24
>> --- /dev/null
>> +++ b/drivers/char/hw_random/exynos-trng.c
>> @@ -0,0 +1,256 @@
>> +/*
>> + * RNG driver for Exynos TRNGs
>> + *
>> + * Author: Łukasz Stelmach <l.stelmach@samsung.com>
>> + *
>> + * Copyright 2017 (c) Samsung Electronics Software, Inc.
>> + *
>> + * Based on the Exynos PRNG driver drivers/crypto/exynos-rng by
>> + * Krzysztof Kozłowski <krzk@kernel.org>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/crypto.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/hw_random.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#define EXYNOS_TRNG_CLKDIV (0x0)
>> +#define EXYNOS_TRNG_CTRL (0x20)
>> +#define EXYNOS_TRNG_POST_CTRL (0x30)
>> +#define EXYNOS_TRNG_ONLINE_CTRL (0x40)
>> +#define EXYNOS_TRNG_ONLINE_STAT (0x44)
>> +#define EXYNOS_TRNG_ONLINE_MAXCHI2 (0x48)
>> +#define EXYNOS_TRNG_FIFO_CTRL (0x50)
>> +#define EXYNOS_TRNG_FIFO_0 (0x80)
>> +#define EXYNOS_TRNG_FIFO_1 (0x84)
>> +#define EXYNOS_TRNG_FIFO_2 (0x88)
>> +#define EXYNOS_TRNG_FIFO_3 (0x8c)
>> +#define EXYNOS_TRNG_FIFO_4 (0x90)
>> +#define EXYNOS_TRNG_FIFO_5 (0x94)
>> +#define EXYNOS_TRNG_FIFO_6 (0x98)
>> +#define EXYNOS_TRNG_FIFO_7 (0x9c)
>> +#define EXYNOS_TRNG_FIFO_LEN (8)
>> +#define EXYNOS_TRNG_CLOCK_RATE (500000)
>> +
>> +struct exynos_trng_dev {
>> + struct device *dev;
>> + void __iomem *mem;
>> + struct clk *clk;
>> + struct hwrng rng;
>> +};
>> +
>> +struct exynos_trng_dev *exynos_trng_dev;
>> +
>> +static inline void exynos_trng_set_reg(struct exynos_trng_dev *trng, u16 reg,
>> + u32 val)
>> +{
>> + /* Check range of reg? */
>> + __raw_writel(val, trng->mem + reg);
>
> Any specific reason to use __raw_writel? Why not just writel?
>
>> +}
>> +
>> +static inline u32 exynos_trng_get_reg(struct exynos_trng_dev *trng, u16 reg)
>> +{
>> + /* Check range of reg? */
>> + return __raw_readl(trng->mem + reg);
>
> Same as above.
>
>> +}
>> +
>> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
>> + bool wait)
>> +{
>> + struct exynos_trng_dev *trng;
>> + u32 val;
>> +
>> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
>> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
>
> max is always > 32. This condition is not required.
>
>> +
>> + trng = (struct exynos_trng_dev *)rng->priv;
>> +
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_FIFO_CTRL, max * 8);
>> + val = readl_poll_timeout(trng->mem + EXYNOS_TRNG_FIFO_CTRL, val,
>> + val == 0, 200, 1000000);
>> + if (val < 0)
>> + return val;
>> +
>> + memcpy_fromio(data, trng->mem + EXYNOS_TRNG_FIFO_0, max);
>> +
>> + return max;
>> +}
>> +
>> +static int exynos_trng_init(struct hwrng *rng)
>> +{
>> + struct exynos_trng_dev *trng;
>> + unsigned long sss_rate;
>> + u32 val;
>> +
>> + trng = (struct exynos_trng_dev *)rng->priv;
>> + sss_rate = clk_get_rate(trng->clk);
>> +
>> + /* For most TRNG circuits the clock frequency of under 500 kHz
>> + * is safe.
>> + */
>> + val = sss_rate / (EXYNOS_TRNG_CLOCK_RATE * 2);
>> + if (val > 0x7fff) {
>> + dev_err(trng->dev, "clock divider too large: %d", val);
>> + return -ERANGE;
>> + }
>> + val = val << 1;
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CLKDIV, val);
>> +
>> + /* Enable the generator. */
>> + val = 1 << 31;
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_CTRL, val);
>> +
>> + /* Disable post processing. /dev/hwrng is supposed to deliver
>> + * unprocessed data.
>> + */
>> + exynos_trng_set_reg(trng, EXYNOS_TRNG_POST_CTRL, 0);
>> +
>> + return 0;
>> +}
>> +
>> +static int exynos_trng_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_trng_dev *trng;
>> + struct resource *res;
>> + int ret = -ENOMEM;
>> +
>> + if (exynos_trng_dev)
>> + return -EEXIST;
>> +
>> + trng = devm_kzalloc(&pdev->dev, sizeof(*trng), GFP_KERNEL);
>> + if (!trng)
>> + goto err_ioremap;
>> +
>> + trng->rng.name = devm_kstrdup(&pdev->dev, dev_name(&pdev->dev),
>> + GFP_KERNEL);
>> + if (!trng->rng.name)
>> + goto err_ioremap;
>> +
>> + trng->rng.init = exynos_trng_init;
>> + trng->rng.read = exynos_trng_do_read;
>> + trng->rng.priv = (unsigned long) trng;
>> +
>> + platform_set_drvdata(pdev, trng);
>> + trng->dev = &pdev->dev;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + trng->mem = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(trng->mem)) {
>> + dev_err(&pdev->dev, "Couldn't map IO resources.\n");
>> + ret = PTR_ERR(trng->mem);
>> + goto err_ioremap;
>> + }
>> +
>> + pm_runtime_enable(&pdev->dev);
>> + ret = pm_runtime_get_sync(&pdev->dev);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed: %d.\n", ret);
>> + goto err_ioremap;
>> + }
>> +
>> + trng->clk = devm_clk_get(&pdev->dev, "secss");
>> + if (IS_ERR(trng->clk)) {
>> + /* XXX: EPROBE_DEFER ? */
>> + dev_err(&pdev->dev, "Couldn't get clock.\n");
>> + ret = PTR_ERR(trng->clk);
>> + goto err_clock;
>> + }
>> +
>> + ret = clk_prepare_enable(trng->clk);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Unable to enable the clk: %d.\n", ret);
>> + goto err_clock;
>> + }
>
> Why not move clk_prepare_enable to init call? While at it please add a
> cleanup callback as well.
>
>> +
>> + ret = hwrng_register(&trng->rng);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Couldn't register hwrng device.\n");
>> + goto err_register;
>> + }
>> +
>> + dev_info(&pdev->dev, "Exynos True Random Number Generator.\n");
>> +
>> + return 0;
>> +
>> +err_register:
>> + clk_disable_unprepare(trng->clk);
>> +
>> +err_clock:
>> + trng->mem = NULL;
>> + pm_runtime_put_sync(&pdev->dev);
>> + pm_runtime_disable(&pdev->dev);
>> +
>> +err_ioremap:
>> + dev_err(&pdev->dev, "Initialisation failed.\n");
>> + return ret;
>> +}
>> +
>> +static int exynos_trng_remove(struct platform_device *pdev)
>> +{
>> + exynos_trng_dev = NULL;
>> +
>> + return 0;
>> +}
>
> remove is not required as it does not do anything significant.
Should unregister from hwrng in remove callback.
>
>> +
>> +static int __maybe_unused exynos_trng_suspend(struct device *dev)
>> +{
>> + pm_runtime_put_sync(dev);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused exynos_trng_resume(struct device *dev)
>> +{
>> + int ret;
>> +
>> + ret = pm_runtime_get_sync(dev);
>> + if (ret < 0) {
>> + dev_err(dev, "pm_runtime_get_sync() failed: %d.\n", ret);
>> + pm_runtime_put_noidle(dev);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(exynos_trng_pm_ops, exynos_trng_suspend,
>> + exynos_trng_resume);
>> +
>> +static const struct of_device_id exynos_trng_dt_match[] = {
>> + {
>> + .compatible = "samsung,exynos5250-trng",
>> + },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, exynos_rng_dt_match);
>> +
>> +static struct platform_driver exynos_trng_driver = {
>> + .driver = {
>> + .name = "exynos-trng",
>> + .pm = &exynos_trng_pm_ops,
>> + .of_match_table = exynos_trng_dt_match,
>> + },
>> + .probe = exynos_trng_probe,
>> + .remove = exynos_trng_remove,
>> +};
>> +
>> +module_platform_driver(exynos_trng_driver);
>> +MODULE_AUTHOR("Łukasz Stelmach");
>> +MODULE_DESCRIPTION("H/W TRNG driver for Exynos chips");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.11.0
>>
>
> Regards,
> PrasannaKumar
^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CGME20171127072355eucas1p218e3cbaeb1ba16b5a1262deda350d850@eucas1p2.samsung.com>]
* Re: [PATCH 2/3] hwrng: exynos - add Samsung Exynos True RNG driver
[not found] ` <CGME20171127072355eucas1p218e3cbaeb1ba16b5a1262deda350d850@eucas1p2.samsung.com>
@ 2017-11-27 7:23 ` Łukasz Stelmach
0 siblings, 0 replies; 20+ messages in thread
From: Łukasz Stelmach @ 2017-11-27 7:23 UTC (permalink / raw)
To: PrasannaKumar Muralidharan
Cc: Rob Herring, Matt Mackall, Herbert Xu, Krzysztof Kozlowski,
devicetree, linux-crypto, linux-samsung-soc, linux-kernel,
Marek Szyprowski, Bartlomiej Zolnierkiewicz
[-- Attachment #1: Type: text/plain, Size: 630 bytes --]
It was <2017-11-24 pią 16:25>, when PrasannaKumar Muralidharan wrote:
>> +}
>> +
>> +static int exynos_trng_do_read(struct hwrng *rng, void *data, size_t max,
>> + bool wait)
>> +{
>> + struct exynos_trng_dev *trng;
>> + u32 val;
>> +
>> + max = max > (EXYNOS_TRNG_FIFO_LEN * 4) ?
>> + (EXYNOS_TRNG_FIFO_LEN * 4) : max;
>
> max is always > 32. This condition is not required.
No, it is not. http://elixir.free-electrons.com/linux/latest/source/drivers/char/hw_random/core.c#L67
--
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread