Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver
From: Krishna Kurapati PSSNV
Date: Sun Oct 22 2023 - 14:42:21 EST
On 10/20/2023 6:53 PM, Johan Hovold wrote:
First, drop "QCOM Glue driver" from Subject, you already have the "qcom"
prefix.
On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:
Refactor setup_irq call to facilitate reading multiport IRQ's along
"IRQs" or just "interrupts"
with non mulitport ones. Read through the interrupt-names property
"multiport"
Please spell check all your patches (commit messages and code) before
posting, it's not the reviewers job.
to figure out, the type of interrupt (DP/DM/HS/SS) and to which port
it belongs. Also keep track of port index to calculate port count
based on interrupts provided as input in DT.
Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
---
drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++----------
1 file changed, 154 insertions(+), 56 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index ef2006db7601..863892284146 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -53,14 +53,25 @@
#define APPS_USB_AVG_BW 0
#define APPS_USB_PEAK_BW MBps_to_icc(40)
+#define NUM_PHY_IRQ 4
+
+enum dwc3_qcom_ph_index {
"phy_index"
+ DP_HS_PHY_IRQ_INDEX = 0,
+ DM_HS_PHY_IRQ_INDEX,
+ SS_PHY_IRQ_INDEX,
+ HS_PHY_IRQ_INDEX,
+};
+
struct dwc3_acpi_pdata {
u32 qscratch_base_offset;
u32 qscratch_base_size;
u32 dwc3_core_base_size;
+ /*
+ * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
+ * IRQ's respectively.
+ */
+ 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 +84,12 @@ struct dwc3_qcom {
int num_clocks;
struct reset_control *resets;
+ /*
+ * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
+ * respectively.
+ */
+ int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
int hs_phy_irq;
- int dp_hs_phy_irq;
- int dm_hs_phy_irq;
- int ss_phy_irq;
I'm not sure using arrays like this is a good idea (and haven't you
switched the indexes above?).
Why not add a port structure instead?
struct dwc3_qcom_port {
int hs_phy_irq;
int dp_hs_phy_irq;
int dm_hs_phy_irq;
int ss_phy_irq;
};
and then have
struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
in dwc3_qcom. The port structure can the later also be amended with
whatever other additional per-port data there is need for.
This should make the implementation cleaner.
I also don't like the special handling of hs_phy_irq; if this is really
just another name for the pwr_event_irq then this should be cleaned up
before making the code more complicated than it needs to be.
Make sure to clarify this before posting a new revision.
hs_phy_irq is different from pwr_event_irq.
AFAIK, there is only one of this per controller.
-static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
- char *disp_name, int irq)
+static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
+ const char *disp_name, int irq)
Ok, here you did drop the second name parameter, but without renaming
the first and hidden in a long diff without any mention anywhere.
I didn't understand the comment. Can you please elaborate.
I didn't drop the second parameter. In the usage of this call, I passed
same value to both irq_name and disp_name:
"dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);"
I mentioned in cover-letter that I am using same name as obtained from
DT to register the interrupts as well. Should've mentioned that in
commit text of this patch. Will do it in next version.
+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;
+
No need for newlines after 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;
+ }
+
+ if (port_index > DWC3_MAX_PORTS)
+ port_index = -1;
+
+ return port_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");
of_property_count_strings() can return negative errnos and you don't
have any sanity checks for the return value...
Please slow down, and also make sure to get your patches reviewed
internally before posting new revisions.
It did go through internal review but probably went un-noticed. Will add
sanity checks here and below.
+ irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL);
devm_kcalloc()
+ 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);
No sanity check here either?
+ 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, "Invalid IRQ not handled");
+ continue;
+ }
I'll just stop reviewing here. This is a waste of my time.
Regards,
Krishna,