[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fb5e5e1d-520c-4cbc-adde-f30e853421a1@quicinc.com>
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