Re: [PATCH 3/3] arm64: dts: stratix10: define the i2c1 pins for pinctrl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: stratix10: wrong I2C1_SCL pinmux offset in nand GPIO state
- [High] arm64: dts: stratix10: i2c1 pinmux nodes attached to i2c2 controller
- [Medium] arm64: dts: stratix10: missing scl-gpios and sda-gpios for I2C recovery
--

commit 6b60cc5dbb08c4ec389238c564d8da6f01a7cc28
Author: Dinh Nguyen <dinguyen@kernel.org>

arm64: dts: stratix10: define the i2c1 pins for pinctrl

This commit adds I2C1 pinctrl pin definitions and recovery states to the
Stratix10 SoCDK eMMC and NAND daughter card device trees to support I2C bus
recovery.

> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_emmc.dts b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_emmc.dts
> index b2a3449638dd3..743a4db0423d8 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_emmc.dts
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_emmc.dts
> @@ -12,6 +12,22 @@ / {
>  			"altr,socfpga-stratix10";
>  };
>  
> +&pinctrl0 {
> +	i2c1_pmx_func: i2c1-pmx-func-pins {
> +		pinctrl-single,pins = <
> +			0x78   0x4   /* I2C1_SDA (IO6) PIN30SEL) */
> +			0x7c   0x4   /* I2C1_SCL (IO7) PIN31SEL */
> +		>;
> +	};
> +
> +	i2c1_pmx_func_gpio: i2c1-pmx-func-gpio-pins {
> +		pinctrl-single,pins = <
> +			0x78   0x8   /* I2C1_SDA (IO6) PIN30SEL) */
> +			0x7c   0x8   /* I2C1_SCL (IO7) PIN31SEL */
> +		>;
> +	};
> +};
> +
>  &gmac2 {
>  	status = "okay";
>  	/* PHY delays is configured via skew properties */
> @@ -57,6 +73,10 @@ &i2c2 {
>  	i2c-sda-falling-time-ns = <890>;  /* hcnt */
>  	i2c-scl-falling-time-ns = <890>;  /* lcnt */
>  
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&i2c1_pmx_func>;
> +	pinctrl-1 = <&i2c1_pmx_func_gpio>;

[Severity: High]
Are the i2c1_pmx_func and i2c1_pmx_func_gpio pinctrl states attached to the
right I2C controller node here and in socfpga_stratix10_socdk_nand.dts?

In arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi, i2c1 is i2c@ffc02900
and i2c2 is i2c@ffc02a00. In socfpga_stratix10_socdk.dts, i2c1_pmx_func and
i2c1_pmx_func_gpio configure PIN30SEL (0x78, I2C1_SDA) and PIN31SEL (0x7c,
I2C1_SCL) and are attached to &i2c1.

Here (and in socfpga_stratix10_socdk_nand.dts), those I2C1 pinmux states are
attached to &i2c2 instead of &i2c1, so when &i2c2 probes via
really_probe() -> pinctrl_bind_pins(), it muxes the I2C1 pins rather than
configuring pins for i2c2 or using the &i2c1 controller.

[ ... ]

> diff --git a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_nand.dts b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_nand.dts
> index 7951ce46ae1f6..21d25a10e472a 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_nand.dts
> +++ b/arch/arm64/boot/dts/altera/socfpga_stratix10_socdk_nand.dts
> @@ -56,6 +56,22 @@ &gpio1 {
>  	status = "okay";
>  };
>  
> +&pinctrl0 {
> +	i2c1_pmx_func: i2c1-pmx-func-pins {
> +		pinctrl-single,pins = <
> +			0x78   0x4   /* I2C1_SDA (IO6) PIN30SEL) */
> +			0x7c   0x4   /* I2C1_SCL (IO7) PIN31SEL */
> +		>;
> +	};
> +
> +	i2c1_pmx_func_gpio: i2c1-pmx-func-gpio-pins {
> +		pinctrl-single,pins = <
> +			0x78   0x8   /* I2C1_SDA (IO6) PIN30SEL) */
> +			0x1c   0x8   /* I2C1_SCL (IO7) PIN31SEL */

[Severity: High]
Should the register offset for I2C1_SCL (PIN31SEL) in i2c1_pmx_func_gpio be
0x7c instead of 0x1c?

In i2c1_pmx_func right above (and in socfpga_stratix10_socdk_emmc.dts),
PIN31SEL is at offset 0x7c. Using 0x1c in i2c1_pmx_func_gpio writes 0x8 to
PIN7SEL when the "gpio" pinctrl state is selected via pinctrl_select_state(),
leaving I2C1_SCL (PIN31SEL) in I2C mode and leaving PIN7SEL overwritten even
after switching back to the "default" state.

> +		>;
> +	};
> +};
> +
>  &gmac2 {
>  	status = "okay";
>  	phy-mode = "rgmii";
> @@ -130,6 +146,10 @@ &i2c2 {
>  	i2c-sda-falling-time-ns = <890>;  /* hcnt */
>  	i2c-scl-falling-time-ns = <890>;  /* lcnt */
>  
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&i2c1_pmx_func>;
> +	pinctrl-1 = <&i2c1_pmx_func_gpio>;

[Severity: Medium]
Can DesignWare I2C bus recovery initialize without scl-gpios and sda-gpios
defined in the I2C node in both socfpga_stratix10_socdk_emmc.dts and
socfpga_stratix10_socdk_nand.dts?

When the controller probes via i2c_dw_probe_master() ->
i2c_dw_init_recovery_info() in drivers/i2c/busses/i2c-designware-master.c, it
checks for the "scl" GPIO first:

	gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH);
	if (IS_ERR_OR_NULL(gpio))
		return PTR_ERR_OR_ZERO(gpio);

Without scl-gpios (and sda-gpios, as present in socfpga_stratix10_socdk.dts:

	scl-gpios = <&portb 6 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
	sda-gpios = <&portb 7 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;

), i2c_dw_init_recovery_info() returns early before calling
devm_pinctrl_get() or setting adap->bus_recovery_info, so i2c_init_recovery()
never sets up GPIO bus recovery.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029213201.718625-1-dinguyen@kernel.org?part=3