Re: [PATCH v10 06/11] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

From: Krishna Kurapati PSSNV
Date: Wed Aug 23 2023 - 06:14:04 EST



On 8/12/2023 2:14 PM, Krishna Kurapati PSSNV wrote:
So is "is it okay to add SoC-specific compatibles and add the port number in
match data" what you're asking?

If so, that doesn't seem right.

The user should not "feel free to remove any IRQ", modifying the devicetree to
depict a subset of the hardware is not something we want to support. The driver
has to work with the "full" description in accordance with the bindings.


Hi Konrad,

Thanks for the review.

While I agree with you that we must not skip any hw specifics in DT, there is nothing stopping the user from doing so right ?

And whatever be the case, we must be a fool-proof and fail safe system able to handle all such situations. While we can read interrupt IRQ prefixes to get port count in one way or other, adding a compatible would be the least ambiguous path. Is there any other concern you see in adding a compatible ? I might be missing something because even Bjorn's suggestion too was to try and avoid a new compatible addition and to add it only if we have no other way of reliably reading the port count (which I believe would be an issue if we need to rely on interrupt name reading).

Hi Konrad. Came up with an implementation of reading the interrupt names and parsing for port count and added them as attachments. I still feel adding a compatible is a better option. Let me know which one is the better path. The one in v10, or something similar to the attached patch. (I tested it on sc7280/sc8280 and interrupts are registered properly)

Regards,
Krishna,From 60d5d0a678b864756a18ff17337ecddc257c9b44 Mon Sep 17 00:00:00 2001
From: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Date: Tue, 22 Aug 2023 09:42:59 +0530
Subject: [PATCH 1/2] usb: dwc3: qcom: Add helper function to request threaded
IRQ

Cleanup setup irq call by implementing a new prep_irq helper function
and using it to request threaded IRQ's.

Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Change-Id: Ic136d1cdbf72602a0b8b0bf7ec4d6a7a723431de
---
drivers/usb/dwc3/dwc3-qcom.c | 63 +++++++++++++++++-------------------
1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 3de43df6bbe8..f14ddc9c541d 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -535,6 +535,24 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev,
return ret;
}

+static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
+ char *disp_name, int irq)
+{
+ int ret;
+
+ /* Keep wakeup interrupts disabled until suspend */
+ irq_set_status_flags(irq, IRQ_NOAUTOEN);
+ ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
+ qcom_dwc3_resume_irq,
+ IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+ disp_name, qcom);
+
+ if (ret)
+ dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
+
+ return ret;
+}
+
static int dwc3_qcom_setup_irq(struct platform_device *pdev)
{
struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
@@ -545,61 +563,40 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
pdata ? pdata->hs_phy_irq_index : -1);
if (irq > 0) {
- /* Keep wakeup interrupts disabled until suspend */
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 HS", qcom);
- if (ret) {
- dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq",
+ "qcom_dwc3 HS", irq);
+ if (ret)
return ret;
- }
qcom->hs_phy_irq = irq;
}

irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
pdata ? pdata->dp_hs_phy_irq_index : -1);
if (irq > 0) {
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 DP_HS", qcom);
- if (ret) {
- dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq",
+ "qcom_dwc3 DP_HS", irq);
+ if (ret)
return ret;
- }
qcom->dp_hs_phy_irq = irq;
}

irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
pdata ? pdata->dm_hs_phy_irq_index : -1);
if (irq > 0) {
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 DM_HS", qcom);
- if (ret) {
- dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "dm_hs_phy_irq",
+ "qcom_dwc3 DM_HS", irq);
+ if (ret)
return ret;
- }
qcom->dm_hs_phy_irq = irq;
}

irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
pdata ? pdata->ss_phy_irq_index : -1);
if (irq > 0) {
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 SS", qcom);
- if (ret) {
- dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "ss_phy_irq",
+ "qcom_dwc3 SS", irq);
+ if (ret)
return ret;
- }
qcom->ss_phy_irq = irq;
}

