Re: [PATCH] clk: renesas: cpg-mssr: Add missing 1ms delay into reset toggle callback

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hello Marek,

On 2025-10-05 15:17:02 +0200, Marek Vasut wrote:
> On 10/5/25 9:12 AM, Niklas Söderlund wrote:
> 
> Hello Niklas,
> 
> > On 2025-10-05 06:00:15 +0200, Marek Vasut wrote:
> > > On 10/3/25 5:08 PM, Niklas Söderlund wrote:
> > > 
> > > Hello Niklas,
> > > 
> > > > On 2025-09-18 05:04:43 +0200, Marek Vasut wrote:
> > > > > R-Car V4H Reference Manual R19UH0186EJ0130 Rev.1.30 Apr. 21, 2025 page 583
> > > > > Figure 9.3.1(a) Software Reset flow (A) as well as flow (B) / (C) indicate
> > > > > after reset has been asserted by writing a matching reset bit into register
> > > > > SRCR, it is mandatory to wait 1ms.
> > > > > 
> > > > > This 1ms delay is documented on R-Car V4H and V4M, it is currently unclear
> > > > > whether S4 is affected as well. This patch does apply the extra delay on
> > > > > R-Car S4 as well.
> > > > > 
> > > > > Fix the reset driver to respect the additional delay when toggling resets.
> > > > > Drivers which use separate reset_control_(de)assert() must assure matching
> > > > > delay in their driver code.
> > > > > 
> > > > > Fixes: 0ab55cf18341 ("clk: renesas: cpg-mssr: Add support for R-Car V4H")
> > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > > > > ---
> > > > > Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > > > Cc: linux-clk@vger.kernel.org
> > > > > Cc: linux-renesas-soc@vger.kernel.org
> > > > > ---
> > > > >    drivers/clk/renesas/renesas-cpg-mssr.c | 11 +++++++++--
> > > > >    1 file changed, 9 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> > > > > index be9f59e6975d..65dfaceea71f 100644
> > > > > --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> > > > > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> > > > > @@ -689,8 +689,15 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
> > > > >    	/* Reset module */
> > > > >    	writel(bitmask, priv->pub.base0 + priv->reset_regs[reg]);
> > > > > -	/* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */
> > > > > -	udelay(35);
> > > > > +	/*
> > > > > +	 * On R-Car Gen4, delay after SRCR has been written is 1ms.
> > > > > +	 * On older SoCs, delay after SRCR has been written is 35us
> > > > > +	 * (one cycle of the RCLK clock @ cca. 32 kHz).
> > > > > +	 */
> > > > > +	if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4)
> > > > > +		usleep_range(1000, 2000);
> > > > > +	else
> > > > > +		usleep_range(35, 1000);
> > > > 
> > > > I rebased the R-Car ISP work to renesas-drivers today and it included
> > > > this change, and I seem to have hit an issue with the switch form
> > > > udelay() to usleep_range() I'm afraid. I can't find any other good
> > > > reproducer of the issue however.
> > > > 
> > > > THe core of the issue seems to be that if a reset is issued from an
> > > > atomic context bad things happen if you try to sleep. I get this splat
> > > > and the board is completer dead after it, needing a power cycle to
> > > > recover.
> > > > 
> > > > If I revert this patch things work as expected.
> > > Thank you for testing. Does it work well if you replace those
> > > usleep_range()s with plain udelay() ?
> > 
> > With this change it do work. I'm testing on V4H so I assume it's the
> > Gen4 branch that is taken here?
> Yes it is.
> 
> I sent a V2 of this with udelay(), thank you for testing.

Thanks!

> 
> I do wonder, would it be better if risp used reset_assert()/reset_deassert()
> when performing reset in atomic context ? Also, why is it even performing
> reset in atomic context ?

The ISP driver needs to serialize a set of buffer queues when it want to 
consume from them. This happens at two locations, start and interrupt 
context.

As this was not an issue before a spinlock have been used to marshal 
this. However at start time, as the spinlock is taken anyhow, it have 
also been used to protect against multiple starts that would call reset.

-- 
Kind Regards,
Niklas Söderlund