Re: [PATCH v14 3/6] arm64: dts: qcom: Add device tree for Nord Embedded variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mismatch between `power-domains` and `required-opps` in `ufs_mem_hc`.
- [High] The UFS PHY clocks are not wired to the `negcc` clock controller, breaking the UFS clock hierarchy.
--

commit 7034c99626d2986e3ca10a97a1a9799d821d7a58
Author: Shawn Guo <shengchao.guo@oss.qualcomm.com>

arm64: dts: qcom: Add device tree for Nord Embedded variant
    
Unlike the GearVM variant, Nord Embedded variant has platform resources
(clocks, regulators, powerdomains, pins, etc.) directly controlled by
the operating system. Add a separate dtsi file extending the existing
top-level nord.dtsi with nodes representing these peripherals as well as
describing how they are wired up with the already defined components.

> diff --git a/arch/arm64/boot/dts/qcom/nord-embedded.dtsi b/arch/arm64/boot/dts/qcom/nord-embedded.dtsi
> new file mode 100644
> index 0000000000000..1a2f61a2c9522
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/nord-embedded.dtsi
> @@ -0,0 +1,1820 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */

[ ... ]

> +	negcc: clock-controller@8900000 {
> +		compatible = "qcom,nord-negcc";
> +		reg = <0x0 0x08900000 0x0 0xf4200>;
> +		clocks = <&bi_tcxo_div2>,
> +			 <&sleep_clk>,
> +			 <0>,
> +			 <0>,
> +			 <0>,
> +			 <0>,
> +			 <0>;

[Severity: High]
Does this missing parent clock configuration break the UFS clock hierarchy?

The UFS probe routine acquires its symbol clocks (like NE_GCC_UFS_PHY_TX_SYMBOL_0_CLK)
from negcc. With these dummy <0> entries, the symbol clocks lack a source from the 
UFS PHY (ufs_mem_phy), which will cause clk_get_rate() to return 0 and UFS 
initialization to fail.

Could these be updated to reference the UFS PHY clocks instead?

[ ... ]

> +&ufs_mem_hc {
> +	clocks = <&negcc NE_GCC_UFS_PHY_AXI_CLK>,
> +		 <&negcc NE_GCC_AGGRE_NOC_UFS_PHY_AXI_CLK>,
> +		 <&negcc NE_GCC_UFS_PHY_AHB_CLK>,
> +		 <&negcc NE_GCC_UFS_PHY_UNIPRO_CORE_CLK>,
> +		 <&tcsrcc TCSR_UFS_CLKREF_EN>,
> +		 <&negcc NE_GCC_UFS_PHY_TX_SYMBOL_0_CLK>,
> +		 <&negcc NE_GCC_UFS_PHY_RX_SYMBOL_0_CLK>,
> +		 <&negcc NE_GCC_UFS_PHY_RX_SYMBOL_1_CLK>;
> +	clock-names = "core_clk",
> +		      "bus_aggr_clk",
> +		      "iface_clk",
> +		      "core_clk_unipro",
> +		      "ref_clk",
> +		      "tx_lane0_sync_clk",
> +		      "rx_lane0_sync_clk",
> +		      "rx_lane1_sync_clk";
> +
> +	resets = <&negcc NE_GCC_UFS_PHY_BCR>;
> +	reset-names = "rst";
> +
> +	interconnects = <&aggre1_noc_tile MASTER_UFS_MEM QCOM_ICC_TAG_ALWAYS
> +			 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +			<&hscnoc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
> +			 &config_noc SLAVE_UFS_MEM_CFG QCOM_ICC_TAG_ACTIVE_ONLY>;
> +	interconnect-names = "ufs-ddr",
> +			     "cpu-ufs";
> +
> +	phys = <&ufs_mem_phy>;
> +	phy-names = "ufsphy";
> +
> +	power-domains = <&negcc NE_GCC_UFS_PHY_GDSC>;
> +	operating-points-v2 = <&ufs_opp_table>;
> +	required-opps = <&rpmhpd_opp_nom>;

[Severity: High]
Does this mix of a GDSC power domain and an RPMh performance state cause
frequency scaling failures during UFS probe?

The negcc clock controller that provides NE_GCC_UFS_PHY_GDSC does not appear to
declare a parent power domain (like rpmhpd). Because of this, when dev_pm_opp_set_opp()
is called, the OPP framework won't be able to translate or forward the rpmhpd_opp_nom
performance state, which will result in an error.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903140617.441111-1-shengchao.guo@oss.qualcomm.com?part=3