Re: [PATCH v7 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 3 potential issue(s) to consider:
- [Medium] Copy-paste error in QUP0 SE1 pinmux definitions causes pin conflicts with QUP0 SE0.
- [Medium] Redundant and conflicting `required-opps` property on the `ufs_mem_hc` device node.
- [Low] Invalid OPP node names `opp2` and `opp3` in `rpmhpd_opp_table`.
--

--- Patch [3]: [PATCH v7 3/6] arm64: dts: qcom: Add device tree for Nord Embedded variant ---
commit 117f55e166223f765ed4a271d49a9e24d8a3d432
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..032d384f6eb3e
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/nord-embedded.dtsi

[ ... ]

> +				rpmhpd_opp_min_svs: opp-1 {
> +					opp-level = <RPMH_REGULATOR_LEVEL_MIN_SVS>;
> +				};
> +
> +				rpmhpd_opp_low_svs: opp2 {
> +					opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
> +				};
> +
> +				rpmhpd_opp_svs: opp3 {
> +					opp-level = <RPMH_REGULATOR_LEVEL_SVS>;
> +				};
> +
> +				rpmhpd_opp_svs_l1: opp-4 {
> +					opp-level = <RPMH_REGULATOR_LEVEL_SVS_L1>;
> +				};

[Severity: Low]
This isn't a bug, but there's a missing hyphen in the node names for opp2
and opp3. Will this trigger a dtbs_check warning against the
operating-points-v2.yaml binding schema, which expects node names to match
the standard ^opp(-[0-9]+)?$ regex?

[ ... ]

> +&tlmm {
> +	qup_i2c0_default: qup-i2c0-default-state {
> +		pins = "gpio111", "gpio112";
> +		function = "qup0_se0";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};
> +
> +	qup_i2c1_default: qup-i2c1-default-state {
> +		pins = "gpio111", "gpio112";
> +		function = "qup0_se1";
> +		drive-strength = <2>;
> +		bias-pull-up;
> +	};

[Severity: Medium]
Is this a copy-paste error? This assigns pins gpio111 and gpio112 to
qup_i2c1_default, which are the exact same pins assigned to qup_i2c0_default
just above it. 

Could this cause a pinmux conflict between QUP0 SE0 and QUP0 SE1, breaking
hardware functionality for peripherals connected to SE1 if both are enabled?

[ ... ]

> +	qup_uart0_default: qup-uart0-default-state {
> +		pins = "gpio109", "gpio110";
> +		function = "qup0_se0";
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
> +	qup_uart1_default: qup-uart1-default-state {
> +		pins = "gpio109", "gpio110";
> +		function = "qup0_se1";
> +		drive-strength = <2>;
> +		bias-disable;
> +	};

[Severity: Medium]
Similarly here, qup_uart1_default is using pins gpio109 and gpio110,
identical to the assignments for qup_uart0_default. Should this be assigned
to a different set of pins to avoid a QUP0 SE0 / SE1 conflict?

[ ... ]

> +&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>;
> +	qcom,ice = <&ice>;
> +	#reset-cells = <1>;

[Severity: Medium]
Does setting required-opps statically on the ufs_mem_hc node conflict with
the dynamic performance state votes requested by the ufs_opp_table?

The ufs_opp_table defines operating points that scale down to
rpmhpd_opp_low_svs, but a static device-level required-opps vote
establishes a permanent minimum floor at NOM upon attach.

Will this lock the power domain at the NOM level and prevent the dynamic
power scaling from working as intended?

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