Re: [PATCH v2] Revert "arm64: dts: lx2160a: add pinmux and i2c gpio to support bus recovery"

[Josua Mayer <josua@solid-run.com>]

Hi Frank,

Am 14.07.25 um 16:34 schrieb Frank Li:

> On Mon, Jul 14, 2025 at 10:44:13AM +0300, Josua Mayer wrote:
>> This reverts commit 8a1365c7bbc122bd843096f0008d259e7a8afc61.
>>
>> The commit in questions most notably breaks SD-Card on SolidRun
>> LX2162A Clearfog by corrupting the pinmux in dynamic configuration area
>> for non-i2c pins without pinmux node.
>> It is further expected that it breaks SD Card-Detect, as well as CAN,
>> DSPI and GPIOs on any board based on LX2160 SoC.
> Thank you for your patch. I remember we met similar issue before. Let's
> wait for carlos is back about in next week.
>
> I remember uboot should copy RCWSR to 0x70010012c.
Knowing when and where this was implemented would be helpful,
however need to keep in mind existing systems running stable bootloader.
> I2C recover also is
> important feature.
Agreed.

There is an additional problem with the patch in question:
The pinmux affects both SCL+SDA together, changing between I2C and GPIO,
but in the i2c controller nodes only scl-gpios was specified.
Hence for sda the recovery relies on undefined state of gpio controller registers.

>> Background:
>>
>> The LX2160 SoC is configured at power-on from RCW (Reset
>> Configuration Word) typically located in the first 4k of boot media.
>> This blob configures various clock rates and pin functions.
>> The pinmux for i2c specifically is part of configuration words RCWSR12,
>> RCWSR13 and RCWSR14 size 32 bit each.
>> These values are accessible at read-only addresses 0x01e0012c following.
>>
>> For runtime (re-)configuration the SoC has a dynamic configuration area
>> where alternative settings can be applied. The counterparts of
>> RCWSR[12-14] can be overridden at 0x70010012c following.
>>
>> The commit in question used this area to switch i2c pins between i2c and
>> gpio function at runtime using the pinctrl-single driver - which reads a
>> 32-bit value, makes particular changes by bitmask and writes back the
>> new value.
>>
>> SolidRun have observed that if the dynamic configuration is read first
>> (before a write), it reads as zero regardless the initial values set by
>> RCW. After the first write consecutive reads reflect the written value.
>>
>> Because multiple pins are configured from a single 32-bit value, this
>> causes unintentional change of all bits (except those for i2c) being set
>> to zero when the pinctrl driver applies the first configuration.
>>
>> See below a short list of which functions RCWSR12 alone controls:
>>
>> LX2162-CF RCWSR12: 0b0000100000000000 0000000000000110
>> IIC2_PMUX              |||   |||   || |   |||   |||XXX : I2C/GPIO/CD-WP
>> IIC3_PMUX              |||   |||   || |   |||   XXX    : I2C/GPIO/CAN/EVT
>> IIC4_PMUX              |||   |||   || |   |||XXX|||    : I2C/GPIO/CAN/EVT
>> IIC5_PMUX              |||   |||   || |   XXX   |||    : I2C/GPIO/SDHC-CLK
>> IIC6_PMUX              |||   |||   || |XXX|||   |||    : I2C/GPIO/SDHC-CLK
>> XSPI1_A_DATA74_PMUX    |||   |||   XX X   |||   |||    : XSPI/GPIO
>> XSPI1_A_DATA30_PMUX    |||   |||XXX|| |   |||   |||    : XSPI/GPIO
>> XSPI1_A_BASE_PMUX      |||   XXX   || |   |||   |||    : XSPI/GPIO
>> SDHC1_BASE_PMUX        |||XXX|||   || |   |||   |||    : SDHC/GPIO/SPI
>> SDHC1_DIR_PMUX         XXX   |||   || |   |||   |||    : SDHC/GPIO/SPI
>> RESERVED             XX|||   |||   || |   |||   |||    :
>>
>> On LX2162A Clearfog the initial (ant intended) value is 0x08000006 -
>> enabling card-detect on IIC2_PMUX and some LEDs on SDHC1_DIR_PMUX.
>> Everything else is intentional zero (enabling I2C & XSPI).
>>
>> By reading zero from dynamic configuration area, the commit in question
>> changes IIC2_PMUX to value 0 (I2C function), and SDHC1_DIR_PMUX to 0
>> (SDHC data direction function) - breaking card-detect and led gpios.
>>
>> This issue should affect any board based on LX2160 SoC that is using the
>> same or earlier versions of NXP bootloader as SolidRun have tested, in
>> particular: LSDK-21.08 and LS-5.15.71-2.2.0.
>>
>> Whether NXP added some extra initialisation in the bootloader on later
>> releases was not investigated. However bootloader upgrade should not be
>> necessary to run a newer Linux kernel.
>>
>> To work around this issue it is possible to explicitly define ALL pins
>> controlled by any 32-bit value so that gradually after processing all
>> pinctrl nodes the correct value is reached on all bits.
>>
>> This is a large task that should be done carefully on a per-board basis
>> and not globally through the SoC dtsi.
>> Therefore the commit in question is reverted.
>>
>> Fixes: 8a1365c7bbc1 ("arm64: dts: lx2160a: add pinmux and i2c gpio to support bus recovery")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> ---
>> Changes in v2:
>> - changed to revert problematic commit, workaround is large effort
>> - Link to v1: https://lore.kernel.org/r/f32c5525-3162-4acd-880c-99fc46d3a63d@solid-run.com
>> ---
>>  arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 106 -------------------------
>>  1 file changed, 106 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
>> index c9541403bcd8..eb1b4e607e2b 100644
>> --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
>> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
>> @@ -749,10 +749,6 @@ i2c0: i2c@2000000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c0_scl>;
>> -			pinctrl-1 = <&i2c0_scl_gpio>;
>> -			scl-gpios = <&gpio0 3 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -765,10 +761,6 @@ i2c1: i2c@2010000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c1_scl>;
>> -			pinctrl-1 = <&i2c1_scl_gpio>;
>> -			scl-gpios = <&gpio0 31 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -781,10 +773,6 @@ i2c2: i2c@2020000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c2_scl>;
>> -			pinctrl-1 = <&i2c2_scl_gpio>;
>> -			scl-gpios = <&gpio0 29 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -797,10 +785,6 @@ i2c3: i2c@2030000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c3_scl>;
>> -			pinctrl-1 = <&i2c3_scl_gpio>;
>> -			scl-gpios = <&gpio0 27 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -813,10 +797,6 @@ i2c4: i2c@2040000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c4_scl>;
>> -			pinctrl-1 = <&i2c4_scl_gpio>;
>> -			scl-gpios = <&gpio0 25 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -829,10 +809,6 @@ i2c5: i2c@2050000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c5_scl>;
>> -			pinctrl-1 = <&i2c5_scl_gpio>;
>> -			scl-gpios = <&gpio0 23 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -845,10 +821,6 @@ i2c6: i2c@2060000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c6_scl>;
>> -			pinctrl-1 = <&i2c6_scl_gpio>;
>> -			scl-gpios = <&gpio1 16 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -861,10 +833,6 @@ i2c7: i2c@2070000 {
>>  			clock-names = "ipg";
>>  			clocks = <&clockgen QORIQ_CLK_PLATFORM_PLL
>>  					    QORIQ_CLK_PLL_DIV(16)>;
>> -			pinctrl-names = "default", "gpio";
>> -			pinctrl-0 = <&i2c7_scl>;
>> -			pinctrl-1 = <&i2c7_scl_gpio>;
>> -			scl-gpios = <&gpio1 18 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>  			status = "disabled";
>>  		};
>>
>> @@ -1700,80 +1668,6 @@ pcs18: ethernet-phy@0 {
>>  			};
>>  		};
>>
>> -		pinmux_i2crv: pinmux@70010012c {
>> -			compatible = "pinctrl-single";
>> -			reg = <0x00000007 0x0010012c 0x0 0xc>;
>> -			#address-cells = <1>;
>> -			#size-cells = <0>;
>> -			pinctrl-single,bit-per-mux;
>> -			pinctrl-single,register-width = <32>;
>> -			pinctrl-single,function-mask = <0x7>;
>> -
>> -			i2c1_scl: i2c1-scl-pins {
>> -				pinctrl-single,bits = <0x0 0 0x7>;
>> -			};
>> -
>> -			i2c1_scl_gpio: i2c1-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x0 0x1 0x7>;
>> -			};
>> -
>> -			i2c2_scl: i2c2-scl-pins {
>> -				pinctrl-single,bits = <0x0 0 (0x7 << 3)>;
>> -			};
>> -
>> -			i2c2_scl_gpio: i2c2-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x0 (0x1 << 3) (0x7 << 3)>;
>> -			};
>> -
>> -			i2c3_scl: i2c3-scl-pins {
>> -				pinctrl-single,bits = <0x0 0 (0x7 << 6)>;
>> -			};
>> -
>> -			i2c3_scl_gpio: i2c3-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x0 (0x1 << 6) (0x7 << 6)>;
>> -			};
>> -
>> -			i2c4_scl: i2c4-scl-pins {
>> -				pinctrl-single,bits = <0x0 0 (0x7 << 9)>;
>> -			};
>> -
>> -			i2c4_scl_gpio: i2c4-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x0 (0x1 << 9) (0x7 << 9)>;
>> -			};
>> -
>> -			i2c5_scl: i2c5-scl-pins {
>> -				pinctrl-single,bits = <0x0 0 (0x7 << 12)>;
>> -			};
>> -
>> -			i2c5_scl_gpio: i2c5-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x0 (0x1 << 12) (0x7 << 12)>;
>> -			};
>> -
>> -			i2c6_scl: i2c6-scl-pins {
>> -				pinctrl-single,bits = <0x4 0x2 0x7>;
>> -			};
>> -
>> -			i2c6_scl_gpio: i2c6-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x4 0x1 0x7>;
>> -			};
>> -
>> -			i2c7_scl: i2c7-scl-pins {
>> -				pinctrl-single,bits = <0x4 0x2 0x7>;
>> -			};
>> -
>> -			i2c7_scl_gpio: i2c7-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x4 0x1 0x7>;
>> -			};
>> -
>> -			i2c0_scl: i2c0-scl-pins {
>> -				pinctrl-single,bits = <0x8 0 (0x7 << 10)>;
>> -			};
>> -
>> -			i2c0_scl_gpio: i2c0-scl-gpio-pins {
>> -				pinctrl-single,bits = <0x8 (0x1 << 10) (0x7 << 10)>;
>> -			};
>> -		};
>> -
>>  		fsl_mc: fsl-mc@80c000000 {
>>  			compatible = "fsl,qoriq-mc";
>>  			reg = <0x00000008 0x0c000000 0 0x40>,
>>
>> ---
>> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
>> change-id: 20250710-lx2160-sd-cd-00bf38ae169e
>>
>> Best regards,
>> --
>> Josua Mayer <josua@solid-run.com>
>>