Hi Matthias,
> -----Original Message-----
> From: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
> Sent: Saturday, April 16, 2022 6:00 AM
> To: Sandeep Maheswaram (Temp) (QUIC) <quic_c_sanm@xxxxxxxxxxx>
> Cc: Pavan Kumar Kondeti (QUIC) <quic_pkondeti@xxxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Andy Gross <agross@xxxxxxxxxx>;
> bjorn.andersson@xxxxxxxxxx; Greg Kroah-Hartman
> <gregkh@xxxxxxxxxxxxxxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Stephen
> Boyd <swboyd@xxxxxxxxxxxx>; Doug Anderson
> <dianders@xxxxxxxxxxxx>; Mathias Nyman <mathias.nyman@xxxxxxxxx>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-
> usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Pratham Pratap (QUIC)
> <quic_ppratap@xxxxxxxxxxx>; Krishna Kurapati PSSNV (QUIC)
> <quic_kriskura@xxxxxxxxxxx>; Vidya Sagar Pulyala (Temp) (QUIC)
> <quic_vpulyala@xxxxxxxxxxx>
> Subject: Re: [PATCH v13 2/6] usb: dwc3: core: Host wake up support from
> system suspend
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On Thu, Apr 14, 2022 at 11:27:31AM +0530, Sandeep Maheswaram (Temp)
> wrote:
> > Hi Matthias,
> >
> > On 4/13/2022 9:26 PM, Matthias Kaehlcke wrote:
> > > On Wed, Apr 13, 2022 at 02:38:33PM +0530, Sandeep Maheswaram
> (Temp) wrote:
> > > > Hi Matthias,
> > > >
> > > > On 4/13/2022 12:35 AM, Matthias Kaehlcke wrote:
> > > > > On Tue, Apr 12, 2022 at 12:08:02PM +0530, Sandeep Maheswaram
> (Temp) wrote:
> > > > > > Hi Pavan,
> > > > > >
> > > > > > On 4/12/2022 10:30 AM, Pavan Kondeti wrote:
> > > > > > > Hi Sandeep,
> > > > > > >
> > > > > > > On Tue, Apr 12, 2022 at 10:16:39AM +0530, Sandeep Maheswaram
> (Temp) wrote:
> > > > > > > > Hi Matthias,
> > > > > > > >
> > > > > > > > On 4/12/2022 2:24 AM, Matthias Kaehlcke wrote:
> > > > > > > > > On Tue, Apr 12, 2022 at 12:46:50AM +0530, Sandeep
> Maheswaram wrote:
> > > > > > > > > > During suspend read the status of all port and set hs
> > > > > > > > > > phy mode based on current speed. Use this hs phy mode
> > > > > > > > > > to configure wakeup interrupts in qcom glue driver.
> > > > > > > > > >
> > > > > > > > > > Check wakep-source property for dwc3 core node to set
> > > > > > > > > > the
> > > > > > > > > s/wakep/wakeup/
> > > > > > > > Okay. Will update in next version.
> > > > > > > > > > wakeup capability. Drop the device_init_wakeup call
> > > > > > > > > > from runtime suspend and resume.
> > > > > > > > > >
> > > > > > > > > > Also check during suspend if any wakeup capable
> > > > > > > > > > devices are connected to the controller (directly or
> > > > > > > > > > through hubs), if there are none set a flag to
> > > > > > > > > > indicate that the PHY is powered down during suspend.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Sandeep Maheswaram
> > > > > > > > > > <quic_c_sanm@xxxxxxxxxxx>
> > > > > > > > > > ---
> > > > > > > > > A per-patch change log would be really helpful for
> > > > > > > > > reviewers, even if it doesn't include older versions.
> > > > > > > > Okay. Will update in next version.
> > > > > > > > > > drivers/usb/dwc3/core.c | 33 ++++++++++++++++++++---
> ----------
> > > > > > > > > > drivers/usb/dwc3/core.h | 4 ++++
> > > > > > > > > > drivers/usb/dwc3/host.c | 25
> +++++++++++++++++++++++++
> > > > > > > > > > 3 files changed, 49 insertions(+), 13 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c
> > > > > > > > > > b/drivers/usb/dwc3/core.c index 1170b80..effaa43
> > > > > > > > > > 100644
> > > > > > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > > > > > @@ -32,6 +32,7 @@
> > > > > > > > > > #include <linux/usb/gadget.h>
> > > > > > > > > > #include <linux/usb/of.h>
> > > > > > > > > > #include <linux/usb/otg.h>
> > > > > > > > > > +#include <linux/usb/hcd.h>
> > > > > > > > > > #include "core.h"
> > > > > > > > > > #include "gadget.h"
> > > > > > > > > > @@ -1723,6 +1724,7 @@ static int dwc3_probe(struct
> platform_device *pdev)
> > > > > > > > > > platform_set_drvdata(pdev, dwc);
> > > > > > > > > > dwc3_cache_hwparams(dwc);
> > > > > > > > > > + device_init_wakeup(&pdev->dev,
> > > > > > > > > > + of_property_read_bool(dev->of_node,
> > > > > > > > > > + "wakeup-source"));
> > > > > > > > > > spin_lock_init(&dwc->lock);
> > > > > > > > > > mutex_init(&dwc->mutex); @@ -1865,6 +1867,7 @@
> > > > > > > > > > static int dwc3_suspend_common(struct dwc3 *dwc,
> pm_message_t msg)
> > > > > > > > > > {
> > > > > > > > > > unsigned long flags;
> > > > > > > > > > u32 reg;
> > > > > > > > > > + struct usb_hcd *hcd =
> > > > > > > > > > + platform_get_drvdata(dwc->xhci);
> > > > > > > > > > switch (dwc->current_dr_role) {
> > > > > > > > > > case DWC3_GCTL_PRTCAP_DEVICE:
> > > > > > > > > > @@ -1877,10 +1880,7 @@ static int
> dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > > > > > > > > dwc3_core_exit(dwc);
> > > > > > > > > > break;
> > > > > > > > > > case DWC3_GCTL_PRTCAP_HOST:
> > > > > > > > > > - if (!PMSG_IS_AUTO(msg)) {
> > > > > > > > > > - dwc3_core_exit(dwc);
> > > > > > > > > > - break;
> > > > > > > > > > - }
> > > > > > > > > > + dwc3_check_phy_speed_mode(dwc);
> > > > > > > > > > /* Let controller to suspend HSPHY before PHY driver
> suspends */
> > > > > > > > > > if (dwc->dis_u2_susphy_quirk || @@
> > > > > > > > > > -1896,6 +1896,16 @@ static int
> dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> > > > > > > > > > phy_pm_runtime_put_sync(dwc-
> >usb2_generic_phy);
> > > > > > > > > >
> > > > > > > > > > phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> > > > > > > > > > +
> > > > > > > > > > + if (!PMSG_IS_AUTO(msg)) {
> > > > > > > > > > + if (device_may_wakeup(dwc->dev) &&
> > > > > > > > > > +
> > > > > > > > > > + usb_wakeup_enabled_descendants(hcd->self.root_hub))
> > > > > > > > > > + {
> > > > > > > > > You did not answer my question on v12, reposting it:
> > > > > > > > >
> > > > > > > > > Did you ever try whether you could use
> device_children_wakeup_capable() from
> > > > > > > > > [1] instead of usb_wakeup_enabled_descendants()?
> > > > > > > > >
> > > > > > > > > [1]
> > > > > > > > > https://patchwork.kernel.org/project/linux-usb/patch/163
> > > > > > > > > 5753224-23975-2-git-send-email-quic_c_sanm@xxxxxxxxxxx/#
> > > > > > > > > 24566065
> > > > > > > > Sorry ..I have replied in mail yesterday but it is not
> > > > > > > > showing up in patchwork link.
> > > > > > > >
> > > > > > > > Tried with device_children_wakeup_capable(dwc->dev)
> > > > > > > > instead of usb_wakeup_enabled_descendants and it always
> > > > > > > > returns true even
> > > > > > > >
> > > > > > > > when no devices are connected.
> > > > > > > >
> > > > > > > What do you mean by when no devices are connected? There is
> > > > > > > always root hub connected and we should not power down the
> > > > > > > DWC3 here even when remote wakeup for root hub is enabled.
> > > > > > > Essentially
> > > > > > > usb_wakeup_enabled_descendants() returns true even without
> > > > > > > any physical devices connected.
> > > > > > >
> > > > > > > What does device_children_wakeup_capable() do? Sorry, I
> > > > > > > could not find this function definition.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Pavan
> > > > > > usb_wakeup_enabled_descendants() doesn't consider hubs. It
> > > > > > only returns true if any devices are connected with wakeup
> capability apart from hubs.
> > > > > Actually it considers hubs:
> > > > >
> > > > > unsigned usb_wakeup_enabled_descendants(struct usb_device
> *udev)
> > > > > {
> > > > > struct usb_hub *hub = usb_hub_to_struct_hub(udev);
> > > > >
> > > > > return udev->do_remote_wakeup +
> > > > > (hub ? hub->wakeup_enabled_descendants : 0); }
> > > > >
> > > > > 'udev' may or may not be a hub, if 'do_remote_wakeup' is set
> > > > > then the device is considered a wakeup enabled descendant.
> > > > >
> > > > > And for system suspebd 'do_remote_wakeup' is set based on the
> > > > > wakeup config of the device:
> > > > >
> > > > > static void choose_wakeup(struct usb_device *udev, pm_message_t
> > > > > msg) {
> > > > > ...
> > > > > w = device_may_wakeup(&udev->dev);
> > > > > ...
> > > > > udev->do_remote_wakeup = w; }
> > > > >
> > > > > I checked on three systems with different Linux distributions,
> > > > > on all of the wakeup flag of a connected hub is 'disabled'.
> > > > > Wakeup still works, so apparently that flag doesn't really have an
> impact for child ports.
> > > > >
> > > > > > If we consider hubs also dwc3 core exit and phy exit will never be
> called.
> > > > > >
> > > > > > device_children_wakeup_capable() implementation was shared by
> > > > > > Matthias in below thread
> > > > > > https://patchwork.kernel.org/project/linux-usb/patch/163575322
> > > > > > 4-23975-2-git-send-email-quic_c_sanm@xxxxxxxxxxx/#24566065
> > > > > >
> > > > > > Probably device_children_wakeup_capable() is returning true
> because it considers hubs also.
> > > > > I thought I did a basic test when I sent the patch, I did
> > > > > another (?) one with v13 of your patch set. In this tests with a
> > > > > hub connected the function returns true when an HID device is
> > > > > connected, and false when nothing is connected. The wakeup flag of
> the hub is disabled (default).
> > > > >
> > > > > Sandeep, are the wakeup flags of the child hub(s) set to
> > > > > 'enabled' on the system you tested on?
> > > > The wakeup flags of hub is 'disabled' on system I tested.
> > > >
> > > > What is the input param you are giving to
> device_children_wakeup_capable() function ?
> > > I passed '&hcd->self.root_hub->dev'
> >
> > Thanks. It is working with this change device_children_wakeup_capable
> > (&hcd->self.root_hub->dev).
> >
> > But I am not sure if it is better than usb_wakeup_enabled_descendants.
> > Still we are accessing xhci layer
> >
> > from dwc which Felipe suggested to avoid.
>
> True, it still needs access to the data structure(s), even though it doesn't use
> a USB specific API.
>
> Would be good to get feedback from Felipe on the current approach in
> general, we haven't heard from him in some time.
Will send the new version with device_children_wakeup_capable including your patch
And ask Felipe for his opinion.
Regards
Sandeep
