Re: [PATCH v2 3/3] usb: renesas_usbhs: Reorder clock handling and power management in probe
From: Lad, Prabhakar
Date: Thu Apr 17 2025 - 12:33:19 EST
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.
I reverted the patch and made the changes shown below. As you can see,
two read and two write operations are triggered before the clock is
enabled. Since I return early when the clock is not enabled, I didn’t
encounter any synchronous aborts. However, once I removed this check,
I hit the synchronous abort on the RZ/V2H. Hence, the need for this
patch to enable the clock early in the probe.
----------------------
[ 11.799862] g_serial gadget.0: Gadget Serial v2.4
[ 11.804323] videodev: Linux video capture interface: v2.00
[ 11.808019] g_serial gadget.0: g_serial ready
[ 11.818591] renesas_usbhs 15820000.usb: usbhs_read: reg = 0
[ 11.824173] renesas_usbhs 15820000.usb: usbhs_write: reg = 0
[ 11.830178] [drm] Initialized panfrost 1.3.0 for 14850000.gpu on minor 0
[ 11.841619] renesas_usbhs 15820000.usb: gadget probed
[ 11.847552] renesas_usbhs 15820000.usb: usbhs_probe:714
[ 11.852923] renesas_usbhs 15820000.usb: usbhs_probe:722
[ 11.858214] renesas_usbhs 15820000.usb: usbhs_probe:726
[ 11.863478] renesas_usbhs 15820000.usb: usbhs_read: reg = 0
[ 11.869139] renesas_usbhs 15820000.usb: usbhs_write: reg = 0
[ 11.874831] renesas_usbhs 15820000.usb: usbhs_probe:733
[ 11.880081] renesas_usbhs 15820000.usb: usbhs_probe:744
[ 11.885502] renesas_usbhs 15820000.usb: usbhs_probe:758
[ 11.890782] renesas_usbhs 15820000.usb: usbhs_probe:762
[ 11.896222] renesas_usbhs 15820000.usb: probed
----------------------
diff --git a/drivers/usb/renesas_usbhs/common.c
b/drivers/usb/renesas_usbhs/common.c
index 703cf5d0cb41..8893d02ae4b4 100644
--- a/drivers/usb/renesas_usbhs/common.c
+++ b/drivers/usb/renesas_usbhs/common.c
@@ -55,16 +55,26 @@
!((priv)->pfunc->func) ? 0 : \
(priv)->pfunc->func(args))
+static bool clock_enabled = false;
+
/*
* common functions
*/
u16 usbhs_read(struct usbhs_priv *priv, u32 reg)
{
+ if (!clock_enabled) {
+ dev_info(&priv->pdev->dev, "%s: reg = %x\n", __func__, reg);
+ return 0;
+ }
return ioread16(priv->base + reg);
}
void usbhs_write(struct usbhs_priv *priv, u32 reg, u16 data)
{
+ if (!clock_enabled) {
+ dev_info(&priv->pdev->dev, "%s: reg = %x\n", __func__, reg);
+ return;
+ }
iowrite16(data, priv->base + reg);
}
@@ -685,19 +695,23 @@ 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));
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/* call pipe and module init */
ret = usbhs_pipe_probe(priv);
if (ret < 0)
return ret;
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
ret = usbhs_fifo_probe(priv);
if (ret < 0)
goto probe_end_pipe_exit;
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
ret = usbhs_mod_probe(priv);
if (ret < 0)
goto probe_end_fifo_exit;
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/* platform_set_drvdata() should be called after usbhs_mod_probe() */
platform_set_drvdata(pdev, priv);
@@ -705,16 +719,18 @@ static int usbhs_probe(struct platform_device *pdev)
if (ret)
goto probe_fail_rst;
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
ret = usbhsc_clk_get(dev, priv);
if (ret)
goto probe_fail_clks;
-
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/*
* device reset here because
* USB device might be used in boot loader.
*/
usbhs_sys_clock_ctrl(priv, 0);
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/* check GPIO determining if USB function should be enabled */
if (gpiod) {
ret = !gpiod_get_value(gpiod);
@@ -725,6 +741,7 @@ static int usbhs_probe(struct platform_device *pdev)
}
}
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/*
* platform call
*
@@ -738,11 +755,14 @@ static int usbhs_probe(struct platform_device *pdev)
goto probe_end_mod_exit;
}
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/* reset phy for connection */
usbhs_platform_call(priv, phy_reset, pdev);
+ dev_info(dev, "%s:%d\n", __func__, __LINE__);
/* power control */
pm_runtime_enable(dev);
+ clock_enabled = true;
if (!usbhs_get_dparam(priv, runtime_pwctrl)) {
usbhsc_power_ctrl(priv, 1);
usbhs_mod_autonomy_mode(priv);
Cheers,
Prabhakar