--
2.40.0

From 418ac10e88c53f67e947f754aebd3bd97ea67e8a Mon Sep 17 00:00:00 2001
From: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Date: Tue, 22 Aug 2023 15:25:12 +0530
Subject: [PATCH 2/2] usb: dwc3: qcom: Read multiport interrupts

Read multiport interrupts in qcom driver.

Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Change-Id: Ic2bf8cad70c1478c79eb7e9d3dc5d9082c5bb1f9
---
drivers/usb/dwc3/dwc3-qcom.c | 193 ++++++++++++++++++++++++-----------
1 file changed, 134 insertions(+), 59 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index f14ddc9c541d..a0ba71ea9541 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -53,14 +53,19 @@
#define APPS_USB_AVG_BW 0
#define APPS_USB_PEAK_BW MBps_to_icc(40)

+#define NUM_PHY_IRQ 4
+
+#define DP_HS_PHY_IRQ_INDEX 0
+#define DM_HS_PHY_IRQ_INDEX 1
+#define SS_PHY_IRQ_INDEX 2
+#define HS_PHY_IRQ_INDEX 3
+
struct dwc3_acpi_pdata {
u32 qscratch_base_offset;
u32 qscratch_base_size;
u32 dwc3_core_base_size;
+ 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 +78,8 @@ struct dwc3_qcom {
int num_clocks;
struct reset_control *resets;

- int hs_phy_irq;
- int dp_hs_phy_irq;
- int dm_hs_phy_irq;
- int ss_phy_irq;
+ int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
+ int hs_phy_irq;
enum usb_device_speed usb2_speed;

struct extcon_dev *edev;
@@ -91,6 +94,8 @@ struct dwc3_qcom {
bool pm_suspended;
struct icc_path *icc_path_ddr;
struct icc_path *icc_path_apps;
+
+ int num_ports;
};

static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
@@ -375,16 +380,16 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);

if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
} else {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
}

- dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
}

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
@@ -401,20 +406,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
*/

if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_FALLING);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_FALLING);
} else {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_RISING);
}

- dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0], 0);
}

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -553,51 +558,125 @@ static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
return ret;
}

+static int dwc3_qcom_get_irq_index(const char *irq_name)
+{
+ /*
+ * If we are reading IRQ not supported by the driver
+ * like pwr_event_irq, then return -1 indicating the next
+ * helper function to skip processing IRQ name further.
+ */
+ int irq_index = -1;
+
+ if (strncmp(irq_name, "dp_hs_phy", 9) == 0)
+ irq_index = DP_HS_PHY_IRQ_INDEX;
+ else if(strncmp(irq_name, "dm_hs_phy", 9) == 0)
+ irq_index = DM_HS_PHY_IRQ_INDEX;
+ else if(strncmp(irq_name, "ss_phy", 6) == 0)
+ irq_index = SS_PHY_IRQ_INDEX;
+ else if(strncmp(irq_name, "hs_phy", 6) == 0)
+ irq_index = HS_PHY_IRQ_INDEX;
+
+ return irq_index;
+}
+
+static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index)
+{
+ int port_index = -1;
+
+ switch (irq_index) {
+ case DP_HS_PHY_IRQ_INDEX:
+ if (strcmp(irq_name, "dp_hs_phy_irq") == 0)
+ port_index = 1;
+ else
+ sscanf(irq_name, "dp_hs_phy_%d", &port_index);
+ break;
+
+ case DM_HS_PHY_IRQ_INDEX:
+ if (strcmp(irq_name, "dm_hs_phy_irq") == 0)
+ port_index = 1;
+ else
+ sscanf(irq_name, "dm_hs_phy_%d", &port_index);
+ break;
+
+ case SS_PHY_IRQ_INDEX:
+ if (strcmp(irq_name, "ss_phy_irq") == 0)
+ port_index = 1;
+ else
+ sscanf(irq_name, "ss_phy_%d", &port_index);
+ break;
+
+ case HS_PHY_IRQ_INDEX:
+ port_index = 1;
+ break;
+ }
+
+done:
+ return port_index;
+}
+
+static int dwc3_qcom_get_acpi_index(struct dwc3_qcom *qcom, int irq_index,
+ int port_index)
+{
+ const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
+ int acpi_index = -1;
+
+ if ((pdata == NULL) || (port_index != 1))
+ goto done;
+
+ if (irq_index == HS_PHY_IRQ_INDEX)
+ acpi_index = pdata->hs_phy_irq_index;
+ else if (port_index != -1)
+ acpi_index = pdata->phy_irq_index[irq_index];
+
+done:
+ return acpi_index;
+}
+
static int dwc3_qcom_setup_irq(struct platform_device *pdev)
{
struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
- const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
+ struct device_node *np = pdev->dev.of_node;
+ const char **irq_names;
+ int port_index;
+ int acpi_index;
+ int irq_count;
+ int irq_index;
int irq;
int ret;
+ int i;

- irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
- pdata ? pdata->hs_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq",
- "qcom_dwc3 HS", irq);
- if (ret)
- return ret;
- qcom->hs_phy_irq = irq;
- }
+ irq_count = of_property_count_strings(np, "interrupt-names");
+ irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL);
+ if (!irq_names)
+ return -ENOMEM;

- irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
- pdata ? pdata->dp_hs_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq",
- "qcom_dwc3 DP_HS", irq);
- if (ret)
- return ret;
- qcom->dp_hs_phy_irq = irq;
- }
+ ret = of_property_read_string_array(np, "interrupt-names",
+ irq_names, irq_count);
+ for (i = 0; i < irq_count; i++) {
+ irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
+ if (irq_index == -1) {
+ dev_dbg(&pdev->dev, "IRQ not handled");
+ continue;
+ }

- irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
- pdata ? pdata->dm_hs_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "dm_hs_phy_irq",
- "qcom_dwc3 DM_HS", irq);
- if (ret)
- return ret;
- qcom->dm_hs_phy_irq = irq;
- }
+ port_index = dwc3_qcom_get_port_index(irq_names[i], irq_index);
+ acpi_index = dwc3_qcom_get_acpi_index(qcom, irq_index, port_index);

- irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
- pdata ? pdata->ss_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "ss_phy_irq",
- "qcom_dwc3 SS", irq);
- if (ret)
- return ret;
- qcom->ss_phy_irq = irq;
+ irq = dwc3_qcom_get_irq(pdev, irq_names[i], acpi_index);
+ if (irq > 0) {
+ ret = dwc3_qcom_prep_irq(qcom, irq_names[i],
+ irq_names[i], irq);
+ if (ret)
+ return ret;
+
+ if (irq_index == HS_PHY_IRQ_INDEX)
+ qcom->hs_phy_irq = irq;
+ else
+ qcom->phy_irq[irq_index][port_index-1] = irq;
+
+ if (qcom->num_ports < port_index)
+ qcom->num_ports = port_index;
+ }
}

return 0;
@@ -1030,20 +1109,16 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
.qscratch_base_size = SDM845_QSCRATCH_SIZE,
.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
+ .phy_irq_index = {4, 3, 2},
.hs_phy_irq_index = 1,
- .dp_hs_phy_irq_index = 4,
- .dm_hs_phy_irq_index = 3,
- .ss_phy_irq_index = 2
};

static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = {
.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
.qscratch_base_size = SDM845_QSCRATCH_SIZE,
.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
+ .phy_irq_index = {4, 3, 2},
.hs_phy_irq_index = 1,
- .dp_hs_phy_irq_index = 4,
- .dm_hs_phy_irq_index = 3,
- .ss_phy_irq_index = 2,
.is_urs = true,
};

--
2.40.0