Re: [PATCH v3 05/10] ARM: dts: qcom: msm8960: add RPM clock controller and fix USB clocks

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Device Tree binding schema violation: `qcom,ci-hdrc` defines 3 clocks, but the `ci-hdrc-usb2.yaml` schema restricts `clocks` and `clock-names` properties to a maximum of 2 items.

Pre-existing issues:
- [High] The APQ8064 device tree misses the RPM fabric clock dependency for its USB nodes (`usb1`, `usb3`, `usb4`), leading to broken USB functionality.
--

commit bf5021df5ce13a6b631bed12c18c025b975c7150
Author: Antony Kurniawan Soemardi <linux@smankusors.com>

ARM: dts: qcom: msm8960: add RPM clock controller and fix USB clocks

The RPM clock controller manages clocks shared between the application
processor and the RPM firmware, including fabric and bus clocks required
by several peripherals.

With the RPM clock controller now available in the device tree, the USB
controller must explicitly declare its dependency on
RPM_DAYTONA_FABRIC_CLK. Without this declaration, the clock framework
would consider it unused and disable it, breaking USB functionality.

This also corrects the previous misuse of USB_HS1_XCVR_CLK as the core
clock. The XCVR clock is in fact used for PHY/reset handling rather than
as the main core clock.

> A similar issue has been observed on APQ8064, where missing the RPM
> fabric clock dependency leads to broken USB.

[Severity: High]
This is a pre-existing issue, but since the commit message notes that
APQ8064 is broken due to the missing RPM fabric clock dependency, should we
fix the usb1, usb3, and usb4 nodes in arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
as well?

For example, in qcom-apq8064.dtsi the usb1 node currently looks like this:

usb1: usb@12500000 {
    ...
    clocks = <&gcc USB_HS1_XCVR_CLK>, <&gcc USB_HS1_H_CLK>;
    clock-names = "core", "iface";
}

Since rpmcc is already enabled for APQ8064, the clock framework will disable
the RPM fabric clock as unused, rendering the APQ8064 USB controller fully
inoperable. Is it worth including a fix for qcom-apq8064.dtsi in this patch
series to restore that functionality?

> diff --git a/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
> index fd28401cebb5e..1ae399c5a81f0 100644
> --- a/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi
> +++ b/arch/arm/boot/dts/qcom/qcom-msm8960.dtsi

[ ... ]

> @@ -507,8 +519,12 @@ usb1: usb@12500000 {
>  			reg = <0x12500000 0x200>,
>  			      <0x12500200 0x200>;
>  			interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks = <&gcc USB_HS1_XCVR_CLK>, <&gcc USB_HS1_H_CLK>;
> -			clock-names = "core", "iface";
> +			clocks = <&gcc USB_HS1_H_CLK>,
> +				 <&rpmcc RPM_DAYTONA_FABRIC_CLK>,
> +				 <&gcc USB_HS1_XCVR_CLK>;
> +			clock-names = "iface",
> +				      "core",
> +				      "fs";

[Severity: Low]
Does adding a third clock here violate the device tree binding schema?

Looking at Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml, the clocks
and clock-names properties are restricted to a maximum of 2 items:

  clocks:
    minItems: 1
    maxItems: 2

  clock-names:
    minItems: 1
    maxItems: 2

Will this cause dtbs_check validation failures? Even if the ci_hdrc_msm
driver handles the optional fs clock correctly at runtime using
devm_clk_get_optional(), should the schema be updated to allow up to 3
items before adding them to the device tree?

>  			assigned-clocks = <&gcc USB_HS1_XCVR_CLK>;
>  			assigned-clock-rates = <60000000>;
>  			resets = <&gcc USB_HS1_RESET>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-msm8960-wifi-v3-0-fec6ac8dba02@smankusors.com?part=5