| Message ID | 20230814185043.9252-4-quic_eserrao@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support dwc3 runtime suspend during bus suspend | expand |
Hi Elson, On 14/08/2023 21:50, Elson Roy Serrao wrote: > The current implementation blocks the runtime pm operations when cable > is connected. This would block dwc3 to enter a low power state during > bus suspend scenario. Modify the runtime pm ops to handle bus suspend > case for such platforms where the controller low power mode entry/exit > is handled by the glue driver. This enablement is controlled through a > dt property and platforms capable of detecting bus resume can benefit > from this feature. Also modify the remote wakeup operations to trigger > runtime resume before sending wakeup signal. > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > --- > drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++-- > drivers/usb/dwc3/core.h | 3 +++ > drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- > 3 files changed, 54 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 9c6bf054f15d..9bfd9bb18caf 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->dis_split_quirk = device_property_read_bool(dev, > "snps,dis-split-quirk"); > > + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, > + "snps,runtime-suspend-on-usb-suspend"); > + > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > dwc->tx_de_emphasis = tx_de_emphasis; > > @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > + /* runtime resume on bus resume scenario */ > + if (PMSG_IS_AUTO(msg) && dwc->connected) > + break; > ret = dwc3_core_init_for_resume(dwc); > if (ret) > return ret; > @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) > { > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > - if (dwc->connected) > + if (dwc->connected) { > + /* bus suspend scenario */ > + if (dwc->runtime_suspend_on_usb_suspend && > + dwc->suspended) If dwc is already suspended why do we return -EBUSY? Should this be !dwc->suspended? > + break; > return -EBUSY; > + } > break; > case DWC3_GCTL_PRTCAP_HOST: > default: > @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) > struct dwc3 *dwc = dev_get_drvdata(dev); > int ret; > > - if (dwc3_runtime_checks(dwc)) > + ret = dwc3_runtime_checks(dwc); > + if (ret) > return -EBUSY; > > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > + /* bus suspend case */ > + if (!ret && dwc->connected) No need to check !ret again as it will never happen because we are returning -EBUSY earlier if (ret); > + return 0; > + break; > + case DWC3_GCTL_PRTCAP_HOST: > + default: > + /* do nothing */ > + break; > + } > + While this takes care of runtime suspend case, what about system_suspend? Should this check be moved to dwc3_suspend_common() instead? > ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND); > if (ret) > return ret; > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index a69ac67d89fe..f2f788a6b4b5 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1124,6 +1124,8 @@ struct dwc3_scratchpad_array { > * @num_ep_resized: carries the current number endpoints which have had its tx > * fifo resized. > * @debug_root: root debugfs directory for this device to put its files in. > + * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed > + * during bus suspend scenario. > */ > struct dwc3 { > struct work_struct drd_work; > @@ -1340,6 +1342,7 @@ struct dwc3 { > int last_fifo_depth; > int num_ep_resized; > struct dentry *debug_root; > + bool runtime_suspend_on_usb_suspend; > }; > > #define INCRX_BURST_MODE 0 > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 5fd067151fbf..978ce0e91164 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g) > return -EINVAL; > } > > - spin_lock_irqsave(&dwc->lock, flags); > if (!dwc->gadget->wakeup_armed) { > dev_err(dwc->dev, "not armed for remote wakeup\n"); > - spin_unlock_irqrestore(&dwc->lock, flags); > return -EINVAL; > } > - ret = __dwc3_gadget_wakeup(dwc, true); > > + ret = pm_runtime_resume_and_get(dwc->dev); > + if (ret < 0) { > + pm_runtime_set_suspended(dwc->dev); > + return ret; > + } > + > + spin_lock_irqsave(&dwc->lock, flags); > + ret = __dwc3_gadget_wakeup(dwc, true); > spin_unlock_irqrestore(&dwc->lock, flags); > + pm_runtime_put_noidle(dwc->dev); > > return ret; > } > @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) > return -EINVAL; > } > > + ret = pm_runtime_resume_and_get(dwc->dev); > + if (ret < 0) { > + pm_runtime_set_suspended(dwc->dev); > + return ret; > + } > + > spin_lock_irqsave(&dwc->lock, flags); > /* > * If the link is in U3, signal for remote wakeup and wait for the > @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) > ret = __dwc3_gadget_wakeup(dwc, false); > if (ret) { > spin_unlock_irqrestore(&dwc->lock, flags); > + pm_runtime_put_noidle(dwc->dev); > return -EINVAL; > } > dwc3_resume_gadget(dwc); > @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) > dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret); > > spin_unlock_irqrestore(&dwc->lock, flags); > + pm_runtime_put_noidle(dwc->dev); > > return ret; > } > @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) > /* > * Avoid issuing a runtime resume if the device is already in the > * suspended state during gadget disconnect. DWC3 gadget was already > - * halted/stopped during runtime suspend. > + * halted/stopped during runtime suspend except for bus suspend case > + * where we would have skipped the controller halt. > */ > if (!is_on) { > pm_runtime_barrier(dwc->dev); > - if (pm_runtime_suspended(dwc->dev)) > + if (pm_runtime_suspended(dwc->dev) && !dwc->connected) > return 0; > } > > /* > * Check the return value for successful resume, or error. For a > * successful resume, the DWC3 runtime PM resume routine will handle > - * the run stop sequence, so avoid duplicate operations here. > + * the run stop sequence except for bus resume case, so avoid > + * duplicate operations here. > */ > ret = pm_runtime_get_sync(dwc->dev); > - if (!ret || ret < 0) { > + if ((!ret && !dwc->connected) || ret < 0) { > pm_runtime_put(dwc->dev); > if (ret < 0) > pm_runtime_set_suspended(dwc->dev); > @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc, > } > > dwc->link_state = next; > + pm_runtime_mark_last_busy(dwc->dev); > + pm_request_autosuspend(dwc->dev); > } > > static void dwc3_gadget_interrupt(struct dwc3 *dwc,
On 10/24/2023 3:14 AM, Roger Quadros wrote: > Hi Elson, > > On 14/08/2023 21:50, Elson Roy Serrao wrote: >> The current implementation blocks the runtime pm operations when cable >> is connected. This would block dwc3 to enter a low power state during >> bus suspend scenario. Modify the runtime pm ops to handle bus suspend >> case for such platforms where the controller low power mode entry/exit >> is handled by the glue driver. This enablement is controlled through a >> dt property and platforms capable of detecting bus resume can benefit >> from this feature. Also modify the remote wakeup operations to trigger >> runtime resume before sending wakeup signal. >> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++-- >> drivers/usb/dwc3/core.h | 3 +++ >> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- >> 3 files changed, 54 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 9c6bf054f15d..9bfd9bb18caf 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >> dwc->dis_split_quirk = device_property_read_bool(dev, >> "snps,dis-split-quirk"); >> >> + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, >> + "snps,runtime-suspend-on-usb-suspend"); >> + >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >> dwc->tx_de_emphasis = tx_de_emphasis; >> >> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> + /* runtime resume on bus resume scenario */ >> + if (PMSG_IS_AUTO(msg) && dwc->connected) >> + break; >> ret = dwc3_core_init_for_resume(dwc); >> if (ret) >> return ret; >> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) >> { >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> - if (dwc->connected) >> + if (dwc->connected) { >> + /* bus suspend scenario */ >> + if (dwc->runtime_suspend_on_usb_suspend && >> + dwc->suspended) > > If dwc is already suspended why do we return -EBUSY? > Should this be !dwc->suspended? > Hi Roger Thank you for reviewing. If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0. >> + break; >> return -EBUSY; >> + } >> break; >> case DWC3_GCTL_PRTCAP_HOST: >> default: >> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) >> struct dwc3 *dwc = dev_get_drvdata(dev); >> int ret; >> >> - if (dwc3_runtime_checks(dwc)) >> + ret = dwc3_runtime_checks(dwc); >> + if (ret) >> return -EBUSY; >> >> + switch (dwc->current_dr_role) { >> + case DWC3_GCTL_PRTCAP_DEVICE: >> + /* bus suspend case */ >> + if (!ret && dwc->connected) > > No need to check !ret again as it will never happen because > we are returning -EBUSY earlier if (ret); > Thanks for this catch. I will remove !ret check in v5. >> + return 0; >> + break; >> + case DWC3_GCTL_PRTCAP_HOST: >> + default: >> + /* do nothing */ >> + break; >> + } >> + > > While this takes care of runtime suspend case, what about system_suspend? > Should this check be moved to dwc3_suspend_common() instead? > Sure I can move these checks to dwc3_suspend_common to make it generic. Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback. I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback . I will accommodate above feedback as well and upload v5. Thanks Elson >> ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND); >> if (ret) >> return ret; >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index a69ac67d89fe..f2f788a6b4b5 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -1124,6 +1124,8 @@ struct dwc3_scratchpad_array { >> * @num_ep_resized: carries the current number endpoints which have had its tx >> * fifo resized. >> * @debug_root: root debugfs directory for this device to put its files in. >> + * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed >> + * during bus suspend scenario. >> */ >> struct dwc3 { >> struct work_struct drd_work; >> @@ -1340,6 +1342,7 @@ struct dwc3 { >> int last_fifo_depth; >> int num_ep_resized; >> struct dentry *debug_root; >> + bool runtime_suspend_on_usb_suspend; >> }; >> >> #define INCRX_BURST_MODE 0 >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 5fd067151fbf..978ce0e91164 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g) >> return -EINVAL; >> } >> >> - spin_lock_irqsave(&dwc->lock, flags); >> if (!dwc->gadget->wakeup_armed) { >> dev_err(dwc->dev, "not armed for remote wakeup\n"); >> - spin_unlock_irqrestore(&dwc->lock, flags); >> return -EINVAL; >> } >> - ret = __dwc3_gadget_wakeup(dwc, true); >> >> + ret = pm_runtime_resume_and_get(dwc->dev); >> + if (ret < 0) { >> + pm_runtime_set_suspended(dwc->dev); >> + return ret; >> + } >> + >> + spin_lock_irqsave(&dwc->lock, flags); >> + ret = __dwc3_gadget_wakeup(dwc, true); >> spin_unlock_irqrestore(&dwc->lock, flags); >> + pm_runtime_put_noidle(dwc->dev); >> >> return ret; >> } >> @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) >> return -EINVAL; >> } >> >> + ret = pm_runtime_resume_and_get(dwc->dev); >> + if (ret < 0) { >> + pm_runtime_set_suspended(dwc->dev); >> + return ret; >> + } >> + >> spin_lock_irqsave(&dwc->lock, flags); >> /* >> * If the link is in U3, signal for remote wakeup and wait for the >> @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) >> ret = __dwc3_gadget_wakeup(dwc, false); >> if (ret) { >> spin_unlock_irqrestore(&dwc->lock, flags); >> + pm_runtime_put_noidle(dwc->dev); >> return -EINVAL; >> } >> dwc3_resume_gadget(dwc); >> @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) >> dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret); >> >> spin_unlock_irqrestore(&dwc->lock, flags); >> + pm_runtime_put_noidle(dwc->dev); >> >> return ret; >> } >> @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) >> /* >> * Avoid issuing a runtime resume if the device is already in the >> * suspended state during gadget disconnect. DWC3 gadget was already >> - * halted/stopped during runtime suspend. >> + * halted/stopped during runtime suspend except for bus suspend case >> + * where we would have skipped the controller halt. >> */ >> if (!is_on) { >> pm_runtime_barrier(dwc->dev); >> - if (pm_runtime_suspended(dwc->dev)) >> + if (pm_runtime_suspended(dwc->dev) && !dwc->connected) >> return 0; >> } >> >> /* >> * Check the return value for successful resume, or error. For a >> * successful resume, the DWC3 runtime PM resume routine will handle >> - * the run stop sequence, so avoid duplicate operations here. >> + * the run stop sequence except for bus resume case, so avoid >> + * duplicate operations here. >> */ >> ret = pm_runtime_get_sync(dwc->dev); >> - if (!ret || ret < 0) { >> + if ((!ret && !dwc->connected) || ret < 0) { >> pm_runtime_put(dwc->dev); >> if (ret < 0) >> pm_runtime_set_suspended(dwc->dev); >> @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc, >> } >> >> dwc->link_state = next; >> + pm_runtime_mark_last_busy(dwc->dev); >> + pm_request_autosuspend(dwc->dev); >> } >> >> static void dwc3_gadget_interrupt(struct dwc3 *dwc, >
On 24/10/2023 21:41, Elson Serrao wrote: > > > On 10/24/2023 3:14 AM, Roger Quadros wrote: >> Hi Elson, >> >> On 14/08/2023 21:50, Elson Roy Serrao wrote: >>> The current implementation blocks the runtime pm operations when cable >>> is connected. This would block dwc3 to enter a low power state during >>> bus suspend scenario. Modify the runtime pm ops to handle bus suspend >>> case for such platforms where the controller low power mode entry/exit >>> is handled by the glue driver. This enablement is controlled through a >>> dt property and platforms capable of detecting bus resume can benefit >>> from this feature. Also modify the remote wakeup operations to trigger >>> runtime resume before sending wakeup signal. >>> >>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >>> --- >>> drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++-- >>> drivers/usb/dwc3/core.h | 3 +++ >>> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- >>> 3 files changed, 54 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 9c6bf054f15d..9bfd9bb18caf 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>> dwc->dis_split_quirk = device_property_read_bool(dev, >>> "snps,dis-split-quirk"); >>> + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, >>> + "snps,runtime-suspend-on-usb-suspend"); >>> + >>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>> dwc->tx_de_emphasis = tx_de_emphasis; >>> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>> switch (dwc->current_dr_role) { >>> case DWC3_GCTL_PRTCAP_DEVICE: >>> + /* runtime resume on bus resume scenario */ >>> + if (PMSG_IS_AUTO(msg) && dwc->connected) >>> + break; >>> ret = dwc3_core_init_for_resume(dwc); >>> if (ret) >>> return ret; >>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) >>> { >>> switch (dwc->current_dr_role) { >>> case DWC3_GCTL_PRTCAP_DEVICE: >>> - if (dwc->connected) >>> + if (dwc->connected) { >>> + /* bus suspend scenario */ >>> + if (dwc->runtime_suspend_on_usb_suspend && >>> + dwc->suspended) >> >> If dwc is already suspended why do we return -EBUSY? >> Should this be !dwc->suspended? >> > > Hi Roger > > Thank you for reviewing. > If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0. Of course. I missed the break :) > >>> + break; >>> return -EBUSY; >>> + } >>> break; >>> case DWC3_GCTL_PRTCAP_HOST: >>> default: >>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) >>> struct dwc3 *dwc = dev_get_drvdata(dev); >>> int ret; >>> - if (dwc3_runtime_checks(dwc)) >>> + ret = dwc3_runtime_checks(dwc); >>> + if (ret) >>> return -EBUSY; >>> + switch (dwc->current_dr_role) { >>> + case DWC3_GCTL_PRTCAP_DEVICE: >>> + /* bus suspend case */ >>> + if (!ret && dwc->connected) >> >> No need to check !ret again as it will never happen because >> we are returning -EBUSY earlier if (ret); >> > Thanks for this catch. I will remove !ret check in v5. > >>> + return 0; >>> + break; >>> + case DWC3_GCTL_PRTCAP_HOST: >>> + default: >>> + /* do nothing */ >>> + break; >>> + } >>> + >> >> While this takes care of runtime suspend case, what about system_suspend? >> Should this check be moved to dwc3_suspend_common() instead? >> > > Sure I can move these checks to dwc3_suspend_common to make it generic. Before you do that let's first decide how we want the gadget driver to behave in system_suspend case. Current behavior is to Disconnect from the Host. Earlier I was thinking on the lines that we prevent system suspend if we are not already in USB suspend. But I'm not sure if that is the right thing to do anymore. Mainly because, system suspend is a result of user request and it may not be nice to not to meet his/her request. Maybe best to leave this policy handling to user space? i.e. if user wants USB gadget operation to be alive, he will not issue system suspend? > Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback. > > I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback . > I will accommodate above feedback as well and upload v5. > > Thanks > Elson
On 10/25/2023 1:02 AM, Roger Quadros wrote: > > > On 24/10/2023 21:41, Elson Serrao wrote: >> >> >> On 10/24/2023 3:14 AM, Roger Quadros wrote: >>> Hi Elson, >>> >>> On 14/08/2023 21:50, Elson Roy Serrao wrote: >>>> The current implementation blocks the runtime pm operations when cable >>>> is connected. This would block dwc3 to enter a low power state during >>>> bus suspend scenario. Modify the runtime pm ops to handle bus suspend >>>> case for such platforms where the controller low power mode entry/exit >>>> is handled by the glue driver. This enablement is controlled through a >>>> dt property and platforms capable of detecting bus resume can benefit >>>> from this feature. Also modify the remote wakeup operations to trigger >>>> runtime resume before sending wakeup signal. >>>> >>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >>>> --- >>>> drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++-- >>>> drivers/usb/dwc3/core.h | 3 +++ >>>> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- >>>> 3 files changed, 54 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index 9c6bf054f15d..9bfd9bb18caf 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>> "snps,dis-split-quirk"); >>>> + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, >>>> + "snps,runtime-suspend-on-usb-suspend"); >>>> + >>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>> switch (dwc->current_dr_role) { >>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>> + /* runtime resume on bus resume scenario */ >>>> + if (PMSG_IS_AUTO(msg) && dwc->connected) >>>> + break; >>>> ret = dwc3_core_init_for_resume(dwc); >>>> if (ret) >>>> return ret; >>>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) >>>> { >>>> switch (dwc->current_dr_role) { >>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>> - if (dwc->connected) >>>> + if (dwc->connected) { >>>> + /* bus suspend scenario */ >>>> + if (dwc->runtime_suspend_on_usb_suspend && >>>> + dwc->suspended) >>> >>> If dwc is already suspended why do we return -EBUSY? >>> Should this be !dwc->suspended? >>> >> >> Hi Roger >> >> Thank you for reviewing. >> If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0. > > Of course. I missed the break :) > >> >>>> + break; >>>> return -EBUSY; >>>> + } >>>> break; >>>> case DWC3_GCTL_PRTCAP_HOST: >>>> default: >>>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) >>>> struct dwc3 *dwc = dev_get_drvdata(dev); >>>> int ret; >>>> - if (dwc3_runtime_checks(dwc)) >>>> + ret = dwc3_runtime_checks(dwc); >>>> + if (ret) >>>> return -EBUSY; >>>> + switch (dwc->current_dr_role) { >>>> + case DWC3_GCTL_PRTCAP_DEVICE: >>>> + /* bus suspend case */ >>>> + if (!ret && dwc->connected) >>> >>> No need to check !ret again as it will never happen because >>> we are returning -EBUSY earlier if (ret); >>> >> Thanks for this catch. I will remove !ret check in v5. >> >>>> + return 0; >>>> + break; >>>> + case DWC3_GCTL_PRTCAP_HOST: >>>> + default: >>>> + /* do nothing */ >>>> + break; >>>> + } >>>> + >>> >>> While this takes care of runtime suspend case, what about system_suspend? >>> Should this check be moved to dwc3_suspend_common() instead? >>> >> >> Sure I can move these checks to dwc3_suspend_common to make it generic. > > Before you do that let's first decide how we want the gadget driver to behave > in system_suspend case. > > Current behavior is to Disconnect from the Host. > > Earlier I was thinking on the lines that we prevent system suspend if > we are not already in USB suspend. But I'm not sure if that is the right > thing to do anymore. Mainly because, system suspend is a result of user > request and it may not be nice to not to meet his/her request. Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user. > Maybe best to leave this policy handling to user space? > i.e. if user wants USB gadget operation to be alive, he will not issue > system suspend? > Sure. So below two cases Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT Case2: User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT. For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common() Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1). Thanks Elson > >> Will rename this patch to "Modify pm ops to handle bus suspend" since this is now not limited to only runtime suspend/resume. Will also rename dwc->runtime_suspend_on_usb_suspend to dwc->delegate_wakeup_interrupt based on earlier feedback. >> >> I am still working on a clean way to enable/disable this feature (i.e set dwc->delegate_wakeup_interrupt flag) from the glue driver based on Thinh's feedback . >> I will accommodate above feedback as well and upload v5. >> >> Thanks >> Elson >
On 26/10/2023 01:21, Elson Serrao wrote: > > > On 10/25/2023 1:02 AM, Roger Quadros wrote: >> >> >> On 24/10/2023 21:41, Elson Serrao wrote: >>> >>> >>> On 10/24/2023 3:14 AM, Roger Quadros wrote: >>>> Hi Elson, >>>> >>>> On 14/08/2023 21:50, Elson Roy Serrao wrote: >>>>> The current implementation blocks the runtime pm operations when cable >>>>> is connected. This would block dwc3 to enter a low power state during >>>>> bus suspend scenario. Modify the runtime pm ops to handle bus suspend >>>>> case for such platforms where the controller low power mode entry/exit >>>>> is handled by the glue driver. This enablement is controlled through a >>>>> dt property and platforms capable of detecting bus resume can benefit >>>>> from this feature. Also modify the remote wakeup operations to trigger >>>>> runtime resume before sending wakeup signal. >>>>> >>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++-- >>>>> drivers/usb/dwc3/core.h | 3 +++ >>>>> drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- >>>>> 3 files changed, 54 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 9c6bf054f15d..9bfd9bb18caf 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>> "snps,dis-split-quirk"); >>>>> + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, >>>>> + "snps,runtime-suspend-on-usb-suspend"); >>>>> + >>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>> @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>> switch (dwc->current_dr_role) { >>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>> + /* runtime resume on bus resume scenario */ >>>>> + if (PMSG_IS_AUTO(msg) && dwc->connected) >>>>> + break; >>>>> ret = dwc3_core_init_for_resume(dwc); >>>>> if (ret) >>>>> return ret; >>>>> @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) >>>>> { >>>>> switch (dwc->current_dr_role) { >>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>> - if (dwc->connected) >>>>> + if (dwc->connected) { >>>>> + /* bus suspend scenario */ >>>>> + if (dwc->runtime_suspend_on_usb_suspend && >>>>> + dwc->suspended) >>>> >>>> If dwc is already suspended why do we return -EBUSY? >>>> Should this be !dwc->suspended? >>>> >>> >>> Hi Roger >>> >>> Thank you for reviewing. >>> If dwc->suspended is true (i.e suspend event due to U3/L2 is received), I am actually breaking from this switch statement and returning 0. >> >> Of course. I missed the break :) >> >>> >>>>> + break; >>>>> return -EBUSY; >>>>> + } >>>>> break; >>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>> default: >>>>> @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) >>>>> struct dwc3 *dwc = dev_get_drvdata(dev); >>>>> int ret; >>>>> - if (dwc3_runtime_checks(dwc)) >>>>> + ret = dwc3_runtime_checks(dwc); >>>>> + if (ret) >>>>> return -EBUSY; >>>>> + switch (dwc->current_dr_role) { >>>>> + case DWC3_GCTL_PRTCAP_DEVICE: >>>>> + /* bus suspend case */ >>>>> + if (!ret && dwc->connected) >>>> >>>> No need to check !ret again as it will never happen because >>>> we are returning -EBUSY earlier if (ret); >>>> >>> Thanks for this catch. I will remove !ret check in v5. >>> >>>>> + return 0; >>>>> + break; >>>>> + case DWC3_GCTL_PRTCAP_HOST: >>>>> + default: >>>>> + /* do nothing */ >>>>> + break; >>>>> + } >>>>> + >>>> >>>> While this takes care of runtime suspend case, what about system_suspend? >>>> Should this check be moved to dwc3_suspend_common() instead? >>>> >>> >>> Sure I can move these checks to dwc3_suspend_common to make it generic. >> >> Before you do that let's first decide how we want the gadget driver to behave >> in system_suspend case. >> >> Current behavior is to Disconnect from the Host. >> >> Earlier I was thinking on the lines that we prevent system suspend if >> we are not already in USB suspend. But I'm not sure if that is the right >> thing to do anymore. Mainly because, system suspend is a result of user >> request and it may not be nice to not to meet his/her request. > > Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user. > >> Maybe best to leave this policy handling to user space? >> i.e. if user wants USB gadget operation to be alive, he will not issue >> system suspend? >> > > Sure. So below two cases > > Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT > > Case2: User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT. > > For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common() > > Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1). But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event? Why do we need separate flags for?
>>>>> >>>>> While this takes care of runtime suspend case, what about system_suspend? >>>>> Should this check be moved to dwc3_suspend_common() instead? >>>>> >>>> >>>> Sure I can move these checks to dwc3_suspend_common to make it generic. >>> >>> Before you do that let's first decide how we want the gadget driver to behave >>> in system_suspend case. >>> >>> Current behavior is to Disconnect from the Host. >>> >>> Earlier I was thinking on the lines that we prevent system suspend if >>> we are not already in USB suspend. But I'm not sure if that is the right >>> thing to do anymore. Mainly because, system suspend is a result of user >>> request and it may not be nice to not to meet his/her request. >> >> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user. >> >>> Maybe best to leave this policy handling to user space? >>> i.e. if user wants USB gadget operation to be alive, he will not issue >>> system suspend? >>> >> >> Sure. So below two cases >> >> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT >> >> Case2: User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT. >> >> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common() >> >> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1). > > But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event? > Why do we need separate flags for? > Sorry let me clarify. This is in reference to deciding how we want the dwc3 driver to behave in system_suspend case. One option is to continue with the existing behavior where USB gadget would disconnect from Host irrespective of bus suspend state. We dont need any modification in this case and we can leave this series limited to runtime suspend only. Second option is to stay connected IF we are in bus suspend state (U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The main motivation is to preserve the ongoing usb session without going through a re-enumeration (ofcourse true only if we are in bus suspend state). This would need relevant checks in suspend_common(). Which option do you think is more suitable? IMO option2 is better. For example if we are in a scenario where there is a network session (over USB) open between Host and the device and usb bus is suspended due to data inactivity. Option2 would preserve the session whereas Option1 we would terminate this session when a system_suspend happens. Thanks Elson
On 27/10/2023 03:07, Elson Serrao wrote: > > > >>>>>> >>>>>> While this takes care of runtime suspend case, what about system_suspend? >>>>>> Should this check be moved to dwc3_suspend_common() instead? >>>>>> >>>>> >>>>> Sure I can move these checks to dwc3_suspend_common to make it generic. >>>> >>>> Before you do that let's first decide how we want the gadget driver to behave >>>> in system_suspend case. >>>> >>>> Current behavior is to Disconnect from the Host. >>>> >>>> Earlier I was thinking on the lines that we prevent system suspend if >>>> we are not already in USB suspend. But I'm not sure if that is the right >>>> thing to do anymore. Mainly because, system suspend is a result of user >>>> request and it may not be nice to not to meet his/her request. >>> >>> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user. >>> >>>> Maybe best to leave this policy handling to user space? >>>> i.e. if user wants USB gadget operation to be alive, he will not issue >>>> system suspend? >>>> >>> >>> Sure. So below two cases >>> >>> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT >>> >>> Case2: User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT. >>> >>> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common() >>> >>> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1). >> >> But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event? >> Why do we need separate flags for? >> > > Sorry let me clarify. This is in reference to deciding how we want the dwc3 driver to behave in system_suspend case. > > One option is to continue with the existing behavior where USB gadget would disconnect from Host irrespective of bus suspend state. We dont need any modification in this case and we can leave this series limited to runtime suspend only. > > Second option is to stay connected IF we are in bus suspend state (U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The main motivation is to preserve the ongoing usb session > without going through a re-enumeration (ofcourse true only if we are in bus suspend state). This would need relevant checks in suspend_common(). The catch here is, what to do if the USB device is not in bus suspend state but user wants to put the system in suspend state? Do we still disconnect? You might also want to refer to the discussion in [1] [1] - https://lore.kernel.org/all/Y+z9NK6AyhvTQMir@rowland.harvard.edu/ > > Which option do you think is more suitable? IMO option2 is better. For example if we are in a scenario where there is a network session (over USB) open between Host and the device and usb bus is suspended due to data inactivity. Option2 would preserve the session whereas Option1 we would terminate this session when a system_suspend happens. > > Thanks > Elson
On 10/26/2023 11:37 PM, Roger Quadros wrote: > > > On 27/10/2023 03:07, Elson Serrao wrote: >> >> >> >>>>>>> >>>>>>> While this takes care of runtime suspend case, what about system_suspend? >>>>>>> Should this check be moved to dwc3_suspend_common() instead? >>>>>>> >>>>>> >>>>>> Sure I can move these checks to dwc3_suspend_common to make it generic. >>>>> >>>>> Before you do that let's first decide how we want the gadget driver to behave >>>>> in system_suspend case. >>>>> >>>>> Current behavior is to Disconnect from the Host. >>>>> >>>>> Earlier I was thinking on the lines that we prevent system suspend if >>>>> we are not already in USB suspend. But I'm not sure if that is the right >>>>> thing to do anymore. Mainly because, system suspend is a result of user >>>>> request and it may not be nice to not to meet his/her request. >>>> >>>> Agree. Irrespective of whether USB is suspended or not it is better to honor the system suspend request from user. >>>> >>>>> Maybe best to leave this policy handling to user space? >>>>> i.e. if user wants USB gadget operation to be alive, he will not issue >>>>> system suspend? >>>>> >>>> >>>> Sure. So below two cases >>>> >>>> Case1: User doesn't care if gadget operation is alive and triggers system suspend irrespective of USB suspend. Like you mentioned, current behavior already takes care of this and initiates a DISCONNECT >>>> >>>> Case2: User wants gadget to stay alive and hence can trigger system suspend only when USB is suspended (there are already user space hooks that read cdev->suspended bit to tell whether USB is suspended or not for user to decide). Attempts to request system suspend when USB is not suspended, would result in a DISCONNECT. >>>> >>>> For supporting Case2 from gadget driver point of view, we need to extend this series by having relevant checks in suspend_common() >>>> >>>> Also, is it better to provide separate flags to control the gadget driver behavior for runtime suspend Vs system suspend when USB is suspended ? For example, what if we want to enable bus suspend handling for runtime suspend only and not for system suspend (Case1). >>> >>> But you mentioned that for Case1, USB gadget would disconnect from Host. So USB will be in disconnected state and USB controller can be fully de-activated? Except maybe wakeup handling to bring system out of suspend on a USB plug/unplug event? >>> Why do we need separate flags for? >>> >> >> Sorry let me clarify. This is in reference to deciding how we want the dwc3 driver to behave in system_suspend case. >> >> One option is to continue with the existing behavior where USB gadget would disconnect from Host irrespective of bus suspend state. We dont need any modification in this case and we can leave this series limited to runtime suspend only. >> >> Second option is to stay connected IF we are in bus suspend state (U3/L2) otherwise DISCONNECT IF we are not in bus suspend state. The main motivation is to preserve the ongoing usb session >> without going through a re-enumeration (ofcourse true only if we are in bus suspend state). This would need relevant checks in suspend_common(). > > The catch here is, what to do if the USB device is not in bus suspend state but user wants to put the system in suspend state? Do we still disconnect? > > You might also want to refer to the discussion in [1] > > [1] - https://lore.kernel.org/all/Y+z9NK6AyhvTQMir@rowland.harvard.edu/ > Thanks for the details. If we dont DISCONNECT when the USB link is NOT in bus suspend, the other alternatives we have are below ones 1.) Abort system_suspend: In addition to not honoring the users request to put the system in suspend, there is always a possibility of host driver never sending bus suspend interrupt. If that happens we would be denying system_suspend every time user requests it. So this is not a right approach. 2.) Keep the gadget connected and proceed with system_suspend: Now the system is suspended but from host point of view the USB link is active and would continue the communication normally. The signalling probably is not going to to be detected as a wakeup by the Rx hardware as there is no explicit resume signal involved here . The only exception is if we can somehow configure each and every event from the host as a wakeup signal(not sure if this is even possible) So IMO it is best to DISCONNECT from the host when USB link is NOT in bus suspend state and user requests system_suspend. Thanks Elson
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 9c6bf054f15d..9bfd9bb18caf 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1518,6 +1518,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->dis_split_quirk = device_property_read_bool(dev, "snps,dis-split-quirk"); + dwc->runtime_suspend_on_usb_suspend = device_property_read_bool(dev, + "snps,runtime-suspend-on-usb-suspend"); + dwc->lpm_nyet_threshold = lpm_nyet_threshold; dwc->tx_de_emphasis = tx_de_emphasis; @@ -2029,6 +2032,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: + /* runtime resume on bus resume scenario */ + if (PMSG_IS_AUTO(msg) && dwc->connected) + break; ret = dwc3_core_init_for_resume(dwc); if (ret) return ret; @@ -2090,8 +2096,13 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) { switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: - if (dwc->connected) + if (dwc->connected) { + /* bus suspend scenario */ + if (dwc->runtime_suspend_on_usb_suspend && + dwc->suspended) + break; return -EBUSY; + } break; case DWC3_GCTL_PRTCAP_HOST: default: @@ -2107,9 +2118,22 @@ static int dwc3_runtime_suspend(struct device *dev) struct dwc3 *dwc = dev_get_drvdata(dev); int ret; - if (dwc3_runtime_checks(dwc)) + ret = dwc3_runtime_checks(dwc); + if (ret) return -EBUSY; + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_DEVICE: + /* bus suspend case */ + if (!ret && dwc->connected) + return 0; + break; + case DWC3_GCTL_PRTCAP_HOST: + default: + /* do nothing */ + break; + } + ret = dwc3_suspend_common(dwc, PMSG_AUTO_SUSPEND); if (ret) return ret; diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index a69ac67d89fe..f2f788a6b4b5 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1124,6 +1124,8 @@ struct dwc3_scratchpad_array { * @num_ep_resized: carries the current number endpoints which have had its tx * fifo resized. * @debug_root: root debugfs directory for this device to put its files in. + * @runtime_suspend_on_usb_suspend: true if dwc3 runtime suspend is allowed + * during bus suspend scenario. */ struct dwc3 { struct work_struct drd_work; @@ -1340,6 +1342,7 @@ struct dwc3 { int last_fifo_depth; int num_ep_resized; struct dentry *debug_root; + bool runtime_suspend_on_usb_suspend; }; #define INCRX_BURST_MODE 0 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 5fd067151fbf..978ce0e91164 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2401,15 +2401,21 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g) return -EINVAL; } - spin_lock_irqsave(&dwc->lock, flags); if (!dwc->gadget->wakeup_armed) { dev_err(dwc->dev, "not armed for remote wakeup\n"); - spin_unlock_irqrestore(&dwc->lock, flags); return -EINVAL; } - ret = __dwc3_gadget_wakeup(dwc, true); + ret = pm_runtime_resume_and_get(dwc->dev); + if (ret < 0) { + pm_runtime_set_suspended(dwc->dev); + return ret; + } + + spin_lock_irqsave(&dwc->lock, flags); + ret = __dwc3_gadget_wakeup(dwc, true); spin_unlock_irqrestore(&dwc->lock, flags); + pm_runtime_put_noidle(dwc->dev); return ret; } @@ -2428,6 +2434,12 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) return -EINVAL; } + ret = pm_runtime_resume_and_get(dwc->dev); + if (ret < 0) { + pm_runtime_set_suspended(dwc->dev); + return ret; + } + spin_lock_irqsave(&dwc->lock, flags); /* * If the link is in U3, signal for remote wakeup and wait for the @@ -2438,6 +2450,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) ret = __dwc3_gadget_wakeup(dwc, false); if (ret) { spin_unlock_irqrestore(&dwc->lock, flags); + pm_runtime_put_noidle(dwc->dev); return -EINVAL; } dwc3_resume_gadget(dwc); @@ -2452,6 +2465,7 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id) dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret); spin_unlock_irqrestore(&dwc->lock, flags); + pm_runtime_put_noidle(dwc->dev); return ret; } @@ -2732,21 +2746,23 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) /* * Avoid issuing a runtime resume if the device is already in the * suspended state during gadget disconnect. DWC3 gadget was already - * halted/stopped during runtime suspend. + * halted/stopped during runtime suspend except for bus suspend case + * where we would have skipped the controller halt. */ if (!is_on) { pm_runtime_barrier(dwc->dev); - if (pm_runtime_suspended(dwc->dev)) + if (pm_runtime_suspended(dwc->dev) && !dwc->connected) return 0; } /* * Check the return value for successful resume, or error. For a * successful resume, the DWC3 runtime PM resume routine will handle - * the run stop sequence, so avoid duplicate operations here. + * the run stop sequence except for bus resume case, so avoid + * duplicate operations here. */ ret = pm_runtime_get_sync(dwc->dev); - if (!ret || ret < 0) { + if ((!ret && !dwc->connected) || ret < 0) { pm_runtime_put(dwc->dev); if (ret < 0) pm_runtime_set_suspended(dwc->dev); @@ -4331,6 +4347,8 @@ static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc, } dwc->link_state = next; + pm_runtime_mark_last_busy(dwc->dev); + pm_request_autosuspend(dwc->dev); } static void dwc3_gadget_interrupt(struct dwc3 *dwc,
The current implementation blocks the runtime pm operations when cable is connected. This would block dwc3 to enter a low power state during bus suspend scenario. Modify the runtime pm ops to handle bus suspend case for such platforms where the controller low power mode entry/exit is handled by the glue driver. This enablement is controlled through a dt property and platforms capable of detecting bus resume can benefit from this feature. Also modify the remote wakeup operations to trigger runtime resume before sending wakeup signal. Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> --- drivers/usb/dwc3/core.c | 28 ++++++++++++++++++++++++++-- drivers/usb/dwc3/core.h | 3 +++ drivers/usb/dwc3/gadget.c | 32 +++++++++++++++++++++++++------- 3 files changed, 54 insertions(+), 9 deletions(-)