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,
