Re: [PATCH v3 2/3] hwrng: add Rockchip SoC hwrng driver

[Chen-Yu Tsai <wens@kernel.org>]

On Fri, Jun 21, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 21/06/2024 03:25, Daniel Golle wrote:
> > From: Aurelien Jarno <aurelien@aurel32.net>
> >
>
> > +
> > +static int rk_rng_init(struct hwrng *rng)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +     int ret;
> > +
> > +     /* start clocks */
> > +     ret = clk_bulk_prepare_enable(rk_rng->clk_num, rk_rng->clk_bulks);
> > +     if (ret < 0) {
> > +             dev_err((struct device *) rk_rng->rng.priv,
> > +                     "Failed to enable clks %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     /* set the sample period */
> > +     writel(RK_RNG_SAMPLE_CNT, rk_rng->base + TRNG_RNG_SAMPLE_CNT);
> > +
> > +     /* set osc ring speed and enable it */
> > +     writel_relaxed(TRNG_RNG_CTL_LEN_256_BIT |
> > +                    TRNG_RNG_CTL_OSC_RING_SPEED_0 |
> > +                    TRNG_RNG_CTL_ENABLE,
> > +                    rk_rng->base + TRNG_RNG_CTL);
>
> I doubt relaxed write is here intentional. Enabling should be last
> instruction, so this should be ordered write.

I agree that the driver should just do all non-relaxed writes for simplicity.
The penalty isn't that severe since commit 22ec71615d82 ("arm64: io: Relax
implicit barriers in default I/O accessors").

Just to clarify, writes to devices are always ordered. The non-relaxed
writes are ordered to _memory writes_, which doesn't really matter for
this driver.

ChenYu


> > +
> > +     return 0;
> > +}
> > +
> > +static void rk_rng_cleanup(struct hwrng *rng)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +
> > +     /* stop TRNG */
> > +     writel_relaxed(0, rk_rng->base + TRNG_RNG_CTL);
>
> This should not be relaxed. This might lead to very tricky to debug issues.
>
> > +
> > +     /* stop clocks */
> > +     clk_bulk_disable_unprepare(rk_rng->clk_num, rk_rng->clk_bulks);
> > +}
> > +
> > +static int rk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
> > +{
> > +     struct rk_rng *rk_rng = container_of(rng, struct rk_rng, rng);
> > +     size_t to_read = min_t(size_t, max, RK_RNG_MAX_BYTE);
> > +     u32 reg;
> > +     int ret = 0;
> > +
> > +     ret = pm_runtime_get_sync((struct device *) rk_rng->rng.priv);
>
> Why this cannot be just simpler pm_runtime_resume_and_get()?
>
> > +     if (ret < 0)
> > +             goto out;
>
> This does not look like correct error path. Device was not busy here.
>
> > +
> > +     /* Start collecting random data */
> > +     writel_relaxed(TRNG_RNG_CTL_START, rk_rng->base + TRNG_RNG_CTL);
> > +
> > +     ret = readl_poll_timeout(rk_rng->base + TRNG_RNG_CTL, reg,
> > +                              !(reg & TRNG_RNG_CTL_START),
> > +                              RK_RNG_POLL_PERIOD_US,
> > +                              RK_RNG_POLL_TIMEOUT_US);
> > +     if (ret < 0)
> > +             goto out;
> > +
> > +     /* Read random data stored in the registers */
> > +     memcpy_fromio(buf, rk_rng->base + TRNG_RNG_DOUT, to_read);
> > +out:
> > +     pm_runtime_mark_last_busy((struct device *) rk_rng->rng.priv);
> > +     pm_runtime_put_sync_autosuspend((struct device *) rk_rng->rng.priv);
> > +
> > +     return to_read;
> > +}
> > +
> > +static int rk_rng_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct rk_rng *rk_rng;
> > +     int ret;
> > +
> > +     rk_rng = devm_kzalloc(dev, sizeof(*rk_rng), GFP_KERNEL);
> > +     if (!rk_rng)
> > +             return -ENOMEM;
> > +
> > +     rk_rng->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(rk_rng->base))
> > +             return PTR_ERR(rk_rng->base);
> > +
> > +     rk_rng->clk_num = devm_clk_bulk_get_all(dev, &rk_rng->clk_bulks);
> > +     if (rk_rng->clk_num < 0)
> > +             return dev_err_probe(dev, rk_rng->clk_num,
> > +                                  "Failed to get clks property\n");
> > +
> > +     rk_rng->rst = devm_reset_control_array_get(&pdev->dev, false, false);
> > +     if (IS_ERR(rk_rng->rst))
> > +             return dev_err_probe(dev, PTR_ERR(rk_rng->rst),
> > +                                  "Failed to get reset property\n");
> > +
> > +     reset_control_assert(rk_rng->rst);
> > +     udelay(2);
> > +     reset_control_deassert(rk_rng->rst);
> > +
> > +     platform_set_drvdata(pdev, rk_rng);
> > +
> > +     rk_rng->rng.name = dev_driver_string(dev);
> > +#ifndef CONFIG_PM
> > +     rk_rng->rng.init = rk_rng_init;
> > +     rk_rng->rng.cleanup = rk_rng_cleanup;
> > +#endif
> > +     rk_rng->rng.read = rk_rng_read;
> > +     rk_rng->rng.priv = (unsigned long) dev;
> > +     rk_rng->rng.quality = 900;
> > +
> > +     pm_runtime_set_autosuspend_delay(dev, RK_RNG_AUTOSUSPEND_DELAY);
> > +     pm_runtime_use_autosuspend(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     ret = devm_hwrng_register(dev, &rk_rng->rng);
> > +     if (ret)
> > +             return dev_err_probe(&pdev->dev, ret, "Failed to register Rockchip hwrng\n");
> > +
> > +     dev_info(&pdev->dev, "Registered Rockchip hwrng\n");
>
> Drop, driver should be silent on success.
>
>
> Best regards,
> Krzysztof
>
>