Re: [PATCH v15 7/9] usb: dwc3: qcom: Refactor IRQ handling in glue driver

[Krishna Kurapati PSSNV]

On 3/1/2024 7:23 PM, Johan Hovold wrote:

[...]

> > +
> >   struct dwc3_acpi_pdata {
> >   	u32			qscratch_base_offset;
> >   	u32			qscratch_base_size;
> >   	u32			dwc3_core_base_size;
> > -	int			qusb2_phy_irq_index;
> > -	int			dp_hs_phy_irq_index;
> > -	int			dm_hs_phy_irq_index;
> > -	int			ss_phy_irq_index;
> > +	/*
> > +	 * The phy_irq_index corresponds to ACPI indexes of (in order)
> > +	 * DP/DM/SS/QUSB2 IRQ's respectively.
> > +	 */
> > +	int			phy_irq_index[NUM_PHY_IRQ];
> >   	bool			is_urs;
> >   };
>
> I asked you to add a port structure and get rid of the PHY indexes in
> v13, and so you did for the diver data below, but you still have an
> array of indexes here for the ACPI data.
>
> I don't think ever got around to actually reviewing the ACPI hack (and
> maybe I was hoping that we'd be able to drop ACPI support before merging
> multi-port support), but removing these fields and replacing them with
> an array is a step in the wrong direction (e.g. making the code harder
> to read).
>
> Why can't you just add a helper function which returns one of these
> fields based on the interrupt name string?

I think since [1] has been accepted, this comment has been taken care of.

> > +struct dwc3_qcom_port {
> > +	int			dp_hs_phy_irq;
> > +	int			dm_hs_phy_irq;
> > +	int			ss_phy_irq;
> > +};
>
> And as I've explicitly said before, you should include hs_phy_irq here.
>
> It's a port interrupt and special casing just this one make no sense at
> all even if there are no multi-port controller that use it.

Okay. Will add it to port structure.
I only kept it outside because there are no real devices which has 
multiple ports and qusb2_phy_irq in them.

> > +
> >   struct dwc3_qcom {
> >   	struct device		*dev;
> >   	void __iomem		*qscratch_base;
> > @@ -74,9 +90,7 @@ struct dwc3_qcom {
> >   	struct reset_control	*resets;
> >   
> >   	int			qusb2_phy_irq;
> > -	int			dp_hs_phy_irq;
> > -	int			dm_hs_phy_irq;
> > -	int			ss_phy_irq;
> > +	struct dwc3_qcom_port	port_info[DWC3_MAX_PORTS];
>
> Just name the array 'ports' as I already suggested. It's more succinct
> and makes the code that uses it easier to read.
>
> >   	enum usb_device_speed	usb2_speed;
> >   
> >   	struct extcon_dev	*edev;
> > @@ -91,6 +105,7 @@ struct dwc3_qcom {
> >   	bool			pm_suspended;
> >   	struct icc_path		*icc_path_ddr;
> >   	struct icc_path		*icc_path_apps;
> > +	u8			num_ports;
>
> Any reason not to keep this one closer to the ports array?
>
> >   };
>   

[...]

> > -	irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
> > -				pdata ? pdata->ss_phy_irq_index : -1);
> > -	if (irq > 0) {
> > -		ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq");
> > -		if (ret)
> > -			return ret;
> > -		qcom->ss_phy_irq = irq;
> > +	for (i = 0; i < irq_count; i++) {
> > +		irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
> > +		if (irq_index == -1) {
> > +			dev_err(&pdev->dev, "Unknown interrupt-name \"%s\" found\n", irq_names[i]);
>
> This is now spamming the logs with errors like
>
> 	dwc3-qcom a6f8800.usb: Unknown interrupt-name "pwr_event" found
>
> which is clearly just broken.
>
> > +			continue;
> > +		}
> > +		port_index = dwc3_qcom_get_port_index(irq_names[i], irq_index);
> > +		if (port_index == -1) {
> > +			dev_err(&pdev->dev, "Invalid interrupt-name suffix \"%s\"\n", irq_names[i]);
> > +			continue;
> > +		}
> > +
> > +		acpi_index = dwc3_qcom_get_acpi_index(qcom, irq_index, port_index);
> > +
> > +		irq = dwc3_qcom_get_irq(pdev, irq_names[i], acpi_index);
> > +		if (irq > 0) {
> > +			ret = dwc3_qcom_request_irq(qcom, irq, irq_names[i]);
> > +			if (ret)
> > +				return ret;
> > +
> > +			switch (irq_index) {
> > +			case DP_HS_PHY_IRQ_INDEX:
> > +				qcom->port_info[port_index - 1].dp_hs_phy_irq = irq;
> > +				break;
> > +			case DM_HS_PHY_IRQ_INDEX:
> > +				qcom->port_info[port_index - 1].dm_hs_phy_irq = irq;
> > +				break;
> > +			case SS_PHY_IRQ_INDEX:
> > +				qcom->port_info[port_index - 1].ss_phy_irq = irq;
> > +				break;
> > +			case QUSB2_PHY_IRQ_INDEX:
> > +				qcom->qusb2_phy_irq = irq;
> > +				break;
> > +			}
> > +
> > +			if (qcom->num_ports < port_index)
> > +				qcom->num_ports = port_index;
> > +		}
> >   	}
>
> Why don't you add a port helper for fetching the interrupts instead?
>
> There are multiple ways that you can use to determine if this is a
> multiport controller or not; you can use OF match data, or simply look
> at one of the interrupts that would always be there for a multiport
> (or single port) controller (e.g. "dp_hs_phy_1").
>
> You can even determine the number of ports first by parsing the
> interrupts names and looking for the highest port number.
>
> Then you can iterate over the ports and parse the interrupts for each
> port in turn, which should allow for a much cleaner and less
> error-prone implementation.

With [1] merged, I think I can use your suggestion of going through and 
checking if dp_hs_phy_X is present or not and if present, it is 
multiport and go through dp_hs_phy_{1/2/3/4} to see the num of ports 
present.

That must simplify the code and make it clean as well.

[1]: 
https://lore.kernel.org/all/20240305093216.3814787-1-quic_kriskura@xxxxxxxxxxx/

Regards,
Krishna,