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

From: Krishna Kurapati PSSNV
Date: Mon Oct 23 2023 - 13:13:00 EST



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

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

Regards,
Krishna,