lkml.org 
[lkml]   [2020]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v4 3/5] usb: dwc3: qcom: Configure wakeup interrupts and set genpd active wakeup flag
    On Wed, Oct 28, 2020 at 02:08:01AM +0530, Sandeep Maheswaram wrote:
    > Configure interrupts based on hs_phy_mode to avoid triggering of
    > interrupts during system suspend and suspends successfully.
    > Set genpd active wakeup flag for usb gdsc if wakeup capable devices
    > are connected so that wake up happens without reenumeration.
    > Add helper functions to enable,disable wake irqs.
    >
    > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org>
    > ---
    > drivers/usb/dwc3/dwc3-qcom.c | 82 ++++++++++++++++++++++++++++----------------
    > 1 file changed, 52 insertions(+), 30 deletions(-)
    >
    > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
    > index c703d55..c93f7bb 100644
    > --- a/drivers/usb/dwc3/dwc3-qcom.c
    > +++ b/drivers/usb/dwc3/dwc3-qcom.c
    > @@ -17,9 +17,11 @@
    > #include <linux/of_platform.h>
    > #include <linux/platform_device.h>
    > #include <linux/phy/phy.h>
    > +#include <linux/pm_domain.h>
    > #include <linux/usb/of.h>
    > #include <linux/reset.h>
    > #include <linux/iopoll.h>
    > +#include <linux/usb/hcd.h>
    >
    > #include "core.h"
    >
    > @@ -291,60 +293,75 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
    > icc_put(qcom->icc_path_apps);
    > }
    >
    > -static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
    > +static void dwc3_qcom_enable_wakeup_irq(int wake_irq)

    super-nit: from the name of the function it's evident that this is a
    wakeup irq, you could omit the 'wake_' prefix.

    > {
    > - if (qcom->hs_phy_irq) {
    > - disable_irq_wake(qcom->hs_phy_irq);
    > - disable_irq_nosync(qcom->hs_phy_irq);
    > + if (wake_irq) {
    > + enable_irq(wake_irq);
    > + enable_irq_wake(wake_irq);
    > }

    nit: the following would be a more common structure:

    if (!wake_irq)
    return;

    enable_irq(wake_irq);
    enable_irq_wake(wake_irq);

    > +}
    >
    > - if (qcom->dp_hs_phy_irq) {
    > - disable_irq_wake(qcom->dp_hs_phy_irq);
    > - disable_irq_nosync(qcom->dp_hs_phy_irq);
    > +static void dwc3_qcom_disable_wakeup_irq(int wake_irq)

    same as above

    > +{
    > + if (wake_irq) {
    > + disable_irq_wake(wake_irq);
    > + disable_irq_nosync(wake_irq);
    > }

    same as above

    > +}
    >
    > - if (qcom->dm_hs_phy_irq) {
    > - disable_irq_wake(qcom->dm_hs_phy_irq);
    > - disable_irq_nosync(qcom->dm_hs_phy_irq);
    > - }
    > +static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
    > +{
    > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
    > +
    > + dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
    >
    > - if (qcom->ss_phy_irq) {
    > - disable_irq_wake(qcom->ss_phy_irq);
    > - disable_irq_nosync(qcom->ss_phy_irq);
    > + if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS)
    > + dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
    > + else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS)
    > + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
    > + else {
    > + dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
    > + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
    > }

    The following would be clearer IMO:

    if (dwc->hs_phy_mode & (PHY_MODE_USB_HOST_LS | PHY_MODE_USB_HOST_SS))
    dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
    if (dwc->hs_phy_mode & (PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_SS))
    dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);

    > +
    > + dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);

    Why isn't this conditional on 'dwc->hs_phy_mode & PHY_MODE_USB_HOST_SS'?

    > }
    >
    > static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
    > {
    > - if (qcom->hs_phy_irq) {
    > - enable_irq(qcom->hs_phy_irq);
    > - enable_irq_wake(qcom->hs_phy_irq);
    > - }
    > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
    >
    > - if (qcom->dp_hs_phy_irq) {
    > - enable_irq(qcom->dp_hs_phy_irq);
    > - enable_irq_wake(qcom->dp_hs_phy_irq);
    > - }
    > + dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq);
    >
    > - if (qcom->dm_hs_phy_irq) {
    > - enable_irq(qcom->dm_hs_phy_irq);
    > - enable_irq_wake(qcom->dm_hs_phy_irq);
    > + if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_LS)
    > + dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
    > + else if (dwc->hs_phy_mode & PHY_MODE_USB_HOST_HS)
    > + dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
    > + else {
    > + dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq);
    > + dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq);
    > }
    >
    > - if (qcom->ss_phy_irq) {
    > - enable_irq(qcom->ss_phy_irq);
    > - enable_irq_wake(qcom->ss_phy_irq);
    > - }
    > + dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq);

    same comments as for dwc3_qcom_disable_interrupts()

    > }
    >
    > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
    > {
    > u32 val;
    > int i, ret;
    > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
    > + struct usb_hcd *hcd;

    nit: remove one blank

    > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
    >
    > if (qcom->is_suspended)
    > return 0;
    >
    > + if (dwc->xhci) {
    > + hcd = platform_get_drvdata(dwc->xhci);
    > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
    > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP;

    Shouldn't this only be done when wakeup is enabled?

    > + }
    > +
    > val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
    > if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
    > dev_err(qcom->dev, "HS-PHY not in L2\n");
    > @@ -366,10 +383,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
    > {
    > int ret;
    > int i;
    > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
    > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain);
    >
    > if (!qcom->is_suspended)
    > return 0;
    >
    > + if (dwc->xhci)
    > + genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP;

    Could also depend on wakeup being enabled, but less important.

    > +
    > dwc3_qcom_disable_interrupts(qcom);
    >
    > for (i = 0; i < qcom->num_clocks; i++) {
    > @@ -764,7 +786,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
    > if (ret)
    > goto interconnect_exit;
    >
    > - device_init_wakeup(&pdev->dev, 1);
    > + device_init_wakeup(&pdev->dev, of_property_read_bool(np, "wakeup-source"));
    > qcom->is_suspended = false;
    > pm_runtime_set_active(dev);
    > pm_runtime_enable(dev);
    > --
    > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
    > of Code Aurora Forum, hosted by The Linux Foundation
    >

    \
     
     \ /
      Last update: 2020-12-11 01:53    [W:2.748 / U:1.620 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site