Skip to contentSkip to conversation

Add support for IPQ9650 USB controller

PATCH Superseded Varadarajan Narayanan 3 participants 7 messages Aug 25 – Sep 7

Subsystems (4)
Device tree bindings USB Arm Qualcomm mailing list DesignWare USB3 DRD IP driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Varadarajan Narayanan

author
#0 lore raw

cover Add support for IPQ9650 USB controller

Add bindings and dwc3-qcom glue driver changes for adding support to the two USB controllers present in IPQ9650.

Signed-off-by Varadarajan Narayanan <[email protected]>

---
Varadarajan Narayanan (2):
dt-bindings: usb: qcom,snps-dwc3: Add IPQ9650 compatible
usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

 .../devicetree/bindings/usb/qcom,snps-dwc3.yaml    | 22 ++++++++++++++++++++++
 drivers/usb/dwc3/dwc3-qcom.c                       |  7 +++++++
 2 files changed, 29 insertions(+)

---
base-commit: 7c5f62582256b505a57975d1a8faa501c8d8878e
change-id: 20260820-ctrl-6015e1503b24

Best regards,

Signature · 1 line
Varadarajan Narayanan <[email protected]>
author
#1 lore raw

PATCH 1/2 dt-bindings: usb: qcom,snps-dwc3: Add IPQ9650 compatible

Add the qcom,ipq9650-dwc3 compatible for the two DWC3 USB controllers found on IPQ9650, using qcom,snps-dwc3 as the fallback compatible.

One controller is connected to a High-Speed PHY, while the other is connected to both High-Speed and SuperSpeed PHYs.

Signed-off-by Varadarajan Narayanan <[email protected]>
---
 .../devicetree/bindings/usb/qcom,snps-dwc3.yaml    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml +22 -0
index ea60f7220afe..603d509a6ba1 100644
@@ -36,6 +36,7 @@ properties:
           - qcom,ipq8064-dwc3
           - qcom,ipq8074-dwc3
           - qcom,ipq9574-dwc3
+          - qcom,ipq9650-dwc3
           - qcom,kaanapali-dwc3
           - qcom,maili-dwc3
           - qcom,milos-dwc3

@@ -387,6 +388,26 @@ allOf:
             - const: mock_utmi
             - const: xo
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq9650-dwc3
+    then:
+      properties:
+        clocks:
+          minItems: 6
+          maxItems: 6
+        clock-names:
+          items:
+            - const: cfg_noc
+            - const: core
+            - const: iface
+            - const: sleep
+            - const: mock_utmi
+            - const: ref_clk
+
   - if:
       properties:
         compatible:

@@ -506,6 +527,7 @@ allOf:
               - qcom,ipq5210-dwc3
               - qcom,ipq5424-dwc3
               - qcom,ipq9574-dwc3
+              - qcom,ipq9650-dwc3
     then:
       properties:
         interrupts:
Signature · 1 line
2.34.1
#2 lore raw

Re: [PATCH 1/2] dt-bindings: usb: qcom,snps-dwc3: Add IPQ9650 compatible

Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml allOf: · line 409

Quoted 13 lines · Varadarajan Narayanan
Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml +13 -0
+    then:
+      properties:
+        clocks:
+          minItems: 6
+          maxItems: 6
+        clock-names:
+          items:
+            - const: cfg_noc
+            - const: core
+            - const: iface
+            - const: sleep
+            - const: mock_utmi
+            - const: ref_clk

Just "ref" or "xo" (if it has purpose of XO), so this would fit the eliza block

Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml allOf: · line 531

Quoted 10 lines
Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml +2 -0
+
   - if:
       properties:
         compatible:

@@ -506,6 +527,7 @@ allOf:
               - qcom,ipq5210-dwc3
               - qcom,ipq5424-dwc3
               - qcom,ipq9574-dwc3
+              - qcom,ipq9650-dwc3
     then:

Best regards,
Krzysztof

author
#3 lore raw

