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

[Johan Hovold <johan@kernel.org>]

On Tue, Oct 24, 2023 at 02:11:31PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/24/2023 12:40 PM, Johan Hovold wrote:
 
> >>> 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.

You need to look at the implementation of usb_hub_for_each_child() and
usb_hub_find_child() to determine that, which you now forced me to
do; and yes, you're right, this shouldn't matter from an efficiency
standpoint.

> >>> [ 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.

It is a shortcut as this interrupt is per-port and some SoC's already
use it. So you're making a mess of the implementation for no good
reason.

> 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.

As I've mentioned a few times now, the hs_phy_irq is already used by a
few SoC's so you can't just pretend it doesn't exist and mess up the
implementation for no good reason.

Just find out how it is used and why only some Qualcomm SoC's use it
currently. It appears to be used in parallel with the DP/DM interrupts,
and it has been there from the start:

	a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

Sure, the wakeup implementation was incomplete and broken for a long
time, but I'm not going to let you continue this practise of pushing
incomplete hacks upstream which someone else will eventually be forced
to clean up. You have the documentation, just use it.

Johan