Re: [PATCH v13 07/10] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper

[Johan Hovold <johan@kernel.org>]

On Sat, Oct 07, 2023 at 09:18:03PM +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.

No need to mention SA8295P as this is needed for all multiport
controllers.

Say something about adding support for multiport controllers generally
instead and mention what the power event irqs are used for.
 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 651b9775a0c2..dbd4239e61c9 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

Not sure these defines makes sense on their own. You now only use them
via the array below.

I think I already asked you whether these offsets depend on SoC and you
said no, right?

> +
>  #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;
>  };
>  
> +/*
> + * Currently non-multiport controller have only one PWR_EVENT_IRQ register,
> + * but multiport controllers like SA8295 contain upto 4 of them.
> + */

Please try not talk about "currently" and as things are likely to
change or, in fact, even *are* changing with your very patch series.

Again, this is not SA8295 specific.

> +#define NUM_PWR_EVENT_STAT_REGS	4

You already have MAX_PORTS, why are you defining a new define that will
always have to be equal to MAX_PORTS?

> +
> +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = {

missing const

> +	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;
> @@ -446,9 +463,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  	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 < qcom->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");

Error message should contain the port number.

> +	}
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--)
>  		clk_disable_unprepare(qcom->clks[i]);
> @@ -494,9 +513,11 @@ 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);
> -
> +	for (i = 0; i < qcom->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);

Again, continuation lines should be indented at least two tabs further.

> +	}
>  	qcom->is_suspended = false;
>  
>  	return 0;

Johan