Re: [PATCH RFC 21/23] arm64: dts: qcom: add SM8650 LPASS CPU dtsi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] arm64: dts: qcom: unsupported power-domains on WSA macros in
  sm8650-lpass-cpu
- [Medium] arm64: dts: qcom: unattached multiple power-domains on
  lpass_tlmm
- [Medium] arm64: dts: qcom: lpass_core clock count mismatches
  lpasscorecc-sm8650 driver
- [Low] arm64: dts: qcom: node and property ordering in
  sm8650-lpass-cpu.dtsi
--

commit 1f3d7332ea1210e666215d104062d22d42ebae2c
Author: Neil Armstrong <neil.armstrong@linaro.org>

arm64: dts: qcom: add SM8650 LPASS CPU dtsi

This commit adds sm8650-lpass-cpu.dtsi with the LPASS clock controllers,
lpass-cpu node, LPASS macro clocks, and I2S2 pinctrl state for SM8650
boards booting at EL2 without ADSP audio firmware.

> diff --git a/arch/arm64/boot/dts/qcom/sm8650-lpass-cpu.dtsi b/arch/arm64/boot/dts/qcom/sm8650-lpass-cpu.dtsi
> new file mode 100644
> index 0000000000000..43d0d8daa4fe2
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sm8650-lpass-cpu.dtsi

[ ... ]

> +&lpass_tlmm {
> +	clocks = <&lpass_core LPASS_CORE_CC_CORE_CLK>;
> +	clock-names = "core";
> +
> +	power-domains = <&lpass_hm LPASS_CORE_HM_GDSC>,
> +			<&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
> +	power-domain-names = "core",
> +			     "audio";
> +
> +	status = "okay";
> +};

[Severity: Medium]
Does specifying two entries in power-domains leave both GDSCs unattached on
lpass_tlmm?

genpd_dev_pm_attach() returns early without attaching any domain when a node
lists more than one power domain, and lpi_pinctrl_probe() only sets up PM
clocks without calling devm_pm_domain_attach_list() or
dev_pm_domain_attach_by_name().  As a result, runtime PM resume of
lpass_tlmm will not power on LPASS_AON_CC_LPASS_AUDIO_HM_GDSC before
accessing LPI TLMM registers.

[ ... ]

> +&lpass_wsa2macro {
> +	clocks = <&lpass_audiocc LPASS_AUDIO_CC_WSA2_MCLK_CLK>,
> +		 <&lpass_audiocc LPASS_AUDIO_CC_TX_MCLK_WSA2_CLK>,
> +		 <&lpass_vamacro>;
> +	clock-names = "mclk",
> +		      "macro",
> +		      "fsgen";
> +
> +	power-domains = <&lpass_hm LPASS_CORE_HM_GDSC>,
> +			<&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
> +	power-domain-names = "macro",
> +			     "dcodec";
> +};
> +
> +&lpass_wsamacro {
> +	clocks = <&lpass_audiocc LPASS_AUDIO_CC_WSA_MCLK_CLK>,
> +		 <&lpass_audiocc LPASS_AUDIO_CC_TX_MCLK_WSA_CLK>,
> +		 <&lpass_vamacro>;
> +	clock-names = "mclk",
> +		      "macro",
> +		      "fsgen";
> +
> +	power-domains = <&lpass_hm LPASS_CORE_HM_GDSC>,
> +			<&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
> +	power-domain-names = "macro",
> +			     "dcodec";
> +};

[Severity: Medium]
Are power-domains and power-domain-names supported on lpass_wsa2macro and
lpass_wsamacro?

Documentation/devicetree/bindings/sound/qcom,lpass-wsa-macro.yaml does not
define power-domains or power-domain-names and sets unevaluatedProperties:
false, so dtbs_check fails when sm8650-lpass-cpu.dtsi is included.  In
addition, unlike the rx, tx, and va macro drivers, wsa_macro_probe() never
calls lpass_macro_pds_init(), while genpd_dev_pm_attach() skips automatic
attachment when more than one domain is listed, leaving both GDSCs
unattached during register access.

> +
> +&soc {
> +	lpass_audiocc: clock-controller@6bc0000 {
> +		reg = <0 0x06bc0000 0 0x38000>;
> +		compatible = "qcom,sm8650-lpassaudiocc";
> +
> +		clocks = <&bi_tcxo_div2>,
> +			 <&lpass_aon LPASS_AON_CC_MAIN_RCG_CLK_SRC>;
> +		clock-names = "bi_tcxo",
> +			      "iface";
> +
> +		power-domains = <&lpass_aon LPASS_AON_CC_LPASS_AUDIO_HM_GDSC>;
> +
> +		#clock-cells = <1>;
> +	};
> +
> +	lpass_aon: clock-controller@6e00000 {
> +		reg = <0 0x06e00000 0 0x30000>;
> +		compatible = "qcom,sm8650-lpassaoncc";
> +
> +		clocks = <&bi_tcxo_div2>,
> +			 <&lpass_core LPASS_CORE_CC_CORE_CLK>;
> +		clock-names = "bi_tcxo",
> +			      "iface";
> +
> +		#clock-cells = <1>;
> +		#power-domain-cells = <1>;
> +	};
> +
> +	lpass_core: clock-controller@7b00000 {
> +		compatible = "qcom,sm8650-lpasscorecc";
> +		reg = <0 0x07b00000 0 0x58000>;
> +
> +		clocks = <&bi_tcxo_div2>;
> +		clock-names = "bi_tcxo";
> +
> +		power-domains = <&lpass_hm LPASS_CORE_HM_GDSC>;
> +
> +		#clock-cells = <1>;
> +	};

[Severity: Medium]
Does providing only a single clock at index 0 on lpass_core leave the
P_BI_TCXO parent of lpass_core_cc_core_clk_src unresolved?

In drivers/clk/qcom/lpasscorecc-sm8650.c, lpass_core_cc_parent_data_ao_2
looks up DT clock index 1 for P_BI_TCXO:

drivers/clk/qcom/lpasscorecc-sm8650.c:
static const struct clk_parent_data lpass_core_cc_parent_data_ao_2[] = {
	{ .index = 1 },
	{ .hw = &lpass_core_cc_dig_pll.clkr.hw },
	{ .hw = &lpass_core_cc_dig_pll_out_main_div_clk_src.clkr.hw },
};

Because lpass_core only provides index 0, of_parse_clkspec() fails with
-ENOENT for index 1, leaving lpass_core_cc_core_clk_src and its child
branch clocks (such as LPASS_CORE_CC_CORE_CLK and
LPASS_CORE_CC_SYSNOC_MPORT_CORE_CLK) without their safe XO parent when
clk_rcg2_shared_init() parks the RCG.

> +
> +	lpass_hm: clock-controller@7e00000 {
> +		compatible = "qcom,sm8650-lpasshm";
> +		reg = <0 0x07e00000 0 0xa000>;
> +
> +		clocks = <&bi_tcxo_div2>;
> +		clock-names = "bi_tcxo";
> +
> +		power-domains = <&rpmhpd RPMHPD_LCX>;
> +
> +		required-opps = <&rpmhpd_opp_nom>;
> +
> +		#clock-cells = <1>;
> +		#power-domain-cells = <1>;
> +	};
> +
> +	lpass_cpu: audio@7b87000 {
> +		compatible = "qcom,sm8650-lpass-cpu";

[Severity: Low]
This isn't a bug, but audio@7b87000 is placed after
clock-controller@7e00000 instead of in unit-address order, and compatible
is placed after reg in clock-controller@6bc0000 and
clock-controller@6e00000 above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=21