Re: [PATCH v3 1/5] arm64: dts: lx2160a-cex7/lx2162a-sr-som: fix usd-cd & gpio pinmux

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

Am 04.03.26 um 12:21 schrieb Josua Mayer:
> Commit 8a1365c7bbc1 ("arm64: dts: lx2160a: add pinmux and i2c gpio to
> support bus recovery") introduced pinmux nodes for lx2160 i2c
> interfaces, allowing runtime change between i2c and gpio functions
> implementing bus recovery.
>
> This has caused unintended side-effects on SolidRun boards where the
> first application of a pinmux node cleared all bits in a 32-bit word
> cleared, corrupting the configuration previously set by bootloader.
>
> 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 (and intended) value is 0x08000006 -
> enabling card-detect on IIC2_PMUX and control GPIOs 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 reverting the commit in question altogether was considered,
> but received pushback in review with the argument that bus recovery was
> important.
>
> Instead add pinmux nodes for all fields or rcwsr12 as used by affected
> SolidRun LX2160A Clearfog-CX & Honeycomb, and LX2162A Clearfog boards.
>
> 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>
> ---
>  .../arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi |  7 +++++++
>  .../dts/freescale/fsl-lx2160a-clearfog-itx.dtsi    |  2 ++
>  arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi     | 24 ++++++++++++++++++++++
>  .../boot/dts/freescale/fsl-lx2162a-clearfog.dts    |  2 ++
>  .../boot/dts/freescale/fsl-lx2162a-sr-som.dtsi     |  7 +++++++
>  5 files changed, 42 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi
> index eec2cd6c6d32a..7f6e39e27ce5c 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-cex7.dtsi
> @@ -162,6 +162,8 @@ rtc@51 {
>  };
>  
>  &fspi {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&fspi_data74_pins>, <&fspi_data30_pins>, <&fspi_dqs_sck_cs10_pins>;
>  	status = "okay";
>  
>  	flash@0 {
> @@ -177,6 +179,11 @@ flash@0 {
>  	};
>  };
>  
> +&pinmux_i2crv {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gpio0_14_12_pins>;
> +};
> +
>  &usb0 {
>  	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-itx.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-itx.dtsi
> index af6258b2fe826..580ee9b3026e3 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-itx.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a-clearfog-itx.dtsi
> @@ -89,6 +89,8 @@ &emdio2 {
>  };
>  
>  &esdhc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&esdhc0_cd_wp_pins>, <&esdhc0_cmd_data30_clk_vsel_pins>;
>  	sd-uhs-sdr104;
>  	sd-uhs-sdr50;
>  	sd-uhs-sdr25;
> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> index 853b01452813a..be0ccab5a626b 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi
> @@ -1721,6 +1721,10 @@ i2c1_scl_gpio: i2c1-scl-gpio-pins {
>  				pinctrl-single,bits = <0x0 0x1 0x7>;
>  			};
>  
> +			esdhc0_cd_wp_pins: iic2-sdhc-pins {
> +				pinctrl-single,bits = <0x0 0x6 0x7>;
> +			};
> +
>  			i2c2_scl: i2c2-scl-pins {
>  				pinctrl-single,bits = <0x0 0 (0x7 << 3)>;
>  			};
> @@ -1753,6 +1757,26 @@ i2c5_scl_gpio: i2c5-scl-gpio-pins {
>  				pinctrl-single,bits = <0x0 (0x1 << 12) (0x7 << 12)>;
>  			};
>  
> +			fspi_data74_pins: xspi1-data74-pins {
> +				pinctrl-single,bits = <0x0 0 (0x7 << 15)>;
> +			};
Here I changed the naming scheme, to follow lx2160 reference manual bitfield names
closely, instead of device-tree bus names.

Should I rename the existing nodes, too, considering this change should go to stable?
I.e. "i2c1-scl-gpio-pins" -> "iic2-gpio-pins".

> +
> +			fspi_data30_pins: xspi1-data30-pins {
> +				pinctrl-single,bits = <0x0 0 (0x7 << 18)>;
I meant to replace "0" with "0x0" for consistency here ...
> +			};
> +
> +			fspi_dqs_sck_cs10_pins: xspi1-base-pins {
> +				pinctrl-single,bits = <0x0 0x0 (0x7 << 21)>;
> +			};
> +
> +			esdhc0_cmd_data30_clk_vsel_pins: sdhc1-base-sdhc-vsel-pins {
> +				pinctrl-single,bits = <0x0 0x0 (0x7 << 24)>;
> +			};
> +
> +			gpio0_14_12_pins: sdhc1-dir-gpio-pins {
> +				pinctrl-single,bits = <0x0 (0x1 << 27) (0x7 << 27)>;
> +			};
> +
>  			i2c6_scl: i2c6-scl-pins {
>  				pinctrl-single,bits = <0x4 0x2 0x7>;
>  			};
> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2162a-clearfog.dts b/arch/arm64/boot/dts/freescale/fsl-lx2162a-clearfog.dts
> index eafef8718a0fe..8920326a06735 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-lx2162a-clearfog.dts
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2162a-clearfog.dts
> @@ -223,6 +223,8 @@ ethernet_phy8: ethernet-phy@15 {
>  };
>  
>  &esdhc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&esdhc0_cd_wp_pins>, <&esdhc0_cmd_data30_clk_vsel_pins>;
>  	sd-uhs-sdr104;
>  	sd-uhs-sdr50;
>  	sd-uhs-sdr25;
> diff --git a/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi b/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi
> index e914291e63a1a..e1344942eaaee 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-lx2162a-sr-som.dtsi
> @@ -30,6 +30,8 @@ &esdhc1 {
>  };
>  
>  &fspi {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&fspi_data74_pins>, <&fspi_data30_pins>, <&fspi_dqs_sck_cs10_pins>;
>  	status = "okay";
>  
>  	flash@0 {
> @@ -80,3 +82,8 @@ rtc@6f {
>  		reg = <0x6f>;
>  	};
>  };
> +
> +&pinmux_i2crv {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gpio0_14_12_pins>;
> +};
>