On 23/03/2023 04:17, Thinh Nguyen wrote:
> On Wed, Mar 22, 2023, Thinh Nguyen wrote:
>> On Wed, Mar 22, 2023, Roger Quadros wrote:
>>> 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 rely on PME. The PME is generated from the PMU of the usb controller
>> when it detects a resume. If your platform supports hibernation and if
>> the resume signal is connected to the lower layer power manager of your
>> device, then you can wakeup the system one level at a time. For example,
>> if your device is a pci device, that wakeup signal would tie to the pci
>> power manager, waking up the pci layer before waking up the core of the
>> usb controller. That's how the host wakes up the host system (e.g. from
>> remote wakeup). For this to work, we expect something similar on the
>> device side.
>>
>>>>>
>>>>
>>>> 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.
>>>
>>
>> You may be able to detect vbus change through the connector controller.
>> However, the usb controller is the one that detects host resume. What
>> platform specific implementation do you have outside of the usb
>> controller do you have to get around that?
>>
>> I'm not sure if your platform supports hibernation or if the PME signal
>> on your platform can wakeup the system, but currently dwc3 driver
>> doesn't handle hibernation (device side). If there's no hibernation,
>> there's no PME.
No, in this TI SoC, hibernation feature is not supported in the dwc3 core.
>>
>
> Actually, I think the dwc3 core is still on during system suspend for
> you right? Then I think we can use the wakeup event to wakeup system
> suspend on host resume? You can ignore about PME in this case. You may
> need to look into what needs stay awake to allow for handling of the
> dwc3 event.
But in SoC deep-sleep state, all clocks to the dwc3 core are stopped.
So I'm not sure if dwc3 events will work.
cheers,
-roger
