Re: [PATCH v2 3/4] usb: dwc3: qcom: Facilitate autosuspend during host mode

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

On Tue, Jun 10, 2025, Krishna Kurapati wrote:
> When in host mode, it is intended that the controller goes to suspend
> state to save power and wait for interrupts from connected peripheral
> to wake it up. This is particularly used in cases where a HID or Audio
> device is connected. In such scenarios, the usb controller can enter
> auto suspend and resume action after getting interrupts from the
> connected device.
> 
> Allow autosuspend for and xhci device and allow userspace to decide
> whether to enable this functionality.
> 
> a) Register to usb-core notifications in set_role vendor callback to
> identify when root hubs are being created. Configure them to
> use_autosuspend.
> 
> b) Identify usb core notifications where the HCD is being added and
> enable autosuspend for that particular xhci device.
> 
> Signed-off-by: Krishna Kurapati <krishna.kurapati@oss.qualcomm.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 62 ++++++++++++++++++++++++++++++++----
>  1 file changed, 56 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index d40b52e2ba01..17bbd5a06c08 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -95,6 +95,8 @@ struct dwc3_qcom {
>  	 * internally by mutex lock.
>  	 */
>  	enum usb_role		current_role;
> +
> +	struct notifier_block	xhci_nb;
>  };
>  
>  #define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
> @@ -647,6 +649,39 @@ static int dwc3_qcom_setup_irq(struct dwc3_qcom *qcom, struct platform_device *p
>  	return 0;
>  }
>  
> +static int dwc3_xhci_event_notifier(struct notifier_block *nb,
> +				    unsigned long event, void *ptr)
> +{
> +	struct dwc3_qcom  *qcom	= container_of(nb, struct dwc3_qcom, xhci_nb);
> +	struct dwc3	  *dwc	= &qcom->dwc;
> +	struct usb_bus	  *ubus	= ptr;
> +	struct usb_hcd	  *hcd;
> +
> +	if (!dwc->xhci)
> +		goto done;
> +
> +	hcd = platform_get_drvdata(dwc->xhci);
> +	if (!hcd)
> +		goto done;
> +
> +	if (event != USB_BUS_ADD)
> +		goto done;
> +
> +	if (strcmp(dev_name(ubus->sysdev), dev_name(dwc->sysdev)) != 0)

Can this be false? If possible, I'd like to avoid these pointers and
strcmp here.

> +		goto done;
> +
> +	if (event == USB_BUS_ADD) {

This condition is redundant when you have the check a few lines above.

> +		/*
> +		 * Identify instant of creation of primary hcd and
> +		 * mark xhci as autosuspend capable at this point.
> +		 */
> +		pm_runtime_use_autosuspend(&dwc->xhci->dev);
> +	}
> +
> +done:
> +	return NOTIFY_DONE;
> +}
> +
>  static void dwc3_qcom_set_role_notifier(struct dwc3 *dwc, enum usb_role next_role)
>  {
>  	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> @@ -659,12 +694,22 @@ static void dwc3_qcom_set_role_notifier(struct dwc3 *dwc, enum usb_role next_rol
>  		return;
>  	}
>  
> -	if (qcom->current_role == USB_ROLE_DEVICE &&
> -	    next_role != USB_ROLE_DEVICE)
> +	if (qcom->current_role == USB_ROLE_NONE) {
> +		if (next_role == USB_ROLE_DEVICE) {
> +			dwc3_qcom_vbus_override_enable(qcom, true);
> +		} else if (next_role == USB_ROLE_HOST) {
> +			qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
> +			usb_register_notify(&qcom->xhci_nb);
> +		}
> +	} else if (qcom->current_role == USB_ROLE_DEVICE &&
> +		   next_role != USB_ROLE_DEVICE) {
>  		dwc3_qcom_vbus_override_enable(qcom, false);
> -	else if ((qcom->current_role != USB_ROLE_DEVICE) &&
> -		 (next_role == USB_ROLE_DEVICE))
> -		dwc3_qcom_vbus_override_enable(qcom, true);
> +	} else if (qcom->current_role == USB_ROLE_HOST) {
> +		if (next_role == USB_ROLE_NONE)
> +			usb_unregister_notify(&qcom->xhci_nb);
> +		else if (next_role == USB_ROLE_DEVICE)
> +			dwc3_qcom_vbus_override_enable(qcom, true);

We don't unregister the notifier when switching from host to device?

> +	}
>  
>  	pm_runtime_mark_last_busy(qcom->dev);
>  	pm_runtime_put_sync(qcom->dev);
> @@ -774,6 +819,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  	if (qcom->mode == USB_DR_MODE_HOST) {
>  		qcom->current_role = USB_ROLE_HOST;
> +		qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
> +		usb_register_notify(&qcom->xhci_nb);
>  	} else if (qcom->mode == USB_DR_MODE_PERIPHERAL) {
>  		qcom->current_role = USB_ROLE_DEVICE;
>  		dwc3_qcom_vbus_override_enable(qcom, true);
> @@ -794,7 +841,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	ret = dwc3_core_probe(&probe_data);
>  	if (ret)  {
>  		ret = dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
> -		goto clk_disable;
> +		goto unregister_notify;
>  	}
>  
>  	ret = dwc3_qcom_interconnect_init(qcom);
> @@ -817,6 +864,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	dwc3_qcom_interconnect_exit(qcom);
>  remove_core:
>  	dwc3_core_remove(&qcom->dwc);
> +unregister_notify:
> +	if (qcom->mode == USB_DR_MODE_HOST)
> +		usb_unregister_notify(&qcom->xhci_nb);
>  clk_disable:
>  	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
>  
> -- 
> 2.34.1
> 

BR,
Thinh