Re: [PATCH v8 06/11] arm64: dts: qcom: ipq5210: Add ICE support to SDHCI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] arm64: dts: qcom: ipq5210: missing qcom,ice phandle on sdhc
  node
- [High] arm64: dts: qcom: ipq5210: 300 MHz ICE OPP table attached to
  sdhc node
- [Low] arm64: dts: qcom: ipq5210: undocumented sdhc_ice compatible in
  DT schema
--

commit bf4c3fadb586445746ef6dd1251be30175cb3e65
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

arm64: dts: qcom: ipq5210: Add ICE support to SDHCI

This commit adds the SDHCI Inline Crypto Engine (ICE) node, a 300 MHz OPP
table, and CQE support for the IPQ5210 SDHCI controller, and enables the
ICE node on RDP504.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index 6dfc89903d2fc..705d7ed487138 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> @@ -151,6 +151,14 @@ opp-1416000000 {
>  		};
>  	};
>  
> +	sdhc_opp_table_300mhz: opp-table-300mhz {
> +		compatible = "operating-points-v2";
> +
> +		opp-300000000 {
> +			opp-hz = /bits/ 64 <300000000>;
> +		};
> +	};
> +
>  	pmu {
>  		compatible = "arm,cortex-a53-pmu";
>  		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> @@ -432,6 +440,21 @@ sdhc: mmc@7804000 {
>  				      "core",
>  				      "xo";
>  			non-removable;
> +			operating-points-v2 = <&sdhc_opp_table_300mhz>;

[Severity: High]
Does attaching sdhc_opp_table_300mhz to sdhc break SDHCI bus clock scaling on
GCC_SDCC1_APPS_CLK?

sdhci_msm_probe() binds the sdhc node's OPP table to its "core" clock
(GCC_SDCC1_APPS_CLK), which only supports frequencies from 144 kHz up to
200 MHz in ftbl_gcc_sdcc1_apps_clk_src, whereas 300 MHz is the rate for
GCC_SDCC1_ICE_CORE_CLK on sdhc_ice in ftbl_gcc_sdcc1_ice_core_clk_src.

With a single 300 MHz entry attached to sdhc, every dev_pm_opp_set_rate()
call in msm_set_clock_rate_for_bus_mode() rounds the requested bus rate
(including 400 kHz card initialization and 192 MHz operation) up to 300 MHz,
driving GCC_SDCC1_APPS_CLK at 200 MHz while leaving GCC_SDCC1_ICE_CORE_CLK on
sdhc_ice unconfigured.

> +			supports-cqe;
> +
> +			status = "disabled";
> +		};

[Severity: High]
Is sdhc missing a qcom,ice = <&sdhc_ice> property to link the controller to
the standalone ICE node?

Because sdhc has no "ice" register region, sdhci_msm_ice_init() ->
devm_of_qcom_ice_get() -> of_qcom_ice_get() looks up the "qcom,ice" phandle on
the sdhc node.  Without it, of_qcom_ice_get() returns -EOPNOTSUPP and
sdhci_msm_ice_init() disables inline encryption support, while sdhc_ice probes
on RDP504 and leaves GCC_SDCC1_ICE_CORE_CLK and GCC_SDCC1_AHB_CLK permanently
enabled with no consumer to suspend them.

> +
> +		sdhc_ice: crypto@7808000 {
> +			compatible = "qcom,ipq5210-inline-crypto-engine",
> +				     "qcom,inline-crypto-engine";
> +			reg = <0x0 0x07808000 0x0 0x2000>;
> +
> +			clocks = <&gcc GCC_SDCC1_ICE_CORE_CLK>,
> +				 <&gcc GCC_SDCC1_AHB_CLK>;
> +			clock-names = "core",
> +				      "iface";
>  
>  			status = "disabled";
>  		};

[Severity: Low]
Does "qcom,ipq5210-inline-crypto-engine" need to be documented in
Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml?

That compatible string is not in the binding's compatible enum, and the schema
also requires power-domains for compatibles not listed in its exemption list,
causing dtbs_check failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-08-dts-v8-0-d91401955039@oss.qualcomm.com?part=6