RE: [PATCH v2 3/3] usb: renesas_usbhs: Reorder clock handling and power management in probe

[Yoshihiro Shimoda]

Hello Prabhakar-san,

Thank you for the patch!

> From: Prabhakar, Sent: Monday, April 7, 2025 7:50 PM
> 
> Reorder the initialization sequence in `usbhs_probe()` to enable runtime
> PM before accessing registers, preventing potential crashes due to
> uninitialized clocks.

Just for a record. I don't know why, but the issue didn't occur on the original code
with my environment (R-Car H3). But, anyway, I understood that we need this patch for RZ/V2H.

----- I added some debug printk -----
<snip>
[    3.193400] usbhs_probe:706
[    3.196204] usbhs_probe:710
[    3.199012] usbhs_probe:715
[    3.201808] usbhs_probe:720
[    3.204605] usbhs_read: reg = 0
[    3.207754] usbhs_write: reg = 0, data = 20
[    3.211941] usbhs_probe:727
[    3.214738] usbhs_read: reg = 102
[    3.218061] usbhs_write: reg = 102, data = 4000
[    3.222697] usbhs_read: reg = 0
[    3.225845] usbhs_write: reg = 0, data = 420
[    3.230118] usbhs_write: reg = 30, data = 0
[    3.234304] usbhs_write: reg = 32, data = 0
[    3.238489] usbhs_write: reg = 3a, data = 0
[    3.242673] usbhs_write: reg = 36, data = 0
[    3.246859] usbhs_write: reg = 30, data = 8000
[    3.251312] usbhs_read: reg = 40
[    3.254540] renesas_usbhs e659c000.usb: probed
[    3.802010] usbhs_probe:690
[    3.808677] renesas-cpg-mssr e6150000.clock-controller: MSTP 704/hsusb ON
-----

