From: Roger Quadros <rogerq@kernel.org>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc: "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>,
"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"vigneshr@ti.com" <vigneshr@ti.com>, "srk@ti.com" <srk@ti.com>,
"r-gunasekaran@ti.com" <r-gunasekaran@ti.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature
Date: Fri, 31 Mar 2023 14:05:33 +0300 [thread overview]
Message-ID: <4d2f628e-6adc-5190-61b3-cc9d61f34a84@kernel.org> (raw)
In-Reply-To: <20230323205139.4on6vx555ohdec7y@synopsys.com>
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@kernel.org>
>>>>>>>>>> ---
>>>>>>>>>> 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?
cheers,
-roger
next prev parent reply other threads:[~2023-03-31 11:07 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-20 9:34 [RFC PATCH 0/2] usb: dwc3: Support wake-up from USB suspend Roger Quadros
2023-03-20 9:34 ` [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' Roger Quadros
2023-03-20 13:11 ` Rob Herring
2023-03-20 13:22 ` Rob Herring
2023-03-21 9:44 ` Roger Quadros
2023-03-20 9:34 ` [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature Roger Quadros
2023-03-20 18:52 ` Thinh Nguyen
2023-03-21 10:20 ` Roger Quadros
2023-03-21 18:43 ` Thinh Nguyen
2023-03-21 19:05 ` Thinh Nguyen
2023-03-22 8:11 ` Roger Quadros
2023-03-22 17:31 ` Thinh Nguyen
2023-03-23 2:17 ` Thinh Nguyen
2023-03-23 9:29 ` Roger Quadros
2023-03-23 20:51 ` Thinh Nguyen
2023-03-31 11:05 ` Roger Quadros [this message]
2023-04-03 23:37 ` Thinh Nguyen
2023-04-04 8:01 ` Roger Quadros
2023-04-04 21:53 ` Thinh Nguyen
2023-04-05 8:56 ` Roger Quadros
2023-04-06 1:38 ` Thinh Nguyen
2023-04-12 7:46 ` Roger Quadros
2023-04-12 20:59 ` Thinh Nguyen
2023-04-13 11:31 ` Roger Quadros
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4d2f628e-6adc-5190-61b3-cc9d61f34a84@kernel.org \
--to=rogerq@kernel.org \
--cc=Thinh.Nguyen@synopsys.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=r-gunasekaran@ti.com \
--cc=srk@ti.com \
--cc=stern@rowland.harvard.edu \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).