On Fri, Jun 21, 2024 at 5:58 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
>
> On 21/06/2024 03:25, Daniel Golle wrote:
> > From: Aurelien Jarno <aurelien@xxxxxxxxxxx>
> >
>
> > +
> > +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
>
>
