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

From: Krishna Kurapati PSSNV
Date: Sat Aug 12 2023 - 04:44:45 EST




On 8/11/2023 10:35 PM, Konrad Dybcio wrote:
+static int dwc3_get_port_irq(struct platform_device *pdev, u8 port_index)
+{
+    struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
+    bool is_mp_supported = (qcom->data->num_ports > 1) ? true : false;
+    const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
+    char *disp_name;
+    int acpi_index;
+    char *dt_name;
+    int ret;
+    int irq;
+    int i;
+
+    /*
+     * We need to read only DP/DM/SS IRQ's here.
+     * So loop over from 1->3 and accordingly modify respective phy_irq[].
+     */
+    for (i = 1; i < MAX_PHY_IRQ; i++) {
+
+        if (!is_mp_supported && (port_index == 0)) {
+            if (i == DP_HS_PHY_IRQ) {
+                dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                    "dp_hs_phy_irq");
+                disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                    "qcom_dwc3 DP_HS");
+            } else if (i == DM_HS_PHY_IRQ) {
+                dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                    "dm_hs_phy_irq");
+                disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                    "qcom_dwc3 DM_HS");
+            } else if (i == SS_PHY_IRQ) {
+                dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                    "ss_phy_irq");
+                disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
+                    "qcom_dwc3 SS");
Bjorn, Konrad,

If we are to remove this repetitive loops, we might need to make a 2D array for all of Dp/Dm/Ss interrutps and make a global array of names to be used for irq lookup and use them to reduce the if-else-if stuff here. If that is fine, I can make those changes, else I would like to stick to this approach for now because if we don't add the global array of names, prepping them seperately for dp/dm/ss would again lead us to making if-else loops like above.

Please let me know your thoughts on this.
Can we not just reuse the associated interrupt-names from the devicetree
if present?

Hi Konrad,

 Thanks for the comments but one more confirmation.
We can read the interrupts from DT but I believe the compatible would still need to stay. We need the num_ports information not just for registering interrupts but for modifying the pwr_event_irq registers during suspend/resume. If we rely on the interrupts to find the number of ports, the user is free to remove any IRQ and we might end up in a situation where glue and core are not having same view of how many number of ports present. So I believe its best to keep the compatible and get num_ports info from there and rely on reading interrupt-names to get interrupts cleanly. Can you let me know your view on the same.
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).

Regards,
Krishna,