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

[Johan Hovold]

On Mon, Oct 23, 2023 at 10:52:38PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 9:28 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:03PM +0530, Krishna Kurapati 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
> > 
> > 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?
> > 
> There are only 3 QC SoC's today that support multiport.
> The offsets mentioned here are for SC8280 based platforms.
> 
> For Sc8180 based platforms, these are the offsets:
> USB3_MP_PWR_EVNT_IRQ_STAT	0xA4F8858
> USB3_MP_PWR_EVNT_IRQ_1_STAT	0xA4F89DC
> 
> These would translate to 0x58 and 0x1DC
> 
> And for SX8380 the values are as follows:
> 
> USB3_MP_PWR_EVNT_IRQ_STAT	0xA4F8858
> USB3_MP_PWR_EVNT_IRQ_1_STAT	0xA4F89DC
> 
> So here also, the offsets are same. 0x58 and 0x1DC.
> So these are not SoC specific (atleast looking at the controllers 
> present). But there is no mathematical pattern to denote this as in the 
> following form (x + (port_num) * y). So made an array like this.

Sounds good. Thanks for confirming.

> >> +#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?
> > 
> Do you recommend using the same max_ports ? If so, I can remove this 
> macro altogether.

Indeed, and perhaps also some (compile-time) assert as the driver breaks
if they ever get out of sync.

Johan