[PATCH] arm64: dts: qcom: glymur: Add missing USB clock, power and bandwidth votes

[PATCH] arm64: dts: qcom: glymur: Add missing USB clock, power and bandwidth votes

[Greg Ociepka]

The four USB3 controller nodes on Glymur (usb_0, usb_1, usb_2, usb_mp)
are missing three property groups that Glymur's own usb_hs node at
a200000 already carries, and that Hamoa (x1e80100) carries on all five
of its USB controllers:

  - assigned-clocks / assigned-clock-rates
  - required-opps
  - interconnects / interconnect-names

Without required-opps the controllers place no performance state vote
on their power domain. Measured on an Asus Zenbook A16 (UX3607OA) with
a SuperSpeed device enumerated, CX aggregates to SVS while the
controllers run their master clock at 200 MHz:

  $ grep -E "^cx " /sys/kernel/debug/pm_genpd/pm_genpd_summary
  cx    on    128

  $ grep "usb30.*master_clk " /sys/kernel/debug/clk/clk_summary
  gcc_usb30_sec_master_clk   ...  200000000
  gcc_usb30_prim_master_clk  ...  200000000
  gcc_usb30_mp_master_clk    ...  200000000

128 is RPMH_REGULATOR_LEVEL_SVS; rpmhpd_opp_nom is 256. With this
patch applied the same measurement reads 256 while a controller is
active.

Without interconnects, dwc3_qcom_interconnect_init() silently does
nothing: of_icc_get() returns NULL rather than an error pointer for a
device without the property, so probe succeeds and icc_set_bw() on a
NULL path is a no-op. No USB-DDR bandwidth is ever requested.

Without assigned-clock-rates nothing programs the master clock RCG,
which keeps whatever the bootloader left behind. On this machine that
happens to be the correct 200 MHz, but ftbl_gcc_usb30_mp_master_clk_src
also allows 66.67 and 133.33 MHz, neither of which sustains SuperSpeed.

Add all three groups to the four nodes, mirroring usb_hs. The
interconnect paths follow the provider layout in
drivers/interconnect/qcom/glymur.c: usb_0/usb_1 sit on aggre4_noc,
usb_mp on aggre3_noc and usb_2 on aggre2_noc.

Fixes: 4eee57dd4df9 ("arm64: dts: qcom: glymur: Add USB related nodes")
Assisted-by: Claude:fable-5
Signed-off-by: Greg Ociepka <greg@ferrisoft.com>
---
Note for reviewers: Hamoa additionally marks all five USB controllers
dma-coherent, while Glymur marks no peripheral DMA master as coherent
(only the SMMUs, PCIe controllers and remoteprocs). That difference is
deliberately not touched here since getting it wrong is a silent
data-corruption class of bug, but it looks like part of the same
omission. Is USB DMA coherent on Glymur?

--- a/arch/arm64/boot/dts/qcom/glymur.dtsi
+++ b/arch/arm64/boot/dts/qcom/glymur.dtsi
@@ -4572,6 +4572,10 @@
 				      "noc_aggr_north",
 				      "noc_aggr_south";
 
+			assigned-clocks = <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_PRIM_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
 			interrupts-extended = <&intc GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>,
 					      <&intc GIC_SPI 371 IRQ_TYPE_LEVEL_HIGH>,
 					      <&pdc 90 IRQ_TYPE_EDGE_BOTH>,
@@ -4584,9 +4588,18 @@
 					  "ss_phy_irq";
 
 			power-domains = <&gcc GCC_USB30_PRIM_GDSC>;
+			required-opps = <&rpmhpd_opp_nom>;
 			resets = <&gcc GCC_USB30_PRIM_BCR>;
 
 			iommus = <&apps_smmu 0x1420 0x0>;
+
+			interconnects = <&aggre4_noc MASTER_USB3_0 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&hsc_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					 &config_noc SLAVE_USB3_0 QCOM_ICC_TAG_ACTIVE_ONLY>;
+			interconnect-names = "usb-ddr",
+					     "apps-usb";
+
 			phys = <&usb_0_hsphy>,
 			       <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
 			phy-names = "usb2-phy",
@@ -4647,6 +4660,10 @@
 				      "noc_aggr_north",
 				      "noc_aggr_south";
 
+			assigned-clocks = <&gcc GCC_USB30_SEC_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_SEC_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
 			interrupts-extended = <&intc GIC_SPI 875 IRQ_TYPE_LEVEL_HIGH>,
 					      <&intc GIC_SPI 369 IRQ_TYPE_LEVEL_HIGH>,
 					      <&pdc 88 IRQ_TYPE_EDGE_BOTH>,
