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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@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]
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,




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux