[PATCH 4/5] rng: Add Exynos TRNG driver

[Sam Protsenko]

On Sat, Jul 13, 2024 at 10:13 AM Simon Glass <sjg at chromium.org> wrote:
>
> Hi Sam,
>

Hi Simon,

Thank you for the review!

> On Sat, 13 Jul 2024 at 00:44, Sam Protsenko <semen.protsenko at linaro.org> wrote:
> > +config RNG_EXYNOS
> > +       bool "Samsung Exynos True Random Number Generator support"
> > +       depends on DM_RNG
> > +       help
> > +         Enable support for True Random Number Generator (TRNG)
> > +         available in Exynos SoCs.
>
> Can you mention the needed firmware here?
>

Will do in v2.

> > +
> > +struct exynos_trng_variant {
>
> please add comments
>

Will do in v2.

> > +       bool smc;
> > +       int (*init)(struct udevice *dev);
> > +       void (*exit)(struct udevice *dev);
> > +       int (*read)(struct udevice *dev, void *data, size_t len);
> > +};
> > +
> > +struct exynos_trng {
>
> The convention is to add a _priv suffix
>

Will add in v2.

> > +static int exynos_trng_init_smc(struct udevice *dev)
> > +{
> > +       struct arm_smccc_res res;
> > +       int ret = 0;
> > +
> > +       arm_smccc_smc(SMC_CMD_RANDOM, HWRNG_INIT, 0, 0, 0, 0, 0, 0, &res);
> > +       if (res.a0 != HWRNG_RET_OK) {
> > +               dev_err(dev, "SMC command for TRNG init failed (%d)\n",
> > +                       (int)res.a0);
> > +               ret = -EIO;
> > +       }
> > +       if ((int)res.a0 == -1)
> > +               dev_info(dev, "Make sure LDFW is loaded\n");
>
> return error here?
>

It's already done above: (res.a0 != HWRNG_RET_OK) condition includes
-1 case. This line is basically only to help user figure out what
might be wrong.

> > +static int exynos_trng_of_to_plat(struct udevice *dev)
> > +{
> > +       struct exynos_trng *trng = dev_get_priv(dev);
> > +
> > +       trng->data = (struct exynos_trng_variant *)dev_get_driver_data(dev);
> > +       if (!trng->data->smc) {
> > +               trng->base = dev_read_addr_ptr(dev);
> > +               if (!trng->base)
> > +                       return -ENODEV;
>
> That has a special meaning in U-Boot (i.e. that there is no device).
> Devicetree problems should return -EINVAL
>

Will fix in v2.

> > +       }
> > +
> > +       trng->clk = devm_clk_get(dev, "secss");
> > +       if (IS_ERR(trng->clk))
> > +               return -ENODEV;
>
> Should return the actual error
>

Will fix in v2.

> > +
> > +       trng->pclk = devm_clk_get_optional(dev, "pclk");
> > +       if (IS_ERR(trng->pclk))
> > +               return -ENODEV;
>
> same here
>

Will fix in v2.

> > +
> > +       return 0;
> > +}
> > +
> > +static int exynos_trng_probe(struct udevice *dev)
> > +{
> > +       struct exynos_trng *trng = dev_get_priv(dev);
> > +       int err;
>
> s/err/ret/
>

Will fix in v2.

[snip]

>
> Regards,
> Simon