Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport

[Krishna Kurapati PSSNV]

On 10/24/2023 12:40 PM, Johan Hovold wrote:
> > > This comment no longer makes sense with your current implementation.
> > Can you help elaborate on your comment ? Do you mean that this API
> > doesn't get speed on all ports, but this has to be called in a loop to
> > get all the port speeds ? In that sense, I agree, I can change the
> > comments here.
>
> It does not make sense to keep only half the comment after your update
> as it is a suggestion for how one could go about and generalise this for
> multiport, which is what you are now doing.
>   

Thanks for explanation. Will update the comments.

> > > But perhaps this should be done using usb_hub_for_each_child() instead
> > > as that may be more efficient. Then you use this function to read out
> > > the speed for all the ports in go (and store it in the port structures I
> > > mentioned). Please determine which alternative is best.
> > Either ways is fine. We would have qcom->num_ports to determine how many
> > speeds we can read.
>
> That's not the point. I'm referring to which alternative is less
> computationally expensive and allows for a clean implementation.
>
> Please do try to figure it out yourself.
I don't think its much of a difference:

while (loop over num_ports) {
	read_usb2_speed()
}

read_usb2_speed() {
	while (loop over num_ports) {
		hub api to read speed.
	}
}

The second one would avoid calling read_usb2_speed multiple times. Will 
take that path.

> > > [ I realise that the confusion around hs_phy_irq may be partly to blame
> > > for this but since that one is also a per-port interrupt, that's no
> > > longer an issue. ]
> >
> > I don't want to add support for this right away [1]. I would like to
> > keep hs_phy_irq outside the loop for now.
>
> No. Stop trying to take shortcuts. Again, this is upstream, not
> Qualcomm's vendor kernel.

I don't think it is a shortcut.

The reason I said I would keep it out of loop is I know why we need 
DP/DM/SS IRQ's during wakeup. The wakeup signals come in as 
rising/falling edges in high speed on DP/DM lines and LFPS terminations 
come on SS lines.

So we need these 3 interrupts for sure in wakeup context.
hs_phy_irq is not mandatory for wakeup. Any particular reason why it is 
needed to add driver support for hs_phy_irq's of multiport now ? May be 
I am missing something. If there is any reason why we need to add it 
now, I would try to learn and see if it has any side effects (like 
generating spurious wakeup's) and if nothing, I would add it back to 
port structure.

Regards,
Krishna,