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
