Date:   Mon, 23 Oct 2023 22:42: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 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM
 Glue driver



On 10/23/2023 7:37 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 04:54:11PM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/23/2023 2:51 PM, Johan Hovold wrote:
>>> On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
>>>> On 10/20/2023 6:53 PM, Johan Hovold wrote:
> 
>>>>> I also don't like the special handling of hs_phy_irq; if this is really
>>>>> just another name for the pwr_event_irq then this should be cleaned up
>>>>> before making the code more complicated than it needs to be.
>>>>>
>>>>> Make sure to clarify this before posting a new revision.
>>>>
>>>> hs_phy_irq is different from pwr_event_irq.
>>>
>>> How is it different and how are they used?
>>>
>>>> AFAIK, there is only one of this per controller.
>>>
>>> But previous controllers were all single port so this interrupt is
>>> likely also per-port, even if your comment below seems to suggest even
>>> SC8280XP has one, which is unexpected (and not described in the updated
>>> binding):
>>>
>>> 	Yes, all targets have the same IRQ's. Just that MP one's have
>>> 	multiple IRQ's of each type. But hs-phy_irq is only one in
>>> 	SC8280 as well.
>>>
>>> 	https://lore.kernel.org/lkml/70b2495f-1305-05b1-2039-9573d171fe24@quicinc.com/
>>>
>>> Please clarify.
>>>
>>
>> For sure pwr_event_irq and hs_phy_irq are different. I assumed it was
>> per-controller and not per-phy because I took reference from software
>> code we have on downstream and hs_phy for multiport is not used
>> anywhere. I don't see any functionality implemented in downstream for
>> that IRQ. And it is only one for single port controllers.
>>
>> But I got the following info from HW page and these are all the
>> interrupts (on apss processor) for multiport (extra details removed):
>>
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_0	SYS_apcsQgicSPI[130]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_1	SYS_apcsQgicSPI[135]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_3	SYS_apcsQgicSPI[856]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_2	SYS_apcsQgicSPI[857]
>>
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[0]	SYS_apcsQgicSPI[133]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[1]	SYS_apcsQgicSPI[134]
> 
> This second core interrupt is also missing in the updated binding... It
> is defined in the ACPI tables so presumably it is needed for the
> multiport controller.
> 
> Do you have any more details on this one?
> 
>> u_cm_usb3_uni_wrapper_mp0_usb3phy_debug_irq	SYS_apcsQgicSPI[668]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]	SYS_apcsQgicSPI[830]
>> u_cm_usb3_uni_wrapper_mp1_usb3phy_debug_irq	SYS_apcsQgicSPI[855]
>>
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_0	SYS_apcsQgicSPI[131]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_1	SYS_apcsQgicSPI[136]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_3	SYS_apcsQgicSPI[859]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_2	SYS_apcsQgicSPI[860]
> 
> Ok, so at least we know hs_phy_irq and pwr_event_irq are distinct and
> both per-port.
> 
> The ACPI tables do not seem to include these, but yeah, that doesn't say
> much more than that the Windows implementation doesn't currently use
> them either.
> 
>> u_cm_dwc_usb2_hs0_usb2_dpse	apps_pdc_irq_out[127]
>> u_cm_dwc_usb2_hs0_usb2_dmse	apps_pdc_irq_out[126]
>> u_cm_dwc_usb2_hs1_usb2_dpse	apps_pdc_irq_out[129]
>> u_cm_dwc_usb2_hs1_usb2_dmse	apps_pdc_irq_out[128]
>> u_cm_dwc_usb2_hs2_usb2_dpse	apps_pdc_irq_out[131]
>> u_cm_dwc_usb2_hs2_usb2_dmse	apps_pdc_irq_out[130]
>> u_cm_dwc_usb2_hs3_usb2_dpse	apps_pdc_irq_out[133]
>> u_cm_dwc_usb2_hs3_usb2_dmse	apps_pdc_irq_out[132]
>> u_cm_usb3_uni_wrapper_mp0_qmp_usb3_lfps_rxterm_irq	apps_pdc_irq_out[16]
>> u_cm_usb3_uni_wrapper_mp1_qmp_usb3_lfps_rxterm_irq	apps_pdc_irq_out[17]
>>
>> Seems like there are 4 IRQ's for HS.
> 
> Right. And I assume there are hs_phy_irqs also for the first two USB
> controllers on sc8280xp?

Hi Johan,

There are, I can dig through and find out. Atleast in downstream I don't 
see any use of them.

> 
> 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).

Also I plan on splitting the patchset into 4 parts (essentially 4 diff 
series):

1. Bindings update for hs_phy_irq's
2. DT patches for MP controller and platform specific files
3. Core driver update for supporting multiport
4. QCOM driver update for supporting wakeup/suspend/resume

This is in accordance to [1] and that way qcom code won't block core 
driver changes from getting merged. Core driver changes are independent 
and are sufficient to get multiport working.

[1]: 
https://lore.kernel.org/all/d4663197-8295-4967-a4f5-6cc91638fc0d@linaro.org/

Regards,
Krishna,

Powered by blists - more mailing lists