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

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

[Marek Vasut]

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);
 
 	/* Release module from reset state */
 	writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
-- 
2.51.0

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

[Geert Uytterhoeven]

Hi Marek,

On Thu, 18 Sept 2025 at 05:06, Marek Vasut
<marek.vasut+renesas@mailbox.org> 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>

Thanks for your patch!

> --- 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).

s/cca/ca/ (I can fix that myself).

> +        */
> +       if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4)
> +               usleep_range(1000, 2000);
> +       else
> +               usleep_range(35, 1000);
>
>         /* Release module from reset state */
>         writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);

LGTM, but wait for more feedback?
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

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

[Marek Vasut]

On 9/22/25 1:37 PM, Geert Uytterhoeven wrote:

Hello Geert,

>> --- 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).
> 
> s/cca/ca/ (I can fix that myself).

cca - circa , it seems all of c., ca., ca, cca are valid.

>> +        */
>> +       if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4)
>> +               usleep_range(1000, 2000);
>> +       else
>> +               usleep_range(35, 1000);
>>
>>          /* Release module from reset state */
>>          writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
> 
> LGTM, but wait for more feedback?
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Thank you. I wait.

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

[Geert Uytterhoeven]

Hi Marek,

CC shimoda-san

On Thu, 18 Sept 2025 at 05:06, Marek Vasut
<marek.vasut+renesas@mailbox.org> 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.

Given
https://elixir.bootlin.com/linux/v6.17/source/drivers/phy/renesas/r8a779f0-ether-serdes.c#L219
I assume R-Car S4 is affected, too.

> 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>

Thanks, queuing tentatively in renesas-clk for v6.19.

> --- 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);
>
>         /* Release module from reset state */
>         writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);

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

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

[Niklas Söderlund]

Hi Marek,

Thanks for your work.

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.

