[PATCH 1/3] arm64: dts: socfpga: agilex5: add the pinctrl node

[PATCH 1/3] arm64: dts: socfpga: agilex5: add the pinctrl node

[Dinh Nguyen]

The Agilex5 also has a pinctrl module that does the pinmuxing for the
various pins on the platform.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 arch/arm64/boot/dts/altera/socfpga_agilex5.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/altera/socfpga_agilex5.dtsi b/arch/arm64/boot/dts/altera/socfpga_agilex5.dtsi
index 0c501a93a8127..161f3d4bbefe8 100644
--- a/arch/arm64/boot/dts/altera/socfpga_agilex5.dtsi
+++ b/arch/arm64/boot/dts/altera/socfpga_agilex5.dtsi
@@ -446,6 +446,14 @@ spi1: spi@10da5000 {
 			status = "disabled";
 		};
 
+		pinctrl0: pinctrl@10d13000 {
+			compatible = "pinctrl-single";
+			#pinctrl-cells = <1>;
+			reg = <0x10d13000 0x120>;
+			pinctrl-single,register-width = <32>;
+			pinctrl-single,function-mask = <0x0000000f>;
+		};
+
 		sysmgr: sysmgr@10d12000 {
 			compatible = "altr,sys-mgr-s10","altr,sys-mgr";
 			reg = <0x10d12000 0x500>;
-- 
2.42.0.411.g813d9a9188

[PATCH 2/3] arm64: dts: agilex5: define the i2c0 pins for pinctrl

[Dinh Nguyen]

Add the I2C pins definition to the Agilex5 SoCDK NAND daughter card. This
allows the I2C driver to use pinctrl on the pins to perform I2C recovery.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 .../dts/altera/socfpga_agilex5_socdk_nand.dts | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts b/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
index 81443096cf991..346a4ee22f95b 100644
--- a/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
+++ b/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
@@ -39,6 +39,22 @@ memory@80000000 {
 	};
 };
 
+&pinctrl0 {
+	i2c0_pmx_func: i2c0-pmx-func-pins {
+		pinctrl-single,pins = <
+			0x10   0x4   /* I2C0_SDA (IOA4) PIN4SEL) */
+			0x14   0x4   /* I2C0_SCL (IOA5) PIN5SEL */
+		>;
+	};
+
+	i2c0_pmx_func_gpio: i2c0-pmx-func-gpio-pins {
+		pinctrl-single,pins = <
+			0x10   0x8   /* I2C0_SDA (IOA4) PIN4SEL) */
+			0x14   0x8   /* I2C0_SCL (IOA5) PIN5SEL */
+		>;
+	};
+};
+
 &gmac0 {
 	status = "okay";
 	phy-mode = "rgmii"; /* TX/RX clock delays provided by Agilex5 I/O hardware */
@@ -66,6 +82,10 @@ &gpio1 {
 
 &i2c0 {
 	status = "okay";
+
+	pinctrl-names = "default", "gpio";
+	pinctrl-0 = <&i2c0_pmx_func>;
+	pinctrl-1 = <&i2c0_pmx_func_gpio>;
 };
 
 &i3c0 {
-- 
2.42.0.411.g813d9a9188

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

[Dinh Nguyen]

Add the I2C pins definition to the Stratix10 SoCDK NAND and eMMC daughter
cards. This allows the I2C driver to use pinctrl on the pins to perform
I2C recovery.

Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
---
 .../altera/socfpga_stratix10_socdk_emmc.dts   | 20 +++++++++++++++++++
 .../altera/socfpga_stratix10_socdk_nand.dts   | 20 +++++++++++++++++++
 2 files changed, 40 insertions(+)

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 bda2fe059beba..c63af8fa0917a 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 @@ &i2c1 {
 	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>;
+
 	adc@14 {
 		compatible = "lltc,ltc2497";
 		reg = <0x14>;
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 a450c42a6b35f..f69ec2f5b94b6 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 */
+		>;
+	};
+};
+
 &gmac2 {
 	status = "okay";
 	phy-mode = "rgmii";
@@ -130,6 +146,10 @@ &i2c1 {
 	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>;
+
 	adc@14 {
 		compatible = "lltc,ltc2497";
 		reg = <0x14>;
-- 
2.42.0.411.g813d9a9188

Re: [PATCH 2/3] arm64: dts: agilex5: define the i2c0 pins for pinctrl

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] arm64: dts: agilex5: missing scl-gpios and sda-gpios on i2c0 prevents recovery
--

commit 4fe1ecb21c260e09211a4f5ec837b9b80b38c36d
Author: Dinh Nguyen <dinguyen@kernel.org>

arm64: dts: agilex5: define the i2c0 pins for pinctrl

This commit adds pinctrl definitions for i2c0 default and gpio states on the
Agilex5 SoCDK NAND daughter card to support I2C bus recovery.

> diff --git a/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts b/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
> index 81443096cf991..346a4ee22f95b 100644
> --- a/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
> +++ b/arch/arm64/boot/dts/altera/socfpga_agilex5_socdk_nand.dts
> @@ -39,6 +39,22 @@ memory@80000000 {
>  	};
>  };
>  
> +&pinctrl0 {
> +	i2c0_pmx_func: i2c0-pmx-func-pins {
> +		pinctrl-single,pins = <
> +			0x10   0x4   /* I2C0_SDA (IOA4) PIN4SEL) */
> +			0x14   0x4   /* I2C0_SCL (IOA5) PIN5SEL */
> +		>;
> +	};
> +
> +	i2c0_pmx_func_gpio: i2c0-pmx-func-gpio-pins {
> +		pinctrl-single,pins = <
> +			0x10   0x8   /* I2C0_SDA (IOA4) PIN4SEL) */
> +			0x14   0x8   /* I2C0_SCL (IOA5) PIN5SEL */
> +		>;
> +	};
> +};
> +
>  &gmac0 {
>  	status = "okay";
>  	phy-mode = "rgmii"; /* TX/RX clock delays provided by Agilex5 I/O hardware */
> @@ -66,6 +82,10 @@ &gpio1 {
>  
>  &i2c0 {
>  	status = "okay";
> +
> +	pinctrl-names = "default", "gpio";
> +	pinctrl-0 = <&i2c0_pmx_func>;
> +	pinctrl-1 = <&i2c0_pmx_func_gpio>;
>  };

[Severity: Medium]
Does i2c0 also need scl-gpios and sda-gpios properties for I2C recovery to
work?

When probing i2c0, i2c_dw_probe_master() calls i2c_dw_init_recovery_info() in
drivers/i2c/busses/i2c-designware-master.c:

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

	rinfo->scl_gpiod = gpio;

Without scl-gpios in the i2c0 node, devm_gpiod_get_optional() returns NULL and
i2c_dw_init_recovery_info() returns early before calling devm_pinctrl_get() or
populating adap->bus_recovery_info.

Consequently, i2c_init_recovery() in drivers/i2c/i2c-core-base.c sees
adap->bus_recovery_info as NULL and skips i2c_gpio_init_recovery(), leaving
the gpio pinctrl state unused and bus recovery disabled.

In arch/arm64/boot/dts/altera/socfpga_stratix10_socdk.dts, the pinctrl states
are paired with the GPIO descriptors:

	pinctrl-names = "default", "gpio";
	pinctrl-0 = <&i2c1_pmx_func>;
	pinctrl-1 = <&i2c1_pmx_func_gpio>;

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

Should i2c0 here also specify scl-gpios and sda-gpios (for example on &porta 5
and &porta 4 for IOA5 and IOA4)?

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

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

[sashiko-bot]

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