[PATCH] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[PATCH] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[Sebastian Reichel]

On RK3588 the OHCI controller registers can only be accessed when the
PHY's 480MHz clock is running. After system suspend the controller is
resumed before the PHY. The controller requests the clock, which opens
the gate in the PHY's clock prepare function. But with the PHY suspended
this just results in a dead clock being routed. The OHCI driver will
then continue to access its registers resulting in a board hang.

Fix this by resuming the suspended PHY in the clock's prepare function,
so that the clock is really prepared once the function returns.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
This was noticed on RK3588 EVB1 when resuming from system suspend. This
is technically a fix, but its unclear when the bug was introduced and
system suspend is broken on RK3588 for quite a while and not just due
to this problem. So I think this fix can be merged the normal way via
linux-next.
---
 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 37 +++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 7d8a533f24ae..07d400967def 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -332,6 +332,39 @@ rockchip_usb2phy_clk480m_clkout_ctl(struct clk_hw *hw, struct regmap **base,
 	}
 }
 
+static int rockchip_usb2phy_clk480m_leave_suspend(struct clk_hw *hw)
+{
+	struct rockchip_usb2phy *rphy = container_of(hw, struct rockchip_usb2phy, clk480m_hw);
+	bool relock = false;
+	int ret, i;
+
+	/* Limit to single port; it's unclear how multi-port should be handled */
+	if (rphy->phy_cfg->num_ports > 1)
+		return 0;
+
+	for (i = 0; i < rphy->phy_cfg->num_ports; i++) {
+		struct rockchip_usb2phy_port *rport = &rphy->ports[i];
+		const struct rockchip_usb2phy_port_cfg *port_cfg = rport->port_cfg;
+
+		if (!rport->phy || !port_cfg || !port_cfg->phy_sus.enable)
+			continue;
+		if (property_enabled(rphy->grf, &port_cfg->phy_sus)) {
+			property_enable(rphy->grf, &port_cfg->phy_sus,
+					false);
+			relock = true;
+		}
+	}
+
+	if (relock) {
+		ret = rockchip_usb2phy_reset(rphy);
+		if (ret)
+			return ret;
+		usleep_range(1500, 2000);
+	}
+
+	return 0;
+}
+
 static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
 {
 	const struct usb2phy_reg *clkout_ctl;
@@ -340,6 +373,10 @@ static int rockchip_usb2phy_clk480m_prepare(struct clk_hw *hw)
 
 	rockchip_usb2phy_clk480m_clkout_ctl(hw, &base, &clkout_ctl);
 
+	ret = rockchip_usb2phy_clk480m_leave_suspend(hw);
+	if (ret)
+		return ret;
+
 	/* turn on 480m clk output if it is off */
 	if (!property_enabled(base, clkout_ctl)) {
 		ret = property_enable(base, clkout_ctl, true);

---
base-commit: cee9395acd8043be0644b25c34bfa86623f2b935
change-id: 20260908-phy-rockchip-inno-usb2-clock-fix-edd65b57f884

Best regards,
--  
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[Igor Paunovic]

Hi Sebastian,

Tested-by: Igor Paunovic <royalnet026@gmail.com> # Orange Pi 5 Plus (RK3588)

Repeating the tag here so it is picked up with the patch: I tested this
in your rockchip-devel branch earlier today and reported the details in
[1], but that was in my own bug report thread rather than under the
patch. The code you posted is identical to what I tested, only the base
differs.

Briefly: 4 full s2idle cycles with all four USB 2.0 hosts bound, which
hung every time before and needed a cold reset. The OHCI whose 480 MHz
clock had it as its only user went from "enters ohci_platform_resume and
never returns" to returning 0 after ~20.7 ms, 8 out of 8 resume passes.

One data point for the reviewers here, since it concerns the guard at the
top of the new function:

    if (rphy->phy_cfg->num_ports > 1)
            return 0;

All four entries in rk3588_phy_cfgs[] have num_ports = 1 (0x0000 and
0x4000 as OTG, 0x8000 and 0xc000 as HOST), so the new code does run on
RK3588, including u2phy2 at 0x8000 whose controller is the one that hung
for me. The num_ports = 2 configs are the older SoCs, so the guard costs
RK3588 nothing.

[1] https://lore.kernel.org/all/20260908144245.10700-1-royalnet026@gmail.com/

Igor

Re: [PATCH] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[Manivannan Sadhasivam]

On Tue, Sep 08, 2026 at 06:07:42PM +0200, Sebastian Reichel wrote:
> On RK3588 the OHCI controller registers can only be accessed when the
> PHY's 480MHz clock is running. After system suspend the controller is
> resumed before the PHY.

This statement is slightly confusing. There is no PM ops in this PHY driver.

> The controller requests the clock, which opens
> the gate in the PHY's clock prepare function. But with the PHY suspended
> this just results in a dead clock being routed. The OHCI driver will
> then continue to access its registers resulting in a board hang.
> 
> Fix this by resuming the suspended PHY in the clock's prepare function,
> so that the clock is really prepared once the function returns.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
> This was noticed on RK3588 EVB1 when resuming from system suspend. This
> is technically a fix, but its unclear when the bug was introduced and
> system suspend is broken on RK3588 for quite a while and not just due
> to this problem. So I think this fix can be merged the normal way via
> linux-next.
> ---
>  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 37 +++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 7d8a533f24ae..07d400967def 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -332,6 +332,39 @@ rockchip_usb2phy_clk480m_clkout_ctl(struct clk_hw *hw, struct regmap **base,
>  	}
>  }
>  
> +static int rockchip_usb2phy_clk480m_leave_suspend(struct clk_hw *hw)
> +{
> +	struct rockchip_usb2phy *rphy = container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> +	bool relock = false;
> +	int ret, i;
> +
> +	/* Limit to single port; it's unclear how multi-port should be handled */
> +	if (rphy->phy_cfg->num_ports > 1)
> +		return 0;
> +
> +	for (i = 0; i < rphy->phy_cfg->num_ports; i++) {
> +		struct rockchip_usb2phy_port *rport = &rphy->ports[i];
> +		const struct rockchip_usb2phy_port_cfg *port_cfg = rport->port_cfg;
> +
> +		if (!rport->phy || !port_cfg || !port_cfg->phy_sus.enable)
> +			continue;
> +		if (property_enabled(rphy->grf, &port_cfg->phy_sus)) {
> +			property_enable(rphy->grf, &port_cfg->phy_sus,
> +					false);
> +			relock = true;
> +		}
> +	}
> +
> +	if (relock) {
> +		ret = rockchip_usb2phy_reset(rphy);
> +		if (ret)
> +			return ret;
> +		usleep_range(1500, 2000);
> +	}
> +

This looks like a duplication of rockchip_usb2phy_power_on(). So I'm assuming
that phy_power_on() is not called by the OHCI driver before accessing the
registers. So why don't you fix that instead?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3374 bytes --]

Hello Mani,

On Sat, Sep 26, 2026 at 04:50:07AM +0200, Manivannan Sadhasivam wrote:
> On Tue, Sep 08, 2026 at 06:07:42PM +0200, Sebastian Reichel wrote:
> > On RK3588 the OHCI controller registers can only be accessed when the
> > PHY's 480MHz clock is running. After system suspend the controller is
> > resumed before the PHY.
> 
> This statement is slightly confusing. There is no PM ops in this
> PHY driver.

I will take a closer look how it is powered off during suspend.
Generally I expect it to loose state in any case as the related
power domain should be disabled.

> > The controller requests the clock, which opens
> > the gate in the PHY's clock prepare function. But with the PHY suspended
> > this just results in a dead clock being routed. The OHCI driver will
> > then continue to access its registers resulting in a board hang.
> > 
> > Fix this by resuming the suspended PHY in the clock's prepare function,
> > so that the clock is really prepared once the function returns.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> > This was noticed on RK3588 EVB1 when resuming from system suspend. This
> > is technically a fix, but its unclear when the bug was introduced and
> > system suspend is broken on RK3588 for quite a while and not just due
> > to this problem. So I think this fix can be merged the normal way via
> > linux-next.
> > ---
> >  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 37 +++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index 7d8a533f24ae..07d400967def 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -332,6 +332,39 @@ rockchip_usb2phy_clk480m_clkout_ctl(struct clk_hw *hw, struct regmap **base,
> >  	}
> >  }
> >  
> > +static int rockchip_usb2phy_clk480m_leave_suspend(struct clk_hw *hw)
> > +{
> > +	struct rockchip_usb2phy *rphy = container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> > +	bool relock = false;
> > +	int ret, i;
> > +
> > +	/* Limit to single port; it's unclear how multi-port should be handled */
> > +	if (rphy->phy_cfg->num_ports > 1)
> > +		return 0;
> > +
> > +	for (i = 0; i < rphy->phy_cfg->num_ports; i++) {
> > +		struct rockchip_usb2phy_port *rport = &rphy->ports[i];
> > +		const struct rockchip_usb2phy_port_cfg *port_cfg = rport->port_cfg;
> > +
> > +		if (!rport->phy || !port_cfg || !port_cfg->phy_sus.enable)
> > +			continue;
> > +		if (property_enabled(rphy->grf, &port_cfg->phy_sus)) {
> > +			property_enable(rphy->grf, &port_cfg->phy_sus,
> > +					false);
> > +			relock = true;
> > +		}
> > +	}
> > +
> > +	if (relock) {
> > +		ret = rockchip_usb2phy_reset(rphy);
> > +		if (ret)
> > +			return ret;
> > +		usleep_range(1500, 2000);
> > +	}
> > +
> 
> This looks like a duplication of rockchip_usb2phy_power_on(). So I'm assuming
> that phy_power_on() is not called by the OHCI driver before accessing the
> registers. So why don't you fix that instead?

I can look into it. My way of thinking was, that the clock should be
running independently of that when the clock has been requested via
common clock framework.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] phy: rockchip: inno-usb2: ensure PHY is running when clock is requested

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 5254 bytes --]

