| Message ID | 20220504184406.93788-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | clk: renesas: cpg-mssr: Add a delay after deassert | expand |
Hi Biju, On Wed, May 4, 2022 at 8:44 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> 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? > --- 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. > 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
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@bp.renesas.com> 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@bp.renesas.com> > > 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
Adding initial logs with just delay in deassert. > Subject: RE: [PATCH] clk: renesas: cpg-mssr: Add a delay after deassert > > 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@bp.renesas.com> > 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@bp.renesas.com> > > > > 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() > ------------------------------------ [ 1.938822] renesas-cpg-mssr e6150000.clock-controller: deassert 131 [ 1.945239] renesas-cpg-mssr e6150000.clock-controller: assert 131 [ 1.956273] renesas-cpg-mssr e6150000.clock-controller: deassert 131 [ 1.962874] renesas-cpg-mssr e6150000.clock-controller: deassert 128 [ 1.969271] renesas-cpg-mssr e6150000.clock-controller: assert 128 [ 1.975659] renesas-cpg-mssr e6150000.clock-controller: assert 131 [ 1.983799] renesas-cpg-mssr e6150000.clock-controller: deassert 128 [ 1.990344] renesas-cpg-mssr e6150000.clock-controller: deassert 127 [ 1.996760] renesas-cpg-mssr e6150000.clock-controller: assert 127 [ 2.003106] renesas-cpg-mssr e6150000.clock-controller: assert 128 [ 2.011224] renesas-cpg-mssr e6150000.clock-controller: deassert 127 [ 2.018421] renesas-cpg-mssr e6150000.clock-controller: assert 127 > [ 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
Hi Biju, On Thu, May 5, 2022 at 12:01 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> 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@bp.renesas.com> > > > > > 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. 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? [1] Commit 3b770017b03a4cdf ("i2c: rcar: handle RXDMA HW behaviour on Gen3"). 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
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@bp.renesas.com> > 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@bp.renesas.com> 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@bp.renesas.com> > > > > > > > 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
Hi Biju, On Tue, May 17, 2022 at 3:36 PM Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> > > 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@bp.renesas.com> 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@bp.renesas.com> > > > > > > > > > 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@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
diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c index 5d2c3edbaa14..025a75a3484c 100644 --- 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]); + 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; }
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@bp.renesas.com> --- Hi All, 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(). [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220428065333.3108-3-biju.das.jz@bp.renesas.com/ --- drivers/clk/renesas/renesas-cpg-mssr.c | 5 +++++ 1 file changed, 5 insertions(+)