Re: [PATCH v16 4/6] usb: dwc3: rockchip: support PHY reset notifications

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Thu, Sep 24, 2026, Sebastian Reichel wrote:
> On recent Rockchip platforms (at least RK3588 & RK3576), DWC3 IP is used
> with a USBDP PHY providing USB3 and DP. This PHY needs to be reset when
> the mode changes, which may happen when plugging in different USB-C
> devices.
> 
> If the USBDP PHY resets with the DWC3 IP running, its internal state
> corrupts resulting in the USBDP PHY not being able to lock some PLL
> clocks, which effectively renders USB3 unusable.
> 
> To fix the issue this adds handling for the new PHY framework reset
> notifications, which will assert PHYSOFTRST before the actual PHY
> is disabled and will deassert it once the PHY returns.
> 
> Tested-by: Igor Paunovic <royalnet026@gmail.com> # Orange Pi 5 Plus
> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/usb/dwc3/dwc3-rockchip.c | 127 ++++++++++++++++++++++++++++++++++++++-
>  drivers/usb/dwc3/trace.c         |   3 +
>  2 files changed, 129 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-rockchip.c b/drivers/usb/dwc3/dwc3-rockchip.c
> index 4129aefd6a8b..17b6c2ca9e19 100644
> --- a/drivers/usb/dwc3/dwc3-rockchip.c
> +++ b/drivers/usb/dwc3/dwc3-rockchip.c
> @@ -2,11 +2,136 @@
>  /* Copyright (c) 2026, Collabora Ltd. */
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/phy/phy.h>
>  #include <linux/pm_runtime.h>
>  #include "glue.h"
> +#include "io.h"
> +
> +struct dwc3_rockchip;
> +
> +/**
> + * struct dwc3_rk_phy_nb - wrapper for PHY notifier block
> + * @nb: notifier block
> + * @dwc: back-pointer to the DWC3 controller

I missed this from the last review: dwc here is supposed to be dwc_rk.

> + * @port_index: USB3 port index this notifier is registered for
> + */
> +struct dwc3_rk_phy_nb {
> +	struct notifier_block	nb;
> +	struct dwc3_rockchip	*dwc_rk;
> +	u8			port_index;
> +};
>  
>  struct dwc3_rockchip {
>  	struct dwc3		dwc;
> +	struct dwc3_rk_phy_nb	usb3_phy_nb[DWC3_USB3_MAX_PORTS];
> +	u8			phy_reset_active;
> +};
> +
> +static int dwc3_usb3_phy_notify(struct notifier_block *nb,
> +				unsigned long action, void *data)
> +{
> +	struct dwc3_rk_phy_nb *pnb = container_of(nb, struct dwc3_rk_phy_nb, nb);
> +	struct dwc3_rockchip *dwc_rk = pnb->dwc_rk;
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	int port = pnb->port_index;
> +	unsigned long flags;
> +	u32 reg;
> +	int ret;
> +
> +	switch (action) {
> +	case PHY_NOTIFY_PRE_RESET:
> +		/*
> +		 * If already suspended, the resume path will reinit GUSB3PIPECTL
> +		 * via dwc3_core_init(). A forced resume is not possible as that
> +		 * would call phy_init() resulting in a deadlock. Due to the
> +		 * phy_init() in the resume path there is also no need to block
> +		 * async RPM resume on our side, since the PHY synchronizes it
> +		 * for us.
> +		 *
> +		 * pm_runtime_get_if_active() returns 0 when suspended (skip),
> +		 * 1 when active (ref held), or -EINVAL when PM is disabled
> +		 * (device always active). In the -EINVAL case PM ref counting
> +		 * is a no-op, so the unconditional put in POST_RESET is safe.

The -EINVAL case is not no-op here. No reference is taken, but
pm_runtime_put_autosuspend() in POST_RESET still decrements the usage
count, causing underflow or taking some other reference. Should we skip
if ret < 0 too?

> +		 */
> +		ret = pm_runtime_get_if_active(dwc->dev);
> +		if (!ret)
> +			return NOTIFY_OK;
> +
> +		/*
> +		 * Assert USB3 PHY soft reset within DWC3 before the external
> +		 * PHY resets. This disconnects the PIPE interface, preventing
> +		 * the DWC3 from interfering with PHY reinitialization and
> +		 * avoiding LCPLL lock failures.
> +		 */
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc_rk->phy_reset_active |= BIT(port);
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg |= DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		break;
> +
> +	case PHY_NOTIFY_POST_RESET:
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		if (!(dwc_rk->phy_reset_active & BIT(port))) {
> +			spin_unlock_irqrestore(&dwc->lock, flags);
> +			return NOTIFY_OK;
> +		}
> +
> +		dwc_rk->phy_reset_active &= ~BIT(port);
> +
> +		/*
> +		 * Deassert PHY soft reset to reconnect the PIPE interface
> +		 * after PHY reinitialization.
> +		 */
> +		reg = dwc3_readl(dwc, DWC3_GUSB3PIPECTL(port));
> +		reg &= ~DWC3_GUSB3PIPECTL_PHYSOFTRST;
> +		dwc3_writel(dwc, DWC3_GUSB3PIPECTL(port), reg);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +		pm_runtime_put_autosuspend(dwc->dev);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static void dwc3_rk_phy_unregister_notifiers(void *data)

BTW, when does this get executed? Could it be executed after the
dwc3_core_exit() powering off the phy and turning off the clocks? The
reset notifier may still be called in this case and read dwc3 registers.
Can we make sure this is done before dwc3_core_remove() in
dwc3_rockchip_remove()?

Thanks,
Thinh

> +{
> +	struct dwc3_rockchip *dwc_rk = data;
> +	struct dwc3 *dwc = &dwc_rk->dwc;
> +	int i;
> +
> +	for (i = 0; i < dwc->num_usb3_ports; i++)
> +		phy_unregister_notifier(dwc->usb3_generic_phy[i],
> +					&dwc_rk->usb3_phy_nb[i].nb);
> +
> +	/* Release any PM references from in-flight resets */
> +	for (i = 0; i < dwc->num_usb3_ports; i++) {
> +		if (dwc_rk->phy_reset_active & BIT(i))
> +			pm_runtime_put_autosuspend(dwc->dev);
> +	}
> +	dwc_rk->phy_reset_active = 0;
> +}
> +
> +static int dwc3_rk_phy_register_notifiers(struct dwc3 *dwc)
> +{
> +	struct dwc3_rockchip *dwc_rk = container_of(dwc, struct dwc3_rockchip, dwc);
> +	int i;
> +
> +	for (i = 0; i < dwc->num_usb3_ports; i++) {
> +		dwc_rk->usb3_phy_nb[i].nb.notifier_call = dwc3_usb3_phy_notify;
> +		dwc_rk->usb3_phy_nb[i].dwc_rk = dwc_rk;
> +		dwc_rk->usb3_phy_nb[i].port_index = i;
> +		phy_register_notifier(dwc->usb3_generic_phy[i],
> +				      &dwc_rk->usb3_phy_nb[i].nb);
> +	}
> +
> +	return devm_add_action_or_reset(dwc->dev, dwc3_rk_phy_unregister_notifiers, dwc_rk);
> +}
> +
> +static struct dwc3_glue_ops dwc3_rockchip_glue_ops = {
> +	.post_phy_registration = dwc3_rk_phy_register_notifiers,
>  };
>  
>  static int dwc3_rockchip_probe(struct platform_device *pdev)
> @@ -26,7 +151,7 @@ static int dwc3_rockchip_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	dwc_rk->dwc.dev = &pdev->dev;
> -	dwc_rk->dwc.glue_ops = NULL;
> +	dwc_rk->dwc.glue_ops = &dwc3_rockchip_glue_ops;
>  
>  	probe_data.dwc = &dwc_rk->dwc;
>  	probe_data.res = res;
> diff --git a/drivers/usb/dwc3/trace.c b/drivers/usb/dwc3/trace.c
> index 088995885678..8c4e2a7b142e 100644
> --- a/drivers/usb/dwc3/trace.c
> +++ b/drivers/usb/dwc3/trace.c
> @@ -9,3 +9,6 @@
>  
>  #define CREATE_TRACE_POINTS
>  #include "trace.h"
> +
> +EXPORT_TRACEPOINT_SYMBOL_GPL(dwc3_readl);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(dwc3_writel);
> 
> -- 
> 2.53.0
>