| Message ID | 20250327120737.230041-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: renesas_usbhs: Reorder clock handling | expand |
Hi Prabhakark Thank you for the patch > Reorder the initialization sequence in `usbhs_probe()` to enable runtime > PM before accessing registers, preventing potential crashes due to > uninitialized clocks. > > 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 This patch itself is not so bad idea, but basically, we should not assume the power/clock was enabled since kernel boot. In this driver, register access happen only during power enable <-> power disable (except your issue case), and this is good idea. So, the strange is usbhs_sys_clock_ctrl() call in usbhs_probe() (without power enable) I guess. According to the comment, it is just caring boot loader, and usbhs_sys_clock_ctrl() itself will be called when usbhsc_power_ctrl() was called anyway. And more, if my understanding was correct, Renesas clock driver will stop all unused devices clock/power after boot. So maybe we can just remove strange usbhs_sys_clock_ctrl() from usbhs_probe() ? Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Kuninori san, Thank you for the review. On Fri, Mar 28, 2025 at 12:40 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > Hi Prabhakark > > Thank you for the patch > > > Reorder the initialization sequence in `usbhs_probe()` to enable runtime > > PM before accessing registers, preventing potential crashes due to > > uninitialized clocks. > > > > 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 > > This patch itself is not so bad idea, but basically, we should not assume > the power/clock was enabled since kernel boot. > In this driver, register access happen only during power enable <-> power > disable (except your issue case), and this is good idea. So, the strange > is usbhs_sys_clock_ctrl() call in usbhs_probe() (without power enable) I > guess. > > According to the comment, it is just caring boot loader, and > usbhs_sys_clock_ctrl() itself will be called when usbhsc_power_ctrl() was > called anyway. And more, if my understanding was correct, Renesas clock > driver will stop all unused devices clock/power after boot. > So maybe we can just remove strange usbhs_sys_clock_ctrl() from usbhs_probe() ? > After dropping usbhs_sys_clock_ctrl and removing the clock enabling done in this patch and with `CONFIG_USB_G_SERIAL=y` I hit the same issue. [14.318626] g_serial gadget.0: g_serial ready [14.323923] Internal error: synchronous external abort: 0000000096000010 [#1] PREEMPT SMP [14.332107] Modules linked in: rzg2l_mipi_dsi rcar_fcp renesas_usbhs(+) drm_shmem_helper display_connector gpu_sched drm_kms_helper fuse drm backlight ipv6 [14.346025] CPU: 1 UID: 0 PID: 187 Comm: (udev-worker) Not tainted 6.14.0-rc7+ #99 [14.353578] Hardware name: Renesas RZ/V2H EVK Board based on r9a09g057h44 (DT) [14.360775] pstate: 204000c5 (nzCv daIF +PAN -UAO -TCO -DIT -SSBS BTYPE=--) [14.367711] pc : usbhs_sys_function_pullup+0x10/0x40 [renesas_usbhs] [14.374082] lr : usbhsg_update_pullup+0x58/0x68 [renesas_usbhs] [14.374107] sp : ffff80008278b460 [14.374110] x29: ffff80008278b460 x28: ffff800079a355f0 x27: 000000000000000a [14.374122] x26: ffff800079a35d00 x25: ffff0000c3c26b40 x24: ffff8000817bb1c0 [14.374131] x23: ffff0000c33cd800 x22: ffff0000c221e480 x21: ffff0000c221e540 [14.374140] x20: 0000000000000001 x19: ffff0000c221e480 x18: 0000000000000000 [14.374148] x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 [14.374156] x14: 0000000000000074 x13: ffff0000c33ac680 x12: 0000000000000000 [14.374164] x11: ffff0000c33ac680 x10: 0000000000000aa0 x9 : ffff80008278b160 [14.374172] x8 : ffff0000c33ad100 x7 : 0000000002307a83 x6 : 000000000000032c [14.374180] x5 : ffff0003fdfb4388 x4 : 0000000000000000 x3 : 0000000000000000 [14.374187] x2 : 0000000000000010 x1 : ffff8000825a0000 x0 : ffff0000c221e480 [14.374197] Call trace: [14.374200] usbhs_sys_function_pullup+0x10/0x40 [renesas_usbhs] (P) [14.374220] usbhsg_pullup+0x54/0x78 [renesas_usbhs] [14.374236] usb_gadget_connect_locked+0x44/0x8c [14.374249] gadget_bind_driver+0x198/0x284 [14.374256] really_probe+0xbc/0x2c0 [14.374266] __driver_probe_device+0x78/0x120 [14.374272] driver_probe_device+0x3c/0x154 [14.374278] __device_attach_driver+0xb8/0x140 [14.374285] bus_for_each_drv+0x88/0xe8 [14.374292] __device_attach+0xa0/0x190 [14.374299] device_initial_probe+0x14/0x20 [14.374306] bus_probe_device+0xb4/0xc0 [14.374312] device_add+0x5c4/0x7a0 [14.374318] usb_add_gadget+0x198/0x220 [14.374324] usb_add_gadget_udc+0x68/0xa0 [14.374330] usbhs_mod_gadget_probe+0x218/0x2c4 [renesas_usbhs] [14.374347] usbhs_mod_probe+0x30/0xc0 [renesas_usbhs] [14.374363] usbhs_probe+0x208/0x5a0 [renesas_usbhs] [14.374378] platform_probe+0x68/0xdc [14.374387] really_probe+0xbc/0x2c0 [14.374394] __driver_probe_device+0x78/0x120 [14.374400] driver_probe_device+0x3c/0x154 [14.374407] __driver_attach+0x90/0x1a0 [14.374413] bus_for_each_dev+0x7c/0xe0 [14.374419] driver_attach+0x24/0x30 [14.374425] bus_add_driver+0xe4/0x208 [14.374432] driver_register+0x68/0x130 [14.374438] __platform_driver_register+0x24/0x30 [14.374446] renesas_usbhs_driver_init+0x20/0x1000 [renesas_usbhs] [14.374462] do_one_initcall+0x60/0x1d4 [14.374474] do_init_module+0x54/0x1f8 [14.374484] load_module+0x1754/0x1c98 [14.374492] init_module_from_file+0x88/0xcc [14.374499] __arm64_sys_finit_module+0x1c4/0x328 [14.374506] invoke_syscall+0x48/0x104 [14.374516] el0_svc_common.constprop.0+0xc0/0xe0 [14.374524] do_el0_svc+0x1c/0x28 [14.374530] el0_svc+0x30/0xcc [14.374539] el0t_64_sync_handler+0x10c/0x138 [14.374546] el0t_64_sync+0x198/0x19c [14.374558] Code: 7100003f 1a9f07e2 f9400001 531c6c42 (79400021) [14.374564] ---[ end trace 0000000000000000 ]--- Cheers, Prabhakar
Hi Lad, Prabhakar > > > usbhs_probe() > > > usbhs_sys_clock_ctrl() > > > usbhs_bset() > > > usbhs_write() > > > iowrite16() <-- Register access before enabling clocks > > > > This patch itself is not so bad idea, but basically, we should not assume > > the power/clock was enabled since kernel boot. > > In this driver, register access happen only during power enable <-> power > > disable (except your issue case), and this is good idea. So, the strange > > is usbhs_sys_clock_ctrl() call in usbhs_probe() (without power enable) I > > guess. > > > > According to the comment, it is just caring boot loader, and > > usbhs_sys_clock_ctrl() itself will be called when usbhsc_power_ctrl() was > > called anyway. And more, if my understanding was correct, Renesas clock > > driver will stop all unused devices clock/power after boot. > > So maybe we can just remove strange usbhs_sys_clock_ctrl() from usbhs_probe() ? > > > > After dropping usbhs_sys_clock_ctrl and removing the clock enabling > done in this patch and with `CONFIG_USB_G_SERIAL=y` I hit the same > issue. Ah... OK, not only usbhs_sys_clock_ctrl(), but other initializer also missing power in probe(). Thank you for reporting, your original patch is reasonable. Thank you for your help !! Best regards --- Kuninori Morimoto
Hi Morimoto san, On Mon, Mar 31, 2025 at 1:05 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > Hi Lad, Prabhakar > > > > > usbhs_probe() > > > > usbhs_sys_clock_ctrl() > > > > usbhs_bset() > > > > usbhs_write() > > > > iowrite16() <-- Register access before enabling clocks > > > > > > This patch itself is not so bad idea, but basically, we should not assume > > > the power/clock was enabled since kernel boot. > > > In this driver, register access happen only during power enable <-> power > > > disable (except your issue case), and this is good idea. So, the strange > > > is usbhs_sys_clock_ctrl() call in usbhs_probe() (without power enable) I > > > guess. > > > > > > According to the comment, it is just caring boot loader, and > > > usbhs_sys_clock_ctrl() itself will be called when usbhsc_power_ctrl() was > > > called anyway. And more, if my understanding was correct, Renesas clock > > > driver will stop all unused devices clock/power after boot. > > > So maybe we can just remove strange usbhs_sys_clock_ctrl() from usbhs_probe() ? > > > > > > > After dropping usbhs_sys_clock_ctrl and removing the clock enabling > > done in this patch and with `CONFIG_USB_G_SERIAL=y` I hit the same > > issue. > > Ah... > OK, not only usbhs_sys_clock_ctrl(), but other initializer also > missing power in probe(). Thank you for reporting, your original patch > is reasonable. > Thanks! I'll send a V2 incorporating your review comments next week after v6.15-rc1 is released. Cheers, Prabhakar
diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index 0678830dc7ce..c504726ccec8 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; + /* 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; - /* * 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);