Messages in this thread | | | Date | Mon, 23 Oct 2023 16:54:11 +0530 | Subject | Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver | From | Krishna Kurapati PSSNV <> |
| |
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: >>> On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote: > >>>> +#define NUM_PHY_IRQ 4 >>>> + >>>> +enum dwc3_qcom_ph_index { >>> >>> "phy_index" >>> >>>> + DP_HS_PHY_IRQ_INDEX = 0, >>>> + DM_HS_PHY_IRQ_INDEX, >>>> + SS_PHY_IRQ_INDEX, >>>> + HS_PHY_IRQ_INDEX, >>>> +}; >>>> + >>>> struct dwc3_acpi_pdata { >>>> u32 qscratch_base_offset; >>>> u32 qscratch_base_size; >>>> u32 dwc3_core_base_size; >>>> + /* >>>> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS >>>> + * IRQ's respectively. >>>> + */ >>>> + int phy_irq_index[NUM_PHY_IRQ - 1]; >>>> int hs_phy_irq_index; >>>> - int dp_hs_phy_irq_index; >>>> - int dm_hs_phy_irq_index; >>>> - int ss_phy_irq_index; >>>> bool is_urs; >>>> }; >>>> >>>> @@ -73,10 +84,12 @@ struct dwc3_qcom { >>>> int num_clocks; >>>> struct reset_control *resets; >>>> >>>> + /* >>>> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS >>>> + * respectively. >>>> + */ >>>> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS]; >>>> int hs_phy_irq; >>>> - int dp_hs_phy_irq; >>>> - int dm_hs_phy_irq; >>>> - int ss_phy_irq; >>> >>> I'm not sure using arrays like this is a good idea (and haven't you >>> switched the indexes above?). >>> >>> Why not add a port structure instead? >>> >>> struct dwc3_qcom_port { >>> int hs_phy_irq; >>> int dp_hs_phy_irq; >>> int dm_hs_phy_irq; >>> int ss_phy_irq; >>> }; >>> >>> and then have >>> >>> struct dwc3_qcom_port ports[DWC3_MAX_PORTS]; >>> >>> in dwc3_qcom. The port structure can the later also be amended with >>> whatever other additional per-port data there is need for. >>> >>> This should make the implementation cleaner. >>> >>> 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] 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]
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.
Regards, Krishna,
| |