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

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 unconditiona=
lly 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 c=
locks ---
commit 3215e94f01093ac3a7be7c7e9537c0ae784e33fa
Author: Alexandre MINETTE <[email protected]>
ARM: dts: qcom: apq8064: Fix USB controller clocks

This patch modifies the clock assignments for the APQ8064 HS USB controller=
s.
It maps the RPM Daytona fabric clock to "core", the AHB clock to "iface", a=
nd
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-aaec=
[email protected]/

> 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 =3D <0x12500000 0x200>,
>  			      <0x12500200 0x200>;
>  			interrupts =3D <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
> -			clocks =3D <&gcc USB_HS1_XCVR_CLK>, <&gcc USB_HS1_H_CLK>;
> -			clock-names =3D "core", "iface";
> +			clocks =3D <&gcc USB_HS1_H_CLK>,
> +				 <&rpmcc RPM_DAYTONA_FABRIC_CLK>,
> +				 <&gcc USB_HS1_XCVR_CLK>;
> +			clock-names =3D "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 appea=
rs
to treat the "fs" clock as a temporary reset clock, turning it off immediat=
ely
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 =3D clk_prepare_enable(ci->core_clk);
    ...
}

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

[ ... ]

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-mainline-s=
[email protected]?part=3D3