The HW randon number generator requires a clock and we shouldn't rely
on the boot loader to enable it.
Heiner Kallweit (2):
hwrng: meson: expose RNG0 clock via DT
hwrng: meson: add clock handling to driver
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 2 +-
arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 5 +++++
drivers/char/hw_random/meson-rng.c | 27 ++++++++++++++++++++++++++-
include/dt-bindings/clock/gxbb-clkc.h | 1 +
4 files changed, 33 insertions(+), 2 deletions(-)
--
2.11.1
Expose the RNG0 clock via DT.
Signed-off-by: Heiner Kallweit <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 2 +-
arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 5 +++++
include/dt-bindings/clock/gxbb-clkc.h | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 5d995f77..620495a4 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -380,7 +380,7 @@
#size-cells = <2>;
ranges = <0x0 0x0 0x0 0xc8834000 0x0 0x2000>;
- rng {
+ hwrng: rng {
compatible = "amlogic,meson-rng";
reg = <0x0 0x0 0x0 0x4>;
};
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index 04b3324b..a375cb21 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -524,3 +524,8 @@
&vpu {
compatible = "amlogic,meson-gxbb-vpu", "amlogic,meson-gx-vpu";
};
+
+&hwrng {
+ clocks = <&clkc CLKID_RNG0>;
+ clock-names = "core";
+};
diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-bindings/clock/gxbb-clkc.h
index 692846c7..473676b1 100644
--- a/include/dt-bindings/clock/gxbb-clkc.h
+++ b/include/dt-bindings/clock/gxbb-clkc.h
@@ -15,6 +15,7 @@
#define CLKID_SPI 34
#define CLKID_I2C 22
#define CLKID_SAR_ADC 23
+#define CLKID_RNG0 25
#define CLKID_ETH 36
#define CLKID_USB0 50
#define CLKID_USB1 51
--
2.11.1
Add handling of RNG0 clock to the driver.
Signed-off-by: Heiner Kallweit <[email protected]>
---
drivers/char/hw_random/meson-rng.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/meson-rng.c b/drivers/char/hw_random/meson-rng.c
index 119d6984..1f586e48 100644
--- a/drivers/char/hw_random/meson-rng.c
+++ b/drivers/char/hw_random/meson-rng.c
@@ -62,6 +62,7 @@
#include <linux/slab.h>
#include <linux/types.h>
#include <linux/of.h>
+#include <linux/clk.h>
#define RNG_DATA 0x00
@@ -69,6 +70,7 @@ struct meson_rng_data {
void __iomem *base;
struct platform_device *pdev;
struct hwrng rng;
+ struct clk *core_clk;
};
static int meson_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
@@ -86,6 +88,7 @@ static int meson_rng_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct meson_rng_data *data;
struct resource *res;
+ int ret;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -98,12 +101,33 @@ static int meson_rng_probe(struct platform_device *pdev)
if (IS_ERR(data->base))
return PTR_ERR(data->base);
+ data->core_clk = devm_clk_get(dev, "core");
+ if (IS_ERR(data->core_clk))
+ return PTR_ERR(data->core_clk);
+
+ ret = clk_prepare_enable(data->core_clk);
+ if (ret)
+ return ret;
+
data->rng.name = pdev->name;
data->rng.read = meson_rng_read;
platform_set_drvdata(pdev, data);
- return devm_hwrng_register(dev, &data->rng);
+ ret = devm_hwrng_register(dev, &data->rng);
+ if (ret)
+ clk_disable_unprepare(data->core_clk);
+
+ return ret;
+}
+
+static int meson_rng_remove(struct platform_device *pdev)
+{
+ struct meson_rng_data *data = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(data->core_clk);
+
+ return 0;
}
static const struct of_device_id meson_rng_of_match[] = {
@@ -114,6 +138,7 @@ MODULE_DEVICE_TABLE(of, meson_rng_of_match);
static struct platform_driver meson_rng_driver = {
.probe = meson_rng_probe,
+ .remove = meson_rng_remove,
.driver = {
.name = "meson-rng",
.of_match_table = meson_rng_of_match,
--
2.11.1
On Sun, 2017-02-19 at 21:33 +0100, Heiner Kallweit wrote:
> Expose the RNG0 clock via DT.
>
> Signed-off-by: Heiner Kallweit <[email protected]>
> ---
> arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 2 +-
> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 5 +++++
> include/dt-bindings/clock/gxbb-clkc.h | 1 +
> 3 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index 5d995f77..620495a4 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -380,7 +380,7 @@
> #size-cells = <2>;
> ranges = <0x0 0x0 0x0 0xc8834000 0x0
> 0x2000>;
>
> - rng {
> + hwrng: rng {
> compatible = "amlogic,meson-rng";
> reg = <0x0 0x0 0x0 0x4>;
> };
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index 04b3324b..a375cb21 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -524,3 +524,8 @@
> &vpu {
> compatible = "amlogic,meson-gxbb-vpu", "amlogic,meson-gx-
> vpu";
> };
> +
> +&hwrng {
> + clocks = <&clkc CLKID_RNG0>;
> + clock-names = "core";
> +};
> diff --git a/include/dt-bindings/clock/gxbb-clkc.h b/include/dt-
> bindings/clock/gxbb-clkc.h
> index 692846c7..473676b1 100644
> --- a/include/dt-bindings/clock/gxbb-clkc.h
> +++ b/include/dt-bindings/clock/gxbb-clkc.h
> @@ -15,6 +15,7 @@
> #define CLKID_SPI 34
> #define CLKID_I2C 22
> #define CLKID_SAR_ADC 23
> +#define CLKID_RNG0 25
Shouldn't you comment out the corresponding "define" in
drivers/clk/meson/gxbb.h ?
I don't know if it is rule, but we usually "expose" the clock (moving
the define from drivers/clk/meson/gxbb.h to include/dt-
bindings/clock/gxbb-clkc.h) in a dedicated patch, with the subject
starting with "clk: gxbb: expose foobar clock". It helps the
maintainers of the subsystem to give their ack.
For this to happen, please remember the patch should be sent to them as
well. Here you are missing the clock and dt guys:
* Stephen Boyd <[email protected]>
* Michael Turquette <[email protected]>
* [email protected]
* [email protected]
scripts/get_maintainer.pl can help you finding all people your patch
should be sent to.
The remaining part of this patch involves DT, so the subject should
start with "ARM64: dts: meson-gx:"
You also need to add [email protected] in CC.
Apart from these minor remarks, the patchset looks good to me.
Thx !
> #define CLKID_ETH 36
> #define CLKID_USB0 50
> #define CLKID_USB1 51
On 20 February 2017 at 02:05, Heiner Kallweit <[email protected]> wrote:
> Add handling of RNG0 clock to the driver.
>
> Signed-off-by: Heiner Kallweit <[email protected]>
> ---
> drivers/char/hw_random/meson-rng.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/meson-rng.c b/drivers/char/hw_random/meson-rng.c
> index 119d6984..1f586e48 100644
> --- a/drivers/char/hw_random/meson-rng.c
> +++ b/drivers/char/hw_random/meson-rng.c
> @@ -62,6 +62,7 @@
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/of.h>
> +#include <linux/clk.h>
>
> #define RNG_DATA 0x00
>
> @@ -69,6 +70,7 @@ struct meson_rng_data {
> void __iomem *base;
> struct platform_device *pdev;
> struct hwrng rng;
> + struct clk *core_clk;
> };
>
> static int meson_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> @@ -86,6 +88,7 @@ static int meson_rng_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct meson_rng_data *data;
> struct resource *res;
> + int ret;
Variable ret is not used. It can be removed.
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -98,12 +101,33 @@ static int meson_rng_probe(struct platform_device *pdev)
> if (IS_ERR(data->base))
> return PTR_ERR(data->base);
>
> + data->core_clk = devm_clk_get(dev, "core");
> + if (IS_ERR(data->core_clk))
> + return PTR_ERR(data->core_clk);
> +
> + ret = clk_prepare_enable(data->core_clk);
> + if (ret)
> + return ret;
> +
> data->rng.name = pdev->name;
> data->rng.read = meson_rng_read;
>
> platform_set_drvdata(pdev, data);
>
> - return devm_hwrng_register(dev, &data->rng);
> + ret = devm_hwrng_register(dev, &data->rng);
> + if (ret)
> + clk_disable_unprepare(data->core_clk);
> +
> + return ret;
> +}
> +
> +static int meson_rng_remove(struct platform_device *pdev)
> +{
> + struct meson_rng_data *data = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(data->core_clk);
> +
> + return 0;
> }
In .remove clock gets disabled before the hwrng_unregister is called.
The device node '/dev/hwrng' could be accessed while meson_rng_remove
is called which could lead to problems. Instead of devm_hwrng_register
use hwrng_register in .probe and call hwrng_unregister in .remove.
> static const struct of_device_id meson_rng_of_match[] = {
> @@ -114,6 +138,7 @@ MODULE_DEVICE_TABLE(of, meson_rng_of_match);
>
> static struct platform_driver meson_rng_driver = {
> .probe = meson_rng_probe,
> + .remove = meson_rng_remove,
> .driver = {
> .name = "meson-rng",
> .of_match_table = meson_rng_of_match,
> --
> 2.11.1
>
>
On 02/19/2017 09:30 PM, Heiner Kallweit wrote:
> The HW randon number generator requires a clock and we shouldn't rely
> on the boot loader to enable it.
>
> Heiner Kallweit (2):
> hwrng: meson: expose RNG0 clock via DT
> hwrng: meson: add clock handling to driver
>
> arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 2 +-
> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 5 +++++
> drivers/char/hw_random/meson-rng.c | 27 ++++++++++++++++++++++++++-
> include/dt-bindings/clock/gxbb-clkc.h | 1 +
> 4 files changed, 33 insertions(+), 2 deletions(-)
>
Hi Heiner,
Thanks for the fix, but can you also update the dt-bindings document ?
Documentation/devicetree/bindings/rng/amlogic,meson-rng.txt
I also think this clock should be optional, to keep driver working using old bindings, set the core_clk to NULL if not available, clock_prepare_enable and clock_disable_unprepare won't fail with a NULL clock
Neil