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. > + > + 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