[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <00044364-9a6a-4138-9c17-7b0b801e9f10@quicinc.com>
Date: Tue, 24 Oct 2023 14:11:31 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Johan Hovold <johan@...nel.org>
CC: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
"Andy Gross" <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
"Konrad Dybcio" <konrad.dybcio@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Felipe Balbi <balbi@...nel.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<quic_pkondeti@...cinc.com>, <quic_ppratap@...cinc.com>,
<quic_jackp@...cinc.com>, <ahalaney@...hat.com>,
<quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable
ports of multiport
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,
Powered by blists - more mailing lists