> Currently, in the probe path, registers are accessed before enabling the
> clocks, leading to a synchronous external abort on the RZ/V2H SoC.
> The problematic call flow is as follows:
> 
>     usbhs_probe()
>         usbhs_sys_clock_ctrl()
>             usbhs_bset()
>                 usbhs_write()
>                     iowrite16()  <-- Register access before enabling clocks
> 
> Since `iowrite16()` is performed without ensuring the required clocks are
> enabled, this can lead to access errors. To fix this, enable PM runtime
> early in the probe function and ensure clocks are acquired before register
> access, preventing crashes like the following on RZ/V2H:
> 
> [13.272640] Internal error: synchronous external abort: 0000000096000010 [#1] PREEMPT SMP
> [13.280814] Modules linked in: cec renesas_usbhs(+) drm_kms_helper fuse drm backlight ipv6
> [13.289088] CPU: 1 UID: 0 PID: 195 Comm: (udev-worker) Not tainted 6.14.0-rc7+ #98
> [13.296640] Hardware name: Renesas RZ/V2H EVK Board based on r9a09g057h44 (DT)
> [13.303834] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [13.310770] pc : usbhs_bset+0x14/0x4c [renesas_usbhs]
> [13.315831] lr : usbhs_probe+0x2e4/0x5ac [renesas_usbhs]
> [13.321138] sp : ffff8000827e3850
> [13.324438] x29: ffff8000827e3860 x28: 0000000000000000 x27: ffff8000827e3ca0
> [13.331554] x26: ffff8000827e3ba0 x25: ffff800081729668 x24: 0000000000000025
> [13.338670] x23: ffff0000c0f08000 x22: 0000000000000000 x21: ffff0000c0f08010
> [13.345783] x20: 0000000000000000 x19: ffff0000c3b52080 x18: 00000000ffffffff
> [13.352895] x17: 0000000000000000 x16: 0000000000000000 x15: ffff8000827e36ce
> [13.360009] x14: 00000000000003d7 x13: 00000000000003d7 x12: 0000000000000000
> [13.367122] x11: 0000000000000000 x10: 0000000000000aa0 x9 : ffff8000827e3750
> [13.374235] x8 : ffff0000c1850b00 x7 : 0000000003826060 x6 : 000000000000001c
> [13.381347] x5 : 000000030d5fcc00 x4 : ffff8000825c0000 x3 : 0000000000000000
> [13.388459] x2 : 0000000000000400 x1 : 0000000000000000 x0 : ffff0000c3b52080
> [13.395574] Call trace:
> [13.398013]  usbhs_bset+0x14/0x4c [renesas_usbhs] (P)
> [13.403076]  platform_probe+0x68/0xdc
> [13.406738]  really_probe+0xbc/0x2c0
> [13.410306]  __driver_probe_device+0x78/0x120
> [13.414653]  driver_probe_device+0x3c/0x154
> [13.418825]  __driver_attach+0x90/0x1a0
> [13.422647]  bus_for_each_dev+0x7c/0xe0
> [13.426470]  driver_attach+0x24/0x30
> [13.430032]  bus_add_driver+0xe4/0x208
> [13.433766]  driver_register+0x68/0x130
> [13.437587]  __platform_driver_register+0x24/0x30
> [13.442273]  renesas_usbhs_driver_init+0x20/0x1000 [renesas_usbhs]
> [13.448450]  do_one_initcall+0x60/0x1d4
> [13.452276]  do_init_module+0x54/0x1f8
> [13.456014]  load_module+0x1754/0x1c98
> [13.459750]  init_module_from_file+0x88/0xcc
> [13.464004]  __arm64_sys_finit_module+0x1c4/0x328
> [13.468689]  invoke_syscall+0x48/0x104
> [13.472426]  el0_svc_common.constprop.0+0xc0/0xe0
> [13.477113]  do_el0_svc+0x1c/0x28
> [13.480415]  el0_svc+0x30/0xcc
> [13.483460]  el0t_64_sync_handler+0x10c/0x138
> [13.487800]  el0t_64_sync+0x198/0x19c
> [13.491453] Code: 2a0103e1 12003c42 12003c63 8b010084 (79400084)
> [13.497522] ---[ end trace 0000000000000000 ]---
> 
> Fixes: f1407d5c66240 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/renesas_usbhs/common.c | 50 +++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c
> index 703cf5d0cb41..f52418fe3fd4 100644
> --- a/drivers/usb/renesas_usbhs/common.c
> +++ b/drivers/usb/renesas_usbhs/common.c
> @@ -685,10 +685,29 @@ static int usbhs_probe(struct platform_device *pdev)
>  	INIT_DELAYED_WORK(&priv->notify_hotplug_work, usbhsc_notify_hotplug);
>  	spin_lock_init(usbhs_priv_to_lock(priv));
> 
> +	/*
> +	 * Acquire clocks and enable power management (PM) early in the
> +	 * probe process, as the driver accesses registers during
> +	 * initialization. Ensure the device is active before proceeding.
> +	 */
> +	pm_runtime_enable(dev);
> +
> +	ret = usbhsc_clk_get(dev, priv);
> +	if (ret)
> +		goto probe_pm_disable;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		goto probe_clk_put;
> +
> +	ret = usbhsc_clk_prepare_enable(priv);
> +	if (ret)
> +		goto probe_pm_put;
> +

Did you really need to call these functions at this timing?
IIUC, usbhs_{pipe,fifo,mod}_probe() will not access any registers.

>  	/* call pipe and module init */
>  	ret = usbhs_pipe_probe(priv);
>  	if (ret < 0)
> -		return ret;
> +		goto probe_clk_dis_unprepare;
> 
>  	ret = usbhs_fifo_probe(priv);
>  	if (ret < 0)
> @@ -705,10 +724,6 @@ static int usbhs_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto probe_fail_rst;
> 
> -	ret = usbhsc_clk_get(dev, priv);
> -	if (ret)
> -		goto probe_fail_clks;
> -

If my understanding above is correct, I would like to add some functions calling around here
to reduce modification lines.

Best regards,
Yoshihiro Shimoda

>  	/*
>  	 * device reset here because
>  	 * USB device might be used in boot loader.
> @@ -721,7 +736,7 @@ static int usbhs_probe(struct platform_device *pdev)
>  		if (ret) {
>  			dev_warn(dev, "USB function not selected (GPIO)\n");
>  			ret = -ENOTSUPP;
> -			goto probe_end_mod_exit;
> +			goto probe_assert_rest;
>  		}
>  	}
> 
> @@ -735,14 +750,19 @@ static int usbhs_probe(struct platform_device *pdev)
>  	ret = usbhs_platform_call(priv, hardware_init, pdev);
>  	if (ret < 0) {
>  		dev_err(dev, "platform init failed.\n");
> -		goto probe_end_mod_exit;
> +		goto probe_assert_rest;
>  	}
> 
>  	/* reset phy for connection */
>  	usbhs_platform_call(priv, phy_reset, pdev);
> 
> -	/* power control */
> -	pm_runtime_enable(dev);
> +	/*
> +	 * Disable the clocks that were enabled earlier in the probe path,
> +	 * and let the driver handle the clocks beyond this point.
> +	 */
> +	usbhsc_clk_disable_unprepare(priv);
> +	pm_runtime_put(dev);
> +
>  	if (!usbhs_get_dparam(priv, runtime_pwctrl)) {
>  		usbhsc_power_ctrl(priv, 1);
>  		usbhs_mod_autonomy_mode(priv);
> @@ -759,9 +779,7 @@ static int usbhs_probe(struct platform_device *pdev)
> 
>  	return ret;
> 
> -probe_end_mod_exit:
> -	usbhsc_clk_put(priv);
> -probe_fail_clks:
> +probe_assert_rest:
>  	reset_control_assert(priv->rsts);
>  probe_fail_rst:
>  	usbhs_mod_remove(priv);
> @@ -769,6 +787,14 @@ static int usbhs_probe(struct platform_device *pdev)
>  	usbhs_fifo_remove(priv);
>  probe_end_pipe_exit:
>  	usbhs_pipe_remove(priv);
> +probe_clk_dis_unprepare:
> +	usbhsc_clk_disable_unprepare(priv);
> +probe_pm_put:
> +	pm_runtime_put(dev);
> +probe_clk_put:
> +	usbhsc_clk_put(priv);
> +probe_pm_disable:
> +	pm_runtime_disable(dev);
> 
>  	dev_info(dev, "probe failed (%d)\n", ret);
> 
> --
> 2.49.0