Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH] clk: renesas: cpg-mssr: Add a delay after deassert
>
> Hi Biju,
>
> 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.
Please let me know.
Cheers,
Biju
>
> [1] Commit 3b770017b03a4cdf ("i2c: rcar: handle RXDMA HW behaviour on
> Gen3").
Adding
