[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <02bfd468-5f92-4d06-bc90-f138c5153ee0@oss.qualcomm.com>
Date: Tue, 10 Jun 2025 22:10:33 +0530
From: Krishna Kurapati <krishna.kurapati@....qualcomm.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/4] usb: dwc3: qcom: Facilitate autosuspend during
host mode
On 6/10/2025 4:30 PM, Dmitry Baryshkov wrote:
> On Tue, Jun 10, 2025 at 02:43:56PM +0530, 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@....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)
>> + goto done;
>> +
>> + if (event == USB_BUS_ADD) {
>> + /*
>> + * Identify instant of creation of primary hcd and
>> + * mark xhci as autosuspend capable at this point.
>> + */
>> + pm_runtime_use_autosuspend(&dwc->xhci->dev);
>
> This feels like an overkill, using notifiers to set autosuspend flag.
> Please extend platform data and/or other static code in order to set the
> flag on the created xHCI devices.
>
Do you mean modifying pdev of xhci from dwc3/host.c ? Or adding the
use_autosuspend call in xhci-plat.c ?
I thought adding this notifier would be a better way to identify when
the xhci probe is in progress instead of touching pdev of xhci device
from dwc3 layer.
Regards,
Krishna,
>> + }
>> +
>> +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);
>> + }
>>
>> 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
>>
Powered by blists - more mailing lists