Hi Biju,
On Tue, May 17, 2022 at 3:36 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > Subject: Re: [PATCH] clk: renesas: cpg-mssr: Add a delay after deassert
> > On Thu, May 5, 2022 at 12:01 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > wrote:
> > > > Subject: Re: [PATCH] clk: renesas: cpg-mssr: Add a delay after
> > > > deassert On Wed, May 4, 2022 at 8:44 PM Biju Das
> > <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > > After adding reset support to vsp, it needs a delay of 32
> > > > > microseconds after reset operation, otherwise system hangs(due to
> > > > > register
> > > > read/write).
> > > > > This patch fixes the system hang issue by adding delay after
> > > > > deassert operation.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > >
> > > > > After adding reset/deassert support for vsp based on [1], RZ/G1N
> > > > > board hangs. On debugging it found that it needs a delay of 35
> > > > > microseconds after deasserint reset. Wthout delay if there is any
> > > > > register read/write will lead to hang.
> > > > >
> > > > > This 35 microseconds value is picked from the reset().
> > > >
> > > > The 35 µs comes from the Hardware User's Manual: there should be at
> > > > least 1 RCLK cycle _in between_ asserting and deasserting reset.
> > > > The manual doesn't say anything about delays _after_ deasserting
> > reset.
> > > >
> > > > Could it be that the VSP1 driver is actually deasserting reset too
> > early?
> > >
> > > My test results on RZ/G1N shows, it needs 35 micro seconds after
> > deasserting reset.
> >
> > I can confirm that accessing the VSP registers without the delay causes a
> > lock-up on R-Car M2-W, too.
> > I see no such lock-up on R-Car Gen3, but I cannot rule out that it is
> > mitigated by a handler in secure mode, and that VSP initialization may
> > actually fail (accessing registers of non-clocked modules usually causes
> > an imprecise external abort, which is caught by Linux on R-Car Gen2, but
> > turned into a no-op by secure firmware on R-Car Gen3).
> >
> > Instead of adding the explicit delay, I tried added a polling loop after
> > the call to reset_control_deassert() in the vsp1 driver, to wait until the
> > reset is cleared, like is done in the i2c-rcar driver:
> >
> > ret = read_poll_timeout_atomic(reset_control_status, ret, ret ==
> > 0, 1,
> > 100, false, vsp1->rstc);
> > if (ret < 0) {
> > ...
> > }
> >
> > This also fixes the issue for me.
>
> Yes, It is better fix than the explicit delay.
>
> > Adding more debug code shows that reset_control_status() is called only
> > once (both for i2c and vsp1), so the polling completes before any call to
> > udelay().
>
> >
> > Note that at that time[1], we added the delay to the i2c-rcar driver
> > instead of the CPG/MSSR driver, as we were told that i2c reset was
> > special, and other modules do not need this.
> > Perhaps vsp reset is special, too?
> > Or perhaps it is time to revisit this, and add the polling to both
> > cpg_mssr_reset() and cpg_mssr_deassert(), so it can be removed from the
> > drivers?
>
> I feel adding poll in VSP driver is better compared to cpg driver, as
> it reduces wakeup time after "Suspend to RAM" operation.
>
> How do we proceed here? Do you want me to add as part of RZ/G2L patch series? Or you will post separately.
Feel free to incorporate it into v11 of "[2/5] media: renesas: vsp1:
Add support to deassert/assert reset line".
Thanks!
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