@@ -4660,9 +4677,17 @@
 
 			resets = <&gcc GCC_USB30_SEC_BCR>;
 			power-domains = <&gcc GCC_USB30_SEC_GDSC>;
+			required-opps = <&rpmhpd_opp_nom>;
 
 			iommus = <&apps_smmu 0x1460 0x0>;
 
+			interconnects = <&aggre4_noc MASTER_USB3_1 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&hsc_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					 &config_noc SLAVE_USB3_1 QCOM_ICC_TAG_ACTIVE_ONLY>;
+			interconnect-names = "usb-ddr",
+					     "apps-usb";
+
 			phys = <&usb_1_hsphy>,
 			       <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
 			phy-names = "usb2-phy",
@@ -4723,6 +4748,10 @@
 				      "noc_aggr_north",
 				      "noc_aggr_south";
 
+			assigned-clocks = <&gcc GCC_USB30_TERT_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_TERT_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
 			interrupts-extended = <&intc GIC_SPI 871 IRQ_TYPE_LEVEL_HIGH>,
 					      <&intc GIC_SPI 370 IRQ_TYPE_LEVEL_HIGH>,
 					      <&pdc 89 IRQ_TYPE_EDGE_BOTH>,
@@ -4736,9 +4765,17 @@
 
 			resets = <&gcc GCC_USB30_TERT_BCR>;
 			power-domains = <&gcc GCC_USB30_TERT_GDSC>;
+			required-opps = <&rpmhpd_opp_nom>;
 
 			iommus = <&apps_smmu 0x420 0x0>;
 
+			interconnects = <&aggre2_noc MASTER_USB3_2 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&hsc_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					 &config_noc SLAVE_USB3_2 QCOM_ICC_TAG_ACTIVE_ONLY>;
+			interconnect-names = "usb-ddr",
+					     "apps-usb";
+
 			phys = <&usb_2_hsphy>,
 			       <&usb_2_qmpphy QMP_USB43DP_USB3_PHY>;
 			phy-names = "usb2-phy",
@@ -4869,6 +4906,10 @@
 				      "noc_aggr_north",
 				      "noc_aggr_south";
 
+			assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_MP_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
 			interrupts-extended = <&intc GIC_SPI 132 IRQ_TYPE_LEVEL_HIGH>,
 					      <&intc GIC_SPI 345 IRQ_TYPE_LEVEL_HIGH>,
 					      <&intc GIC_SPI 346 IRQ_TYPE_LEVEL_HIGH>,
@@ -4894,9 +4935,17 @@
 
 			resets = <&gcc GCC_USB30_MP_BCR>;
 			power-domains = <&gcc GCC_USB30_MP_GDSC>;
+			required-opps = <&rpmhpd_opp_nom>;
 
 			iommus = <&apps_smmu 0xda0 0x0>;
 
+			interconnects = <&aggre3_noc MASTER_USB3_MP QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&hsc_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
+					 &config_noc SLAVE_USB3_MP QCOM_ICC_TAG_ACTIVE_ONLY>;
+			interconnect-names = "usb-ddr",
+					     "apps-usb";
+
 			phys = <&usb_mp_hsphy0>,
 			       <&usb_mp_qmpphy0>,
 			       <&usb_mp_hsphy1>,

Re: [PATCH] arm64: dts: qcom: glymur: Add missing USB clock, power and bandwidth votes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Early return in `dwc3_qcom_remove()` on `pm_runtime_resume_and_get()` failure bypasses device teardown, leaking interconnect paths and causing a Use-After-Free.
--

--- Patch [1]: [PATCH] arm64: dts: qcom: glymur: Add missing USB clock, power and bandwidth votes ---
commit e8a88eddb13f0c6c7f86603fb6358155107b782d
Author: Greg Ociepka <greg@ferrisoft.com>

arm64: dts: qcom: glymur: Add missing USB clock, power and bandwidth votes
    
This patch adds missing assigned-clocks, required-opps, and interconnects
properties to the four USB3 controller nodes on Glymur. These changes ensure
the controllers properly vote for performance state, configure the master
clock, and request USB-DDR bandwidth.

