Date: Tue, 5 Mar 2024 21:09:57 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Johan Hovold <johan@...nel.org>
CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring
	<robh+dt@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Wesley Cheng
	<quic_wcheng@...cinc.com>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        "Greg
 Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Conor Dooley
	<conor+dt@...nel.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Felipe Balbi
	<balbi@...nel.org>, <devicetree@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_ppratap@...cinc.com>,
        <quic_jackp@...cinc.com>
Subject: Re: [PATCH v15 7/9] usb: dwc3: qcom: Refactor IRQ handling in glue
 driver



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@quicinc.com/

Regards,
Krishna,

Powered by blists - more mailing lists