| Message ID | 20230621043628.21485-8-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Wed, Jun 21, 2023 at 10:06:25AM +0530, Krishna Kurapati wrote: > QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS > ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's > for all the ports during suspend/resume. Please be more specific here. You don't seem to be configuring anything. > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 48 +++++++++++++++++++++++++++++++----- > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 3ab48a6925fe..699485a85233 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -37,7 +37,11 @@ > #define PIPE3_PHYSTATUS_SW BIT(3) > #define PIPE_UTMI_CLK_DIS BIT(8) > > -#define PWR_EVNT_IRQ_STAT_REG 0x58 > +#define PWR_EVNT_IRQ1_STAT_REG 0x58 > +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc > +#define PWR_EVNT_IRQ3_STAT_REG 0x228 > +#define PWR_EVNT_IRQ4_STAT_REG 0x238 > + > #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) > #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) > > @@ -93,6 +97,13 @@ struct dwc3_qcom { > struct icc_path *icc_path_apps; > }; > > +static u32 pwr_evnt_irq_stat_reg_offset[4] = { > + PWR_EVNT_IRQ1_STAT_REG, > + PWR_EVNT_IRQ2_STAT_REG, > + PWR_EVNT_IRQ3_STAT_REG, > + PWR_EVNT_IRQ4_STAT_REG, > +}; > + > static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) > { > u32 reg; > @@ -417,17 +428,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); > } > > +static u8 dwc3_qcom_get_port_info(struct dwc3_qcom *qcom) "port_info" is not very specific, call it get_num_ports() or similar. > +{ > + struct dwc3 __maybe_unused *dwc = platform_get_drvdata(qcom->dwc3); __maybe unused makes no sense here. > + > + if (dwc3_qcom_is_host(qcom)) > + return dwc->num_usb2_ports; Here you're accessing the core driver data again, which we want to avoid going forward so this at least warrants a FIXME to rework this as well. > + > + /* > + * If not in host mode, either dwc was not probed > + * or we are in device mode, either case checking for > + * only first pwr event irq would suffice. Rewrap comment at 80 columns. > + */ > + > + return 1; > +} > + > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > { > u32 val; > int i, ret; > + u8 num_ports; Move first. > if (qcom->is_suspended) > return 0; > > - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); > - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) > + num_ports = dwc3_qcom_get_port_info(qcom); > + for (i = 0; i < num_ports; i++) { > + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); > + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) > dev_err(qcom->dev, "HS-PHY not in L2\n"); This line is not indented properly. Make sure to run checkpatch before submitting so that I don't have to point out things like this again. > + } > > for (i = qcom->num_clocks - 1; i >= 0; i--) > clk_disable_unprepare(qcom->clks[i]); > @@ -452,12 +483,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > > static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) > { > + int num_ports; > int ret; > int i; > > if (!qcom->is_suspended) > return 0; > > + num_ports = dwc3_qcom_get_port_info(qcom); Move below to where you use num_ports. > if (dwc3_qcom_is_host(qcom) && wakeup) > dwc3_qcom_disable_interrupts(qcom); > > @@ -474,9 +507,12 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) > if (ret) > dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret); > > - /* Clear existing events from PHY related to L2 in/out */ No need to move the comment. > - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, > - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); > + for (i = 0; i < num_ports; i++) { > + /* Clear existing events from PHY related to L2 in/out */ > + dwc3_qcom_setbits(qcom->qscratch_base, > + pwr_evnt_irq_stat_reg_offset[i], > + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); Indent continuation lines at least two tabs further than the previous line. > + } > > qcom->is_suspended = false; Johan
On 6/27/2023 8:35 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:25AM +0530, Krishna Kurapati wrote: >> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS >> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's >> for all the ports during suspend/resume. > > Please be more specific here. You don't seem to be configuring anything. > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 48 +++++++++++++++++++++++++++++++----- >> 1 file changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 3ab48a6925fe..699485a85233 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -37,7 +37,11 @@ >> #define PIPE3_PHYSTATUS_SW BIT(3) >> #define PIPE_UTMI_CLK_DIS BIT(8) >> >> -#define PWR_EVNT_IRQ_STAT_REG 0x58 >> +#define PWR_EVNT_IRQ1_STAT_REG 0x58 >> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc >> +#define PWR_EVNT_IRQ3_STAT_REG 0x228 >> +#define PWR_EVNT_IRQ4_STAT_REG 0x238 >> + >> #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) >> #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) >> >> @@ -93,6 +97,13 @@ struct dwc3_qcom { >> struct icc_path *icc_path_apps; >> }; >> >> +static u32 pwr_evnt_irq_stat_reg_offset[4] = { >> + PWR_EVNT_IRQ1_STAT_REG, >> + PWR_EVNT_IRQ2_STAT_REG, >> + PWR_EVNT_IRQ3_STAT_REG, >> + PWR_EVNT_IRQ4_STAT_REG, >> +}; >> + >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) >> { >> u32 reg; >> @@ -417,17 +428,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) >> dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); >> } >> >> +static u8 dwc3_qcom_get_port_info(struct dwc3_qcom *qcom) > > "port_info" is not very specific, call it get_num_ports() or similar. > >> +{ >> + struct dwc3 __maybe_unused *dwc = platform_get_drvdata(qcom->dwc3); > > __maybe unused makes no sense here. > >> + >> + if (dwc3_qcom_is_host(qcom)) >> + return dwc->num_usb2_ports; > > Here you're accessing the core driver data again, which we want to > avoid going forward so this at least warrants a FIXME to rework this as > well. > Ok, will add a FIXME here. Thanks, Krishna, >> + >> + /* >> + * If not in host mode, either dwc was not probed >> + * or we are in device mode, either case checking for >> + * only first pwr event irq would suffice. > > Rewrap comment at 80 columns. > >> + */ >> + >> + return 1; >> +} >> + >> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> { >> u32 val; >> int i, ret; >> + u8 num_ports; > > Move first. > >> if (qcom->is_suspended) >> return 0; >> >> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); >> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >> + num_ports = dwc3_qcom_get_port_info(qcom); >> + for (i = 0; i < num_ports; i++) { >> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); >> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >> dev_err(qcom->dev, "HS-PHY not in L2\n"); > > This line is not indented properly. > > Make sure to run checkpatch before submitting so that I don't have to > point out things like this again. > >> + } >> >> for (i = qcom->num_clocks - 1; i >= 0; i--) >> clk_disable_unprepare(qcom->clks[i]); >> @@ -452,12 +483,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> >> static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) >> { >> + int num_ports; >> int ret; >> int i; >> >> if (!qcom->is_suspended) >> return 0; >> >> + num_ports = dwc3_qcom_get_port_info(qcom); > > Move below to where you use num_ports. > >> if (dwc3_qcom_is_host(qcom) && wakeup) >> dwc3_qcom_disable_interrupts(qcom); >> >> @@ -474,9 +507,12 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) >> if (ret) >> dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret); >> >> - /* Clear existing events from PHY related to L2 in/out */ > > No need to move the comment. > >> - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, >> - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); >> + for (i = 0; i < num_ports; i++) { >> + /* Clear existing events from PHY related to L2 in/out */ >> + dwc3_qcom_setbits(qcom->qscratch_base, >> + pwr_evnt_irq_stat_reg_offset[i], >> + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); > > Indent continuation lines at least two tabs further than the previous > line. > >> + } >> >> qcom->is_suspended = false; > > Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 3ab48a6925fe..699485a85233 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -37,7 +37,11 @@ #define PIPE3_PHYSTATUS_SW BIT(3) #define PIPE_UTMI_CLK_DIS BIT(8) -#define PWR_EVNT_IRQ_STAT_REG 0x58 +#define PWR_EVNT_IRQ1_STAT_REG 0x58 +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc +#define PWR_EVNT_IRQ3_STAT_REG 0x228 +#define PWR_EVNT_IRQ4_STAT_REG 0x238 + #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) @@ -93,6 +97,13 @@ struct dwc3_qcom { struct icc_path *icc_path_apps; }; +static u32 pwr_evnt_irq_stat_reg_offset[4] = { + PWR_EVNT_IRQ1_STAT_REG, + PWR_EVNT_IRQ2_STAT_REG, + PWR_EVNT_IRQ3_STAT_REG, + PWR_EVNT_IRQ4_STAT_REG, +}; + static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) { u32 reg; @@ -417,17 +428,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); } +static u8 dwc3_qcom_get_port_info(struct dwc3_qcom *qcom) +{ + struct dwc3 __maybe_unused *dwc = platform_get_drvdata(qcom->dwc3); + + if (dwc3_qcom_is_host(qcom)) + return dwc->num_usb2_ports; + + /* + * If not in host mode, either dwc was not probed + * or we are in device mode, either case checking for + * only first pwr event irq would suffice. + */ + + return 1; +} + static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) { u32 val; int i, ret; + u8 num_ports; if (qcom->is_suspended) return 0; - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) + num_ports = dwc3_qcom_get_port_info(qcom); + for (i = 0; i < num_ports; i++) { + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) dev_err(qcom->dev, "HS-PHY not in L2\n"); + } for (i = qcom->num_clocks - 1; i >= 0; i--) clk_disable_unprepare(qcom->clks[i]); @@ -452,12 +483,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) { + int num_ports; int ret; int i; if (!qcom->is_suspended) return 0; + num_ports = dwc3_qcom_get_port_info(qcom); if (dwc3_qcom_is_host(qcom) && wakeup) dwc3_qcom_disable_interrupts(qcom); @@ -474,9 +507,12 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) if (ret) dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret); - /* Clear existing events from PHY related to L2 in/out */ - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); + for (i = 0; i < num_ports; i++) { + /* Clear existing events from PHY related to L2 in/out */ + dwc3_qcom_setbits(qcom->qscratch_base, + pwr_evnt_irq_stat_reg_offset[i], + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); + } qcom->is_suspended = false;
QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's for all the ports during suspend/resume. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 48 +++++++++++++++++++++++++++++++----- 1 file changed, 42 insertions(+), 6 deletions(-)