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

[Johan Hovold <johan@kernel.org>]

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?

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.

Johan