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

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

Hello,

On 2025-10-06 14:23:42 +0200, Niklas Söderlund wrote:
> On 2025-10-06 13:53:34 +0200, Geert Uytterhoeven wrote:
> > Hi Niklas,
> > 
> > On Fri, 3 Oct 2025 at 17:08, Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > > 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>
> > 
> > > > --- 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.
> > 
> > Yeah, AFAIK we didn't have any callers of reset_control_assert() from
> > atomic context, but I was already afraid one was going to pop up...
> > 
> > > 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.
> > >
> > > [   29.256947] BUG: scheduling while atomic: yavta/597/0x00000002
> > 
> > > [   29.265595]  reset_control_reset+0x4c/0x160
> > > [   29.265604]  risp_core_start_streaming+0x100/0x440
> > > [   29.265609]  risp_io_start_streaming+0x74/0x108
> > 
> > The existing udelay(2000) after the call to reset_control_reset() is
> > also a bit gross.
> 
> Haha, I agree :-)
> 
> > I understand you are using a spinlock because you
> > need to synchronize with an interrupt handler.  Would converting to a
> > threaded interrupt handler and using a mutex (which the code already
> > uses) instead be an option?
> 
> Yes and no. For the current use-case where the ISP is used in off-line 
> mode, that is userspace dequeue images from VIN and then queues them to 
> the ISP, it could work. But if we ever want to support the ISP in 
> in-line mode, that is the CSI-2 Rx queues the frames directly to the ISP 
> I think a threaded interrupt handler would be to slow to change ISP 
> parameters between each frame.
> 
> But that can also be a future problem, I will see what I can do.

With a bit of work I have reworked the ISP driver to reset the core from 
a context that can sleep. Without the need for a threaded interrupt 
handler. Thanks for all the tips shared on IRC Geert and Marek.

I now have no known case where usleep_range() in cpg_mssr_reset() causes 
issues. If I trip any more I will let you guys know.

> 
> > 
> > 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
> 
> -- 
> Kind Regards,
> Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund