diff mbox series

[2/2] Revert "arm: mvebu: x240: Use i2c-gpio instead of built in controller"

Message ID 20231003035800.2626121-2-judge.packham@gmail.com
State Accepted
Commit 10c937fa23ddb5dca19ddd4a6f587a451c03e07f
Delegated to: Stefan Roese
Headers show
Series [1/2] arm: mvebu: x240: Disable SMBIOS | expand

Commit Message

Chris Packham Oct. 3, 2023, 3:57 a.m. UTC 
This reverts commit 5c1c6b7306f2b4c0fd50c7cb5d757e245b93606e. The reason
for switching to i2c-gpio was due to an issue we were seeing in the
Linux kernel where the CPU would lock up on certain adverse I2C bus
conditions. We were never able to reproduce the lockup in U-Boot but
assumed that was probably just luck.

Since then we have discovered that the lock up was due to the I2C
transaction offload engine in the I2C controller not coping with the
adverse bus conditions (basically it thinks there's another master and
waits for a STOP condition that never comes). U-Boot doesn't use the I2C
offload feature so is not susceptible to the lockup.

We can therefore safely return to using the built-in I2C controller.

Signed-off-by: Chris Packham <judge.packham@gmail.com>
---

 arch/arm/dts/ac5-98dx35xx-atl-x240.dts | 30 ++++++--------------------
 configs/x240_defconfig                 |  1 -
 2 files changed, 7 insertions(+), 24 deletions(-)

Comments

Stefan Roese Oct. 5, 2023, 7:54 a.m. UTC | #1 
On 10/3/23 05:57, Chris Packham wrote:
> This reverts commit 5c1c6b7306f2b4c0fd50c7cb5d757e245b93606e. The reason
> for switching to i2c-gpio was due to an issue we were seeing in the
> Linux kernel where the CPU would lock up on certain adverse I2C bus
> conditions. We were never able to reproduce the lockup in U-Boot but
> assumed that was probably just luck.
> 
> Since then we have discovered that the lock up was due to the I2C
> transaction offload engine in the I2C controller not coping with the
> adverse bus conditions (basically it thinks there's another master and
> waits for a STOP condition that never comes). U-Boot doesn't use the I2C
> offload feature so is not susceptible to the lockup.
> 
> We can therefore safely return to using the built-in I2C controller.
> 
> Signed-off-by: Chris Packham <judge.packham@gmail.com>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
> 
>   arch/arm/dts/ac5-98dx35xx-atl-x240.dts | 30 ++++++--------------------
>   configs/x240_defconfig                 |  1 -
>   2 files changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
> index c19b25925ba2..820ec18b4355 100644
> --- a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
> +++ b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
> @@ -16,7 +16,7 @@
>   		gpio0 = &gpio0;
>   		gpio1 = &gpio1;
>   		spi0 = &spi0;
> -		i2c0 = &i2cgpio;
> +		i2c0 = &i2c0;
>   		usb0 = &usb0;
>   		pinctrl0 = &pinctrl0;
>   	};
> @@ -40,19 +40,6 @@
>   			default-state = "on";
>   		};
>   	};
> -
> -	i2cgpio: i2c-gpio-0 {
> -			compatible = "i2c-gpio";
> -			#address-cells = <1>;
> -			#size-cells = <0>;
> -
> -			pinctrl-names = "default";
> -			pinctrl-0 =  <&i2c0_gpio>;
> -			scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> -			sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> -			i2c-gpio,delay-us = <2>;
> -			status = "okay";
> -		 };
>   };
>   
>   &nand {
> @@ -83,7 +70,9 @@
>   	status = "okay";
>   };
>   
> -&i2cgpio {
> +&i2c0 {
> +	status = "okay";
> +
>   	mux@71 {
>   		#address-cells = <1>;
>   		#size-cells = <0>;
> @@ -188,8 +177,8 @@
>   	 * LED_OE_N              [23]
>   	 * USB_PWR_FLT_N         [24]
>   	 * SFP_INT_N             [25]
> -	 * I2C0_SCL              [26] (GPIO)
> -	 * I2C0_SDA              [27] (GPIO)
> +	 * I2C0_SCL              [26]
> +	 * I2C0_SDA              [27]
>   	 * USB_EN                [28]
>   	 * MONITOR_INT_N         [29]
>   	 * XM1_MDC               [30]
> @@ -212,7 +201,7 @@
>   	/*	     0    1    2    3    4    5    6    7    8    9 */
>   	pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
>   		     0xff 0xff 1    1    1    1    0xff 0xff 0    0
> -		     0    0    0    0    0    0    0xff 0xff 0    0
> +		     0    0    0    0    0    0    1    1    0    0
>   		     1    1    1    1    0    0    0    0    0    0
>   		     0    0    0    1    1    1    >;
>   
> @@ -220,9 +209,4 @@
>   		marvell,pins = <0 1 2 3 4 5 6 7 8 9 10 11 16 17>;
>   		marvell,function = <2>;
>   	};
> -
> -	i2c0_gpio: i2c0-gpio-pins {
> -		marvell,pins = <26 27>;
> -		marvell,function = <0>;
> -	};
>   };
> diff --git a/configs/x240_defconfig b/configs/x240_defconfig
> index 0d5a19df25aa..4b1a761a9086 100644
> --- a/configs/x240_defconfig
> +++ b/configs/x240_defconfig
> @@ -42,7 +42,6 @@ CONFIG_CLK_MVEBU=y
>   CONFIG_GPIO_HOG=y
>   CONFIG_DM_PCA953X=y
>   CONFIG_DM_I2C=y
> -CONFIG_DM_I2C_GPIO=y
>   CONFIG_SYS_I2C_MVTWSI=y
>   CONFIG_I2C_MUX=y
>   CONFIG_I2C_MUX_PCA954x=y

Viele Grüße,
Stefan Roese
Stefan Roese Oct. 16, 2023, 2:43 p.m. UTC | #2 
On 10/5/23 09:54, Stefan Roese wrote:
> On 10/3/23 05:57, Chris Packham wrote:
>> This reverts commit 5c1c6b7306f2b4c0fd50c7cb5d757e245b93606e. The reason
>> for switching to i2c-gpio was due to an issue we were seeing in the
>> Linux kernel where the CPU would lock up on certain adverse I2C bus
>> conditions. We were never able to reproduce the lockup in U-Boot but
>> assumed that was probably just luck.
>>
>> Since then we have discovered that the lock up was due to the I2C
>> transaction offload engine in the I2C controller not coping with the
>> adverse bus conditions (basically it thinks there's another master and
>> waits for a STOP condition that never comes). U-Boot doesn't use the I2C
>> offload feature so is not susceptible to the lockup.
>>
>> We can therefore safely return to using the built-in I2C controller.
>>
>> Signed-off-by: Chris Packham <judge.packham@gmail.com>
> 
> Reviewed-by: Stefan Roese <sr@denx.de>

Applied to u-boot-marvell/master

Thanks,
Stefan


> Thanks,
> Stefan
> 
>> ---
>>
>>   arch/arm/dts/ac5-98dx35xx-atl-x240.dts | 30 ++++++--------------------
>>   configs/x240_defconfig                 |  1 -
>>   2 files changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts 
>> b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
>> index c19b25925ba2..820ec18b4355 100644
>> --- a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
>> +++ b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
>> @@ -16,7 +16,7 @@
>>           gpio0 = &gpio0;
>>           gpio1 = &gpio1;
>>           spi0 = &spi0;
>> -        i2c0 = &i2cgpio;
>> +        i2c0 = &i2c0;
>>           usb0 = &usb0;
>>           pinctrl0 = &pinctrl0;
>>       };
>> @@ -40,19 +40,6 @@
>>               default-state = "on";
>>           };
>>       };
>> -
>> -    i2cgpio: i2c-gpio-0 {
>> -            compatible = "i2c-gpio";
>> -            #address-cells = <1>;
>> -            #size-cells = <0>;
>> -
>> -            pinctrl-names = "default";
>> -            pinctrl-0 =  <&i2c0_gpio>;
>> -            scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | 
>> GPIO_OPEN_DRAIN)>;
>> -            sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | 
>> GPIO_OPEN_DRAIN)>;
>> -            i2c-gpio,delay-us = <2>;
>> -            status = "okay";
>> -         };
>>   };
>>   &nand {
>> @@ -83,7 +70,9 @@
>>       status = "okay";
>>   };
>> -&i2cgpio {
>> +&i2c0 {
>> +    status = "okay";
>> +
>>       mux@71 {
>>           #address-cells = <1>;
>>           #size-cells = <0>;
>> @@ -188,8 +177,8 @@
>>        * LED_OE_N              [23]
>>        * USB_PWR_FLT_N         [24]
>>        * SFP_INT_N             [25]
>> -     * I2C0_SCL              [26] (GPIO)
>> -     * I2C0_SDA              [27] (GPIO)
>> +     * I2C0_SCL              [26]
>> +     * I2C0_SDA              [27]
>>        * USB_EN                [28]
>>        * MONITOR_INT_N         [29]
>>        * XM1_MDC               [30]
>> @@ -212,7 +201,7 @@
>>       /*         0    1    2    3    4    5    6    7    8    9 */
>>       pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
>>                0xff 0xff 1    1    1    1    0xff 0xff 0    0
>> -             0    0    0    0    0    0    0xff 0xff 0    0
>> +             0    0    0    0    0    0    1    1    0    0
>>                1    1    1    1    0    0    0    0    0    0
>>                0    0    0    1    1    1    >;
>> @@ -220,9 +209,4 @@
>>           marvell,pins = <0 1 2 3 4 5 6 7 8 9 10 11 16 17>;
>>           marvell,function = <2>;
>>       };
>> -
>> -    i2c0_gpio: i2c0-gpio-pins {
>> -        marvell,pins = <26 27>;
>> -        marvell,function = <0>;
>> -    };
>>   };
>> diff --git a/configs/x240_defconfig b/configs/x240_defconfig
>> index 0d5a19df25aa..4b1a761a9086 100644
>> --- a/configs/x240_defconfig
>> +++ b/configs/x240_defconfig
>> @@ -42,7 +42,6 @@ CONFIG_CLK_MVEBU=y
>>   CONFIG_GPIO_HOG=y
>>   CONFIG_DM_PCA953X=y
>>   CONFIG_DM_I2C=y
>> -CONFIG_DM_I2C_GPIO=y
>>   CONFIG_SYS_I2C_MVTWSI=y
>>   CONFIG_I2C_MUX=y
>>   CONFIG_I2C_MUX_PCA954x=y
> 
> Viele Grüße,
> Stefan Roese
> 

Viele Grüße,
Stefan Roese
diff mbox series

Patch

diff --git a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
index c19b25925ba2..820ec18b4355 100644
--- a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
+++ b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
@@ -16,7 +16,7 @@ 
 		gpio0 = &gpio0;
 		gpio1 = &gpio1;
 		spi0 = &spi0;
-		i2c0 = &i2cgpio;
+		i2c0 = &i2c0;
 		usb0 = &usb0;
 		pinctrl0 = &pinctrl0;
 	};
@@ -40,19 +40,6 @@ 
 			default-state = "on";
 		};
 	};
-
-	i2cgpio: i2c-gpio-0 {
-			compatible = "i2c-gpio";
-			#address-cells = <1>;
-			#size-cells = <0>;
-
-			pinctrl-names = "default";
-			pinctrl-0 =  <&i2c0_gpio>;
-			scl-gpios = <&gpio0 26 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
-			sda-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
-			i2c-gpio,delay-us = <2>;
-			status = "okay";
-		 };
 };
 
 &nand {
@@ -83,7 +70,9 @@ 
 	status = "okay";
 };
 
-&i2cgpio {
+&i2c0 {
+	status = "okay";
+
 	mux@71 {
 		#address-cells = <1>;
 		#size-cells = <0>;
@@ -188,8 +177,8 @@ 
 	 * LED_OE_N              [23]
 	 * USB_PWR_FLT_N         [24]
 	 * SFP_INT_N             [25]
-	 * I2C0_SCL              [26] (GPIO)
-	 * I2C0_SDA              [27] (GPIO)
+	 * I2C0_SCL              [26]
+	 * I2C0_SDA              [27]
 	 * USB_EN                [28]
 	 * MONITOR_INT_N         [29]
 	 * XM1_MDC               [30]
@@ -212,7 +201,7 @@ 
 	/*	     0    1    2    3    4    5    6    7    8    9 */
 	pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
 		     0xff 0xff 1    1    1    1    0xff 0xff 0    0
-		     0    0    0    0    0    0    0xff 0xff 0    0
+		     0    0    0    0    0    0    1    1    0    0
 		     1    1    1    1    0    0    0    0    0    0
 		     0    0    0    1    1    1    >;
 
@@ -220,9 +209,4 @@ 
 		marvell,pins = <0 1 2 3 4 5 6 7 8 9 10 11 16 17>;
 		marvell,function = <2>;
 	};
-
-	i2c0_gpio: i2c0-gpio-pins {
-		marvell,pins = <26 27>;
-		marvell,function = <0>;
-	};
 };
diff --git a/configs/x240_defconfig b/configs/x240_defconfig
index 0d5a19df25aa..4b1a761a9086 100644
--- a/configs/x240_defconfig
+++ b/configs/x240_defconfig
@@ -42,7 +42,6 @@  CONFIG_CLK_MVEBU=y
 CONFIG_GPIO_HOG=y
 CONFIG_DM_PCA953X=y
 CONFIG_DM_I2C=y
-CONFIG_DM_I2C_GPIO=y
 CONFIG_SYS_I2C_MVTWSI=y
 CONFIG_I2C_MUX=y
 CONFIG_I2C_MUX_PCA954x=y