[PATCH v3 0/4] mtd: spi: spi-nor: rk3399: rock-pi-4: u-boot/next Support SPI NOR Flash in Rock Pi 4 (XTX xt25f32b)

[PATCH v3 0/4] mtd: spi: spi-nor: rk3399: rock-pi-4: u-boot/next Support SPI NOR Flash in Rock Pi 4 (XTX xt25f32b)

[Xavier Drudis Ferran]

The Radxa Rock Pi 4 board is sold from revision 1.4 with a soldered
4Mb SPI NOR Flash. This series allows to use it from U-Boot and boot
from it.

This series applies to u-boot/master.

Changes since v2:

   - rebased on master
 
   - droped 5th path, and enabled CONFIG_SPL_DM_SEQ_ALIAS instead
     to have the flash on spi1 both on SPL and U-Boot proper.

Changes since v1:

   - Changed bus number to 1 in SPL to match U-Boot proper (before it
     was bus 0 in SPL and bus 1 in U-Boot).

   - Generalization. v1 did a first soft_reset in 8_8_8_DTR but that
     wouldn't work since Pratyush Yadav fixed supports_op() in commit
     5752d6ae8daacbd ("spi: spi-mem: add spi_mem_dtr_supports_op()").
     So now we try a few protocols until 1_1_1 is supported by
     controller and device. Might work for more systems.

[PATCH v3 1/4] mtd: spi: spi-nor: Add Rock pi 4b new flash chip

[Xavier Drudis Ferran]

Radxa Rock Pi 4B from version 1.4 on carries a 4MiB XTX Technology Inc
25F32B SPI NOR Flash.  (previous versions had pads where users could
solder different chips).

Add its parameters to spi-nor-ids.c so U-Boot can discover it and
(after further changes) we can boot from SPI.

Note that the Flash is declared to be dual and quad capable because
the datasheet [4] says so but I couldn't try it because in Rock Pi 4
it is not wired for QuadSPI, and rk3399 does not seem to support dual
SPI either (or I couldn't find any register to configure spi1tx and
spi1rx as bidirectional).

But the same Flash part could be used as in quad or dual model in some
other board.

Likewise locks are in the datasheet but untested.

The part has been added to downstream U-Boot [1] and linux [2] [3]:

Changed from v2:

     - no change

Changed from v1:

     - no change

Link: [1] https://github.com/armbian/build/commit/c41cb4c454570127ad3238f6b901fbf3aa773c
Link: [2] https://github.com/radxa/kernel/blob/release-4.4-rockpi4/drivers/mtd/spi-nor/spi-nor.c
Link: [3] https://github.com/radxa/kernel/commit/8216f17965de7bc7ced7092aab0e2bfe16838a4
Link: [4] https://www.xtxtech.com/download/?AId=157


Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>

Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Vignesh R <vigneshr@ti.com>

