Date:   Thu, 23 Nov 2023 01:02:24 +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 11/16/2023 6:33 PM, Johan Hovold wrote:
> On Wed, Nov 15, 2023 at 11:12:16PM +0530, Krishna Kurapati PSSNV wrote:
> 
>>> Are you sure there's no support for hs_phy_irq also in the "femto" PHYs
>>> and that it's just that there is currently no driver support for using
>>> them?
>>>
>>> And why is it defined if there is truly no use for it?
> 
>> We had an internal sync up with HW folks and here is some baseline
>> suggestions we received:
>>
>> If DP/DM interrupts are defined, then that is the preferred path to
>> used, irrespective if HS Phy irq is defined or not / or whether it is
>> Femto / QUSB2 target. There is no target that has femto phy but misses
>> DP/DM today.
> 
> Ok, but just knowing that it is "preferred" does not in itself mean that
> it should be removed from the binding.
> 
> We need to know that it's effectively useless (i.e. that the interrupts
> are defined but cannot be triggered) for that.
> 
> We can still use the DP/DM interrupts in favour of HS in the driver
> however.
> 
>> For cases like sdm660/msm8998/msm8953/msm8956, these targets use
>> hs_phy_irq only and don't rely on DP/DM. So we cannot remove the binding
>> in entirety.
> 
> I fixed the binding for those specific platforms last year:
> 
> 	dd566faebe9f ("dt-bindings: usb: qcom,dwc3: refine interrupt requirements")
> 
> But as I mentioned in that commit message the following platforms do not
> have any wakeup interrupts specified in mainline currently:
> 
>        - qcom,ipq4019-dwc3
>        - qcom,ipq6018-dwc3
>        - qcom,ipq8064-dwc3
>        - qcom,ipq8074-dwc3
>        - qcom,msm8994-dwc3
>        - qcom,qcs404-dwc3
> 
> It would be good to get that cleaned up too (i.e. add the missing
> interrupt definitions and update the binding to match).
> 
>>> Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does
>>> the following Qualcomm SoCs define all three?
> 
>> HS Phy Irq is redundant or functionality is mutually exclusive in this
>> case. If there are targets that define all three, then we need to update
>> those to only utilize DP/DM interrupts.
> 
> No, as I wrote above that depends on if the HS interrupt is truly
> useless. Otherwise it still belongs in the binding, even if the driver
> uses DP/DM in place of it.
> 
> Again, the binding should describe the hardware, not what a particular
> OS chooses to use.
> 

Hi Johan,

  Sorry for delayed response.

  Pushed [1] to address all the queries and comments. I was initially 
looking at only Femto phy targets, but when I looked at all targets in 
general, seems there is one irq not defined in bindings. It is qubs2_phy 
irq which is named as "hs_phy_irq" on QUSB target DT's (both downstream 
and upstream).

There is one actual "hs_phy_irq" as well but it is not used either by hs 
validation team or sw team on any target. It was put in for debug 
purpose only and doesn't have code to trigger it (even downstream never 
implemented it I suppose) Atleast 4.4 onwards I saw the code but I 
didn't see the actual hs_phy_irq being used. It was the qusb2_phy irq 
named as hs_phy_irq.

Even hw folks used it under the same name which is why they recommended 
using it on qusb2 targets and dp/dm on femto targets.

As we moved from qusb2 to femto phys, since qusb2_phy irq was not 
present, the actual hs_phy_irq was put in the interrupts although it is 
never triggered. That is why when I tried to check on sa8295-adp 
multiport, those 4 hs interrupts never got fired. Hope the explanation 
clears the confusion present around the interrupts.

On some targets the hs_phy_irq was given vector number of pwr_event irq 
also like sm8550/sm8450 etc., I tried to address those as well in the 
series.

Also, per your question as to there are some qusb2 targets having dp/dm 
interrupts defined... It is only for SDM845/SDM670/SM6350 which were 
last in line of using qusb2 phy's and they started incorporating dp/dm 
interrupts.

Also added missing interrupts for qcs404/ipq5332.
I didn't add missing interrupts on sc8280xp because I see that current 
interrupts present are working fine (I see ADB working and wakeup 
working as well), but the interrupt vector numbers are off by "1" 
between hs specifics and DT (both upstream and downstream). Will sort it 
out and clean that target up later.

[1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412

Regards,
Krishna,

Powered by blists - more mailing lists