Re: [PATCH v5 3/6] ARM: dts: qcom: apq8064: Fix USB controller clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mapping the transceiver clock to 'fs' causes it to be unconditionally disabled after probe, starving the USB controller.
- [Medium] The patch violates the generic ChipIdea USB2 device tree binding by defining three clocks for the 'qcom,ci-hdrc' compatible.
--

--- Patch [3]: [PATCH v5 3/6] ARM: dts: qcom: apq8064: Fix USB controller clocks ---
commit 3215e94f01093ac3a7be7c7e9537c0ae784e33fa
Author: Alexandre MINETTE <contact@alex-min.fr>
ARM: dts: qcom: apq8064: Fix USB controller clocks

This patch modifies the clock assignments for the APQ8064 HS USB controllers.
It maps the RPM Daytona fabric clock to "core", the AHB clock to "iface", and
the transceiver clock to "fs" to ensure the fabric clock is properly enabled
during controller access.

Link: https://lore.kernel.org/all/20260516-qcom-ci-hdrc-clock-fix-v2-1-aaec8d33d0aa@smankusors.com/

> diff --git a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> --- a/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> +++ b/arch/arm/boot/dts/qcom/qcom-apq8064.dtsi
> @@ -813,8 +813,10 @@ 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: Medium]
Does this change violate the generic ChipIdea USB2 device tree binding?

Looking at the schema in Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml,
it restricts the qcom,ci-hdrc compatible to a maximum of 2 clocks:

Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml:
  clocks:
    minItems: 1
    maxItems: 2

Adding a third clock ("fs") without first updating the binding schema might
cause an ABI validation failure during dtbs_check.

[Severity: High]
Will mapping the transceiver clock to "fs" cause it to be disabled during
probe, starving the USB controller?

In drivers/usb/chipidea/ci_hdrc_msm.c:ci_hdrc_msm_probe(), the driver appears
to treat the "fs" clock as a temporary reset clock, turning it off immediately
after the reset sequence:

drivers/usb/chipidea/ci_hdrc_msm.c:ci_hdrc_msm_probe() {
    ...
    reset_control_deassert(reset);

    clk_disable_unprepare(ci->fs_clk);

    ret = clk_prepare_enable(ci->core_clk);
    ...
}

Since there appear to be no PM runtime callbacks to re-enable it, mapping the
USB_HS1_XCVR_CLK transceiver clock to "fs" seems to mean the 60MHz ULPI clock
will remain disabled unconditionally after probe.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-mainline-send-v1-sending-v5-0-149dd4372ac8@alex-min.fr?part=3