Hello!
This patch series adds hwrng support for Amlogic S4-series.
Now, S4 uses a new random number generation algorithm.
This changes implemnents new algo and also adds description
to meson-s4.dtsi.
Alexey Romanov (3):
drivers: rng: add check status bit feature
dt-bindings: rng: meson: add meson-rng-s4 compatible
arch/arm64: dts: meson-s4: add hwrng node
.../bindings/rng/amlogic,meson-rng.yaml | 1 +
arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 5 ++
drivers/char/hw_random/meson-rng.c | 77 ++++++++++++++++++-
3 files changed, 80 insertions(+), 3 deletions(-)
--
2.38.1
For some Amlogic SOC's, mechanism to obtain random number
has been changed. For example, S4 now uses status bit waiting algo.
Signed-off-by: Alexey Romanov <[email protected]>
---
drivers/char/hw_random/meson-rng.c | 77 ++++++++++++++++++++++++++++--
1 file changed, 74 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/meson-rng.c b/drivers/char/hw_random/meson-rng.c
index a4eb8e35f13d..c6d7349630a1 100644
--- a/drivers/char/hw_random/meson-rng.c
+++ b/drivers/char/hw_random/meson-rng.c
@@ -14,19 +14,65 @@
#include <linux/of.h>
#include <linux/clk.h>
-#define RNG_DATA 0x00
+struct meson_rng_priv {
+ bool check_status_bit;
+ unsigned int data_offset;
+ unsigned int cfg_offset;
+};
struct meson_rng_data {
void __iomem *base;
struct hwrng rng;
+ struct device *dev;
+ const struct meson_rng_priv *priv;
};
+#define RUN_BIT 0
+#define SEED_READY_STS_BIT 31
+#define RETRY_CNT 100
+
+static int meson_rng_wait_status(void __iomem *cfg_addr, int bit)
+{
+ u32 status;
+ u32 cnt = 0;
+
+ do {
+ status = readl_relaxed(cfg_addr) & BIT(bit);
+ cpu_relax();
+ } while (status && (cnt++ < RETRY_CNT));
+
+ if (status)
+ return -EBUSY;
+
+ return 0;
+}
+
static int meson_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
{
struct meson_rng_data *data =
container_of(rng, struct meson_rng_data, rng);
+ const struct meson_rng_priv *priv = data->priv;
+
+ if (priv->check_status_bit) {
+ void __iomem *cfg_addr = data->base + priv->cfg_offset;
+ int err;
+
+ writel_relaxed(readl_relaxed(cfg_addr) | BIT(SEED_READY_STS_BIT), cfg_addr);
- *(u32 *)buf = readl_relaxed(data->base + RNG_DATA);
+ err = meson_rng_wait_status(cfg_addr, SEED_READY_STS_BIT);
+ if (err) {
+ dev_err(data->dev, "Seed isn't ready, try again\n");
+ return err;
+ }
+
+ err = meson_rng_wait_status(cfg_addr, RUN_BIT);
+ if (err) {
+ dev_err(data->dev, "Can't get random number, try again\n");
+ return err;
+ }
+ }
+
+ *(u32 *)buf = readl_relaxed(data->base + priv->data_offset);
return sizeof(u32);
}
@@ -41,6 +87,10 @@ static int meson_rng_probe(struct platform_device *pdev)
if (!data)
return -ENOMEM;
+ data->priv = device_get_match_data(&pdev->dev);
+ if (!data->priv)
+ return -ENODEV;
+
data->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(data->base))
return PTR_ERR(data->base);
@@ -53,11 +103,32 @@ static int meson_rng_probe(struct platform_device *pdev)
data->rng.name = pdev->name;
data->rng.read = meson_rng_read;
+ data->dev = &pdev->dev;
+
return devm_hwrng_register(dev, &data->rng);
}
+static const struct meson_rng_priv meson_rng_priv = {
+ .check_status_bit = false,
+ .data_offset = 0x0,
+ .cfg_offset = 0x0,
+};
+
+static const struct meson_rng_priv meson_rng_priv_s4 = {
+ .check_status_bit = true,
+ .data_offset = 0x8,
+ .cfg_offset = 0x0,
+};
+
static const struct of_device_id meson_rng_of_match[] = {
- { .compatible = "amlogic,meson-rng", },
+ {
+ .compatible = "amlogic,meson-rng",
+ .data = (void *)&meson_rng_priv,
+ },
+ {
+ .compatible = "amlogic,meson-rng-s4",
+ .data = (void *)&meson_rng_priv_s4,
+ },
{},
};
MODULE_DEVICE_TABLE(of, meson_rng_of_match);
--
2.38.1
Alexey Romanov ([email protected]) wrote:
> For some Amlogic SOC's, mechanism to obtain random number
> has been changed. For example, S4 now uses status bit waiting algo.
> Signed-off-by: Alexey Romanov <[email protected]>
> ---
> drivers/char/hw_random/meson-rng.c | 77 ++++++++++++++++++++++++++++--
> 1 file changed, 74 insertions(+), 3 deletions(-)
> diff --git a/drivers/char/hw_random/meson-rng.c b/drivers/char/hw_random/meson-rng.c
> index a4eb8e35f13d..c6d7349630a1 100644
> --- a/drivers/char/hw_random/meson-rng.c
> +++ b/drivers/char/hw_random/meson-rng.c
> @@ -14,19 +14,65 @@
> #include <linux/of.h>
> #include <linux/clk.h>
> -#define RNG_DATA 0x00
> +struct meson_rng_priv {
> + bool check_status_bit;
> + unsigned int data_offset;
> + unsigned int cfg_offset;
> +};
> struct meson_rng_data {
> void __iomem *base;
> struct hwrng rng;
> + struct device *dev;
> + const struct meson_rng_priv *priv;
> };
> +#define RUN_BIT 0
> +#define SEED_READY_STS_BIT 31
> +#define RETRY_CNT 100
> +
> +static int meson_rng_wait_status(void __iomem *cfg_addr, int bit)
> +{
> + u32 status;
> + u32 cnt = 0;
> +
> + do {
> + status = readl_relaxed(cfg_addr) & BIT(bit);
> + cpu_relax();
> + } while (status && (cnt++ < RETRY_CNT));
> +
Could you use readl_relaxed_poll_timeout here instead of open coding the
loop?
> + if (status)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> static int meson_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> {
> struct meson_rng_data *data =
> container_of(rng, struct meson_rng_data, rng);
> + const struct meson_rng_priv *priv = data->priv;
> +
> + if (priv->check_status_bit) {
> + void __iomem *cfg_addr = data->base + priv->cfg_offset;
> + int err;
> +
> + writel_relaxed(readl_relaxed(cfg_addr) | BIT(SEED_READY_STS_BIT), cfg_addr);
> - *(u32 *)buf = readl_relaxed(data->base + RNG_DATA);
> + err = meson_rng_wait_status(cfg_addr, SEED_READY_STS_BIT);
> + if (err) {
> + dev_err(data->dev, "Seed isn't ready, try again\n");
> + return err;
> + }
> +
> + err = meson_rng_wait_status(cfg_addr, RUN_BIT);
> + if (err) {
> + dev_err(data->dev, "Can't get random number, try again\n");
> + return err;
> + }
> + }
> +
> + *(u32 *)buf = readl_relaxed(data->base + priv->data_offset);
> return sizeof(u32);
> }
> @@ -41,6 +87,10 @@ static int meson_rng_probe(struct platform_device *pdev)
> if (!data)
> return -ENOMEM;
> + data->priv = device_get_match_data(&pdev->dev);
> + if (!data->priv)
> + return -ENODEV;
> +
> data->base = devm_platform_ioremap_resource(pdev, 0);
> if (IS_ERR(data->base))
> return PTR_ERR(data->base);
> @@ -53,11 +103,32 @@ static int meson_rng_probe(struct platform_device *pdev)
> data->rng.name = pdev->name;
> data->rng.read = meson_rng_read;
> + data->dev = &pdev->dev;
> +
> return devm_hwrng_register(dev, &data->rng);
> }
> +static const struct meson_rng_priv meson_rng_priv = {
> + .check_status_bit = false,
> + .data_offset = 0x0,
> + .cfg_offset = 0x0,
> +};
> +
> +static const struct meson_rng_priv meson_rng_priv_s4 = {
> + .check_status_bit = true,
> + .data_offset = 0x8,
> + .cfg_offset = 0x0,
> +};
> +
> static const struct of_device_id meson_rng_of_match[] = {
> - { .compatible = "amlogic,meson-rng", },
> + {
> + .compatible = "amlogic,meson-rng",
> + .data = (void *)&meson_rng_priv,
> + },
> + {
> + .compatible = "amlogic,meson-rng-s4",
> + .data = (void *)&meson_rng_priv_s4,
> + },
> {},
> };
> MODULE_DEVICE_TABLE(of, meson_rng_of_match);
> --
> 2.38.1
Hi Martin,
On Tue, Jul 25, 2023 at 09:59:01PM +0200, Martin Kaiser wrote:
> Alexey Romanov ([email protected]) wrote:
>
> > For some Amlogic SOC's, mechanism to obtain random number
> > has been changed. For example, S4 now uses status bit waiting algo.
>
> > Signed-off-by: Alexey Romanov <[email protected]>
> > ---
> > drivers/char/hw_random/meson-rng.c | 77 ++++++++++++++++++++++++++++--
> > 1 file changed, 74 insertions(+), 3 deletions(-)
>
> > diff --git a/drivers/char/hw_random/meson-rng.c b/drivers/char/hw_random/meson-rng.c
> > index a4eb8e35f13d..c6d7349630a1 100644
> > --- a/drivers/char/hw_random/meson-rng.c
> > +++ b/drivers/char/hw_random/meson-rng.c
> > @@ -14,19 +14,65 @@
> > #include <linux/of.h>
> > #include <linux/clk.h>
>
> > -#define RNG_DATA 0x00
> > +struct meson_rng_priv {
> > + bool check_status_bit;
> > + unsigned int data_offset;
> > + unsigned int cfg_offset;
> > +};
>
> > struct meson_rng_data {
> > void __iomem *base;
> > struct hwrng rng;
> > + struct device *dev;
> > + const struct meson_rng_priv *priv;
> > };
>
> > +#define RUN_BIT 0
> > +#define SEED_READY_STS_BIT 31
> > +#define RETRY_CNT 100
> > +
> > +static int meson_rng_wait_status(void __iomem *cfg_addr, int bit)
> > +{
> > + u32 status;
> > + u32 cnt = 0;
> > +
> > + do {
> > + status = readl_relaxed(cfg_addr) & BIT(bit);
> > + cpu_relax();
> > + } while (status && (cnt++ < RETRY_CNT));
> > +
>
> Could you use readl_relaxed_poll_timeout here instead of open coding the
> loop?
At first I also thought about this API. But later I came to the
conclusion that it is inappropriate here:
1. We can't call rng_read from an atomic context.
2. RNG for me looks like a very lightweight primitive to me that
should work quiclky.
But, now I looked again at the API and realized that we can use
readl_relaxed_poll_timeout_atomic() instead of
readl_relaxed_poll_timeout(). What do you think?
>
> > + if (status)
> > + return -EBUSY;
> > +
> > + return 0;
> > +}
> > +
> > static int meson_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> > {
> > struct meson_rng_data *data =
> > container_of(rng, struct meson_rng_data, rng);
> > + const struct meson_rng_priv *priv = data->priv;
> > +
> > + if (priv->check_status_bit) {
> > + void __iomem *cfg_addr = data->base + priv->cfg_offset;
> > + int err;
> > +
> > + writel_relaxed(readl_relaxed(cfg_addr) | BIT(SEED_READY_STS_BIT), cfg_addr);
>
> > - *(u32 *)buf = readl_relaxed(data->base + RNG_DATA);
> > + err = meson_rng_wait_status(cfg_addr, SEED_READY_STS_BIT);
> > + if (err) {
> > + dev_err(data->dev, "Seed isn't ready, try again\n");
> > + return err;
> > + }
> > +
> > + err = meson_rng_wait_status(cfg_addr, RUN_BIT);
> > + if (err) {
> > + dev_err(data->dev, "Can't get random number, try again\n");
> > + return err;
> > + }
> > + }
> > +
> > + *(u32 *)buf = readl_relaxed(data->base + priv->data_offset);
>
> > return sizeof(u32);
> > }
> > @@ -41,6 +87,10 @@ static int meson_rng_probe(struct platform_device *pdev)
> > if (!data)
> > return -ENOMEM;
>
> > + data->priv = device_get_match_data(&pdev->dev);
> > + if (!data->priv)
> > + return -ENODEV;
> > +
> > data->base = devm_platform_ioremap_resource(pdev, 0);
> > if (IS_ERR(data->base))
> > return PTR_ERR(data->base);
> > @@ -53,11 +103,32 @@ static int meson_rng_probe(struct platform_device *pdev)
> > data->rng.name = pdev->name;
> > data->rng.read = meson_rng_read;
>
> > + data->dev = &pdev->dev;
> > +
> > return devm_hwrng_register(dev, &data->rng);
> > }
>
> > +static const struct meson_rng_priv meson_rng_priv = {
> > + .check_status_bit = false,
> > + .data_offset = 0x0,
> > + .cfg_offset = 0x0,
> > +};
> > +
> > +static const struct meson_rng_priv meson_rng_priv_s4 = {
> > + .check_status_bit = true,
> > + .data_offset = 0x8,
> > + .cfg_offset = 0x0,
> > +};
> > +
> > static const struct of_device_id meson_rng_of_match[] = {
> > - { .compatible = "amlogic,meson-rng", },
> > + {
> > + .compatible = "amlogic,meson-rng",
> > + .data = (void *)&meson_rng_priv,
> > + },
> > + {
> > + .compatible = "amlogic,meson-rng-s4",
> > + .data = (void *)&meson_rng_priv_s4,
> > + },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, meson_rng_of_match);
> > --
> > 2.38.1
>
--
Thank you,
Alexey
Hi Alexey,
Alexey Romanov ([email protected]) wrote:
> > > +static int meson_rng_wait_status(void __iomem *cfg_addr, int bit)
> > > +{
> > > + u32 status;
> > > + u32 cnt = 0;
> > > +
> > > + do {
> > > + status = readl_relaxed(cfg_addr) & BIT(bit);
> > > + cpu_relax();
> > > + } while (status && (cnt++ < RETRY_CNT));
> > > +
> > Could you use readl_relaxed_poll_timeout here instead of open coding the
> > loop?
> At first I also thought about this API. But later I came to the
> conclusion that it is inappropriate here:
> 1. We can't call rng_read from an atomic context.
Agreed. A hwrng read function may sleep (an example for this is
exynos_trng_do_read). But this doesn't prevent us from using
readl_relaxed_poll_timeout.
> 2. RNG for me looks like a very lightweight primitive to me that
> should work quiclky.
> But, now I looked again at the API and realized that we can use
> readl_relaxed_poll_timeout_atomic() instead of
> readl_relaxed_poll_timeout(). What do you think?
Ok, if you know that your rng hardware won't need much time to set the
bit that you're checking, you may use readl_relaxed_poll_timeout_atomic.
stm32_rtc_set_alarm does something similar.
Best regards,
Martin
Hello Alexey,
On Tue, Jul 25, 2023 at 4:13 PM Alexey Romanov <[email protected]> wrote:
[...]
> static int meson_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> {
> struct meson_rng_data *data =
> container_of(rng, struct meson_rng_data, rng);
> + const struct meson_rng_priv *priv = data->priv;
> +
> + if (priv->check_status_bit) {
> + void __iomem *cfg_addr = data->base + priv->cfg_offset;
> + int err;
Have you considered just creating two separate functions:
- meson_rng_read
- meson_s4_rng_read
Then you don't need all of these if/else.
You can even skip the whole offset description in your new struct
meson_rng_priv.
All of this can make the code easier to understand.
> + writel_relaxed(readl_relaxed(cfg_addr) | BIT(SEED_READY_STS_BIT), cfg_addr);
Why not just #define SEED_READY_STS as BIT(...) right away?
Best regards,
Martin