On 06/04/2023 04:38, Thinh Nguyen wrote:
> On Wed, Apr 05, 2023, Roger Quadros wrote:
>>
>>
>> On 05/04/2023 00:53, Thinh Nguyen wrote:
>>> On Tue, Apr 04, 2023, Roger Quadros wrote:
>>>>
>>>>
>>>> On 04/04/2023 02:37, Thinh Nguyen wrote:
>>>>> On Fri, Mar 31, 2023, Roger Quadros wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 23/03/2023 22:51, Thinh Nguyen wrote:
>>>>>>> On Thu, Mar 23, 2023, Roger Quadros wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>>
>>>>>>>
>>>>>>> Right, you need to keep those clocks running to detect host resume.
>>>>>>> There's still some power saving through the dwc3 controller's handling
>>>>>>> in suspend. You may have some limited power saving from other suspended
>>>>>>> devices on your setup. However, I don't think we can expect the platform
>>>>>>> to go into deep-sleep and also handle host resume.
>>>>>>
>>>>>> Why not? if the PHY can detect the host resume and wake up the SoC it will
>>>>>> work right?
>>>>>>
>>>>>
>>>>> Hm... I supposed it may be possible. But it may need some unconventional
>>>>> design? The dwc3 controller is currently registered to the phy. For that
>>>>> to work, your phy needs to be able to talk to both the dwc3 controller
>>>>> and some other controller (equivalent to dwc3 PMU) that manages
>>>>> power/interrupt. The dwc3 controller would need to relinquish control to
>>>>> this other phy controller on suspend. The phy driver would then be able
>>>>> to assert interrupt waking up the system on resume sigal detection,
>>>>> which in turn relinquish control to the dwc3 controller. All of this has
>>>>> to work while the phy signaling remains synchronized with the dwc3
>>>>> controller.
>>>>
>>>> My understanding is that all this is taken care by PHY integration design with
>>>> DWC3 core on the TI SoC.
>>>>
>>>>>
>>>>> From the patches you sent, I don't see the changes necesssary for this
>>>>> to work. If there is something that I'm missing, please also note it or
>>>>> add it here to the series.
>>>>
>>>> There is nothing more as the details are taken care by PHY logic and
>>>> necessary integration with DWC3.
>>>>
>>>> For the PHY wake-up programming details you have already checked this series [1].
>>>>
>>>> [1] - https://urldefense.com/v3/__https://lore.kernel.org/all/20230316131226.89540-1-rogerq@xxxxxxxxxx/__;!!A4F2R9G_pg!ayqpaWwnWIO7SKktk4kI2EJLwDTIv2nYYCgBGHUlt56KTzeYkDEdq2Q5ZvZFsuCWIlcXGET230YY5J3y_sHV$
>>>>
>>>
>>> I may have misunderstood your platform implementation. My understanding
>>> is that it can only detect VBUS and that it can only resume on VBUS
>>> valid.
>>>
>>> Does the "LINESTATE" here gets asserted if say there's a LFPS detection?
>>
>> Yes. The wake up logic on the SoC is snooping the UTMI lines from the PHY and on any
>> change it can detect and wake up the SoC.
>>
>
> Are you referring to the utmi_linestate signal? Isn't that for usb2
> speed only? Does your platform support usb3 speed?
The wake-up on deepSleep feature is only supported for USB2 on this particular SoC.
cheers,
-roger
