Hi Biju,
On Tue, May 31, 2022 at 03:19:55PM +0100, Biju Das wrote:
> As the resets DT property is mandatory, and is present in all .dtsi
> in mainline, add support to perform deassert/assert using reference
> counted reset handle.
>
> Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> v10->v11:
> * To avoid lock-up on R-Car Gen2, added poll for reset status after deassert.
I didn't look at this earlier because of my preexisting R-b.
It looks to me like this should be moved into the reset driver.
[...]
> @@ -631,13 +634,33 @@ static int __maybe_unused vsp1_pm_runtime_resume(struct device *dev)
> struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> int ret;
>
> + ret = reset_control_deassert(vsp1->rstc);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * On R-Car Gen2, vsp1 register access after deassert can cause
> + * lock-up. Therefore, we need to poll the status of the reset to
> + * avoid lock-up.
> + */
> + ret = read_poll_timeout_atomic(reset_control_status, ret, ret == 0, 1,
> + 100, false, vsp1->rstc);
So the reset driver does not follow the reset API documentation ("After
calling this function, the reset is guaranteed to be deasserted." [1])?
If so, this status polling should be moved into the reset driver.
Also, why use the atomic poll variant here? As far as I can tell, this
driver doesn't call pm_runtime_irq_safe. The reset_control_deassert()
API does not guarantee that the driver implementation doesn't sleep,
either.
[1] https://docs.kernel.org/driver-api/reset.html?highlight=reset_control_deassert#c.reset_control_deassert
[...]
> @@ -825,6 +848,11 @@ static int vsp1_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> + vsp1->rstc = devm_reset_control_get_shared(&pdev->dev, NULL);
> + if (IS_ERR(vsp1->rstc))
> + return dev_err_probe(&pdev->dev, PTR_ERR(vsp1->rstc),
> + "failed to get reset control\n");
> +
What about the other consumers of this shared reset? Don't they need
the status poll you added here as well?
regards
Philipp
