Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport

From: Krishna Kurapati PSSNV
Date: Fri Jul 21 2023 - 04:20:20 EST

Next message: kernel test robot: "[tip:master] BUILD SUCCESS 8f4995b370a57e7ad92c0f66664d171b23234337"
Previous message: Valentin Schneider: "Re: [RFC PATCH v2 16/20] rcu: Make RCU dynticks counter size configurable"
In reply to: Johan Hovold: "Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport"
Next in thread: Johan Hovold: "Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 7/21/2023 1:44 PM, Johan Hovold wrote:

On Mon, Jul 03, 2023 at 12:29:41AM +0530, Krishna Kurapati PSSNV wrote:

On 6/27/2023 8:01 PM, Johan Hovold wrote:

On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote:

+static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev)
+{
+ struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
+ char irq_name[15];
+ int irq;
+ int ret;
+ int i;
+
+ for (i = 0; i < 4; i++) {

DWC3_MAX_PORTS here too and similar below.

+ if (qcom->dp_hs_phy_irq[i])
+ continue;

This is not very nice. You should try to integrate the current lookup
code as I told you to do with the PHY lookups. That is, use a single
loop for all HS/SS IRQs, and pick the legacy name if the number of ports
is 1.

Of course, you added the xhci capability parsing to the core driver so
that information is not yet available, but you need it in the glue
driver also...

As I mentioned earlier, you can infer the number of ports from the
interrupt names. Alternatively, you can infer it from the compatible
string. In any case, you should not need to ways to determine the same
information in the glue driver, then in the core part, and then yet
again in the xhci driver...

The reason why I didn't integrate this with the original function was
the ACPI stuff. The MP devices have no ACPI variant. And I think for
clarity sake its best to keep these two functions separated.

No. The ACPI stuff may make this a little harder to implement, but
that's not a sufficient reason to not try to refactor things properly.

+
+ sprintf(irq_name, "dp%d_hs_phy_irq", i+1);

Spaces around binary operators. Does not checkpatch warn about that?

+ irq = dwc3_qcom_get_irq(pdev, irq_name, -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,
+ irq_name, qcom);
+ if (ret) {
+ dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
+ return ret;
+ }
+ }
+
+ qcom->dp_hs_phy_irq[i] = irq;
+ }
+
+ for (i = 0; i < 4; i++) {
+ if (qcom->dm_hs_phy_irq[i])
+ continue;
+
+ sprintf(irq_name, "dm%d_hs_phy_irq", i+1);
+ irq = dwc3_qcom_get_irq(pdev, irq_name, -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,
+ irq_name, qcom);
+ if (ret) {
+ dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
+ return ret;
+ }
+ }
+
+ qcom->dm_hs_phy_irq[i] = irq;
+ }
+
+ for (i = 0; i < 2; i++) {
+ if (qcom->ss_phy_irq[i])
+ continue;
+
+ sprintf(irq_name, "ss%d_phy_irq", i+1);
+ irq = dwc3_qcom_get_irq(pdev, irq_name, -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,
+ irq_name, qcom);
+ if (ret) {
+ dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
+ return ret;
+ }
+ }
+
+ qcom->ss_phy_irq[i] = irq;
+ }

So the above should all be merged in either a single helper looking up
all the interrupts for a port and resused for the non-MP case.

I agree, Will merge all under some common helper function.

Good.

Johan

Hi Johan,

How about the implementation in the attached patches. This way we don't need to know if we are multiport capable or not.

Regards,
Krishna,
From c5bf1235d7d1c1b629fda7f321b33671d9705b1f Mon Sep 17 00:00:00 2001
From: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Date: Wed, 19 Jul 2023 19:29:10 +0530
Subject: [PATCH 06/11] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue
driver

Refactor setup_irq call to facilitate reading multiport IRQ's
along with non mulitport ones.

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

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 3de43df6bbe8..ab8664ced255 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -74,9 +74,9 @@ struct dwc3_qcom {
struct reset_control *resets;

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

struct extcon_dev *edev;
@@ -93,6 +93,42 @@ struct dwc3_qcom {
struct icc_path *icc_path_apps;
};

+struct dwc3_qcom_irq_info {
+ char *dt_name;
+ char *disp_name;
+ bool acpi_variant_present;
+};
+
+const struct dwc3_qcom_irq_info non_mp_irq_info[4] = {
+ { "hs_phy_irq", "qcom_dwc3 HS", true, },
+ { "dp_hs_phy_irq", "qcom_dwc3 DP_HS", true, },
+ { "dm_hs_phy_irq", "qcom_dwc3 DM_HS", true, },
+ { "ss_phy_irq", "qcom_dwc3 SS", true, },
+};
+
+const struct dwc3_qcom_irq_info mp_irq_info[3][DWC3_MAX_PORTS] = {
+ {
+ { "dp_hs_phy_1", "qcom_dwc3 DP_HS1", false },
+ { "dp_hs_phy_2", "qcom_dwc3 DP_HS2", false },
+ { "dp_hs_phy_3", "qcom_dwc3 DP_HS3", false },
+ { "dp_hs_phy_4", "qcom_dwc3 DP_HS4", false },
+ },
+
+ {
+ { "dm_hs_phy_1", "qcom_dwc3 DM_HS1", false },
+ { "dm_hs_phy_2", "qcom_dwc3 DM_HS2", false },
+ { "dm_hs_phy_3", "qcom_dwc3 DM_HS3", false },
+ { "dm_hs_phy_4", "qcom_dwc3 DM_HS4", false },
+ },
+
+ {
+ { "ss_phy_1", "qcom_dwc3 SS1", false },
+ { "ss_phy_2", "qcom_dwc3 SS2", false },
+ { "ss_phy_3", "qcom_dwc3 SS3", false },
+ { "ss_phy_4", "qcom_dwc3 SS4", false },
+ },
+};
+
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
{
u32 reg;
@@ -375,16 +411,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->dm_hs_phy_irq[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->dp_hs_phy_irq[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->dp_hs_phy_irq[0]);
+ dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq[0]);
}

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

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
@@ -401,20 +437,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->dm_hs_phy_irq[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->dp_hs_phy_irq[0],
IRQ_TYPE_EDGE_FALLING);
} else {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[0],
IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[0],
IRQ_TYPE_EDGE_RISING);
}

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

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -535,72 +571,114 @@ 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);
const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
+ char *disp_name;
+ char *dt_name;
+ int index;
int irq;
int ret;
+ int i;

- irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
+ irq = dwc3_qcom_get_irq(pdev, non_mp_irq_info[0].dt_name,
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, non_mp_irq_info[0].dt_name,
+ non_mp_irq_info[0].disp_name, 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);
- return ret;
+ for (i = 0; i < DWC3_MAX_PORTS; i++) {
+ dt_name = mp_irq_info[0][i].dt_name;
+ disp_name = mp_irq_info[0][i].disp_name;
+ index = (mp_irq_info[0][i].acpi_variant_present && pdata) ?
+ pdata->dp_hs_phy_irq_index : -1;
+
+ irq = dwc3_qcom_get_irq(pdev, dt_name, index);
+ if ((irq < 0) && (i == 0)) {
+ dt_name = non_mp_irq_info[1].dt_name;
+ disp_name = non_mp_irq_info[1].disp_name;
+ index = (non_mp_irq_info[1].acpi_variant_present && pdata) ?
+ pdata->dp_hs_phy_irq_index : -1;
+
+ irq = dwc3_qcom_get_irq(pdev, dt_name, index);
+ }
+
+ if (irq > 0) {
+ ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq);
+ if (ret)
+ return ret;
+ qcom->dp_hs_phy_irq[i] = irq;
}
- 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);
- return ret;
+ for (i = 0; i < DWC3_MAX_PORTS; i++) {
+ dt_name = mp_irq_info[1][i].dt_name;
+ disp_name = mp_irq_info[1][i].disp_name;
+ index = (mp_irq_info[1][i].acpi_variant_present && pdata) ?
+ pdata->dm_hs_phy_irq_index : -1;
+
+ irq = dwc3_qcom_get_irq(pdev, dt_name, index);
+ if ((irq < 0) && (i == 0)) {
+ dt_name = non_mp_irq_info[2].dt_name;
+ disp_name = non_mp_irq_info[2].disp_name;
+ index = (non_mp_irq_info[2].acpi_variant_present && pdata) ?
+ pdata->dp_hs_phy_irq_index : -1;
+
+ irq = dwc3_qcom_get_irq(pdev, dt_name, index);
+ }
+
+ if (irq > 0) {
+ ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq);
+ if (ret)
+ return ret;
+ qcom->dm_hs_phy_irq[i] = irq;
}
- 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);
- return ret;
+ for (i = 0; i < DWC3_MAX_PORTS; i++) {
+ dt_name = mp_irq_info[2][i].dt_name;
+ disp_name = mp_irq_info[2][i].disp_name;
+ index = (mp_irq_info[2][i].acpi_variant_present && pdata) ?
+ pdata->ss_phy_irq_index : -1;
+
+ irq = dwc3_qcom_get_irq(pdev, dt_name, index);
+ if ((irq < 0) && (i == 0)) {
+ dt_name = non_mp_irq_info[3].dt_name;
+ disp_name = non_mp_irq_info[3].disp_name;
+ index = (non_mp_irq_info[3].acpi_variant_present && pdata) ?
+ pdata->dp_hs_phy_irq_index : -1;
+
+ irq = dwc3_qcom_get_irq(pdev, dt_name, index);
+ }
+
+ if (irq > 0) {
+ ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq);
+ if (ret)
+ return ret;
+ qcom->ss_phy_irq[i] = irq;
}
- qcom->ss_phy_irq = irq;
}

return 0;
--
2.40.0

From fa7ee24f3eb109e15b207d8a9f29d4db4d95fb05 Mon Sep 17 00:00:00 2001
From: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Date: Fri, 21 Jul 2023 09:58:49 +0530
Subject: [PATCH 07/11] usb: dwc3: qcom; Enable wakeup for mulitport IRQ's

Enable wakeup for multiport IRQ's to be able to wakeup the DUT
from system suspend when in host mode.

Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
---
drivers/usb/dwc3/dwc3-qcom.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index ab8664ced255..31968620e6ff 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -408,23 +408,32 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)

static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
+ int i;
+
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[0]);
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq[i]);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq[0]);
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq[i]);
} else {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq[0]);
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq[0]);
+ for (i = 0; i < DWC3_MAX_PORTS; i++) {
+ dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq[i]);
+ dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq[i]);
+ }
}

- dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq[0]);
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq[i]);
}

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
{
+ int i;
+
dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0);

/*
@@ -437,20 +446,25 @@ 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[0],
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[i],
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[0],
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[i],
IRQ_TYPE_EDGE_FALLING);
} else {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[0],
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[i],
IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[0],
+ dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[i],
IRQ_TYPE_EDGE_RISING);
+ }
}

- dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0);
+ for (i = 0; i < DWC3_MAX_PORTS; i++)
+ dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[i], 0);
}

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
--
2.40.0

Next message: kernel test robot: "[tip:master] BUILD SUCCESS 8f4995b370a57e7ad92c0f66664d171b23234337"
Previous message: Valentin Schneider: "Re: [RFC PATCH v2 16/20] rcu: Make RCU dynticks counter size configurable"
In reply to: Johan Hovold: "Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport"
Next in thread: Johan Hovold: "Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]