lkml.org 
[lkml]   [2023]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature
    From
    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

    \
     
     \ /
      Last update: 2023-03-31 13:08    [W:7.011 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site