Hi Biju,
On Wed, Jul 13, 2022 at 11:18 AM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to
> > deassert/assert reset line
> >
> > 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.
>
> OK, sorry, I should have removed Rb tag while sending this patch.
>
> > [...]
> > > @@ -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.
> >
>
> Sure, will move it to reset driver. Geert also suggested same thing[1]
Actually I suggested handling this in the VSP driver, as VSP seems
to be "special".
>
> [1]
> https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@xxxxxxxxxxxxxx/
>
>
> > 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.
>
> As per [1], I2C driver uses atomic one, so just used the same here.
>
> OK, will use non atomic variant in deassert().
>
> Do you recommend to fix the reset as well as per [1]?
>
> >
> > [...]
> > > @@ -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?
>
> This lockup issue happens only on Gen2 SoC's. Gen3 SoC's are not affected.
We are not sure about that. On R-Car Gen3, accesses to registers
while a device is not clocked/ready usually do not cause an imprecise
external abort in Linux, unlike on R-Car Gen2. But perhaps the
abort is caught by the firmware, and nullified?
> RZ/G2L SoC is Gen3 variant, and it is the only consumer for shared reset as reset lines are shared between DU and VSPD. Other SoC's have explicit reset for VSP.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
