Re: [PATCH v9 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 0886413b485197e28c9ca095b2bedf736adf3f28
Author: Varadarajan Narayanan <varadarajan.narayanan@oss.qualcomm.com>

arm64: dts: qcom: ipq5210: Add I2C nodes

This commit adds the QUPv3 I2C controller nodes and 32 MHz OPP table 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 4cb9f399d4808..36f874e57b425 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5210.dtsi
> @@ -173,6 +173,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)>;
> @@ -460,6 +468,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]
Will geni_i2c_probe() fail with -EINVAL on i2c0, i2c1, and i2c2 when the
bootloader has not already set the SE clock to 32 MHz?

geni_se_resources_init() registers operating-points-v2 via
devm_pm_opp_of_add_table(), but the qcom,geni-i2c driver never calls
dev_pm_opp_set_rate() or clk_set_rate() before geni_i2c_clk_map_idx()
checks the current 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);
	...
}

In drivers/clk/qcom/gcc-ipq5210.c, ftbl_gcc_qupv3_wrap_se0_clk_src has a
24 MHz XO rate and a 32 MHz rate, but no 19.2 MHz entry.  If the SE clock
is left at its default rate at boot, clk_get_rate() is not 32 MHz, and
geni_se_clk_freq_match() fails looking for 19.2 MHz.  Setting
assigned-clocks and assigned-clock-rates = <32000000> on these nodes (or
applying the OPP rate in the driver before geni_i2c_clk_map_idx()) would
ensure the SE clock runs at 32 MHz before probe.

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