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