Messages in this thread | | | Date | Tue, 24 Oct 2023 14:11:31 +0530 | Subject | Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport | From | 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,
| |