[   29.256947] BUG: scheduling while atomic: yavta/597/0x00000002
[   29.257783] 2 locks held by yavta/597:
[   29.258268]  #0: ffff000442a66418 (&io->lock){+.+.}-{4:4}, at: __video_do_ioctl+0xdc/0x3f0
[   29.259356]  #1: ffff000442a66e18 (&core->lock){....}-{3:3}, at: risp_core_start_streaming+0xec/0x440
[   29.260555] irq event stamp: 3916
[   29.260983] hardirqs last  enabled at (3915): [<ffff800080fb4e04>] _raw_spin_unlock_irqrestore+0x64/0x68
[   29.262205] hardirqs last disabled at (3916): [<ffff800080fb43d8>] _raw_spin_lock_irqsave+0x78/0x80
[   29.263366] softirqs last  enabled at (3848): [<ffff8000800c2478>] handle_softirqs+0x44c/0x4a0
[   29.264476] softirqs last disabled at (3805): [<ffff800080010270>] __do_softirq+0x10/0x18
[   29.265529] CPU: 0 UID: 0 PID: 597 Comm: yavta Not tainted 6.17.0-arm64-renesas-09711-g5173b0d6549f #52 PREEMPT
[   29.265536] Hardware name: Retronix Sparrow Hawk board based on r8a779g3 (DT)
[   29.265540] Call trace:
[   29.265542]  show_stack+0x14/0x1c (C)
[   29.265551]  dump_stack_lvl+0x6c/0x90
[   29.265558]  dump_stack+0x14/0x1c
[   29.265563]  __schedule_bug+0x64/0x78
[   29.265570]  __schedule+0xcfc/0xf90
[   29.265574]  schedule+0x48/0x154
[   29.265577]  schedule_hrtimeout_range_clock+0xd8/0x120
[   29.265582]  usleep_range_state+0x84/0xe0
[   29.265587]  cpg_mssr_reset+0xd8/0xdc
[   29.265595]  reset_control_reset+0x4c/0x160
[   29.265604]  risp_core_start_streaming+0x100/0x440
[   29.265609]  risp_io_start_streaming+0x74/0x108
[   29.265614]  vb2_start_streaming+0x64/0x168
[   29.265618]  vb2_core_streamon+0xd0/0x1b8
[   29.265621]  vb2_ioctl_streamon+0x50/0x8c
[   29.265625]  v4l_streamon+0x20/0x28
[   29.265631]  __video_do_ioctl+0x344/0x3f0
[   29.265635]  video_usercopy+0x2e4/0x870
[   29.265639]  video_ioctl2+0x14/0x20
[   29.265643]  v4l2_ioctl+0x3c/0x60
[   29.265646]  __arm64_sys_ioctl+0x88/0xe0
[   29.265653]  invoke_syscall.constprop.0+0x3c/0xe4
[   29.265659]  el0_svc_common.constprop.0+0x34/0xcc
[   29.265663]  do_el0_svc+0x18/0x20
[   29.265667]  el0_svc+0x3c/0x2a0
[   29.265673]  el0t_64_sync_handler+0x98/0xe0
[   29.265678]  el0t_64_sync+0x154/0x158
[   29.268217] BUG: spinlock wrong CPU on CPU#1, yavta/597
[   29.282715]  lock: 0xffff000442a66e00, .magic: dead4ead, .owner: yavta/597, .owner_cpu: 0
[   29.283783] CPU: 1 UID: 0 PID: 597 Comm: yavta Tainted: G        W           6.17.0-arm64-renesas-09711-g5173b0d6549f #52 PREEMPT
[   29.283790] Tainted: [W]=WARN
[   29.283793] Hardware name: Retronix Sparrow Hawk board based on r8a779g3 (DT)
[   29.283795] Call trace:
[   29.283797]  show_stack+0x14/0x1c (C)
[   29.283812]  dump_stack_lvl+0x6c/0x90
[   29.283821]  dump_stack+0x14/0x1c
[   29.283825]  spin_dump+0x74/0x80
[   29.283831]  do_raw_spin_unlock+0xfc/0x104
[   29.283839]  _raw_spin_unlock_irqrestore+0x2c/0x68
[   29.283848]  risp_core_start_streaming+0x30c/0x440
[   29.283858]  risp_io_start_streaming+0x74/0x108
[   29.283863]  vb2_start_streaming+0x64/0x168
[   29.283867]  vb2_core_streamon+0xd0/0x1b8
[   29.283869]  vb2_ioctl_streamon+0x50/0x8c
[   29.283873]  v4l_streamon+0x20/0x28
[   29.283879]  __video_do_ioctl+0x344/0x3f0
[   29.283885]  video_usercopy+0x2e4/0x870
[   29.283889]  video_ioctl2+0x14/0x20
[   29.283892]  v4l2_ioctl+0x3c/0x60
[   29.283895]  __arm64_sys_ioctl+0x88/0xe0
[   29.283903]  invoke_syscall.constprop.0+0x3c/0xe4
[   29.283908]  el0_svc_common.constprop.0+0x34/0xcc
[   29.283912]  do_el0_svc+0x18/0x20
[   29.283915]  el0_svc+0x3c/0x2a0
[   29.283922]  el0t_64_sync_handler+0x98/0xe0
[   29.283926]  el0t_64_sync+0x154/0x158
[   29.283952] ------------[ cut here ]------------
[   29.298904] WARNING: CPU: 1 PID: 597 at kernel/rcu/srcutree.c:732 __srcu_check_read_flavor+0x50/0xe0
[   29.300062] CPU: 1 UID: 0 PID: 597 Comm: yavta Tainted: G        W           6.17.0-arm64-renesas-09711-g5173b0d6549f #52 PREEMPT
[   29.301538] Tainted: [W]=WARN
[   29.301913] Hardware name: Retronix Sparrow Hawk board based on r8a779g3 (DT)
[   29.302810] pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   29.303689] pc : __srcu_check_read_flavor+0x50/0xe0
[   29.304307] lr : device_links_read_lock+0x20/0x60
[   29.304904] sp : ffff800083c438a0
[   29.305322] x29: ffff800083c438a0 x28: 0000000000000001 x27: 0000000000000001
[   29.306225] x26: 0000000000000000 x25: ffff800081751db8 x24: ffff800080123160
[   29.307125] x23: 0000000000000002 x22: ffff800080726a90 x21: ffff000440c56968
[   29.308026] x20: ffff800080726a90 x19: ffff8000818bc240 x18: 0000000000000010
[   29.308926] x17: 202c3739352f6174 x16: 766179203a72656e x15: 0000000000000020
[   29.309826] x14: 0000000000000000 x13: 00000000ffff1060 x12: 00000000fffffffd
[   29.310726] x11: 0000000000000058 x10: 0000000000000018 x9 : ffff800081777de0
[   29.311626] x8 : ffff800083c437b0 x7 : ffff800083c438c0 x6 : ffff800083c43870
[   29.312526] x5 : ffff800083c437f0 x4 : ffff800083c44000 x3 : 0000000000000000
[   29.313426] x2 : 00000000ffffffff x1 : 0000000000000001 x0 : ffff8000818bc470
[   29.314326] Call trace:
[   29.314636]  __srcu_check_read_flavor+0x50/0xe0 (P)
[   29.315254]  device_links_read_lock+0x20/0x60
[   29.315805]  __rpm_callback+0x14c/0x210
[   29.316294]  rpm_callback+0x6c/0x78
[   29.316737]  rpm_resume+0x50c/0x718
[   29.317179]  __pm_runtime_resume+0x48/0x88
[   29.317698]  vsp1_device_get+0x1c/0x84
[   29.318175]  vsp1_isp_start_streaming+0x7c/0x198
[   29.318761]  risp_core_start_streaming+0x318/0x440
[   29.319368]  risp_io_start_streaming+0x74/0x108
[   29.319942]  vb2_start_streaming+0x64/0x168
[   29.320470]  vb2_core_streamon+0xd0/0x1b8
[   29.320976]  vb2_ioctl_streamon+0x50/0x8c
[   29.321484]  v4l_streamon+0x20/0x28
[   29.321925]  __video_do_ioctl+0x344/0x3f0
[   29.322433]  video_usercopy+0x2e4/0x870
[   29.322920]  video_ioctl2+0x14/0x20
[   29.323362]  v4l2_ioctl+0x3c/0x60
[   29.323781]  __arm64_sys_ioctl+0x88/0xe0
[   29.324279]  invoke_syscall.constprop.0+0x3c/0xe4
[   29.324875]  el0_svc_common.constprop.0+0x34/0xcc
[   29.325469]  do_el0_svc+0x18/0x20
[   29.325890]  el0_svc+0x3c/0x2a0
[   29.326289]  el0t_64_sync_handler+0x98/0xe0
[   29.326819]  el0t_64_sync+0x154/0x158
[   29.327283] irq event stamp: 3935
[   29.327702] hardirqs last  enabled at (3935): [<ffff800080fa655c>] irqentry_exit+0x3c/0x180
[   29.328755] hardirqs last disabled at (3934): [<ffff800080fab110>] preempt_schedule_irq+0x70/0xa0
[   29.329872] softirqs last  enabled at (3930): [<ffff8000800c2478>] handle_softirqs+0x44c/0x4a0
[   29.330958] softirqs last disabled at (3919): [<ffff800080010270>] __do_softirq+0x10/0x18
[   29.331988] ---[ end trace 0000000000000000 ]---
[   29.332579] ------------[ cut here ]------------
[   29.333163] WARNING: CPU: 1 PID: 597 at kernel/irq/irqdesc.c:666 handle_irq_desc+0x3c/0x58
[   29.334207] CPU: 1 UID: 0 PID: 597 Comm: yavta Tainted: G        W           6.17.0-arm64-renesas-09711-g5173b0d6549f #52 PREEMPT
[   29.335683] Tainted: [W]=WARN
[   29.336058] Hardware name: Retronix Sparrow Hawk board based on r8a779g3 (DT)
[   29.336954] pstate: 404000c9 (nZcv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   29.337831] pc : handle_irq_desc+0x3c/0x58
[   29.338348] lr : generic_handle_domain_irq+0x18/0x20
[   29.338975] sp : ffff8000824d3fd0
[   29.339393] x29: ffff8000824d3fd0 x28: ffff000443a9cf80 x27: 0000000000000001
[   29.340293] x26: 0000000000000000 x25: ffff800081751db8 x24: ffff800080123160
[   29.341193] x23: 0000000000400009 x22: ffff80008016c294 x21: ffff800083c438a0
[   29.342093] x20: 0000000000000000 x19: ffff800081684cd8 x18: 0000000000000010
[   29.342993] x17: ffff80063d8b5000 x16: ffff8000824d0000 x15: 0000000000000020
[   29.343893] x14: 0000000000000000 x13: 00000000ffff1060 x12: 00000000fffffffd
[   29.344793] x11: 0000000000000058 x10: 0000000000000018 x9 : 0000000000000040
[   29.345692] x8 : ffff000440013918 x7 : 0000000000000000 x6 : ffff0004404004b8
[   29.346592] x5 : ffff000440400490 x4 : ffff000440400588 x3 : ffff800081751db8
[   29.347492] x2 : 0000000000000000 x1 : 000000000a032a08 x0 : ffff000440100c00
[   29.348392] Call trace:
[   29.348702]  handle_irq_desc+0x3c/0x58 (P)
[   29.349220]  gic_handle_irq+0x48/0xc0
[   29.349683]  call_on_irq_stack+0x30/0x60
[   29.350180]  do_interrupt_handler+0x78/0x7c
[   29.350710]  el1_interrupt+0x34/0x50
[   29.351167]  el1h_64_irq_handler+0x14/0x1c
[   29.351686]  el1h_64_irq+0x6c/0x70
[   29.352116]  __srcu_check_read_flavor+0x54/0xe0 (P)
[   29.352734]  device_links_read_lock+0x20/0x60
[   29.353285]  __rpm_callback+0x14c/0x210
[   29.353771]  rpm_callback+0x6c/0x78
[   29.354213]  rpm_resume+0x50c/0x718
[   29.354655]  __pm_runtime_resume+0x48/0x88
[   29.355174]  vsp1_device_get+0x1c/0x84
[   29.355649]  vsp1_isp_start_streaming+0x7c/0x198
[   29.356233]  risp_core_start_streaming+0x318/0x440
[   29.356839]  risp_io_start_streaming+0x74/0x108
[   29.357413]  vb2_start_streaming+0x64/0x168
[   29.357941]  vb2_core_streamon+0xd0/0x1b8
[   29.358447]  vb2_ioctl_streamon+0x50/0x8c
[   29.358954]  v4l_streamon+0x20/0x28
[   29.359397]  __video_do_ioctl+0x344/0x3f0
[   29.359904]  video_usercopy+0x2e4/0x870
[   29.360390]  video_ioctl2+0x14/0x20
[   29.360832]  v4l2_ioctl+0x3c/0x60
[   29.361252]  __arm64_sys_ioctl+0x88/0xe0
[   29.361749]  invoke_syscall.constprop.0+0x3c/0xe4
[   29.362344]  el0_svc_common.constprop.0+0x34/0xcc
[   29.362938]  do_el0_svc+0x18/0x20
[   29.363359]  el0_svc+0x3c/0x2a0
[   29.363758]  el0t_64_sync_handler+0x98/0xe0
[   29.364288]  el0t_64_sync+0x154/0x158
[   29.364751] irq event stamp: 3935
[   29.365169] hardirqs last  enabled at (3935): [<ffff800080fa655c>] irqentry_exit+0x3c/0x180
[   29.366220] hardirqs last disabled at (3934): [<ffff800080fab110>] preempt_schedule_irq+0x70/0xa0
[   29.367338] softirqs last  enabled at (3930): [<ffff8000800c2478>] handle_softirqs+0x44c/0x4a0
[   29.368422] softirqs last disabled at (3919): [<ffff800080010270>] __do_softirq+0x10/0x18
[   29.369451] ---[ end trace 0000000000000000 ]---
[   29.370033] ------------[ cut here ]------------
[   29.370614] Unexpected interrupt (irqnr 26)
[   29.371159] WARNING: CPU: 1 PID: 597 at drivers/irqchip/irq-gic-v3.c:875 gic_handle_irq+0xb4/0xc0
[   29.372278] CPU: 1 UID: 0 PID: 597 Comm: yavta Tainted: G        W           6.17.0-arm64-renesas-09711-g5173b0d6549f #52 PREEMPT
[   29.373755] Tainted: [W]=WARN
[   29.374129] Hardware name: Retronix Sparrow Hawk board based on r8a779g3 (DT)
[   29.375026] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   29.375902] pc : gic_handle_irq+0xb4/0xc0
[   29.376409] lr : gic_handle_irq+0xb4/0xc0
[   29.376915] sp : ffff8000824d3fe0
[   29.377333] x29: ffff8000824d3fe0 x28: ffff000443a9cf80 x27: 0000000000000001
[   29.378233] x26: 0000000000000000 x25: ffff800081751db8 x24: ffff800080123160
[   29.379133] x23: 0000000000400009 x22: ffff80008016c294 x21: ffff800083c438a0
[   29.380033] x20: 0000000000000000 x19: ffff800081684cd8 x18: 000000000000000a
[   29.380933] x17: ffff80063d8b5000 x16: ffff8000824d0000 x15: 0720072007200720
[   29.381833] x14: 0720072007200720 x13: 0720072007200720 x12: 0720072007200720
[   29.382733] x11: 0000000000000058 x10: 0000000000000018 x9 : ffff800081777de0
[   29.383634] x8 : 0000000000057fa8 x7 : 000000000000031b x6 : ffff8000817cfde0
[   29.384534] x5 : ffff0006bef28448 x4 : ffff80063d8b5000 x3 : ffff000443a9cf80
[   29.385434] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000443a9cf80
[   29.386333] Call trace:
[   29.386643]  gic_handle_irq+0xb4/0xc0 (P)
[   29.387150]  call_on_irq_stack+0x30/0x60
[   29.387647]  do_interrupt_handler+0x78/0x7c
[   29.388176]  el1_interrupt+0x34/0x50
[   29.388630]  el1h_64_irq_handler+0x14/0x1c
[   29.389149]  el1h_64_irq+0x6c/0x70
[   29.389579]  __srcu_check_read_flavor+0x54/0xe0 (P)
[   29.390197]  device_links_read_lock+0x20/0x60
[   29.390748]  __rpm_callback+0x14c/0x210
[   29.391235]  rpm_callback+0x6c/0x78
[   29.391677]  rpm_resume+0x50c/0x718
[   29.392120]  __pm_runtime_resume+0x48/0x88
[   29.392638]  vsp1_device_get+0x1c/0x84
[   29.393114]  vsp1_isp_start_streaming+0x7c/0x198
[   29.393697]  risp_core_start_streaming+0x318/0x440
[   29.394305]  risp_io_start_streaming+0x74/0x108
[   29.394879]  vb2_start_streaming+0x64/0x168
[   29.395407]  vb2_core_streamon+0xd0/0x1b8
[   29.395914]  vb2_ioctl_streamon+0x50/0x8c
[   29.396421]  v4l_streamon+0x20/0x28
[   29.396863]  __video_do_ioctl+0x344/0x3f0
[   29.397372]  video_usercopy+0x2e4/0x870
[   29.397858]  video_ioctl2+0x14/0x20
[   29.398300]  v4l2_ioctl+0x3c/0x60
[   29.398720]  __arm64_sys_ioctl+0x88/0xe0
[   29.399218]  invoke_syscall.constprop.0+0x3c/0xe4
[   29.399814]  el0_svc_common.constprop.0+0x34/0xcc
[   29.400408]  do_el0_svc+0x18/0x20
[   29.400828]  el0_svc+0x3c/0x2a0
[   29.401228]  el0t_64_sync_handler+0x98/0xe0
[   29.401758]  el0t_64_sync+0x154/0x158
[   29.402221] irq event stamp: 3935
[   29.402639] hardirqs last  enabled at (3935): [<ffff800080fa655c>] irqentry_exit+0x3c/0x180
[   29.403690] hardirqs last disabled at (3934): [<ffff800080fab110>] preempt_schedule_irq+0x70/0xa0
[   29.404807] softirqs last  enabled at (3930): [<ffff8000800c2478>] handle_softirqs+0x44c/0x4a0
[   29.405893] softirqs last disabled at (3919): [<ffff800080010270>] __do_softirq+0x10/0x18
[   29.406922] ---[ end trace 0000000000000000 ]---
[   29.363758]  el0t_64_sync_handler+0x98/0xe0
[   29.364288]  el0t_64_sync+0x154/0x158
[   29.364751] irq event stamp: 3935
[   29.365169] hardirqs last  enabled at (3935): [<ffff800080fa655c>] irqentry_exit+0x3c/0x180
[   29.366220] hardirqs last disabled at (3934): [<ffff800080fab110>] preempt_schedule_irq+0x70/0xa0
[   29.367338] softirqs last  enabled at (3930): [<ffff8000800c2478>] handle_softirqs+0x44c/0x4a0
[   29.368422] softirqs last disabled at (3919): [<ffff800080010270>] __do_softirq+0x10/0x18
[   29.369451] ---[ end trace 0000000000000000 ]---
[   29.370033] ------------[ cut here ]------------
[   29.370614] Unexpected interrupt (irqnr 26)
[   29.371159] WARNING: CPU: 1 PID: 597 at drivers/irqchip/irq-gic-v3.c:875 gic_handle_irq+0xb4/0xc0
[   29.372278] CPU: 1 UID: 0 PID: 597 Comm: yavta Tainted: G        W           6.17.0-arm64-renesas-09711-g5173b0d6549f #52 PREEMPT
[   29.373755] Tainted: [W]=WARN
[   29.374129] Hardware name: Retronix Sparrow Hawk board based on r8a779g3 (DT)
[   29.375026] pstate: 604000c9 (nZCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   29.375902] pc : gic_handle_irq+0xb4/0xc0
[   29.376409] lr : gic_handle_irq+0xb4/0xc0
[   29.376915] sp : ffff8000824d3fe0
[   29.377333] x29: ffff8000824d3fe0 x28: ffff000443a9cf80 x27: 0000000000000001
[   29.378233] x26: 0000000000000000 x25: ffff800081751db8 x24: ffff800080123160
[   29.379133] x23: 0000000000400009 x22: ffff80008016c294 x21: ffff800083c438a0
[   29.380033] x20: 0000000000000000 x19: ffff800081684cd8 x18: 000000000000000a
[   29.380933] x17: ffff80063d8b5000 x16: ffff8000824d0000 x15: 0720072007200720
[   29.381833] x14: 0720072007200720 x13: 0720072007200720 x12: 0720072007200720
[   29.382733] x11: 0000000000000058 x10: 0000000000000018 x9 : ffff800081777de0
[   29.383634] x8 : 0000000000057fa8 x7 : 000000000000031b x6 : ffff8000817cfde0
[   29.384534] x5 : ffff0006bef28448 x4 : ffff80063d8b5000 x3 : ffff000443a9cf80
[   29.385434] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff000443a9cf80
[   29.386333] Call trace:
[   29.386643]  gic_handle_irq+0xb4/0xc0 (P)
[   29.387150]  call_on_irq_stack+0x30/0x60
[   29.387647]  do_interrupt_handler+0x78/0x7c
[   29.388176]  el1_interrupt+0x34/0x50
[   29.388630]  el1h_64_irq_handler+0x14/0x1c
[   29.389149]  el1h_64_irq+0x6c/0x70
[   29.389579]  __srcu_check_read_flavor+0x54/0xe0 (P)
[   29.390197]  device_links_read_lock+0x20/0x60
[   29.390748]  __rpm_callback+0x14c/0x210
[   29.391235]  rpm_callback+0x6c/0x78
[   29.391677]  rpm_resume+0x50c/0x718
[   29.392120]  __pm_runtime_resume+0x48/0x88
[   29.392638]  vsp1_device_get+0x1c/0x84
[   29.393114]  vsp1_isp_start_streaming+0x7c/0x198
[   29.393697]  risp_core_start_streaming+0x318/0x440
[   29.394305]  risp_io_start_streaming+0x74/0x108
[   29.394879]  vb2_start_streaming+0x64/0x168
[   29.395407]  vb2_core_streamon+0xd0/0x1b8
[   29.395914]  vb2_ioctl_streamon+0x50/0x8c
[   29.396421]  v4l_streamon+0x20/0x28
[   29.396863]  __video_do_ioctl+0x344/0x3f0
[   29.397372]  video_usercopy+0x2e4/0x870
[   29.397858]  video_ioctl2+0x14/0x20
[   29.398300]  v4l2_ioctl+0x3c/0x60
[   29.398720]  __arm64_sys_ioctl+0x88/0xe0
[   29.399218]  invoke_syscall.constprop.0+0x3c/0xe4
[   29.399814]  el0_svc_common.constprop.0+0x34/0xcc
[   29.400408]  do_el0_svc+0x18/0x20
[   29.400828]  el0_svc+0x3c/0x2a0
[   29.401228]  el0t_64_sync_handler+0x98/0xe0
[   29.401758]  el0t_64_sync+0x154/0x158
[   29.402221] irq event stamp: 3935
[   29.402639] hardirqs last  enabled at (3935): [<ffff800080fa655c>] irqentry_exit+0x3c/0x180
[   29.403690] hardirqs last disabled at (3934): [<ffff800080fab110>] preempt_schedule_irq+0x70/0xa0
[   29.404807] softirqs last  enabled at (3930): [<ffff8000800c2478>] handle_softirqs+0x44c/0x4a0
[   29.405893] softirqs last disabled at (3919): [<ffff800080010270>] __do_softirq+0x10/0x18
[   29.406922] ---[ end trace 0000000000000000 ]---

