| Message ID | 20230725141252.98848-2-avromanov@sberdevices.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Meson S4 HW RNG Support | expand |
Alexey Romanov (avromanov@sberdevices.ru) 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 <avromanov@sberdevices.ru> > --- > 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 (avromanov@sberdevices.ru) 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 <avromanov@sberdevices.ru> > > --- > > 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 >
Hi Alexey, Alexey Romanov (AVRomanov@sberdevices.ru) 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 <avromanov@sberdevices.ru> 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
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);
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 <avromanov@sberdevices.ru> --- drivers/char/hw_random/meson-rng.c | 77 ++++++++++++++++++++++++++++-- 1 file changed, 74 insertions(+), 3 deletions(-)