Hi Geert,
Thanks for the feedback.
> Subject: Re: [PATCH] clk: renesas: cpg-mssr: Add a delay after deassert
>
> Hi Biju,
>
> 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>
>
> Thanks for your patch!
>
> > 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.
Logs with just adding delay in assert()
------------------------------------
[ 1.938991] renesas-cpg-mssr e6150000.clock-controller: deassert 131
[ 1.945373] renesas-cpg-mssr e6150000.clock-controller: assert 131
[ 1.956445] renesas-cpg-mssr e6150000.clock-controller: deassert 131
It hangs after this. After this, there is a read register call which hangs the system.
Looks like after deassert, it needs some delay for accessing registres.
Logs with just adding delay in deassert()
------------------------------------
[ 12.536298] renesas-cpg-mssr e6150000.clock-controller: deassert 131
[ 12.563577] renesas-cpg-mssr e6150000.clock-controller: assert 131
[ 12.571630] renesas-cpg-mssr e6150000.clock-controller: deassert 131
[ 12.578412] renesas-cpg-mssr e6150000.clock-controller: assert 131
[ 12.584702] renesas-cpg-mssr e6150000.clock-controller: deassert 131
[ 12.609875] renesas-cpg-mssr e6150000.clock-controller: assert 131
[ 12.618666] renesas-cpg-mssr e6150000.clock-controller: deassert 131
[ 12.631736] renesas-cpg-mssr e6150000.clock-controller: assert 131
[ 12.679240] renesas-cpg-mssr e6150000.clock-controller: deassert 128
[ 12.689712] renesas-cpg-mssr e6150000.clock-controller: assert 128
[ 12.697249] renesas-cpg-mssr e6150000.clock-controller: deassert 128
[ 12.712796] renesas-cpg-mssr e6150000.clock-controller: assert 128
[ 12.755191] renesas-cpg-mssr e6150000.clock-controller: deassert 127
[ 12.762389] renesas-cpg-mssr e6150000.clock-controller: assert 127
[ 12.775882] renesas-cpg-mssr e6150000.clock-controller: deassert 127
[ 12.791329] renesas-cpg-mssr e6150000.clock-controller: assert 127
It works Ok.
>
> > --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > @@ -637,6 +637,7 @@ static int cpg_mssr_assert(struct
> reset_controller_dev *rcdev, unsigned long id)
> > dev_dbg(priv->dev, "assert %u%02u\n", reg, bit);
> >
> > writel(bitmask, priv->base + priv->reset_regs[reg]);
> > +
>
> Unrelated change.
Oops, Added a debug message there. Will take out this.
Cheers,
Biju
>
> > return 0;
> > }
> >
> > @@ -651,6 +652,10 @@ static int cpg_mssr_deassert(struct
> reset_controller_dev *rcdev,
> > dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit);
> >
> > writel(bitmask, priv->base + priv->reset_clear_regs[reg]);
> > +
> > + /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz)
> */
> > + udelay(35);
> > +
> > return 0;
> > }
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
>
> 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