>  
>  	/* Release module from reset state */
>  	writel(bitmask, priv->pub.base0 + priv->reset_clear_regs[reg]);
> -- 
> 2.51.0
> 
> 

-- 
Kind Regards,
Niklas Söderlund

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

[Marek Vasut]

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() ?

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

[Niklas Söderlund]

Hi Marek,

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?

diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
index a3d171ddaab9..2b45f0aa8d97 100644
--- a/drivers/clk/renesas/renesas-cpg-mssr.c
+++ b/drivers/clk/renesas/renesas-cpg-mssr.c
@@ -709,9 +709,9 @@ static int cpg_mssr_reset(struct reset_controller_dev *rcdev,
         * (one cycle of the RCLK clock @ ca. 32 kHz).
         */
        if (priv->reg_layout == CLK_REG_LAYOUT_RCAR_GEN4)
-               usleep_range(1000, 2000);
+               udelay(1000);
        else
-               usleep_range(35, 1000);
+               udelay(35);

        /* Release module from reset state */
        return cpg_mssr_reset_operate(rcdev, NULL, false, id);

-- 
Kind Regards,
Niklas Söderlund

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

[Marek Vasut]

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.

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 ?

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

[Niklas Söderlund]

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

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

[Marek Vasut]

On 10/5/25 3:42 PM, Niklas Söderlund wrote:

Hello Niklas,

>> 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.
Now that I actually looked at that driver, I understand, thank you for 
pointing me to it.

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

[Geert Uytterhoeven]

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.  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?

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

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

[Niklas Söderlund]

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.

> 
> 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

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

[Niklas Söderlund]

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

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

[Geert Uytterhoeven]

Hi Niklas,

On Thu, 9 Oct 2025 at 20:12, Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2025-10-06 14:23:42 +0200, Niklas Söderlund wrote:
> > On 2025-10-06 13:53:34 +0200, Geert Uytterhoeven wrote:
> > > 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.

Thank you, I still have this v1 in renesas-clk-for-v6.19.

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