| Message ID | 20230828133033.11988-11-quic_kriskura@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On 28.08.2023 15:30, 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. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 39 +++++++++++++++++++++++++++++------- > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index f8f8c5e39a01..34eeebb74a6a 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) > > @@ -107,6 +111,19 @@ struct dwc3_qcom { > int num_ports; > }; > > +/* > + * SA8295 has 4 power event IRQ STAT registers to be checked > + * during suspend resume. > + */ But this driver supports much more than just SA8295? > +#define NUM_PWR_EVENT_STAT_REGS 4 > + > +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = { > + 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; > @@ -440,15 +457,19 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > { > + u8 num_ports; Maybe I'm picky, but I'm not sure defining a variable for a single use of an object with a rather short name (qcom->num_ports) is justified, here and below.. Konrad
On 9/15/2023 7:18 PM, Konrad Dybcio wrote: >> >> -#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) >> >> @@ -107,6 +111,19 @@ struct dwc3_qcom { >> int num_ports; >> }; >> >> +/* >> + * SA8295 has 4 power event IRQ STAT registers to be checked >> + * during suspend resume. >> + */ > But this driver supports much more than just SA8295? > Yes. Other than SA8295, all single port controllers and SA8195(2 port controller), have these reigsters. The rational behind adding this array was that depending on num_ports, any controller can access its required pwr_event_irq_stat register and loop in the suspend/resume code would take care of it. Perhaps I can change the comments to indicate that the array would be used by all controllers and not just SA8295. >> +#define NUM_PWR_EVENT_STAT_REGS 4 >> + >> +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = { >> + 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; >> @@ -440,15 +457,19 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) >> >> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> { >> + u8 num_ports; > Maybe I'm picky, but I'm not sure defining a variable for > a single use of an object with a rather short name > (qcom->num_ports) is justified, here and below.. > Sure, will replace num_ports with (qcom->num_ports) and remove the extra variable. Regards, Krishna,
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index f8f8c5e39a01..34eeebb74a6a 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) @@ -107,6 +111,19 @@ struct dwc3_qcom { int num_ports; }; +/* + * SA8295 has 4 power event IRQ STAT registers to be checked + * during suspend resume. + */ +#define NUM_PWR_EVENT_STAT_REGS 4 + +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = { + 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; @@ -440,15 +457,19 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) { + u8 num_ports; u32 val; int i, ret; if (qcom->is_suspended) return 0; - 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"); + num_ports = qcom->num_ports; + 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]); @@ -471,6 +492,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) { + u8 num_ports; int ret; int i; @@ -494,9 +516,12 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) 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); - + num_ports = qcom->num_ports; + for (i = 0; i < num_ports; i++) { + 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; return 0;
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 | 39 +++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-)