---
 drivers/mtd/spi/spi-nor-ids.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index 4fe8b0d92c..b09df00bab 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -452,6 +452,9 @@ const struct flash_info spi_nor_ids[] = {
 #ifdef CONFIG_SPI_FLASH_XTX
 	/* XTX Technology (Shenzhen) Limited */
 	{ INFO("xt25f128b", 0x0b4018, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{ INFO("xt25f32b", 0x0b4016, 0, 64 * 1024, 64,
+	       SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ
+	       | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_SST26LOCK) },
 #endif
 	{ },
 };
-- 
2.20.1

[PATCH v3 2/4] rockchip: rk-3399: rock-pi-4: dts: Add XTX SPI NOR 4MiB Flash chip in Rock Pi 4 boards from rev 1.4 on.

[Xavier Drudis Ferran]

Configure Rock Pi 4 to boot from SPI NOR Flash.

Based on flash chip, board documentation and tests, this is the
fastest I could use it.

This seems to be the minimum necessary configuration for Rock Pi 4 to
be able to boot from SPI NOR Flash.

With the next patch, it works to sf probe 1:0, sf read, sf erase, sf
write, sf read and then boot linux and flashrom can write to
it. Sometimes flashrom seems to fail to write when at U-Boot stage
there was no sf read or write, not sure why. Might work better after
a little while off power.

Note: It seems to work with or without CONFIG_SPL_SPI_FLASH_TINY If I
      disable it, and enable CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT I have
      to increase CONFIG_SPL_MAX_SIZE (I set it to 0x2e800) because
      the SPL grew over 1200 bytes bigger. It might work better, or
      just be chance, but it boots from SPI 0.3s slower. So I leave
      this out of this patch.

Changed since v2:

    - added CONFIG_SPL_DM_SEQ_ALIAS to keep spi1 numeration consistent
      between SPL and U-Boot

Changed since v1: 

    - include CONFIG_SF_DEFAULT_BUS=1 now that numeration
      will be unified between U-Boot and SPL (at patch 5/5)
 
Cc: Simon Glass <sjg@chromium.org>
Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
Cc: Kever Yang <kever.yang@rock-chips.com>

Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
---
 arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi | 18 ++++++++++++++++++
 configs/rock-pi-4-rk3399_defconfig        | 22 ++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi b/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
index c17e769f64..4c2fe8f6bc 100644
--- a/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
+++ b/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
@@ -15,3 +15,21 @@
 &vdd_log {
 	regulator-init-microvolt = <950000>;
 };
+
+&spi1 {
+	status = "okay";
+	spi-max-frequency = <40000000>;
+	spi-activate-delay = <12000>; /* 12 ms */
+
+	norflash: flash@0 {
+		compatible = "rockchip,spidev", "jedec,spi-nor";
+		reg = <0>;
+
+		spi-max-frequency = <40000000>;
+		spi-cpha;
+		spi-cpol;
+
+		status = "okay";
+		u-boot,dm-pre-reloc;
+	};
+};
diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
index cf2e9fbde3..d5fadcb64f 100644
--- a/configs/rock-pi-4-rk3399_defconfig
+++ b/configs/rock-pi-4-rk3399_defconfig
@@ -10,6 +10,8 @@ CONFIG_ROCKCHIP_RK3399=y
 CONFIG_TARGET_EVB_RK3399=y
 CONFIG_DEBUG_UART_BASE=0xFF1A0000
 CONFIG_DEBUG_UART_CLOCK=24000000
+CONFIG_SPL_SPI_FLASH_SUPPORT=y
+CONFIG_SPL_SPI=y
 CONFIG_SYS_LOAD_ADDR=0x800800
 CONFIG_DEBUG_UART=y
 CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
@@ -28,6 +30,10 @@ CONFIG_SPL_BSS_MAX_SIZE=0x2000
 CONFIG_SPL_STACK=0x400000
 CONFIG_SPL_STACK_R=y
 CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
+CONFIG_SPL_MTD_SUPPORT=y
+CONFIG_SPL_SPI_FLASH_MTD=y
+CONFIG_SPL_SPI_LOAD=y
+CONFIG_SYS_SPI_U_BOOT_OFFS=0xb0000
 CONFIG_TPL=y
 CONFIG_CMD_BOOTZ=y
 CONFIG_CMD_GPT=y
@@ -40,6 +46,7 @@ CONFIG_SPL_OF_CONTROL=y
 CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
 CONFIG_ENV_IS_IN_MMC=y
 CONFIG_SYS_RELOC_GD_ENV_ADDR=y
+CONFIG_SPL_DM_SEQ_ALIAS=y
 CONFIG_ROCKCHIP_GPIO=y
 CONFIG_SYS_I2C_ROCKCHIP=y
 CONFIG_MISC=y
@@ -48,21 +55,36 @@ CONFIG_MMC_DW=y
 CONFIG_MMC_DW_ROCKCHIP=y
 CONFIG_MMC_SDHCI=y
 CONFIG_MMC_SDHCI_ROCKCHIP=y
+CONFIG_MTD=y
+CONFIG_DM_MTD=y
+CONFIG_SF_DEFAULT_BUS=1
+CONFIG_SF_DEFAULT_MODE=0x3
+CONFIG_SF_DEFAULT_SPEED=40000000
+CONFIG_SPI_FLASH_SFDP_SUPPORT=y
+CONFIG_SPI_FLASH_SOFT_RESET=y
+CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT=y
+CONFIG_SPI_FLASH_XTX=y
+CONFIG_SPI_FLASH_MTD=y
 CONFIG_DM_ETH=y
 CONFIG_ETH_DESIGNWARE=y
 CONFIG_GMAC_ROCKCHIP=y
 CONFIG_NVME_PCI=y
 CONFIG_PCI=y
+CONFIG_SPL_PHY=y
 CONFIG_PHY_ROCKCHIP_INNO_USB2=y
 CONFIG_PHY_ROCKCHIP_TYPEC=y
+CONFIG_DM_PMIC_FAN53555=y
 CONFIG_PMIC_RK8XX=y
 CONFIG_REGULATOR_PWM=y
+CONFIG_SPL_REGULATOR_PWM=y
+CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_REGULATOR_RK8XX=y
 CONFIG_PWM_ROCKCHIP=y
 CONFIG_RAM_RK3399_LPDDR4=y
 CONFIG_DM_RESET=y
 CONFIG_BAUDRATE=1500000
 CONFIG_DEBUG_UART_SHIFT=2
+CONFIG_ROCKCHIP_SPI=y
 CONFIG_SYSRESET=y
 CONFIG_USB=y
 CONFIG_USB_XHCI_HCD=y
-- 
2.20.1

Re: [PATCH v3 2/4] rockchip: rk-3399: rock-pi-4: dts: Add XTX SPI NOR 4MiB Flash chip in Rock Pi 4 boards from rev 1.4 on.

[Kever Yang]

On 2022/7/20 23:41, Xavier Drudis Ferran wrote:
> Configure Rock Pi 4 to boot from SPI NOR Flash.
>
> Based on flash chip, board documentation and tests, this is the
> fastest I could use it.
>
> This seems to be the minimum necessary configuration for Rock Pi 4 to
> be able to boot from SPI NOR Flash.
>
> With the next patch, it works to sf probe 1:0, sf read, sf erase, sf
> write, sf read and then boot linux and flashrom can write to
> it. Sometimes flashrom seems to fail to write when at U-Boot stage
> there was no sf read or write, not sure why. Might work better after
> a little while off power.
>
> Note: It seems to work with or without CONFIG_SPL_SPI_FLASH_TINY If I
>        disable it, and enable CONFIG_SPL_SPI_FLASH_SFDP_SUPPORT I have
>        to increase CONFIG_SPL_MAX_SIZE (I set it to 0x2e800) because
>        the SPL grew over 1200 bytes bigger. It might work better, or
>        just be chance, but it boots from SPI 0.3s slower. So I leave
>        this out of this patch.
>
> Changed since v2:
>
>      - added CONFIG_SPL_DM_SEQ_ALIAS to keep spi1 numeration consistent
>        between SPL and U-Boot
>
> Changed since v1:
>
>      - include CONFIG_SF_DEFAULT_BUS=1 now that numeration
>        will be unified between U-Boot and SPL (at patch 5/5)
>   
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu>
> Cc: Kever Yang <kever.yang@rock-chips.com>
>
> Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi | 18 ++++++++++++++++++
>   configs/rock-pi-4-rk3399_defconfig        | 22 ++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>
> diff --git a/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi b/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
> index c17e769f64..4c2fe8f6bc 100644
> --- a/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
> +++ b/arch/arm/dts/rk3399-rock-pi-4-u-boot.dtsi
> @@ -15,3 +15,21 @@
>   &vdd_log {
>   	regulator-init-microvolt = <950000>;
>   };
> +
> +&spi1 {
> +	status = "okay";
> +	spi-max-frequency = <40000000>;
> +	spi-activate-delay = <12000>; /* 12 ms */
> +
> +	norflash: flash@0 {
> +		compatible = "rockchip,spidev", "jedec,spi-nor";
> +		reg = <0>;
> +
> +		spi-max-frequency = <40000000>;
> +		spi-cpha;
> +		spi-cpol;
> +
> +		status = "okay";
> +		u-boot,dm-pre-reloc;
> +	};
> +};
> diff --git a/configs/rock-pi-4-rk3399_defconfig b/configs/rock-pi-4-rk3399_defconfig
> index cf2e9fbde3..d5fadcb64f 100644
> --- a/configs/rock-pi-4-rk3399_defconfig
> +++ b/configs/rock-pi-4-rk3399_defconfig
> @@ -10,6 +10,8 @@ CONFIG_ROCKCHIP_RK3399=y
>   CONFIG_TARGET_EVB_RK3399=y
>   CONFIG_DEBUG_UART_BASE=0xFF1A0000
>   CONFIG_DEBUG_UART_CLOCK=24000000
> +CONFIG_SPL_SPI_FLASH_SUPPORT=y
> +CONFIG_SPL_SPI=y
>   CONFIG_SYS_LOAD_ADDR=0x800800
>   CONFIG_DEBUG_UART=y
>   CONFIG_HAS_CUSTOM_SYS_INIT_SP_ADDR=y
> @@ -28,6 +30,10 @@ CONFIG_SPL_BSS_MAX_SIZE=0x2000
>   CONFIG_SPL_STACK=0x400000
>   CONFIG_SPL_STACK_R=y
>   CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x10000
> +CONFIG_SPL_MTD_SUPPORT=y
> +CONFIG_SPL_SPI_FLASH_MTD=y
> +CONFIG_SPL_SPI_LOAD=y
> +CONFIG_SYS_SPI_U_BOOT_OFFS=0xb0000
>   CONFIG_TPL=y
>   CONFIG_CMD_BOOTZ=y
>   CONFIG_CMD_GPT=y
> @@ -40,6 +46,7 @@ CONFIG_SPL_OF_CONTROL=y
>   CONFIG_OF_SPL_REMOVE_PROPS="pinctrl-0 pinctrl-names clock-names interrupt-parent assigned-clocks assigned-clock-rates assigned-clock-parents"
>   CONFIG_ENV_IS_IN_MMC=y
>   CONFIG_SYS_RELOC_GD_ENV_ADDR=y
> +CONFIG_SPL_DM_SEQ_ALIAS=y
>   CONFIG_ROCKCHIP_GPIO=y
>   CONFIG_SYS_I2C_ROCKCHIP=y
>   CONFIG_MISC=y
> @@ -48,21 +55,36 @@ CONFIG_MMC_DW=y
>   CONFIG_MMC_DW_ROCKCHIP=y
>   CONFIG_MMC_SDHCI=y
>   CONFIG_MMC_SDHCI_ROCKCHIP=y
> +CONFIG_MTD=y
> +CONFIG_DM_MTD=y
> +CONFIG_SF_DEFAULT_BUS=1
> +CONFIG_SF_DEFAULT_MODE=0x3
> +CONFIG_SF_DEFAULT_SPEED=40000000
> +CONFIG_SPI_FLASH_SFDP_SUPPORT=y
> +CONFIG_SPI_FLASH_SOFT_RESET=y
> +CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT=y
> +CONFIG_SPI_FLASH_XTX=y
> +CONFIG_SPI_FLASH_MTD=y
>   CONFIG_DM_ETH=y
>   CONFIG_ETH_DESIGNWARE=y
>   CONFIG_GMAC_ROCKCHIP=y
>   CONFIG_NVME_PCI=y
>   CONFIG_PCI=y
> +CONFIG_SPL_PHY=y
>   CONFIG_PHY_ROCKCHIP_INNO_USB2=y
>   CONFIG_PHY_ROCKCHIP_TYPEC=y
> +CONFIG_DM_PMIC_FAN53555=y
>   CONFIG_PMIC_RK8XX=y
>   CONFIG_REGULATOR_PWM=y
> +CONFIG_SPL_REGULATOR_PWM=y
> +CONFIG_DM_REGULATOR_GPIO=y
>   CONFIG_REGULATOR_RK8XX=y
>   CONFIG_PWM_ROCKCHIP=y
>   CONFIG_RAM_RK3399_LPDDR4=y
>   CONFIG_DM_RESET=y
>   CONFIG_BAUDRATE=1500000
>   CONFIG_DEBUG_UART_SHIFT=2
> +CONFIG_ROCKCHIP_SPI=y
>   CONFIG_SYSRESET=y
>   CONFIG_USB=y
>   CONFIG_USB_XHCI_HCD=y

[PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Xavier Drudis Ferran]

XTX25F32B does not use octal mode and accepts soft reset, despite its
SFDP tables. Soft reset at U-Boot exit seems to be required to write
to /dev/mtd0 from flashrom in linux. Soft reset at U-Boot start seems
to help booting from SPI (at least with the dts properties I'm using).

The first soft reset is done before the flash id is read and the
fixups can be called, and v1 of this patch would now fail in Rock Pi 4
because of unsupported operation if tried with
SNOR_PROTO_8_8_8_DTR. This was since commit 5752d6ae8daa ("spi:
spi-mem: add spi_mem_dtr_supports_op()") by Pratyush Yadav.

So try a few modes until SNOR_PROTO_1_1_1 works and then remember the
reset_proto.  This tries to be useful for other boards, but I still
don't know any other that needs it and what would work there.

Changed since v2: 

   - none (rebase)

Changed since v1:

   - Generalization. First soft reset with SNOR_PROTO_8_8_8_DTR stopped
     working since the improvement by Pratyush Yadav.
     Instead of trying 8_8_8_DTR always at first reset, and force
     1_1_1 once XTX25F32B is detected (so, for last reset) do
     a generic loop for any chip trying protocols until one is
     supported and starting by the 8_8_8_DTR protocol (the one previously
     used). This leverages the fixed supports_op() to take into account
     controller and device.

fixups can be called, and it would now fail in Rock Pi 4 because of
unsupported operation if tried with SNOR_PROTO_8_8_8_DTR. So try a few
modes until SNOR_PROTO_1_1_1 works and then remember the reset_proto.
This tries to be useful for other boards, but I still don't know any
other that needs it and what would work there.

Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>

Cc: Jagan Teki <jagan@amarulasolutions.com>
Cc: Vignesh R <vigneshr@ti.com>

---
 drivers/mtd/spi/spi-nor-core.c | 98 ++++++++++++++++++++++++++++++----
 include/linux/mtd/spi-nor.h    |  5 ++
 2 files changed, 92 insertions(+), 11 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 8a226a7af5..17f0cb51fb 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -3575,6 +3575,18 @@ static struct spi_nor_fixups mt35xu512aba_fixups = {
 };
 #endif /* CONFIG_SPI_FLASH_MT35XU */
 
+#ifdef CONFIG_SPI_FLASH_XTX
+static void xtx25f32b_post_sfdp_fixup(struct spi_nor *nor,
+				      struct spi_nor_flash_parameter *params)
+{
+	nor->flags |= SNOR_F_SOFT_RESET;
+}
+
+static struct spi_nor_fixups xtx25f32b_fixups = {
+	.post_sfdp = xtx25f32b_post_sfdp_fixup,
+};
+#endif
+
 #if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX)
 /**
  * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes.
@@ -3733,6 +3745,53 @@ static int spi_nor_init(struct spi_nor *nor)
 }
 
 #ifdef CONFIG_SPI_FLASH_SOFT_RESET
+/**
+ * spi_nor_soft_reset_setup_op() - Initial setup of op for soft
+ * reset. Initializes nor->reset_proto iff needed.
+ *
+ * @nor:	the spi_nor structure nor->reset_proto will be used or if
+ *		null, it'll be assigned the used protocol, from a few
+ *		protocols that get tried for device and controller
+ *		support.
+ *
+ * @opcode	operation code for soft reset (or soft reset enable,
+ *		we currently assume they work the same in all systems)
+ *
+ * @op:	pointer to operation struct to set up for reset or reset
+ *		enable.
+ */
+static void spi_nor_soft_reset_setup_op(struct spi_nor *nor, u16 opcode, struct spi_mem_op *op)
+{
+	enum spi_nor_protocol reset_proto;
+	/**
+	 *  admitedly arbitrary list, to be improved if there's
+	 *  new knowledge of what works in different systems
+	 */
+	enum spi_nor_protocol reset_proto_candidates[] = {
+		SNOR_PROTO_8_8_8_DTR, SNOR_PROTO_1_1_1_DTR, SNOR_PROTO_8_8_8,
+		SNOR_PROTO_4_4_4,  SNOR_PROTO_2_2_2, SNOR_PROTO_1_1_1
+	};
+
+	if (nor->reset_proto) {
+		*op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+						    SPI_MEM_OP_NO_DUMMY,
+						    SPI_MEM_OP_NO_ADDR,
+						    SPI_MEM_OP_NO_DATA);
+		spi_nor_setup_op(nor, op, nor->reset_proto);
+	} else {
+		for (int i = 0; i < ARRAY_SIZE(reset_proto_candidates) && !nor->reset_proto ; i++) {
+			reset_proto = reset_proto_candidates[i];
+			*op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+							    SPI_MEM_OP_NO_DUMMY,
+							    SPI_MEM_OP_NO_ADDR,
+							    SPI_MEM_OP_NO_DATA);
+			spi_nor_setup_op(nor, op, reset_proto);
+			if (spi_mem_supports_op(nor->spi, op))
+				nor->reset_proto = reset_proto;
+		}
+	}
+}
+
 /**
  * spi_nor_soft_reset() - perform the JEDEC Software Reset sequence
  * @nor:	the spi_nor structure
@@ -3756,11 +3815,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 #endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
 	}
 
-	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
-			SPI_MEM_OP_NO_DUMMY,
-			SPI_MEM_OP_NO_ADDR,
-			SPI_MEM_OP_NO_DATA);
-	spi_nor_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+	spi_nor_soft_reset_setup_op(nor, SPINOR_OP_SRSTEN, &op);
 	ret = spi_mem_exec_op(nor->spi, &op);
 	if (ret) {
 		dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
@@ -3771,7 +3826,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
 			SPI_MEM_OP_NO_DUMMY,
 			SPI_MEM_OP_NO_ADDR,
 			SPI_MEM_OP_NO_DATA);
-	spi_nor_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+	spi_nor_setup_op(nor, &op, nor->reset_proto);
 	ret = spi_mem_exec_op(nor->spi, &op);
 	if (ret) {
 		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
@@ -3789,18 +3844,34 @@ out:
 	nor->cmd_ext_type = ext;
 	return ret;
 }
-#endif /* CONFIG_SPI_FLASH_SOFT_RESET */
+
+static bool flash_supports_proto(const struct flash_info *flash,  enum spi_nor_protocol proto)
+{
+	switch (spi_nor_get_protocol_data_nbits(proto)) {
+	case 8:
+		return flash->flags & (spi_nor_protocol_is_dtr(proto)
+				       ? SPI_NOR_OCTAL_DTR_READ
+				       : SPI_NOR_OCTAL_READ);
+	case 4:	return flash->flags & SPI_NOR_QUAD_READ;
+	case 2: return flash->flags & SPI_NOR_DUAL_READ;
+	default:
+		return 1;
+	}
+}
 
 int spi_nor_remove(struct spi_nor *nor)
 {
-#ifdef CONFIG_SPI_FLASH_SOFT_RESET
-	if (nor->info->flags & SPI_NOR_OCTAL_DTR_READ &&
+	if (flash_supports_proto(nor->info, nor->reset_proto) &&
 	    nor->flags & SNOR_F_SOFT_RESET)
 		return spi_nor_soft_reset(nor);
-#endif
-
 	return 0;
 }
+#else /* !CONFIG_SPI_FLASH_SOFT_RESET */
+int spi_nor_remove(struct spi_nor *nor)
+{
+	return 0;
+}
+#endif /* CONFIG_SPI_FLASH_SOFT_RESET */
 
 void spi_nor_set_fixups(struct spi_nor *nor)
 {
@@ -3835,6 +3906,11 @@ void spi_nor_set_fixups(struct spi_nor *nor)
 #if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX)
 	nor->fixups = &macronix_octal_fixups;
 #endif /* SPI_FLASH_MACRONIX */
+
+#ifdef CONFIG_SPI_FLASH_XTX
+	if (!strcmp(nor->info->name, "xt25f32b"))
+		nor->fixups = &xtx25f32b_fixups;
+#endif
 }
 
 int spi_nor_scan(struct spi_nor *nor)
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 2595bad9df..4fc81e7b8d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -501,6 +501,8 @@ struct spi_flash {
  * @read_proto:		the SPI protocol for read operations
  * @write_proto:	the SPI protocol for write operations
  * @reg_proto		the SPI protocol for read_reg/write_reg/erase operations
+ * @reset_proto:	The SPI protocol for soft reset to return to state expected
+ *			by OS before running the OS (at remove time) or at sf probe
  * @cmd_buf:		used by the write_reg
  * @cmd_ext_type:	the command opcode extension for DTR mode.
  * @fixups:		flash-specific fixup hooks.
@@ -546,6 +548,9 @@ struct spi_nor {
 	enum spi_nor_protocol	read_proto;
 	enum spi_nor_protocol	write_proto;
 	enum spi_nor_protocol	reg_proto;
+#ifdef CONFIG_SPI_FLASH_SOFT_RESET
+	enum spi_nor_protocol	reset_proto;
+#endif
 	bool			sst_write_second;
 	u32			flags;
 	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
-- 
2.20.1

Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Pratyush Yadav]

On 20/07/22 05:42PM, Xavier Drudis Ferran wrote:
> XTX25F32B does not use octal mode and accepts soft reset, despite its
> SFDP tables. Soft reset at U-Boot exit seems to be required to write
> to /dev/mtd0 from flashrom in linux. Soft reset at U-Boot start seems
> to help booting from SPI (at least with the dts properties I'm using).
> 
> The first soft reset is done before the flash id is read and the
> fixups can be called, and v1 of this patch would now fail in Rock Pi 4
> because of unsupported operation if tried with
> SNOR_PROTO_8_8_8_DTR. This was since commit 5752d6ae8daa ("spi:
> spi-mem: add spi_mem_dtr_supports_op()") by Pratyush Yadav.
> 
> So try a few modes until SNOR_PROTO_1_1_1 works and then remember the
> reset_proto.  This tries to be useful for other boards, but I still
> don't know any other that needs it and what would work there.
> 
> Changed since v2: 
> 
>    - none (rebase)
> 
> Changed since v1:
> 
>    - Generalization. First soft reset with SNOR_PROTO_8_8_8_DTR stopped
>      working since the improvement by Pratyush Yadav.
>      Instead of trying 8_8_8_DTR always at first reset, and force
>      1_1_1 once XTX25F32B is detected (so, for last reset) do
>      a generic loop for any chip trying protocols until one is
>      supported and starting by the 8_8_8_DTR protocol (the one previously
>      used). This leverages the fixed supports_op() to take into account
>      controller and device.

Please don't put the changelog in the commit message. Put it below the 3 
dashed lines below.

> 
> fixups can be called, and it would now fail in Rock Pi 4 because of
> unsupported operation if tried with SNOR_PROTO_8_8_8_DTR. So try a few
> modes until SNOR_PROTO_1_1_1 works and then remember the reset_proto.
> This tries to be useful for other boards, but I still don't know any
> other that needs it and what would work there.
> 
> Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
> 
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Vignesh R <vigneshr@ti.com>
> 
> ---
>  drivers/mtd/spi/spi-nor-core.c | 98 ++++++++++++++++++++++++++++++----
>  include/linux/mtd/spi-nor.h    |  5 ++
>  2 files changed, 92 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 8a226a7af5..17f0cb51fb 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3575,6 +3575,18 @@ static struct spi_nor_fixups mt35xu512aba_fixups = {
>  };
>  #endif /* CONFIG_SPI_FLASH_MT35XU */
>  
> +#ifdef CONFIG_SPI_FLASH_XTX
> +static void xtx25f32b_post_sfdp_fixup(struct spi_nor *nor,
> +				      struct spi_nor_flash_parameter *params)
> +{
> +	nor->flags |= SNOR_F_SOFT_RESET;
> +}
> +
> +static struct spi_nor_fixups xtx25f32b_fixups = {
> +	.post_sfdp = xtx25f32b_post_sfdp_fixup,
> +};
> +#endif
> +
>  #if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX)
>  /**
>   * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes.
> @@ -3733,6 +3745,53 @@ static int spi_nor_init(struct spi_nor *nor)
>  }
>  
>  #ifdef CONFIG_SPI_FLASH_SOFT_RESET
> +/**
> + * spi_nor_soft_reset_setup_op() - Initial setup of op for soft
> + * reset. Initializes nor->reset_proto iff needed.
> + *
> + * @nor:	the spi_nor structure nor->reset_proto will be used or if
> + *		null, it'll be assigned the used protocol, from a few
> + *		protocols that get tried for device and controller
> + *		support.
> + *
> + * @opcode	operation code for soft reset (or soft reset enable,
> + *		we currently assume they work the same in all systems)
> + *
> + * @op:	pointer to operation struct to set up for reset or reset
> + *		enable.
> + */
> +static void spi_nor_soft_reset_setup_op(struct spi_nor *nor, u16 opcode, struct spi_mem_op *op)
> +{
> +	enum spi_nor_protocol reset_proto;
> +	/**
> +	 *  admitedly arbitrary list, to be improved if there's
> +	 *  new knowledge of what works in different systems
> +	 */
> +	enum spi_nor_protocol reset_proto_candidates[] = {
> +		SNOR_PROTO_8_8_8_DTR, SNOR_PROTO_1_1_1_DTR, SNOR_PROTO_8_8_8,
> +		SNOR_PROTO_4_4_4,  SNOR_PROTO_2_2_2, SNOR_PROTO_1_1_1
> +	};
> +
> +	if (nor->reset_proto) {
> +		*op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
> +						    SPI_MEM_OP_NO_DUMMY,
> +						    SPI_MEM_OP_NO_ADDR,
> +						    SPI_MEM_OP_NO_DATA);
> +		spi_nor_setup_op(nor, op, nor->reset_proto);
> +	} else {
> +		for (int i = 0; i < ARRAY_SIZE(reset_proto_candidates) && !nor->reset_proto ; i++) {
> +			reset_proto = reset_proto_candidates[i];
> +			*op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
> +							    SPI_MEM_OP_NO_DUMMY,
> +							    SPI_MEM_OP_NO_ADDR,
> +							    SPI_MEM_OP_NO_DATA);
> +			spi_nor_setup_op(nor, op, reset_proto);
> +			if (spi_mem_supports_op(nor->spi, op))

This only tells if the _controller_ supports the op. There is no way to 
find out if the flash supports the op without reading SFDP, which would 
need you to either reset the flash or know which mode it is in. So this 
patch is all wrong.

The only way to find out what the flash supports is to read its ID or 
SFDP.

> +				nor->reset_proto = reset_proto;
> +		}
> +	}
> +}
> +
>  /**
>   * spi_nor_soft_reset() - perform the JEDEC Software Reset sequence
>   * @nor:	the spi_nor structure
> @@ -3756,11 +3815,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  #endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
>  	}
>  
> -	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
> -			SPI_MEM_OP_NO_DUMMY,
> -			SPI_MEM_OP_NO_ADDR,
> -			SPI_MEM_OP_NO_DATA);
> -	spi_nor_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +	spi_nor_soft_reset_setup_op(nor, SPINOR_OP_SRSTEN, &op);
>  	ret = spi_mem_exec_op(nor->spi, &op);
>  	if (ret) {
>  		dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
> @@ -3771,7 +3826,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>  			SPI_MEM_OP_NO_DUMMY,
>  			SPI_MEM_OP_NO_ADDR,
>  			SPI_MEM_OP_NO_DATA);
> -	spi_nor_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +	spi_nor_setup_op(nor, &op, nor->reset_proto);
>  	ret = spi_mem_exec_op(nor->spi, &op);
>  	if (ret) {
>  		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> @@ -3789,18 +3844,34 @@ out:
>  	nor->cmd_ext_type = ext;
>  	return ret;
>  }
> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET */
> +
> +static bool flash_supports_proto(const struct flash_info *flash,  enum spi_nor_protocol proto)
> +{
> +	switch (spi_nor_get_protocol_data_nbits(proto)) {
> +	case 8:
> +		return flash->flags & (spi_nor_protocol_is_dtr(proto)
> +				       ? SPI_NOR_OCTAL_DTR_READ
> +				       : SPI_NOR_OCTAL_READ);
> +	case 4:	return flash->flags & SPI_NOR_QUAD_READ;
> +	case 2: return flash->flags & SPI_NOR_DUAL_READ;
> +	default:
> +		return 1;
> +	}
> +}
>  
>  int spi_nor_remove(struct spi_nor *nor)
>  {
> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET
> -	if (nor->info->flags & SPI_NOR_OCTAL_DTR_READ &&
> +	if (flash_supports_proto(nor->info, nor->reset_proto) &&
>  	    nor->flags & SNOR_F_SOFT_RESET)
>  		return spi_nor_soft_reset(nor);
> -#endif
> -
>  	return 0;
>  }
> +#else /* !CONFIG_SPI_FLASH_SOFT_RESET */
> +int spi_nor_remove(struct spi_nor *nor)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_SPI_FLASH_SOFT_RESET */
>  
>  void spi_nor_set_fixups(struct spi_nor *nor)
>  {
> @@ -3835,6 +3906,11 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>  #if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX)
>  	nor->fixups = &macronix_octal_fixups;
>  #endif /* SPI_FLASH_MACRONIX */
> +
> +#ifdef CONFIG_SPI_FLASH_XTX
> +	if (!strcmp(nor->info->name, "xt25f32b"))
> +		nor->fixups = &xtx25f32b_fixups;
> +#endif
>  }
>  
>  int spi_nor_scan(struct spi_nor *nor)
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 2595bad9df..4fc81e7b8d 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -501,6 +501,8 @@ struct spi_flash {
>   * @read_proto:		the SPI protocol for read operations
>   * @write_proto:	the SPI protocol for write operations
>   * @reg_proto		the SPI protocol for read_reg/write_reg/erase operations
> + * @reset_proto:	The SPI protocol for soft reset to return to state expected
> + *			by OS before running the OS (at remove time) or at sf probe
>   * @cmd_buf:		used by the write_reg
>   * @cmd_ext_type:	the command opcode extension for DTR mode.
>   * @fixups:		flash-specific fixup hooks.
> @@ -546,6 +548,9 @@ struct spi_nor {
>  	enum spi_nor_protocol	read_proto;
>  	enum spi_nor_protocol	write_proto;
>  	enum spi_nor_protocol	reg_proto;
> +#ifdef CONFIG_SPI_FLASH_SOFT_RESET
> +	enum spi_nor_protocol	reset_proto;
> +#endif
>  	bool			sst_write_second;
>  	u32			flags;
>  	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
> -- 
> 2.20.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Xavier Drudis Ferran]

Thank you for your time looking at the patch. 

El Tue, Jul 26, 2022 at 12:43:06PM +0530, Pratyush Yadav deia:
> 
> Please don't put the changelog in the commit message. Put it below the 3 
> dashed lines below.
>

Sorry. Will try to remember it next time. Not sure there's a next version of
this patch, though. 
 
> 
> This only tells if the _controller_ supports the op. There is no way to 
> find out if the flash supports the op without reading SFDP, which would 
> need you to either reset the flash or know which mode it is in.

Correct. But you can't always read the SFDP if you get the flash in 
some wrong state and try to reset it through a mode that the controller
doesn't support. 

>  So this patch is all wrong.

Then I won't send a 4th version unless someone else thinks it's worth
it and I have more time for it.
 
> The only way to find out what the flash supports is to read its ID or 
> SFDP.
> 

Chicken and egg ? The soft_reset is done before reading SFDP. 
It was done always in octal mode before my patch ("always" means
whenever CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT was enabled). My
patch tries to do it in a mode that the controller supports. 

In my experiments this helps being able to program /dev/mtd0 from
linux with flashrom. If that is working in mainstream U-Boot on a Rock
Pi 4B version 1.4 or later (with XTX flash) for the rest of people,
then I must be testing wrong, what do I know?

Thanks. Have a nice day.

Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Pratyush Yadav]

On 26/07/22 10:27AM, Xavier Drudis Ferran wrote:
> 
> Thank you for your time looking at the patch. 
> 
> El Tue, Jul 26, 2022 at 12:43:06PM +0530, Pratyush Yadav deia:
> > 
> > Please don't put the changelog in the commit message. Put it below the 3 
> > dashed lines below.
> >
> 
> Sorry. Will try to remember it next time. Not sure there's a next version of
> this patch, though. 
>  
> > 
> > This only tells if the _controller_ supports the op. There is no way to 
> > find out if the flash supports the op without reading SFDP, which would 
> > need you to either reset the flash or know which mode it is in.
> 
> Correct. But you can't always read the SFDP if you get the flash in 
> some wrong state and try to reset it through a mode that the controller
> doesn't support. 
> 
> >  So this patch is all wrong.
> 
> Then I won't send a 4th version unless someone else thinks it's worth
> it and I have more time for it.
>  
> > The only way to find out what the flash supports is to read its ID or 
> > SFDP.
> > 
> 
> Chicken and egg ? The soft_reset is done before reading SFDP. 
> It was done always in octal mode before my patch ("always" means
> whenever CONFIG_SPI_FLASH_SOFT_RESET_ON_BOOT was enabled). My
> patch tries to do it in a mode that the controller supports. 

Yes, this is indeed a chicken and egg problem of sorts. The octal mode 
soft reset I added is a hack that gets _some_ flashes working but not 
all. I see 3 possible ways to solve this:

  1. Encode soft reset type in device tree. Read DT to issue the correct 
     type of reset. But if a flash supports multiple stateful modes like 
     4D-4D-4D and 8D-8D-8D, this might not work so well.
  2. Do a hardware reset. This needs the RESET# line to be connected on 
     the board and some way to toggle it. Does not work for boards that 
     do not have this.
  3. Discover the mode the flash is in. The SFDP spec (JESD216D-01) does 
     recommend a way to do this in section "4.5 Command Modes". This can 
     let us find out which type of soft reset command the flash takes by 
     reading SFDP. Of course, this needs the flash to implement the read 
     SFDP command in all modes, which is not mandatory as per spec (at 
     least as per xSPI spec (JESD251), which documents 8D-8D-8D 
     flashes).

There is no one size fits all solution that I can see, so I think we 
need to do all 3 to get full coverage of all cases. But start with the 
one that fixes your use case and is easiest for you. I would guess that 
is point 2.

And all this does not even fix the problem of flashes that do not 
support 1S-1S-1S mode at all. There are some flashes out there that 
simply only support 8D-8D-8D mode. For those flashes, a reset would 
still put them in 8D-8D-8D mode. To support those you need to discover 
the flash mode and then initialize the flash in that mode itself, 
without resetting.

I would recommending working with the Linux SPI NOR subsystem first. 
That has more eyes looking at it so it would be easier to come up with a 
good solution. Then you can backport it to U-Boot.

> 
> In my experiments this helps being able to program /dev/mtd0 from
> linux with flashrom. If that is working in mainstream U-Boot on a Rock
> Pi 4B version 1.4 or later (with XTX flash) for the rest of people,
> then I must be testing wrong, what do I know?
> 
> Thanks. Have a nice day.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [SPAM] Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Xavier Drudis Ferran]

El Tue, Jul 26, 2022 at 02:39:09PM +0530, Pratyush Yadav deia:
> 
> Yes, this is indeed a chicken and egg problem of sorts. The octal mode 
> soft reset I added is a hack that gets _some_ flashes working but not 
> all. I see 3 possible ways to solve this:
> 

But it was a hack that solved a problem. We still have hunger, war and
disease, but we have some more flashes working. That's some good,
isn't it?

>   1. Encode soft reset type in device tree. Read DT to issue the correct 
>      type of reset. But if a flash supports multiple stateful modes like 
>      4D-4D-4D and 8D-8D-8D, this might not work so well.

So must flash chips always be listed in dts ?
Can some boards have a socket and users plug different flashes to it ? 
Might it then not work so well also ?
My idea was try what you can with the controller you have because you 
know more what controller you have than what flash you have (until you
can ask your flash who it is and what it looks like). This is only active when 
a nondefault kconfig is enabled, anyway.

>   2. Do a hardware reset. This needs the RESET# line to be connected on 
>      the board and some way to toggle it. Does not work for boards that 
>      do not have this.

ACK.
Unfortunately not in my case[1]. 

>   3. Discover the mode the flash is in. The SFDP spec (JESD216D-01) does 
>      recommend a way to do this in section "4.5 Command Modes". This can 
>      let us find out which type of soft reset command the flash takes by 
>      reading SFDP. Of course, this needs the flash to implement the read 
>      SFDP command in all modes, which is not mandatory as per spec (at 
>      least as per xSPI spec (JESD251), which documents 8D-8D-8D 
>      flashes).
>

I think my XTX supports the soft reset according to the manual
(commands 66 and 99), but the SFDP info does not include info about
soft reset. I might misremember though.
 
> There is no one size fits all solution that I can see, so I think we 
> need to do all 3 to get full coverage of all cases. But start with the 
> one that fixes your use case and is easiest for you. I would guess that 
> is point 2.
>

My XT25F32B-S has no hardware reset pin. The manual[2] says so in a sentence, 
in page 49 (out of context) and the pinout is : 
   
   - Chip Select Input (CS#)
   - Data Output(SO)  or I/O 1 (IO1)
   - Write Protect Input (WP#), or I/O2 (IO2)
   - Ground (VSS)
   - Data Input (SI), or I/O 0 (IO0)
   - Serial Clock Input (SCLK)
   - Hold Input (HOLD#)
   - Power Supply (VCC)
    
And of course the board does not have a line for the nonexistent pin,
so no, the closest to hardware reset would be board poweroff.  SI, SO,
CS and SCLK are connected to a 40 pin connector though, so I can
ground them or pull them up or something. But no use.

> And all this does not even fix the problem of flashes that do not 
> support 1S-1S-1S mode at all. There are some flashes out there that 
> simply only support 8D-8D-8D mode. For those flashes, a reset would 
> still put them in 8D-8D-8D mode. To support those you need to discover 
> the flash mode and then initialize the flash in that mode itself, 
> without resetting.
> 

Well, maybe we can limit ourselves to solve solvable problems... That
should keep us busy for a while...

> I would recommending working with the Linux SPI NOR subsystem first. 
> That has more eyes looking at it so it would be easier to come up with a 
> good solution. Then you can backport it to U-Boot.
>

Thanks for the advice. Makes sense I guess. I'll see if I have the time and
motivation. 

[1] https://dl.radxa.com/rockpi4/docs/hw/rockpi4/rockpi4b_plus_v16_sch_20200628.pdf
[2] https://www.xtxtech.com/download/?AId=157

Re: [PATCH v3 3/4] spi-nor: Adapt soft reset to XTX25F32B in Rock Pi 4 rev 1.4

[Kever Yang]

Hi Jagan,

     This patchset goes into my todo list, but I'm not very good at SPI 
module, how do you think about this patchset?

     I will happy to merge this if there is no regression for other SPI 
NOR device.


Thanks,

  - Kever

On 2022/7/20 23:42, Xavier Drudis Ferran wrote:
> XTX25F32B does not use octal mode and accepts soft reset, despite its
> SFDP tables. Soft reset at U-Boot exit seems to be required to write
> to /dev/mtd0 from flashrom in linux. Soft reset at U-Boot start seems
> to help booting from SPI (at least with the dts properties I'm using).
>
> The first soft reset is done before the flash id is read and the
> fixups can be called, and v1 of this patch would now fail in Rock Pi 4
> because of unsupported operation if tried with
> SNOR_PROTO_8_8_8_DTR. This was since commit 5752d6ae8daa ("spi:
> spi-mem: add spi_mem_dtr_supports_op()") by Pratyush Yadav.
>
> So try a few modes until SNOR_PROTO_1_1_1 works and then remember the
> reset_proto.  This tries to be useful for other boards, but I still
> don't know any other that needs it and what would work there.
>
> Changed since v2:
>
>     - none (rebase)
>
> Changed since v1:
>
>     - Generalization. First soft reset with SNOR_PROTO_8_8_8_DTR stopped
>       working since the improvement by Pratyush Yadav.
>       Instead of trying 8_8_8_DTR always at first reset, and force
>       1_1_1 once XTX25F32B is detected (so, for last reset) do
>       a generic loop for any chip trying protocols until one is
>       supported and starting by the 8_8_8_DTR protocol (the one previously
>       used). This leverages the fixed supports_op() to take into account
>       controller and device.
>
> fixups can be called, and it would now fail in Rock Pi 4 because of
> unsupported operation if tried with SNOR_PROTO_8_8_8_DTR. So try a few
> modes until SNOR_PROTO_1_1_1 works and then remember the reset_proto.
> This tries to be useful for other boards, but I still don't know any
> other that needs it and what would work there.
>
> Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>
>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Vignesh R <vigneshr@ti.com>
>
> ---
>   drivers/mtd/spi/spi-nor-core.c | 98 ++++++++++++++++++++++++++++++----
>   include/linux/mtd/spi-nor.h    |  5 ++
>   2 files changed, 92 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 8a226a7af5..17f0cb51fb 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -3575,6 +3575,18 @@ static struct spi_nor_fixups mt35xu512aba_fixups = {
>   };
>   #endif /* CONFIG_SPI_FLASH_MT35XU */
>   
> +#ifdef CONFIG_SPI_FLASH_XTX
> +static void xtx25f32b_post_sfdp_fixup(struct spi_nor *nor,
> +				      struct spi_nor_flash_parameter *params)
> +{
> +	nor->flags |= SNOR_F_SOFT_RESET;
> +}
> +
> +static struct spi_nor_fixups xtx25f32b_fixups = {
> +	.post_sfdp = xtx25f32b_post_sfdp_fixup,
> +};
> +#endif
> +
>   #if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX)
>   /**
>    * spi_nor_macronix_octal_dtr_enable() - Enable octal DTR on Macronix flashes.
> @@ -3733,6 +3745,53 @@ static int spi_nor_init(struct spi_nor *nor)
>   }
>   
>   #ifdef CONFIG_SPI_FLASH_SOFT_RESET
> +/**
> + * spi_nor_soft_reset_setup_op() - Initial setup of op for soft
> + * reset. Initializes nor->reset_proto iff needed.
> + *
> + * @nor:	the spi_nor structure nor->reset_proto will be used or if
> + *		null, it'll be assigned the used protocol, from a few
> + *		protocols that get tried for device and controller
> + *		support.
> + *
> + * @opcode	operation code for soft reset (or soft reset enable,
> + *		we currently assume they work the same in all systems)
> + *
> + * @op:	pointer to operation struct to set up for reset or reset
> + *		enable.
> + */
> +static void spi_nor_soft_reset_setup_op(struct spi_nor *nor, u16 opcode, struct spi_mem_op *op)
> +{
> +	enum spi_nor_protocol reset_proto;
> +	/**
> +	 *  admitedly arbitrary list, to be improved if there's
> +	 *  new knowledge of what works in different systems
> +	 */
> +	enum spi_nor_protocol reset_proto_candidates[] = {
> +		SNOR_PROTO_8_8_8_DTR, SNOR_PROTO_1_1_1_DTR, SNOR_PROTO_8_8_8,
> +		SNOR_PROTO_4_4_4,  SNOR_PROTO_2_2_2, SNOR_PROTO_1_1_1
> +	};
> +
> +	if (nor->reset_proto) {
> +		*op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
> +						    SPI_MEM_OP_NO_DUMMY,
> +						    SPI_MEM_OP_NO_ADDR,
> +						    SPI_MEM_OP_NO_DATA);
> +		spi_nor_setup_op(nor, op, nor->reset_proto);
> +	} else {
> +		for (int i = 0; i < ARRAY_SIZE(reset_proto_candidates) && !nor->reset_proto ; i++) {
> +			reset_proto = reset_proto_candidates[i];
> +			*op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
> +							    SPI_MEM_OP_NO_DUMMY,
> +							    SPI_MEM_OP_NO_ADDR,
> +							    SPI_MEM_OP_NO_DATA);
> +			spi_nor_setup_op(nor, op, reset_proto);
> +			if (spi_mem_supports_op(nor->spi, op))
> +				nor->reset_proto = reset_proto;
> +		}
> +	}
> +}
> +
>   /**
>    * spi_nor_soft_reset() - perform the JEDEC Software Reset sequence
>    * @nor:	the spi_nor structure
> @@ -3756,11 +3815,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>   #endif /* SPI_NOR_BOOT_SOFT_RESET_EXT_INVERT */
>   	}
>   
> -	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 0),
> -			SPI_MEM_OP_NO_DUMMY,
> -			SPI_MEM_OP_NO_ADDR,
> -			SPI_MEM_OP_NO_DATA);
> -	spi_nor_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +	spi_nor_soft_reset_setup_op(nor, SPINOR_OP_SRSTEN, &op);
>   	ret = spi_mem_exec_op(nor->spi, &op);
>   	if (ret) {
>   		dev_warn(nor->dev, "Software reset enable failed: %d\n", ret);
> @@ -3771,7 +3826,7 @@ static int spi_nor_soft_reset(struct spi_nor *nor)
>   			SPI_MEM_OP_NO_DUMMY,
>   			SPI_MEM_OP_NO_ADDR,
>   			SPI_MEM_OP_NO_DATA);
> -	spi_nor_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +	spi_nor_setup_op(nor, &op, nor->reset_proto);
>   	ret = spi_mem_exec_op(nor->spi, &op);
>   	if (ret) {
>   		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> @@ -3789,18 +3844,34 @@ out:
>   	nor->cmd_ext_type = ext;
>   	return ret;
>   }
> -#endif /* CONFIG_SPI_FLASH_SOFT_RESET */
> +
> +static bool flash_supports_proto(const struct flash_info *flash,  enum spi_nor_protocol proto)
> +{
> +	switch (spi_nor_get_protocol_data_nbits(proto)) {
> +	case 8:
> +		return flash->flags & (spi_nor_protocol_is_dtr(proto)
> +				       ? SPI_NOR_OCTAL_DTR_READ
> +				       : SPI_NOR_OCTAL_READ);
> +	case 4:	return flash->flags & SPI_NOR_QUAD_READ;
> +	case 2: return flash->flags & SPI_NOR_DUAL_READ;
> +	default:
> +		return 1;
> +	}
> +}
>   
>   int spi_nor_remove(struct spi_nor *nor)
>   {
> -#ifdef CONFIG_SPI_FLASH_SOFT_RESET
> -	if (nor->info->flags & SPI_NOR_OCTAL_DTR_READ &&
> +	if (flash_supports_proto(nor->info, nor->reset_proto) &&
>   	    nor->flags & SNOR_F_SOFT_RESET)
>   		return spi_nor_soft_reset(nor);
> -#endif
> -
>   	return 0;
>   }
> +#else /* !CONFIG_SPI_FLASH_SOFT_RESET */
> +int spi_nor_remove(struct spi_nor *nor)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_SPI_FLASH_SOFT_RESET */
>   
>   void spi_nor_set_fixups(struct spi_nor *nor)
>   {
> @@ -3835,6 +3906,11 @@ void spi_nor_set_fixups(struct spi_nor *nor)
>   #if CONFIG_IS_ENABLED(SPI_FLASH_MACRONIX)
>   	nor->fixups = &macronix_octal_fixups;
>   #endif /* SPI_FLASH_MACRONIX */
> +
> +#ifdef CONFIG_SPI_FLASH_XTX
> +	if (!strcmp(nor->info->name, "xt25f32b"))
> +		nor->fixups = &xtx25f32b_fixups;
> +#endif
>   }
>   
>   int spi_nor_scan(struct spi_nor *nor)
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 2595bad9df..4fc81e7b8d 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -501,6 +501,8 @@ struct spi_flash {
>    * @read_proto:		the SPI protocol for read operations
>    * @write_proto:	the SPI protocol for write operations
>    * @reg_proto		the SPI protocol for read_reg/write_reg/erase operations
> + * @reset_proto:	The SPI protocol for soft reset to return to state expected
> + *			by OS before running the OS (at remove time) or at sf probe
>    * @cmd_buf:		used by the write_reg
>    * @cmd_ext_type:	the command opcode extension for DTR mode.
>    * @fixups:		flash-specific fixup hooks.
> @@ -546,6 +548,9 @@ struct spi_nor {
>   	enum spi_nor_protocol	read_proto;
>   	enum spi_nor_protocol	write_proto;
>   	enum spi_nor_protocol	reg_proto;
> +#ifdef CONFIG_SPI_FLASH_SOFT_RESET
> +	enum spi_nor_protocol	reset_proto;
> +#endif
>   	bool			sst_write_second;
>   	u32			flags;
>   	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];

[PATCH v3 4/4] spi: spi-mem: Allow address 0 for SPI mem operations

[Xavier Drudis Ferran]

Trying to boot my Rock Pi 4B from its XTX SPI NOR Flash failed when my
custom compiled TF-A had a load address of 0.

The same TF-A booted correctly from MMC.

Add a local variable to spi_mem_exec_op() to determine operation
direction, instead of testing rx_buf or tx_buf for null value, so that
a buffer at RAM address 0 is accepted.

This commit also cuts short a debug dump of the image loaded to show
only the first 0x1000 and the last 0x100 bytes, and not swamping the
serial log. When adding the #define DEBUG to the .c file one can
change these limits at the same time if they don't fit.

Changed since v2: 

    - no changes

Changed since v1: 

    - no changes

Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>

Cc: Jagan Teki <jagan@amarulasolutions.com>

---
 drivers/spi/spi-mem.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 9c1ede1b61..4dc90addb3 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -21,6 +21,8 @@
 #include <spi.h>
 #include <spi-mem.h>
 #include <dm/device_compat.h>
+#define DEBUG_DUMP_START_LENGTH	0x1000
+#define DEBUG_DUMP_END_LENGTH	0x100
 #endif
 
 #ifndef __UBOOT__
@@ -373,12 +375,21 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	if (msg.actual_length != totalxferlen)
 		return -EIO;
 #else
+	enum spi_mem_data_dir dir = SPI_MEM_NO_DATA;
 
 	if (op->data.nbytes) {
-		if (op->data.dir == SPI_MEM_DATA_IN)
+		dir = op->data.dir;
+		if (dir == SPI_MEM_DATA_IN) {
 			rx_buf = op->data.buf.in;
-		else
+		} else {
 			tx_buf = op->data.buf.out;
+			/**
+			 * keep old behaviour, to assume SPI_MEM_DATA_OUT
+			 * if ever data.nbytes!=0 but data.dir==SPI_MEM_NO_DATA
+			 * (hopefully never)
+			 */
+			dir = SPI_MEM_DATA_OUT;
+		}
 	}
 
 	op_len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
@@ -410,7 +421,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	/* 1st transfer: opcode + address + dummy cycles */
 	flag = SPI_XFER_BEGIN;
 	/* Make sure to set END bit if no tx or rx data messages follow */
-	if (!tx_buf && !rx_buf)
+	if (dir == SPI_MEM_NO_DATA)
 		flag |= SPI_XFER_END;
 
 	ret = spi_xfer(slave, op_len * 8, op_buf, NULL, flag);
@@ -418,7 +429,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		return ret;
 
 	/* 2nd transfer: rx or tx data path */
-	if (tx_buf || rx_buf) {
+	if (dir != SPI_MEM_NO_DATA) {
 		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
 			       rx_buf, SPI_XFER_END);
 		if (ret)
@@ -430,10 +441,17 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	for (i = 0; i < pos; i++)
 		debug("%02x ", op_buf[i]);
 	debug("| [%dB %s] ",
-	      tx_buf || rx_buf ? op->data.nbytes : 0,
-	      tx_buf || rx_buf ? (tx_buf ? "out" : "in") : "-");
-	for (i = 0; i < op->data.nbytes; i++)
-		debug("%02x ", tx_buf ? tx_buf[i] : rx_buf[i]);
+	      op->data.nbytes,
+	      dir == SPI_MEM_DATA_IN ? "in" : (dir == SPI_MEM_DATA_OUT ? "out" : "-"));
+	for (i = 0; i < op->data.nbytes && i < DEBUG_DUMP_START_LENGTH ; i++)
+		debug("%02x ", dir == SPI_MEM_DATA_OUT ? tx_buf[i] : rx_buf[i]);
+	if (op->data.nbytes > DEBUG_DUMP_END_LENGTH && op->data.nbytes > DEBUG_DUMP_START_LENGTH &&
+	    i < op->data.nbytes - DEBUG_DUMP_END_LENGTH) {
+		debug(" ... ");
+		i = op->data.nbytes - DEBUG_DUMP_END_LENGTH;
+	}
+	for (; i < op->data.nbytes  ; i++)
+		debug("%02x ", dir == SPI_MEM_DATA_OUT ? tx_buf[i] : rx_buf[i]);
 	debug("[ret %d]\n", ret);
 
 	if (ret < 0)
-- 
2.20.1