[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aHUVc5SV3yzhDBf6@lizhi-Precision-Tower-5810>
Date: Mon, 14 Jul 2025 10:34:27 -0400
From: Frank Li <Frank.li@....com>
To: Josua Mayer <josua@...id-run.com>
Cc: Shawn Guo <shawnguo@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Carlos Song <carlos.song@....com>,
Jon Nettleton <jon@...id-run.com>,
Rabeeh Khoury <rabeeh@...id-run.com>,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux@...tq-group.com,
stable@...r.kernel.org
Subject: Re: [PATCH v2] Revert "arm64: dts: lx2160a: add pinmux and i2c gpio
to support bus recovery"
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. I2C recover also is
important feature.
Frank
>
> 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@...r.kernel.org
> Signed-off-by: Josua Mayer <josua@...id-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@...0000 {
> 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@...0000 {
> 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@...0000 {
> 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@...0000 {
> 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@...0000 {
> 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@...0000 {
> 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@...0000 {
> 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@...0000 {
> 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@...10012c {
> - 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@...000000 {
> compatible = "fsl,qoriq-mc";
> reg = <0x00000008 0x0c000000 0 0x40>,
>
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250710-lx2160-sd-cd-00bf38ae169e
>
> Best regards,
> --
> Josua Mayer <josua@...id-run.com>
>
Powered by blists - more mailing lists