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

[Johan Hovold]

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.

> >> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
> >> -				char *disp_name, int irq)
> >> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
> >> +				const char *disp_name, int irq)
> > 
> > Ok, here you did drop the second name parameter, but without renaming
> > the first and hidden in a long diff without any mention anywhere.
> > 
> I didn't understand the comment. Can you please elaborate.
> I didn't drop the second parameter. In the usage of this call, I passed 
> same value to both irq_name and disp_name:
> 
> "dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);"
> 
> I mentioned in cover-letter that I am using same name as obtained from 
> DT to register the interrupts as well. Should've mentioned that in 
> commit text of this patch. Will do it in next version.

Ah, sorry I misread the diff. You never drop the second name even though
it is now redundant as I pointed on in a comment to one of the earlier
patches.

Johan