Message-ID: <20230322173150.nscqyzwcrecxjuaa@synopsys.com>
Date:   Wed, 22 Mar 2023 17:31:56 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Roger Quadros <rogerq@...nel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "stern@...land.harvard.edu" <stern@...land.harvard.edu>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "vigneshr@...com" <vigneshr@...com>, "srk@...com" <srk@...com>,
        "r-gunasekaran@...com" <r-gunasekaran@...com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] usb: dwc3: Support
 'snps,gadget-keep-connect-sys-sleep' feature

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@...nel.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.

BR,
Thinh

Powered by blists - more mailing lists