From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
This patchset introduces support for Mediatek hardware random generator (RNG)
Currently, the driver is already tested successfully with rng-tools on MT7623
SoC. And it should also be workable on other similar Mediatek SoCs.
SoC that also works on other similar SoCs.
Sean Wang (2):
dt-bindings: hwrng: Add Mediatek hardware random generator bindings
hwrng: mtk: Add driver for hardware random generator on MT7623 SoC
Documentation/devicetree/bindings/rng/mtk-rng.txt | 18 +++
drivers/char/hw_random/Kconfig | 16 +-
drivers/char/hw_random/Makefile | 2 +-
drivers/char/hw_random/mtk-rng.c | 174 ++++++++++++++++++++++
4 files changed, 208 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/rng/mtk-rng.txt
create mode 100644 drivers/char/hw_random/mtk-rng.c
--
1.9.1
From: Sean Wang <[email protected]>
This patch adds support for hardware random generator on MT7623 SoC
and should also work on other similar Mediatek SoCs. Currently,
the driver is already tested successfully with rng-tools.
Signed-off-by: Sean Wang <[email protected]>
---
drivers/char/hw_random/Kconfig | 16 +++-
drivers/char/hw_random/Makefile | 2 +-
drivers/char/hw_random/mtk-rng.c | 174 +++++++++++++++++++++++++++++++++++++++
3 files changed, 190 insertions(+), 2 deletions(-)
create mode 100644 drivers/char/hw_random/mtk-rng.c
diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
index 0cafe08..af782ce 100644
--- a/drivers/char/hw_random/Kconfig
+++ b/drivers/char/hw_random/Kconfig
@@ -419,10 +419,24 @@ config HW_RANDOM_CAVIUM
Generator hardware found on Cavium SoCs.
To compile this driver as a module, choose M here: the
- module will be called cavium_rng.
+ module will be called mtk-rng.
If unsure, say Y.
+config HW_RANDOM_MTK
+ tristate "Mediatek Random Number Generator support"
+ depends on HW_RANDOM
+ depends on ARCH_MEDIATEK || COMPILE_TEST
+ default y
+ ---help---
+ This driver provides kernel-side support for the Random Number
+ Generator hardware found on Mediatek SoCs.
+
+ To compile this driver as a module, choose M here. the
+ module will be called mtk-rng.
+
+ 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 5f52b1e..68be716 100644
--- a/drivers/char/hw_random/Makefile
+++ b/drivers/char/hw_random/Makefile
@@ -1,7 +1,6 @@
#
# Makefile for HW Random Number Generator (RNG) device drivers.
#
-
obj-$(CONFIG_HW_RANDOM) += rng-core.o
rng-core-y := core.o
obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
@@ -36,3 +35,4 @@ obj-$(CONFIG_HW_RANDOM_STM32) += stm32-rng.o
obj-$(CONFIG_HW_RANDOM_PIC32) += pic32-rng.o
obj-$(CONFIG_HW_RANDOM_MESON) += meson-rng.o
obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o
+obj-$(CONFIG_HW_RANDOM_MTK) += mtk-rng.o
diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
new file mode 100644
index 0000000..6561ee0
--- /dev/null
+++ b/drivers/char/hw_random/mtk-rng.c
@@ -0,0 +1,174 @@
+/*
+ * Driver for Mediatek Hardware Random Number Generator
+ *
+ * Copyright (C) 2017 Sean Wang <[email protected]>
+ *
+ * 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; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ */
+#define MTK_RNG_DEV KBUILD_MODNAME
+
+#include <linux/clk.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/of.h>
+#include <linux/platform_device.h>
+
+#define USEC_POLL 2
+#define TIMEOUT_POLL 20
+
+#define RNG_CTRL 0x00
+#define RNG_EN BIT(0)
+#define RNG_READY BIT(31)
+
+#define RNG_DATA 0x08
+
+#define to_mtk_rng(p) container_of(p, struct mtk_rng, rng)
+
+struct mtk_rng {
+ struct device *dev;
+ void __iomem *base;
+ struct clk *clk;
+ struct hwrng rng;
+};
+
+static int mtk_rng_init(struct hwrng *rng)
+{
+ struct mtk_rng *priv = to_mtk_rng(rng);
+ u32 val;
+ int err;
+
+ err = clk_prepare_enable(priv->clk);
+ if (err)
+ return err;
+
+ val = readl(priv->base + RNG_CTRL);
+ val |= RNG_EN;
+ writel(val, priv->base + RNG_CTRL);
+
+ return 0;
+}
+
+static void mtk_rng_cleanup(struct hwrng *rng)
+{
+ struct mtk_rng *priv = to_mtk_rng(rng);
+ u32 val;
+
+ val = readl(priv->base + RNG_CTRL);
+ val &= ~RNG_EN;
+ writel(val, priv->base + RNG_CTRL);
+
+ clk_disable_unprepare(priv->clk);
+}
+
+static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
+{
+ struct mtk_rng *priv = to_mtk_rng(rng);
+ int ready;
+
+ ready = readl(priv->base + RNG_CTRL) & RNG_READY;
+ if (!ready && wait)
+ readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
+ ready & RNG_READY, USEC_POLL,
+ TIMEOUT_POLL);
+ return !!ready;
+}
+
+static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
+{
+ struct mtk_rng *priv = to_mtk_rng(rng);
+ int retval = 0;
+
+ while (max >= sizeof(u32)) {
+ if (!mtk_rng_wait_ready(rng, wait))
+ break;
+
+ *(u32 *)buf = readl(priv->base + RNG_DATA);
+ retval += sizeof(u32);
+ buf += sizeof(u32);
+ max -= sizeof(u32);
+ }
+
+ if (unlikely(wait && max))
+ dev_warn(priv->dev, "timeout might be not properly set\n");
+
+ return retval || !wait ? retval : -EIO;
+}
+
+static int mtk_rng_probe(struct platform_device *pdev)
+{
+ struct resource *res;
+ int ret;
+ struct mtk_rng *priv;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "no iomem resource\n");
+ return -ENXIO;
+ }
+
+ priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = &pdev->dev;
+ priv->rng.name = pdev->name;
+ priv->rng.init = mtk_rng_init;
+ priv->rng.cleanup = mtk_rng_cleanup;
+ priv->rng.read = mtk_rng_read;
+
+ priv->clk = devm_clk_get(&pdev->dev, "rng");
+ if (IS_ERR(priv->clk)) {
+ ret = PTR_ERR(priv->clk);
+ dev_err(&pdev->dev, "no clock for device: %d\n", ret);
+ return ret;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ priv->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ ret = devm_hwrng_register(&pdev->dev, &priv->rng);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register rng device: %d\n",
+ ret);
+ return ret;
+ }
+
+ dev_info(&pdev->dev, "registered RNG driver\n");
+
+ return 0;
+}
+
+static const struct of_device_id mtk_rng_match[] = {
+ { .compatible = "mediatek,mt7623-rng" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, mtk_rng_match);
+
+static struct platform_driver mtk_rng_driver = {
+ .probe = mtk_rng_probe,
+ .driver = {
+ .name = MTK_RNG_DEV,
+ .of_match_table = mtk_rng_match,
+ },
+};
+
+module_platform_driver(mtk_rng_driver);
+
+MODULE_DESCRIPTION("Mediatek Random Number Generator Driver");
+MODULE_AUTHOR("Sean Wang <[email protected]>");
+MODULE_LICENSE("GPL");
--
1.9.1
From: Sean Wang <[email protected]>
Document the devicetree bindings for Mediatek random number
generator which could be found on MT7623 SoC or other similar
Mediatek SoCs.
Signed-off-by: Sean Wang <[email protected]>
---
Documentation/devicetree/bindings/rng/mtk-rng.txt | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
create mode 100644 Documentation/devicetree/bindings/rng/mtk-rng.txt
diff --git a/Documentation/devicetree/bindings/rng/mtk-rng.txt b/Documentation/devicetree/bindings/rng/mtk-rng.txt
new file mode 100644
index 0000000..a6d62a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/rng/mtk-rng.txt
@@ -0,0 +1,18 @@
+Device-Tree bindings for Mediatek random number generator
+found in Mediatek SoC family
+
+Required properties:
+- compatible : Should be "mediatek,mt7623-rng"
+- clocks : list of clock specifiers, corresponding to
+ entries in clock-names property;
+- clock-names : Should contain "rng" entries;
+- reg : Specifies base physical address and size of the registers
+
+Example:
+
+rng: rng@1020f000 {
+ compatible = "mediatek,mt7623-rng";
+ reg = <0 0x1020f000 0 0x1000>;
+ clocks = <&infracfg CLK_INFRA_TRNG>;
+ clock-names = "rng";
+};
--
1.9.1
Hi Sean,
Mostly looks good, have few minor comments.
On 13 April 2017 at 12:35, <[email protected]> wrote:
> +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + int ready;
> +
> + ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> + if (!ready && wait)
> + readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
> + ready & RNG_READY, USEC_POLL,
> + TIMEOUT_POLL);
> + return !!ready;
> +}
Use readl_poll_timeout_atomic's return value or -EIO instead of
!!ready. This will simplify mtk_rng_read.
> +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + int retval = 0;
> +
> + while (max >= sizeof(u32)) {
> + if (!mtk_rng_wait_ready(rng, wait))
> + break;
> +
> + *(u32 *)buf = readl(priv->base + RNG_DATA);
> + retval += sizeof(u32);
> + buf += sizeof(u32);
> + max -= sizeof(u32);
> + }
> +
> + if (unlikely(wait && max))
> + dev_warn(priv->dev, "timeout might be not properly set\n");
Is this really necessary? Better to choose proper timeout than
providing this warning message. In rare cases if the timeout could
occur due to some reason (may be a hardware fault) print appropriate
warning message.
> + return retval || !wait ? retval : -EIO;
> +}
Set retavl to mtk_rng_wait_ready and return retval.
Regards,
Prasanna
Hello
I have some minor comment below:
On Thu, Apr 13, 2017 at 03:05:08PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected] wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
>
> This patch adds support for hardware random generator on MT7623 SoC
> and should also work on other similar Mediatek SoCs. Currently,
> the driver is already tested successfully with rng-tools.
>
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
> ---
> drivers/char/hw_random/Kconfig | 16 +++-
> drivers/char/hw_random/Makefile | 2 +-
> drivers/char/hw_random/mtk-rng.c | 174 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 190 insertions(+), 2 deletions(-)
> create mode 100644 drivers/char/hw_random/mtk-rng.c
>
> diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> index 0cafe08..af782ce 100644
> --- a/drivers/char/hw_random/Kconfig
> +++ b/drivers/char/hw_random/Kconfig
> @@ -419,10 +419,24 @@ config HW_RANDOM_CAVIUM
> Generator hardware found on Cavium SoCs.
>
> To compile this driver as a module, choose M here: the
> - module will be called cavium_rng.
> + module will be called mtk-rng.
Unwanted change
>
> If unsure, say Y.
>
> +config HW_RANDOM_MTK
> + tristate "Mediatek Random Number Generator support"
> + depends on HW_RANDOM
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + default y
> + ---help---
> + This driver provides kernel-side support for the Random Number
> + Generator hardware found on Mediatek SoCs.
> +
> + To compile this driver as a module, choose M here. the
> + module will be called mtk-rng.
> +
> + 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 5f52b1e..68be716 100644
> --- a/drivers/char/hw_random/Makefile
> +++ b/drivers/char/hw_random/Makefile
> @@ -1,7 +1,6 @@
> #
> # Makefile for HW Random Number Generator (RNG) device drivers.
> #
> -
Another unwanted change
> obj-$(CONFIG_HW_RANDOM) += rng-core.o
> rng-core-y := core.o
> obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
> @@ -36,3 +35,4 @@ obj-$(CONFIG_HW_RANDOM_STM32) += stm32-rng.o
> obj-$(CONFIG_HW_RANDOM_PIC32) += pic32-rng.o
> obj-$(CONFIG_HW_RANDOM_MESON) += meson-rng.o
> obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o
> +obj-$(CONFIG_HW_RANDOM_MTK) += mtk-rng.o
> diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
> new file mode 100644
> index 0000000..6561ee0
> --- /dev/null
> +++ b/drivers/char/hw_random/mtk-rng.c
> @@ -0,0 +1,174 @@
> +/*
> + * Driver for Mediatek Hardware Random Number Generator
> + *
> + * Copyright (C) 2017 Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
> + *
> + * 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; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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.
> + */
> +#define MTK_RNG_DEV KBUILD_MODNAME
> +
> +#include <linux/clk.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/of.h>
> +#include <linux/platform_device.h>
> +
> +#define USEC_POLL 2
> +#define TIMEOUT_POLL 20
> +
> +#define RNG_CTRL 0x00
> +#define RNG_EN BIT(0)
> +#define RNG_READY BIT(31)
Keep only one space between define and name
> +
> +#define RNG_DATA 0x08
> +
> +#define to_mtk_rng(p) container_of(p, struct mtk_rng, rng)
> +
> +struct mtk_rng {
> + struct device *dev;
> + void __iomem *base;
> + struct clk *clk;
> + struct hwrng rng;
> +};
> +
> +static int mtk_rng_init(struct hwrng *rng)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + u32 val;
> + int err;
> +
> + err = clk_prepare_enable(priv->clk);
> + if (err)
> + return err;
> +
> + val = readl(priv->base + RNG_CTRL);
> + val |= RNG_EN;
> + writel(val, priv->base + RNG_CTRL);
> +
> + return 0;
> +}
> +
> +static void mtk_rng_cleanup(struct hwrng *rng)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + u32 val;
> +
> + val = readl(priv->base + RNG_CTRL);
> + val &= ~RNG_EN;
> + writel(val, priv->base + RNG_CTRL);
> +
> + clk_disable_unprepare(priv->clk);
> +}
> +
> +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + int ready;
> +
> + ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> + if (!ready && wait)
> + readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
> + ready & RNG_READY, USEC_POLL,
> + TIMEOUT_POLL);
> + return !!ready;
> +}
> +
> +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> +{
> + struct mtk_rng *priv = to_mtk_rng(rng);
> + int retval = 0;
> +
> + while (max >= sizeof(u32)) {
> + if (!mtk_rng_wait_ready(rng, wait))
> + break;
> +
> + *(u32 *)buf = readl(priv->base + RNG_DATA);
> + retval += sizeof(u32);
> + buf += sizeof(u32);
> + max -= sizeof(u32);
> + }
> +
> + if (unlikely(wait && max))
> + dev_warn(priv->dev, "timeout might be not properly set\n");
> +
> + return retval || !wait ? retval : -EIO;
> +}
> +
> +static int mtk_rng_probe(struct platform_device *pdev)
> +{
> + struct resource *res;
> + int ret;
> + struct mtk_rng *priv;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "no iomem resource\n");
> + return -ENXIO;
> + }
> +
> + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = &pdev->dev;
> + priv->rng.name = pdev->name;
> + priv->rng.init = mtk_rng_init;
> + priv->rng.cleanup = mtk_rng_cleanup;
> + priv->rng.read = mtk_rng_read;
> +
> + priv->clk = devm_clk_get(&pdev->dev, "rng");
> + if (IS_ERR(priv->clk)) {
> + ret = PTR_ERR(priv->clk);
> + dev_err(&pdev->dev, "no clock for device: %d\n", ret);
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
You get that resource twice
Regards
Corentin Labbe
Hi Corentin,
I all agree and appreciate your careful reviewing.
They will be added into the next one.
Sean
On Thu, 2017-04-13 at 13:06 +0200, Corentin Labbe wrote:
> Hello
>
> I have some minor comment below:
>
> On Thu, Apr 13, 2017 at 03:05:08PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected] wrote:
> > From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
> >
> > This patch adds support for hardware random generator on MT7623 SoC
> > and should also work on other similar Mediatek SoCs. Currently,
> > the driver is already tested successfully with rng-tools.
> >
> > Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
> > ---
> > drivers/char/hw_random/Kconfig | 16 +++-
> > drivers/char/hw_random/Makefile | 2 +-
> > drivers/char/hw_random/mtk-rng.c | 174 +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 190 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/char/hw_random/mtk-rng.c
> >
> > diff --git a/drivers/char/hw_random/Kconfig b/drivers/char/hw_random/Kconfig
> > index 0cafe08..af782ce 100644
> > --- a/drivers/char/hw_random/Kconfig
> > +++ b/drivers/char/hw_random/Kconfig
> > @@ -419,10 +419,24 @@ config HW_RANDOM_CAVIUM
> > Generator hardware found on Cavium SoCs.
> >
> > To compile this driver as a module, choose M here: the
> > - module will be called cavium_rng.
> > + module will be called mtk-rng.
>
> Unwanted change
>
> >
> > If unsure, say Y.
> >
> > +config HW_RANDOM_MTK
> > + tristate "Mediatek Random Number Generator support"
> > + depends on HW_RANDOM
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + default y
> > + ---help---
> > + This driver provides kernel-side support for the Random Number
> > + Generator hardware found on Mediatek SoCs.
> > +
> > + To compile this driver as a module, choose M here. the
> > + module will be called mtk-rng.
> > +
> > + 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 5f52b1e..68be716 100644
> > --- a/drivers/char/hw_random/Makefile
> > +++ b/drivers/char/hw_random/Makefile
> > @@ -1,7 +1,6 @@
> > #
> > # Makefile for HW Random Number Generator (RNG) device drivers.
> > #
> > -
>
> Another unwanted change
>
> > obj-$(CONFIG_HW_RANDOM) += rng-core.o
> > rng-core-y := core.o
> > obj-$(CONFIG_HW_RANDOM_TIMERIOMEM) += timeriomem-rng.o
> > @@ -36,3 +35,4 @@ obj-$(CONFIG_HW_RANDOM_STM32) += stm32-rng.o
> > obj-$(CONFIG_HW_RANDOM_PIC32) += pic32-rng.o
> > obj-$(CONFIG_HW_RANDOM_MESON) += meson-rng.o
> > obj-$(CONFIG_HW_RANDOM_CAVIUM) += cavium-rng.o cavium-rng-vf.o
> > +obj-$(CONFIG_HW_RANDOM_MTK) += mtk-rng.o
> > diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
> > new file mode 100644
> > index 0000000..6561ee0
> > --- /dev/null
> > +++ b/drivers/char/hw_random/mtk-rng.c
> > @@ -0,0 +1,174 @@
> > +/*
> > + * Driver for Mediatek Hardware Random Number Generator
> > + *
> > + * Copyright (C) 2017 Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
> > + *
> > + * 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; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * 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.
> > + */
> > +#define MTK_RNG_DEV KBUILD_MODNAME
> > +
> > +#include <linux/clk.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/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define USEC_POLL 2
> > +#define TIMEOUT_POLL 20
> > +
> > +#define RNG_CTRL 0x00
> > +#define RNG_EN BIT(0)
> > +#define RNG_READY BIT(31)
>
> Keep only one space between define and name
>
> > +
> > +#define RNG_DATA 0x08
> > +
> > +#define to_mtk_rng(p) container_of(p, struct mtk_rng, rng)
> > +
> > +struct mtk_rng {
> > + struct device *dev;
> > + void __iomem *base;
> > + struct clk *clk;
> > + struct hwrng rng;
> > +};
> > +
> > +static int mtk_rng_init(struct hwrng *rng)
> > +{
> > + struct mtk_rng *priv = to_mtk_rng(rng);
> > + u32 val;
> > + int err;
> > +
> > + err = clk_prepare_enable(priv->clk);
> > + if (err)
> > + return err;
> > +
> > + val = readl(priv->base + RNG_CTRL);
> > + val |= RNG_EN;
> > + writel(val, priv->base + RNG_CTRL);
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_rng_cleanup(struct hwrng *rng)
> > +{
> > + struct mtk_rng *priv = to_mtk_rng(rng);
> > + u32 val;
> > +
> > + val = readl(priv->base + RNG_CTRL);
> > + val &= ~RNG_EN;
> > + writel(val, priv->base + RNG_CTRL);
> > +
> > + clk_disable_unprepare(priv->clk);
> > +}
> > +
> > +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> > +{
> > + struct mtk_rng *priv = to_mtk_rng(rng);
> > + int ready;
> > +
> > + ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> > + if (!ready && wait)
> > + readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
> > + ready & RNG_READY, USEC_POLL,
> > + TIMEOUT_POLL);
> > + return !!ready;
> > +}
> > +
> > +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> > +{
> > + struct mtk_rng *priv = to_mtk_rng(rng);
> > + int retval = 0;
> > +
> > + while (max >= sizeof(u32)) {
> > + if (!mtk_rng_wait_ready(rng, wait))
> > + break;
> > +
> > + *(u32 *)buf = readl(priv->base + RNG_DATA);
> > + retval += sizeof(u32);
> > + buf += sizeof(u32);
> > + max -= sizeof(u32);
> > + }
> > +
> > + if (unlikely(wait && max))
> > + dev_warn(priv->dev, "timeout might be not properly set\n");
> > +
> > + return retval || !wait ? retval : -EIO;
> > +}
> > +
> > +static int mtk_rng_probe(struct platform_device *pdev)
> > +{
> > + struct resource *res;
> > + int ret;
> > + struct mtk_rng *priv;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res) {
> > + dev_err(&pdev->dev, "no iomem resource\n");
> > + return -ENXIO;
> > + }
> > +
> > + priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->dev = &pdev->dev;
> > + priv->rng.name = pdev->name;
> > + priv->rng.init = mtk_rng_init;
> > + priv->rng.cleanup = mtk_rng_cleanup;
> > + priv->rng.read = mtk_rng_read;
> > +
> > + priv->clk = devm_clk_get(&pdev->dev, "rng");
> > + if (IS_ERR(priv->clk)) {
> > + ret = PTR_ERR(priv->clk);
> > + dev_err(&pdev->dev, "no clock for device: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> You get that resource twice
>
> Regards
> Corentin Labbe
Hi PrasannaKumar,
Add my comments inline
On Thu, 2017-04-13 at 14:09 +0530, PrasannaKumar Muralidharan wrote:
> Hi Sean,
>
> Mostly looks good, have few minor comments.
>
> On 13 April 2017 at 12:35, <[email protected]> wrote:
> > +static bool mtk_rng_wait_ready(struct hwrng *rng, bool wait)
> > +{
> > + struct mtk_rng *priv = to_mtk_rng(rng);
> > + int ready;
> > +
> > + ready = readl(priv->base + RNG_CTRL) & RNG_READY;
> > + if (!ready && wait)
> > + readl_poll_timeout_atomic(priv->base + RNG_CTRL, ready,
> > + ready & RNG_READY, USEC_POLL,
> > + TIMEOUT_POLL);
> > + return !!ready;
> > +}
>
> Use readl_poll_timeout_atomic's return value or -EIO instead of
> !!ready. This will simplify mtk_rng_read.
>
!!ready provided is in order to let blocking/non-blocking case could
share same code path. And readl_poll_timeout_atomic only handles
blocking case.
> > +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> > +{
> > + struct mtk_rng *priv = to_mtk_rng(rng);
> > + int retval = 0;
> > +
> > + while (max >= sizeof(u32)) {
> > + if (!mtk_rng_wait_ready(rng, wait))
> > + break;
> > +
> > + *(u32 *)buf = readl(priv->base + RNG_DATA);
> > + retval += sizeof(u32);
> > + buf += sizeof(u32);
> > + max -= sizeof(u32);
> > + }
> > +
> > + if (unlikely(wait && max))
> > + dev_warn(priv->dev, "timeout might be not properly set\n");
>
> Is this really necessary? Better to choose proper timeout than
> providing this warning message. In rare cases if the timeout could
> occur due to some reason (may be a hardware fault) print appropriate
> warning message.
It is good, I will choose the proper timeout and remove the log in the
next one.
>
> > + return retval || !wait ? retval : -EIO;
> > +}
>
> Set retavl to mtk_rng_wait_ready and return retval.
>
Maybe i didn't get your points exactly. Adding some explanation about
thoughts here.
"return retval || !wait ? retval : -EIO;" I use can also help handling
the both cases in one line which i think is elegant enough.
And retval is accumulated with each round if some data's existing in
hardware, so we don't return the value from mtk_rng_wait_ready().
> Regards,
> Prasanna
thanks for all your reviewing and suggestion
Sean
On 14 April 2017 at 09:28, Sean Wang <[email protected]> wrote:
>
> Hi PrasannaKumar,
>
> Add my comments inline
>
>>
>> Use readl_poll_timeout_atomic's return value or -EIO instead of
>> !!ready. This will simplify mtk_rng_read.
>>
>
> !!ready provided is in order to let blocking/non-blocking case could
> share same code path. And readl_poll_timeout_atomic only handles
> blocking case.
Missed this point. Makes sense. My previous comment about return value
in mtk_rng_read is invalid as I based it on a wrong assumption.
>
>> > +static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
>> > +{
>> > + struct mtk_rng *priv = to_mtk_rng(rng);
>> > + int retval = 0;
>> > +
>> > + while (max >= sizeof(u32)) {
>> > + if (!mtk_rng_wait_ready(rng, wait))
>> > + break;
>> > +
>> > + *(u32 *)buf = readl(priv->base + RNG_DATA);
>> > + retval += sizeof(u32);
>> > + buf += sizeof(u32);
>> > + max -= sizeof(u32);
>> > + }
>> > +
>> > + if (unlikely(wait && max))
>> > + dev_warn(priv->dev, "timeout might be not properly set\n");
>>
>> Is this really necessary? Better to choose proper timeout than
>> providing this warning message. In rare cases if the timeout could
>> occur due to some reason (may be a hardware fault) print appropriate
>> warning message.
>
> It is good, I will choose the proper timeout and remove the log in the
> next one.
>
>>
>> > + return retval || !wait ? retval : -EIO;
>> > +}
>>
>> Set retavl to mtk_rng_wait_ready and return retval.
>>
>
> Maybe i didn't get your points exactly. Adding some explanation about
> thoughts here.
>
> "return retval || !wait ? retval : -EIO;" I use can also help handling
> the both cases in one line which i think is elegant enough.
>
> And retval is accumulated with each round if some data's existing in
> hardware, so we don't return the value from mtk_rng_wait_ready().
retval can be 0 only when mkt_rng_wait_ready fails, returning 0 when
wait is true is confusing. Expected return value when 0 bytes is read
from device and wait is true is not clearly documented.
"return retval || !wait ? retval : -EIO;" is also fine.
Overall the code looks good to me. You can add:
Reviewed-by: PrasannaKumar Muralidharan <[email protected]>.
Regards,
PrasannaKumar
On Thu, Apr 13, 2017 at 03:05:07PM +0800, sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected] wrote:
> From: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
>
> Document the devicetree bindings for Mediatek random number
> generator which could be found on MT7623 SoC or other similar
> Mediatek SoCs.
>
> Signed-off-by: Sean Wang <sean.wang-NuS5LvNUpcJWk0Htik3J/[email protected]>
> ---
> Documentation/devicetree/bindings/rng/mtk-rng.txt | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/rng/mtk-rng.txt
Acked-by: Rob Herring <[email protected]>