Re: [PATCH v8 09/11] arm64: dts: qcom: ipq5210: Add I2C nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] arm64: dts: qcom: ipq5210: unconfigured 32 MHz SE clock in
  geni_i2c_probe()
--

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

arm64: dts: qcom: ipq5210: Add I2C nodes

This commit adds the QUPv3 I2C controller nodes for IPQ5210 and enables
I2C0 on RDP504.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5210.dtsi b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> index e4a696a65e550..35cd558819691 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> @@ -160,6 +160,14 @@ opp-300000000 {
>  		};
>  	};
>  
> +	i2c_opp_table_32mhz: opp-table-qup32mhz {
> +		compatible = "operating-points-v2";
> +
> +		opp-32000000 {
> +			opp-hz = /bits/ 64 <32000000>;
> +		};
> +	};
> +
>  	pmu {
>  		compatible = "arm,cortex-a53-pmu";
>  		interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_HIGH)>;
> @@ -435,6 +443,45 @@ uart1: serial@1a84000 {
>  
>  				status = "disabled";
>  			};
> +
> +			i2c0: i2c@1a88000 {
> +				compatible = "qcom,geni-i2c";
> +				reg = <0x0 0x01a88000 0x0 0x4000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&gcc GCC_QUPV3_WRAP_SE2_CLK>;
> +				clock-names = "se";
> +				clock-frequency = <400000>;
> +				interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> +				operating-points-v2 = <&i2c_opp_table_32mhz>;
> +				status = "disabled";
> +			};
> +
> +			i2c1: i2c@1a8c000 {
> +				compatible = "qcom,geni-i2c";
> +				reg = <0x0 0x01a8c000 0x0 0x4000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&gcc GCC_QUPV3_WRAP_SE3_CLK>;
> +				clock-names = "se";
> +				clock-frequency = <400000>;
> +				interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_HIGH>;
> +				operating-points-v2 = <&i2c_opp_table_32mhz>;
> +				status = "disabled";
> +			};
> +
> +			i2c2: i2c@1a94000 {
> +				compatible = "qcom,geni-i2c";
> +				reg = <0x0 0x01a94000 0x0 0x4000>;
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +				clocks = <&gcc GCC_QUPV3_WRAP_SE5_CLK>;
> +				clock-names = "se";
> +				clock-frequency = <400000>;
> +				interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>;
> +				operating-points-v2 = <&i2c_opp_table_32mhz>;
> +				status = "disabled";
> +			};
>  		};

[Severity: High]
Can i2c0, i2c1, and i2c2 fail to probe or lose their SE clock rate across
runtime suspend when using operating-points-v2 instead of assigned-clocks
and assigned-clock-rates?

geni_se_resources_init() registers the OPP table and sets se->has_opp,
which causes geni_se_resources_deactivate() to call
dev_pm_opp_set_rate(se->dev, 0) on runtime suspend, but the "qcom,geni-i2c"
driver never calls dev_pm_opp_set_rate() or clk_set_rate() during probe or
resume.   Instead, geni_i2c_clk_map_idx() only checks the existing clock
rate:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_clk_map_idx() {
	if (clk_get_rate(gi2c->se.clk) == GENI_SE_CLK_32MHZ) {
		req_freq = GENI_SE_CLK_32MHZ;
		itr = geni_i2c_clk_map_32mhz;
	} else {
		req_freq = GENI_SE_CLK_19P2MHZ;
		itr = geni_i2c_clk_map_19p2mhz;
	}

	ret = geni_se_clk_freq_match(&gi2c->se, req_freq, &gi2c->clk_idx,
				     &res_freq, true);

Because ftbl_gcc_qupv3_wrap_se0_clk_src in drivers/clk/qcom/gcc-ipq5210.c
has no 19.2 MHz entry, geni_i2c_probe() fails with -EINVAL unless the
bootloader already set the SE clock to 32 MHz, and runtime suspend clears
the clock rate to 0 without restoring it on resume.  Setting
assigned-clocks and assigned-clock-rates = <32000000> on each I2C node and
dropping operating-points-v2 avoids both issues.

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