Message-ID: <20230406013803.x2fp6c3wpvqtbues@synopsys.com>
Date:   Thu, 6 Apr 2023 01:38:05 +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, 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@...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.
> >>>>>>
> >>>>>> 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@kernel.org/__;!!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?

Thanks,
Thinh

Powered by blists - more mailing lists