linux-kernel - Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <04615205-e380-4719-aff1-f32c26004b14@quicinc.com>
Date:   Fri, 3 Nov 2023 15:34:52 +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 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM
 Glue driver



On 10/24/2023 12:26 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/23/2023 7:37 PM, Johan Hovold wrote:
> 
>>> Right. And I assume there are hs_phy_irqs also for the first two USB
>>> controllers on sc8280xp?
> 
>> There are, I can dig through and find out. Atleast in downstream I don't
>> see any use of them.
> 
> Yes, please do post how these are wired as well for completeness.
> 
>>> Can you find out anything more about what hs_phy_irq is used for? It
>>> appears to be an HS wakeup interrupt like the dp/dm ones, but there are
>>> not really any details on how it is supposed to be used.
>>
>>    This IRQ is really not used in downstream controllers. Not sure if its
>> a good idea to add driver code for that. I did some digging and I got
>> the reason why I first said that there is only one hs_phy_irq for
>> tertiary port of controller. The hardware programming sequence doesn't
>> specify usage of these 4 IRQ's but the hw specifics mention that there
>> are 4 of them. Adding driver support for these IRQ's is not a good idea
>> (atleast at this point because they are not used in downstream and I am
>> not sure what would be the side effect). For now I suggest we can add
>> them in bindings and DT and not handle the 4 hs_phy_irq's in the driver
>> code (meaning not add the hs_phy_irq to port structure we plan on adding
>> to dwc3_qcom).
> 
> But there is already support for these interrupts in the driver. You
> work for Qualcomm who built the thing so surely you can figure how they
> intended these to be used?
> 
> You need to provide this information so that we can determine what the
> binding should look like. The implementation would also be simplified if
> we don't have to add random hacks to it just because we don't know why
> the vendor driver you refer does not use it currently on this particular
> platform.
> 

Hi Johan,

Regarding the points of discussion we had last week on [1], here are 
some clarifications:

1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as 
mentioned. Why do we need them and would we use it in multiport targets ?

DPSE and DMSE are single ended line state of DP and DM lines. The DP 
line and DM line stay in steady High or Low during suspend and they flip 
when there is a RESUME or REMOTE WAKE. This is what we do/check in 
dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed.

Initially in QUSB2 targets, the interrupts were enabled and configured 
in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2].
In that case, we modify DP/DM interrupts in phy registers, specifically 
QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is 
triggered. But in femto targets, this is done via DP/DM interrupts and 
there is no use of hs_phy_irq. Even hw folks confirmed they dont use 
hs_ph_irq in femto phy targets.

As an experiment, I tried to test wakeup by pressing buttons on 
connected keyboard when in suspend state or connecting/disconnecting 
keyboard in suspended state on different ports and only see dp/dm IRQ's 
getting fired although we register for hs_phy_irq as well:

/ # cat /proc/interrupts  |grep phy_
171:   1  0   0   0  0  0  0  0       PDC 127 Edge      dp_hs_phy_1
172:   2  0   0   0  0  0  0  0       PDC 126 Edge      dm_hs_phy_1
173:   3  0   0   0  0  0  0  0       PDC 129 Edge      dp_hs_phy_2
174:   4  0   0   0  0  0  0  0       PDC 128 Edge      dm_hs_phy_2
175:   0  0   0   0  0  0  0  0       PDC 131 Edge      dp_hs_phy_3
176:   2  0   0   0  0  0  0  0       PDC 130 Edge      dm_hs_phy_3
177:   2  0   0   0  0  0  0  0       PDC 133 Edge      dp_hs_phy_4
178:   5  0   0   0  0  0  0  0       PDC 132 Edge      dm_hs_phy_4
179:   0  0   0   0  0  0  0  0       PDC  16 Level     ss_phy_1
180:   0  0   0   0  0  0  0  0       PDC  17 Level     ss_phy_2
181:   0  0   0   0  0  0  0  0     GICv3 163 Level     hs_phy_1
182:   0  0   0   0  0  0  0  0     GICv3 168 Level     hs_phy_2
183:   0  0   0   0  0  0  0  0     GICv3 892 Level     hs_phy_3
184:   0  0   0   0  0  0  0  0     GICv3 891 Level     hs_phy_4

Since the hs_phy_irq is applicable only for qusb2 targets, do we still 
need to add it to DT.

2. BAM Irq usage (u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]):

BAM IRQ is not needed in host-only controller. It was just added in 
process of porting/deriving code from DRD controllers and is 
non-functional (confirmed by HW team here). We can skip this from DT of 
multiport.

3. ctrl_irq[1] usage:

This is a feature of SNPS controller, not qcom glue wrapper, and is 
present on all targets (non-QC as well probably). As mentioned before on 
[3], this is used for HW acceleration.

In host mode, XHCI spec does allow for multiple interrupters when 
multiple event rings are used. A possible usage is multiple execution 
environments something like what we are doing on mobile with ADSP audio 
offload [4]. Another possibility could be some of virtualization where 
host/hyp would manage the first interrupter and could allow a guest to 
operate only with the second (though current design does not go far 
enough to offer true isolation for real VM type workloads). The 
additional interrupts (ones other than ctrl_irq[0]) are either for 
virtualization use cases, or for our various “hw offload” features. In 
device mode, these are used for offloading tethering functionality to 
IPA FW.

Since the DeviceTree passed to the OS, should describe the hardware to 
the OS, and should represent the hardware from the point-of-view of the 
OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux 
would not use the other interrupts. Furthermore AFAIK even UEFI/Windows 
also use only ctrl_irq[0] for host mode in their execution environment 
today. Do we still need to add this to bindings and DT ?

[1]: https://lore.kernel.org/all/ZTJ_T1UL8-s2cgNz@hovoldconsulting.com/
[2]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-qusb2.c?h=v6.6#n626
[3]: https://lore.kernel.org/all/ZTduh5LULBMYf3wq@hovoldconsulting.com/
[4]: 
https://lore.kernel.org/all/20231017200109.11407-1-quic_wcheng@quicinc.com/

Regards,
Krishna,