Messages in this thread | | | Date | Tue, 16 May 2023 07:49:14 +0530 | Subject | Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper | From | Krishna Kurapati PSSNV <> |
| |
On 5/16/2023 3:57 AM, Bjorn Andersson wrote: > On Sun, May 14, 2023 at 11:19:14AM +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. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------ >> 1 file changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 959fc925ca7c..7a9bce66295d 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -37,7 +37,10 @@ >> #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 +96,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, > > Seems to be excessive indentation of these... > > Can you also please confirm that these should be counted starting at 1 - > given that you otherwise talk about port0..N-1? > Hi Bjorn,
I am fine with either way. Since this just denoted 4 different ports, I named them starting with 1. Either ways, we will run through array from (0-3), so we must be fine.
>> +}; >> + >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) >> { >> u32 reg; >> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> { >> u32 val; >> int i, ret; >> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> >> 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"); >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > > In the event that the dwc3 core fails to acquire or enable e.g. clocks > its drvdata will be NULL. If you then hit a runtime pm transition in the > dwc3-qcom glue you will dereference NULL here. (You can force this issue > by e.g. returning -EINVAL from dwc3_clk_enable()). > > So if you're peaking into qcom->dwc3 you need to handle the fact that > dwc might be NULL, here and in resume below. > Thanks for catching this. You are right, there were instances where the we saw probe for dwc3 being deferred while the probe for dwc3-qcom was still successful [1]. In this case, if the dwc3 probe never happened and system tries to enter suspend, we might hit a NULL pointer dereference.
I will fix this in v9.
[1]: https://patchwork.kernel.org/project/linux-usb/patch/1657809067-25815-1-git-send-email-quic_kriskura@quicinc.com/
Regards, Krishna,
| |