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

[Lad, Prabhakar]

Hi Shimoda-san,

On Thu, Apr 17, 2025 at 5:32 PM Lad, Prabhakar
<prabhakar.csengg@xxxxxxxxx> wrote:
>
> Hi Shimoda-san,
>
> Sorry for the delayed response.
>
> On Thu, Apr 10, 2025 at 3:48 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> >
> > Hello Geert-san,
> >
> > > From: Yoshihiro Shimoda, Sent: Wednesday, April 9, 2025 10:10 AM
> > > To: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > Cc: Prabhakar <prabhakar.csengg@xxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Kuninori Morimoto
> > > <kuninori.morimoto.gx@xxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > linux-renesas-soc@xxxxxxxxxxxxxxx; Biju Das <biju.das.jz@xxxxxxxxxxxxxx>; Claudiu Beznea
> > > <claudiu.beznea.uj@xxxxxxxxxxxxxx>; Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>; Prabhakar Mahadev Lad
> > > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > Subject: RE: [PATCH v2 3/3] usb: renesas_usbhs: Reorder clock handling and power management in probe
> > >
> > > Hello Geert-san,
> > >
> > > > From: Geert Uytterhoeven, Sent: Wednesday, April 9, 2025 12:43 AM
> > > >
> > > > Hi Shimoda-san,
> > > >
> > > > On Tue, 8 Apr 2025 at 12:40, Yoshihiro Shimoda
> > > > <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote:
> > > > > > 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.
> > > >
> > > > On R-Car Gen3 and later, the firmware must trap the external abort,
> > > > as usually no crash happens, but register reads return zero when
> > > > the module clock is turned off.  I am wondering why RZ/V2H behaves
> > > > differently than R-Car Gen3?
> > >
> > > I'm guessing that:
> > > - EHCI/OHCI drivers on R-Car Gen3 enabled both the USB clocks (EHCI/OHCI and USBHS).
> > > - RZ/V2H didn't enable the USBHS clock only.
> > >
> > > So, I'm also guessing that the R-Car V2H issue can be reproduced if we disable EHCI/OHCI on R-Car Gen3.
> > > # However, for some reasons, I don't have time to test for it today. (I'll test it tomorrow or later.)
> >
> > I tested this topic, and I realized that my guess was incorrect.
> > In other words, the current code seems no problem except accessing register offset 0.
> As Geert mentioned, we don't get synchronous aborts on R-Car Gen3--we
> only get a 0 for register reads when the clock is not enabled, as seen
> in the test you ran. On the RZ/V2H, however, if we try to access an IP
> before enabling the clocks, error interrupts are triggered, which is
> why we're seeing synchronous aborts.
>
Another simpler way to test this is using devmem2:

Case #1] On RZ/G2M (which is similar to R-Car Gen3) by disabling all
the ehci/ohci/phy/hsusb and running the devem I see the below:

$devmem2 0xe6590000/dev/mem opened.
Memory mapped at address 0xffffa854c000
Read at address 0xE6590000 (0xffffa854c000): 0x00000000

Case #2] If I do the same on RZ/V2H and run devmem I get the below:
root@rzv2h-evk:~# devmem2 0x15820000
/dev/mem opened.[   34.056718] kauditd_printk_skb: 5 callbacks suppressed

Memory mapped [   34.056734] audit: type=1701
audit(1744907878.287:22): auid=4294967295 uid=0 gid=0 ses=4294967295
pid=313 comm="devmem2" exe="/usr/bin/devmem2" sig=7 res=1
at address 0xffff92854000.
Bus error (core dumped)

Cheers,
Prabhakar