lkml.org 
[lkml]   [2023]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport
From

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,

\
 
 \ /
  Last update: 2023-10-24 10:42    [W:0.411 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site