Hello Mani,

On Mon, Sep 28, 2026 at 08:00:24PM +0200, Sebastian Reichel wrote:
> On Sat, Sep 26, 2026 at 04:50:07AM +0200, Manivannan Sadhasivam wrote:
> > On Tue, Sep 08, 2026 at 06:07:42PM +0200, Sebastian Reichel wrote:
> > > On RK3588 the OHCI controller registers can only be accessed when the
> > > PHY's 480MHz clock is running. After system suspend the controller is
> > > resumed before the PHY.
> > 
> > This statement is slightly confusing. There is no PM ops in this
> > PHY driver.
> 
> I will take a closer look how it is powered off during suspend.
> Generally I expect it to loose state in any case as the related
> power domain should be disabled.

The PHY is disabled/enabled via generic hcd_bus_suspend and
hcd_bus_resume, which is called by usb_dev_suspend/usb_dev_resume
(i.e. the child USB bus PM handles the PHY power), which is a child
of the OHCI platform device.

The OHCI platform device itself just handles resets and clocks.

It works in the normal driver probe case, since there are no
controller registers accessed before the USB bus itself is started.

> > > The controller requests the clock, which opens
> > > the gate in the PHY's clock prepare function. But with the PHY suspended
> > > this just results in a dead clock being routed. The OHCI driver will
> > > then continue to access its registers resulting in a board hang.
> > > 
> > > Fix this by resuming the suspended PHY in the clock's prepare function,
> > > so that the clock is really prepared once the function returns.
> > > 
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > > This was noticed on RK3588 EVB1 when resuming from system suspend. This
> > > is technically a fix, but its unclear when the bug was introduced and
> > > system suspend is broken on RK3588 for quite a while and not just due
> > > to this problem. So I think this fix can be merged the normal way via
> > > linux-next.
> > > ---
> > >  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 37 +++++++++++++++++++++++++++
> > >  1 file changed, 37 insertions(+)
> > > 
> > > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > index 7d8a533f24ae..07d400967def 100644
> > > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > > @@ -332,6 +332,39 @@ rockchip_usb2phy_clk480m_clkout_ctl(struct clk_hw *hw, struct regmap **base,
> > >  	}
> > >  }
> > >  
> > > +static int rockchip_usb2phy_clk480m_leave_suspend(struct clk_hw *hw)
> > > +{
> > > +	struct rockchip_usb2phy *rphy = container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> > > +	bool relock = false;
> > > +	int ret, i;
> > > +
> > > +	/* Limit to single port; it's unclear how multi-port should be handled */
> > > +	if (rphy->phy_cfg->num_ports > 1)
> > > +		return 0;
> > > +
> > > +	for (i = 0; i < rphy->phy_cfg->num_ports; i++) {
> > > +		struct rockchip_usb2phy_port *rport = &rphy->ports[i];
> > > +		const struct rockchip_usb2phy_port_cfg *port_cfg = rport->port_cfg;
> > > +
> > > +		if (!rport->phy || !port_cfg || !port_cfg->phy_sus.enable)
> > > +			continue;
> > > +		if (property_enabled(rphy->grf, &port_cfg->phy_sus)) {
> > > +			property_enable(rphy->grf, &port_cfg->phy_sus,
> > > +					false);
> > > +			relock = true;
> > > +		}
> > > +	}
> > > +
> > > +	if (relock) {
> > > +		ret = rockchip_usb2phy_reset(rphy);
> > > +		if (ret)
> > > +			return ret;
> > > +		usleep_range(1500, 2000);
> > > +	}
> > > +
> > 
> > This looks like a duplication of rockchip_usb2phy_power_on(). So I'm assuming
> > that phy_power_on() is not called by the OHCI driver before accessing the
> > registers. So why don't you fix that instead?
> 
> I can look into it. My way of thinking was, that the clock should be
> running independently of that when the clock has been requested via
> common clock framework.

The exact call trace is:

ohci_platform_resume
    -> ohci_platform_resume_common
        -> deassert resets
        -> ohci_platform_power_on -> enable clocks required by controller
        -> ohci_resume
            -> ohci_readl(ohci, &ohci->regs->control); // boom
            -> ...
        -> ...
    -> root hub being resumed will resume the PHY

The ohci_readl results in the mentioned crash, since the clock is
not enabled. The read is used to figure out if the controller is
already running. There is no bus operation, so the PHY is
technically not needed. Of course the controller's clocks have to
be enabled, though. It's just on RK3588 that this means the PHY
must be enabled.

I also checked Rockchip's vendor kernel for their solution: It
solved the problem by using device_link_add() from EHCI to OHCI
based on the RK3588 compatible, so that OHCI is always resumed after
EHCI. This fixes the problem, since both share the same PHY.

I think handling this in rockchip_usb2phy_clk480m_prepare() is the
cleanest option as the disabled clock is the real issue as far as
I can tell. I will look into improving the patch to re-use
rockchip_usb2phy_power_on and improve the commit message.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]