Message-ID: <CA+V-a8uwJasrQZ+3Y1vgTKz+b_SBULXpXdGYxULUOpPkZgXjFg@mail.gmail.com>
Date: Fri, 28 Mar 2025 08:29:11 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
Cc: Geert Uytterhoeven <geert+renesas@...der.be>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>, linux-usb@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 3/3] usb: renesas_usbhs: Reorder clock handling and power
 management in probe

Hi Kuninori san,

Thank you for the review.

On Fri, Mar 28, 2025 at 12:40 AM Kuninori Morimoto
<kuninori.morimoto.gx@...esas.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

Powered by blists - more mailing lists