by Krishna Kurapati

[permalink] [raw]

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/[email protected]/

Regards,
Krishna,