| Message ID | 1635753224-23975-4-git-send-email-quic_c_sanm@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
Hi Felipe, This patch is (supposedly) an improvement regardless of whether the rest of the series lands or not. It hasn't changed in the last iterations nor did it receive any comments. Can this be landed rather than carrying it around until the rest of the series is ready? Thanks Matthias On Mon, Nov 01, 2021 at 01:23:42PM +0530, Sandeep Maheswaram wrote: > Adding helper functions to enable,disable wake irqs to make > the code simple and readable. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 58 ++++++++++++++++++++------------------------ > 1 file changed, 26 insertions(+), 32 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 9abbd01..54461f1 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > icc_put(qcom->icc_path_apps); > } > > +static void dwc3_qcom_enable_wakeup_irq(int irq) > +{ > + if (!irq) > + return; > + > + enable_irq(irq); > + enable_irq_wake(irq); > +} > + > +static void dwc3_qcom_disable_wakeup_irq(int irq) > +{ > + if (!irq) > + return; > + > + disable_irq_wake(irq); > + disable_irq_nosync(irq); > +} > + > static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > { > - if (qcom->hs_phy_irq) { > - disable_irq_wake(qcom->hs_phy_irq); > - disable_irq_nosync(qcom->hs_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); > > - if (qcom->dp_hs_phy_irq) { > - disable_irq_wake(qcom->dp_hs_phy_irq); > - disable_irq_nosync(qcom->dp_hs_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); > > - if (qcom->dm_hs_phy_irq) { > - disable_irq_wake(qcom->dm_hs_phy_irq); > - disable_irq_nosync(qcom->dm_hs_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); > > - if (qcom->ss_phy_irq) { > - disable_irq_wake(qcom->ss_phy_irq); > - disable_irq_nosync(qcom->ss_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); > } > > 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); > - } > + dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq); > > - 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->dp_hs_phy_irq); > > - if (qcom->dm_hs_phy_irq) { > - enable_irq(qcom->dm_hs_phy_irq); > - enable_irq_wake(qcom->dm_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); > } > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > -- > 2.7.4 >
On Mon 01 Nov 00:53 PDT 2021, Sandeep Maheswaram wrote: > Adding helper functions to enable,disable wake irqs to make > the code simple and readable. > > Signed-off-by: Sandeep Maheswaram <quic_c_sanm@quicinc.com> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 58 ++++++++++++++++++++------------------------ > 1 file changed, 26 insertions(+), 32 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 9abbd01..54461f1 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) > icc_put(qcom->icc_path_apps); > } > > +static void dwc3_qcom_enable_wakeup_irq(int irq) > +{ > + if (!irq) > + return; > + > + enable_irq(irq); > + enable_irq_wake(irq); > +} > + > +static void dwc3_qcom_disable_wakeup_irq(int irq) > +{ > + if (!irq) > + return; > + > + disable_irq_wake(irq); Now that you touch this code path. I presume keeping these interrupts enabled during runtime would cause the interrupt to fire during normal operation, but why do we need to toggle the irq_wake? Can't we just leave that flag set always? > + disable_irq_nosync(irq); > +} > + > static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > { > - if (qcom->hs_phy_irq) { > - disable_irq_wake(qcom->hs_phy_irq); > - disable_irq_nosync(qcom->hs_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); Why don't we just replace *_phy_irq with an array and turn these two function into two loops? That seems to be quite suitable for the multi-port dwc found in e.g. sc8180x as well... Regards, Bjorn > > - if (qcom->dp_hs_phy_irq) { > - disable_irq_wake(qcom->dp_hs_phy_irq); > - disable_irq_nosync(qcom->dp_hs_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); > > - if (qcom->dm_hs_phy_irq) { > - disable_irq_wake(qcom->dm_hs_phy_irq); > - disable_irq_nosync(qcom->dm_hs_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); > > - if (qcom->ss_phy_irq) { > - disable_irq_wake(qcom->ss_phy_irq); > - disable_irq_nosync(qcom->ss_phy_irq); > - } > + dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); > } > > 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); > - } > + dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq); > > - 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->dp_hs_phy_irq); > > - if (qcom->dm_hs_phy_irq) { > - enable_irq(qcom->dm_hs_phy_irq); > - enable_irq_wake(qcom->dm_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); > } > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > -- > 2.7.4 >
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 9abbd01..54461f1 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -296,50 +296,44 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) icc_put(qcom->icc_path_apps); } +static void dwc3_qcom_enable_wakeup_irq(int irq) +{ + if (!irq) + return; + + enable_irq(irq); + enable_irq_wake(irq); +} + +static void dwc3_qcom_disable_wakeup_irq(int irq) +{ + if (!irq) + return; + + disable_irq_wake(irq); + disable_irq_nosync(irq); +} + static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) { - if (qcom->hs_phy_irq) { - disable_irq_wake(qcom->hs_phy_irq); - disable_irq_nosync(qcom->hs_phy_irq); - } + dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq); - if (qcom->dp_hs_phy_irq) { - disable_irq_wake(qcom->dp_hs_phy_irq); - disable_irq_nosync(qcom->dp_hs_phy_irq); - } + dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); - if (qcom->dm_hs_phy_irq) { - disable_irq_wake(qcom->dm_hs_phy_irq); - disable_irq_nosync(qcom->dm_hs_phy_irq); - } + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); - if (qcom->ss_phy_irq) { - disable_irq_wake(qcom->ss_phy_irq); - disable_irq_nosync(qcom->ss_phy_irq); - } + dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); } 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); - } + dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq); - 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->dp_hs_phy_irq); - if (qcom->dm_hs_phy_irq) { - enable_irq(qcom->dm_hs_phy_irq); - enable_irq_wake(qcom->dm_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); } static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)