On 21/03/2023 21:05, Thinh Nguyen wrote:
> On Tue, Mar 21, 2023, Thinh Nguyen wrote:
>> On Tue, Mar 21, 2023, Roger Quadros wrote:
>>> Hi Thinh,
>>>
>>> On 20/03/2023 20:52, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> On Mon, Mar 20, 2023, Roger Quadros wrote:
>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property.
>>>>>
>>>>> Do not stop the gadget controller and disconnect if this
>>>>> property is present and we are connected to a USB Host.
>>>>>
>>>>> Prevent System sleep if Gadget is not in USB suspend.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@xxxxxxxxxx>
>>>>> ---
>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------
>>>>> drivers/usb/dwc3/core.h | 2 ++
>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++--
>>>>> 3 files changed, 44 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 476b63618511..a47bbaa27302 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>> dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>> "snps,dis-split-quirk");
>>>>>
>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev,
>>>>> + "snps,gadget-keep-connect-sys-sleep");
>>>>> +
>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>>>> dwc->tx_de_emphasis = tx_de_emphasis;
>>>>>
>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>>>> {
>>>>> unsigned long flags;
>>>>> u32 reg;
>>>>> + int ret;
>>>>>
>>>>> switch (dwc->current_dr_role) {
>>>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> if (pm_runtime_suspended(dwc->dev))
>>>>> break;
>>>>> - dwc3_gadget_suspend(dwc);
>>>>> + ret = dwc3_gadget_suspend(dwc);
>>>>> + if (ret) {
>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> synchronize_irq(dwc->irq_gadget);
>>>>> - dwc3_core_exit(dwc);
>>>>> + if(!dwc->gadget_keep_connect_sys_sleep)
>>>>> + dwc3_core_exit(dwc);
>>>>> break;
>>>>> case DWC3_GCTL_PRTCAP_HOST:
>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) {
>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>>>>
>>>>> switch (dwc->current_dr_role) {
>>>>> case DWC3_GCTL_PRTCAP_DEVICE:
>>>>> - ret = dwc3_core_init_for_resume(dwc);
>>>>> - if (ret)
>>>>> - return ret;
>>>>> + if (!dwc->gadget_keep_connect_sys_sleep)
>>>>> + {
>>>>> + ret = dwc3_core_init_for_resume(dwc);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>>>> + }
>>>>>
>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>>>> dwc3_gadget_resume(dwc);
>>>>> break;
>>>>> case DWC3_GCTL_PRTCAP_HOST:
>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>>> index 582ebd9cf9c2..f84bac815bed 100644
>>>>> --- a/drivers/usb/dwc3/core.h
>>>>> +++ b/drivers/usb/dwc3/core.h
>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 {
>>>>> unsigned dis_split_quirk:1;
>>>>> unsigned async_callbacks:1;
>>>>>
>>>>> + unsigned gadget_keep_connect_sys_sleep:1;
>>>>> +
>>>>> u16 imod_interval;
>>>>>
>>>>> int max_cfg_eps;
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index 3c63fa97a680..8062e44f63f6 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc)
>>>>> {
>>>>> unsigned long flags;
>>>>> + int link_state;
>>>>>
>>>>> if (!dwc->gadget_driver)
>>>>> return 0;
>>>>>
>>>>> - dwc3_gadget_run_stop(dwc, false, false);
>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) {
>>>>> + link_state = dwc3_gadget_get_link_state(dwc);
>>>>> + /* Prevent PM Sleep if not in U3/L2 */
>>>>> + if (link_state != DWC3_LINK_STATE_U3)
>>>>> + return -EBUSY;
>>>>> +
>>>>> + /* don't stop/disconnect */
>>>>> + dwc3_gadget_disable_irq(dwc);
>>>>
>>>> We shouldn't disable event interrupt here. What will happen if the
>>>
>>> Due to some reason, if I don't disable the event interrupts here then
>>> after USB resume the USB controller is malfunctioning.
>>> It no longer responds to any requests from Host.
>>
>> You should look into this. These events are important as they can tell
>> whether the host initiates resume.
>>
>>>
>>>> device is disconnected and reconnect to the host while the device is
>>>> still in system suspend? The host would not be able to communicate with
>>>> the device then.
>>>
>>> In the TI platform, The system is woken up on any VBUS/linestate change
>>> and in dwc3_gadget_resume we enable the events again and check for pending
>>> events. Is it pointless to check for pending events there?
>>>
>>
>> It seems fragile for the implementation to be dependent on platform
>> specific feature right?
>>
>> Also, what will happen in a typical case when the host puts the device
>> in suspend and initiates resume while the device is in system suspend
>> (and stay in suspend over a period of time)? There is no VBUS change.
>> There will be problem if host detects no response from device in time.
>>
>> Don't we need these events to wakeup the device?
That's why the TI implementation has line-state change detection to
detect a USB resume. We are doing a out-of-band wake-up. The wake up
events are configured in the wrapper driver (dwc3-am62.c).
Do you know of any dwc3 implementation that uses in-band mechanism
to wake up the System. i.e. it relies on events enabled in DEVTEN register?
>>
>
> We may not be able to suspend everything in system suspend for this
> case. I'm thinking of treating these events as if they are PME to wakeup
> the device, but they are not the same. It may not be simple to handle
> this. The lower layers may need to stay awake for the dwc3 to handle
> these events. Hm... it gets a bit complicated.
As we are going into suspend, we are not really in a position to handle any
(DEVTEN) events till we have fully resumed.
So yes, we need to rely on platform specific implementation to wake
the System on any USB event.
cheers,
-roger