PATCH 2/2 usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

In reply to Varadarajan Narayanan

The dwc3-qcom glue layer sets probe_data.ignore_clocks_and_resets before calling dwc3_core_probe(), so the DWC3 core does not acquire ref_clk on its own. This prevents dwc3_ref_clk_period() from programming GUCTL and GFLADJ from the reference clock rate.

The deprecated snps,ref-clock-period-ns property is not a suitable replacement on IPQ9650. Its 24 MHz reference clock has a period of about 41.66 ns, while using 41 ns makes the calculated FLADJ value become 0 and leaves the reference clock configuration incorrect.

Allow the Qualcomm glue driver to provide ref_clk to the DWC3 core for platforms that may need it.

Signed-off-by Varadarajan Narayanan <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/dwc3/dwc3-qcom.c +7 -0
index ac68b4218b56..2d6de6469c1e 100644
@@ -705,6 +705,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	probe_data.res = &res;
 	probe_data.ignore_clocks_and_resets = true;
 	probe_data.properties = DWC3_DEFAULT_PROPERTIES;
+
+	qcom->dwc.ref_clk = devm_clk_get_optional(dev, "ref_clk");
+	if (IS_ERR_OR_NULL(qcom->dwc.ref_clk)) {
+		qcom->dwc.ref_clk = NULL;
+		dev_info(dev, "NOT setting dwc.ref_clk\n");
+	}
+
 	ret = dwc3_core_probe(&probe_data);
 	if (ret)  {
 		ret = dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
Signature · 1 line
2.34.1
AI review
#4 lore raw

Re: [PATCH 2/2] usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Masking of legitimate errors (like -EPROBE_DEFER or -ENOMEM) from devm_clk_get_optional() by using…

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Masking of legitimate errors (like -EPROBE_DEFER or -ENOMEM) from devm_clk_get_optional() by using IS_ERR_OR_NULL().
- [High] Passing ref_clk to the DWC3 core causes a reference count leak that leaves the clock physically enabled during runtime suspend.
- [Low] Spammy dev_info() log on every boot for platforms without the optional clock.
--

--- Patch [2]: [PATCH 2/2] usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core --- commit 0c1a156683fd6c3e4e8f63adc608d4af8b20c95e
Author: Varadarajan Narayanan <[email protected]>

usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

The dwc3-qcom glue layer sets probe_data.ignore_clocks_and_resets before calling dwc3_core_probe(), preventing the DWC3 core from acquiring ref_clk. This patch allows the Qualcomm glue driver to provide ref_clk to the core for platforms that need it.

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_probe(struct platform_device *pdev) · line 709

Quoted 10 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
index ac68b4218b56b..2d6de6469c1ea 100644
@@ -705,6 +705,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	probe_data.res = &res;
 	probe_data.ignore_clocks_and_resets = true;
 	probe_data.properties = DWC3_DEFAULT_PROPERTIES;
+
+	qcom->dwc.ref_clk = devm_clk_get_optional(dev, "ref_clk");

[Severity: High]
Does this code leak a clock reference count during runtime suspend?

The glue driver gets and enables all clocks via devm_clk_bulk_get_all(), which includes ref_clk. By passing ref_clk to the DWC3 core here, dwc3_clk_enable() will increment the clock's enable count a second time.

During HOST runtime suspend, dwc3_suspend_common() intentionally skips disabling clocks:

drivers/usb/dwc3/core.c:dwc3_suspend_common() {
...

		if (!PMSG_IS_AUTO(msg) &&
		    (!device_may_wakeup(dwc->dev) || dwc->needs_full_reinit)) {
			dwc3_core_exit(dwc);
			break;
		}

...
}

Because of this, when the glue driver suspends and calls clk_bulk_disable_unprepare() in dwc3_qcom_suspend(), the enable count for ref_clk drops from 2 to 1 instead of reaching 0. Will this leave the clock physically powered on and cause a permanent power management regression?

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_probe(struct platform_device *pdev) · line 710

Quoted 1 line
drivers/usb/dwc3/dwc3-qcom.c +1 -0
+	if (IS_ERR_OR_NULL(qcom->dwc.ref_clk)) {

[Severity: High]
Does masking the error return from devm_clk_get_optional() with IS_ERR_OR_NULL discard legitimate errors?

If the clock provider is not yet ready, devm_clk_get_optional() will return -EPROBE_DEFER. Masking this error means the driver silently proceeds without the required clock, leading to incorrect hardware initialization instead of deferring the probe.

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_probe(struct platform_device *pdev) · line 712

Quoted 2 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
+		qcom->dwc.ref_clk = NULL;
+		dev_info(dev, "NOT setting dwc.ref_clk\n");

[Severity: Low]
Will this dev_info() cause unnecessary log spam on every boot?

When the optional clock is naturally absent from the device tree, devm_clk_get_optional() returns NULL. This logs unconditionally on platforms without the optional clock. Could this be removed or changed to dev_dbg to avoid dmesg spam?

Quoted 5 lines
drivers/usb/dwc3/dwc3-qcom.c +2 -0
+	}
+
 	ret = dwc3_core_probe(&probe_data);
 	if (ret)  {
 		ret = dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
#5 lore raw

Re: [PATCH 2/2] usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

In reply to Varadarajan Narayanan
Quoted 23 lines · Varadarajan Narayanan

The dwc3-qcom glue layer sets probe_data.ignore_clocks_and_resets before calling dwc3_core_probe(), so the DWC3 core does not acquire ref_clk on its own. This prevents dwc3_ref_clk_period() from programming GUCTL and GFLADJ from the reference clock rate.

The deprecated snps,ref-clock-period-ns property is not a suitable replacement on IPQ9650. Its 24 MHz reference clock has a period of about 41.66 ns, while using 41 ns makes the calculated FLADJ value become 0 and leaves the reference clock configuration incorrect.

Allow the Qualcomm glue driver to provide ref_clk to the DWC3 core for platforms that may need it.

Signed-off-by Varadarajan Narayanan <[email protected]>
---
 drivers/usb/dwc3/dwc3-qcom.c | 7 +++++++
 1 file changed, 7 insertions(+)
drivers/usb/dwc3/dwc3-qcom.c +0 -0
index ac68b4218b56..2d6de6469c1e 100644

drivers/usb/dwc3/dwc3-qcom.c static int dwc3_qcom_probe(struct platform_device *pdev) · line 712

Quoted 8 lines
drivers/usb/dwc3/dwc3-qcom.c +5 -0
@@ -705,6 +705,13 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	probe_data.res = &res;
 	probe_data.ignore_clocks_and_resets = true;
 	probe_data.properties = DWC3_DEFAULT_PROPERTIES;
+
+	qcom->dwc.ref_clk = devm_clk_get_optional(dev, "ref_clk");
+	if (IS_ERR_OR_NULL(qcom->dwc.ref_clk)) {
+		qcom->dwc.ref_clk = NULL;
+		dev_info(dev, "NOT setting dwc.ref_clk\n");

No need for a message. Why every device should receive it during probe?

Aren't existing other devices use name "ref"? Why are you coming with completely different name? SNPS binding also asks for ref.

Best regards,
Krzysztof

author
#6 lore raw
In reply to Varadarajan Narayanan
Quoted 8 lines · Varadarajan Narayanan

Add bindings and dwc3-qcom glue driver changes for adding support to the two USB controllers present in IPQ9650.

Signed-off-by Varadarajan Narayanan <[email protected]>

---
Varadarajan Narayanan (2):
dt-bindings: usb: qcom,snps-dwc3: Add IPQ9650 compatible
usb: dwc3: qcom: Allow the glue layer to pass ref_clk to the core

Abandoning this and will use [1].

Thanks
Varada

1 - https://lore.kernel.org/linux-arm-msm/[email protected]/