Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <7f800c67-064e-405a-bc36-39544d9e28ef@quicinc.com>
Date:   Tue, 7 Nov 2023 13:59:32 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM
 Glue driver
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
To:     Johan Hovold <johan@kernel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        "Andy Gross" <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        "Konrad Dybcio" <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>,
        Wesley Cheng <quic_wcheng@quicinc.com>,
        <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <quic_pkondeti@quicinc.com>, <quic_ppratap@quicinc.com>,
        <quic_jackp@quicinc.com>, <ahalaney@redhat.com>,
        <quic_shazhuss@quicinc.com>
References: <20231007154806.605-1-quic_kriskura@quicinc.com>
 <20231007154806.605-6-quic_kriskura@quicinc.com>
 <ZTJ_T1UL8-s2cgNz@hovoldconsulting.com>
 <14fc724c-bc99-4b5d-9893-3e5eff8895f7@quicinc.com>
 <ZTY7Lwjd3_8NlfEi@hovoldconsulting.com>
 <cabf24d0-8eea-4eb5-8205-bf7fe6017ec2@quicinc.com>
 <ZTZ-EvvbuA6HpycT@hovoldconsulting.com>
 <fb5e5e1d-520c-4cbc-adde-f30e853421a1@quicinc.com>
 <ZTdqnSHq_Jo8AuPW@hovoldconsulting.com>
 <04615205-e380-4719-aff1-f32c26004b14@quicinc.com>
Content-Language: en-US
In-Reply-To: <04615205-e380-4719-aff1-f32c26004b14@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

>>
>> 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/
> 

Hi Johan,

  Can you help provide your comments on the above mentioned points so 
that we can take the discussion forward and finalize changes for v14 
patches. And thanks for all the reviews so far on previous revisions.

Regards,
Krishna,