> diff --git a/arch/arm64/boot/dts/qcom/glymur.dtsi b/arch/arm64/boot/dts/qcom/glymur.dtsi
> index f04058d2e0891..fbbc82550c442 100644
> --- a/arch/arm64/boot/dts/qcom/glymur.dtsi
> +++ b/arch/arm64/boot/dts/qcom/glymur.dtsi
> @@ -4584,9 +4588,18 @@ usb_0: usb@a600000 {
>  					  "ss_phy_irq";
>  
>  			power-domains = <&gcc GCC_USB30_PRIM_GDSC>;
> +			required-opps = <&rpmhpd_opp_nom>;
>  			resets = <&gcc GCC_USB30_PRIM_BCR>;
>  
>  			iommus = <&apps_smmu 0x1420 0x0>;
> +
> +			interconnects = <&aggre4_noc MASTER_USB3_0 QCOM_ICC_TAG_ALWAYS
> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +					<&hsc_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
> +					 &config_noc SLAVE_USB3_0 QCOM_ICC_TAG_ACTIVE_ONLY>;

[Severity: High]
This isn't a bug introduced by this patch, but adding these interconnect 
properties exposes a resource leak and use-after-free in the driver's remove 
path during unbind.

In drivers/usb/dwc3/dwc3-qcom.c:dwc3_qcom_remove(), if
pm_runtime_resume_and_get() fails, the function returns early:

	if (pm_runtime_resume_and_get(qcom->dev) < 0)
		return;

	dwc3_core_remove(&qcom->dwc);
	clk_bulk_disable_unprepare(qcom->num_clocks, qcom->clks);
	dwc3_qcom_interconnect_exit(qcom);

This early return skips dwc3_core_remove() and dwc3_qcom_interconnect_exit().

Because the qcom structure is allocated with devm_kzalloc(), it is freed 
immediately after remove() completes, leaving the active dwc3 core operating 
on freed memory. It also leaks the newly populated interconnect paths that 
this patch enables.

Could the driver's error handling in dwc3_qcom_remove() be updated to ensure 
cleanup happens even if runtime resume fails?

> +			interconnect-names = "usb-ddr",
> +					     "apps-usb";
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812083522.774711-1-greg@ferrisoft.com?part=1

Re: [PATCH] arm64: dts: qcom: glymur: Add missing USB clock, power and bandwidth votes

[Abel Vesa]

On 26-08-12 10:35:22, Greg Ociepka wrote:
> The four USB3 controller nodes on Glymur (usb_0, usb_1, usb_2, usb_mp)
> are missing three property groups that Glymur's own usb_hs node at
> a200000 already carries, and that Hamoa (x1e80100) carries on all five
> of its USB controllers:
> 
>   - assigned-clocks / assigned-clock-rates
>   - required-opps
>   - interconnects / interconnect-names
> 
> Without required-opps the controllers place no performance state vote
> on their power domain. Measured on an Asus Zenbook A16 (UX3607OA) with
> a SuperSpeed device enumerated, CX aggregates to SVS while the
> controllers run their master clock at 200 MHz:
> 
>   $ grep -E "^cx " /sys/kernel/debug/pm_genpd/pm_genpd_summary
>   cx    on    128
> 
>   $ grep "usb30.*master_clk " /sys/kernel/debug/clk/clk_summary
>   gcc_usb30_sec_master_clk   ...  200000000
>   gcc_usb30_prim_master_clk  ...  200000000
>   gcc_usb30_mp_master_clk    ...  200000000
> 
> 128 is RPMH_REGULATOR_LEVEL_SVS; rpmhpd_opp_nom is 256. With this
> patch applied the same measurement reads 256 while a controller is
> active.
> 
> Without interconnects, dwc3_qcom_interconnect_init() silently does
> nothing: of_icc_get() returns NULL rather than an error pointer for a
> device without the property, so probe succeeds and icc_set_bw() on a
> NULL path is a no-op. No USB-DDR bandwidth is ever requested.
> 
> Without assigned-clock-rates nothing programs the master clock RCG,
> which keeps whatever the bootloader left behind. On this machine that
> happens to be the correct 200 MHz, but ftbl_gcc_usb30_mp_master_clk_src
> also allows 66.67 and 133.33 MHz, neither of which sustains SuperSpeed.
> 
> Add all three groups to the four nodes, mirroring usb_hs. The
> interconnect paths follow the provider layout in
> drivers/interconnect/qcom/glymur.c: usb_0/usb_1 sit on aggre4_noc,
> usb_mp on aggre3_noc and usb_2 on aggre2_noc.
> 
> Fixes: 4eee57dd4df9 ("arm64: dts: qcom: glymur: Add USB related nodes")
> Assisted-by: Claude:fable-5
> Signed-off-by: Greg Ociepka <greg@ferrisoft.com>

This looks OK to me.

Reviewed-by: Abel Vesa <abel.vesa@oss.qualcomm.com>

> ---
> Note for reviewers: Hamoa additionally marks all five USB controllers
> dma-coherent, while Glymur marks no peripheral DMA master as coherent
> (only the SMMUs, PCIe controllers and remoteprocs). That difference is
> deliberately not touched here since getting it wrong is a silent
> data-corruption class of bug, but it looks like part of the same
> omission. Is USB DMA coherent on Glymur?

I think so, but lets see what Wesley thinks.

+ Wesley