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

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

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,