[PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Tudor Ambarus]

There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
when configured in Octal DTR mode. The byte order of 16-bit words is
swapped when read or written in Octal Double Transfer Rate (DTR) mode
compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
read back D1 D0 D3 D2. Swapping the bytes is a bad design decision because
it may introduce some endianness problems. It can affect the boot sequence
if the entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1
mode. So we must swap the bytes back to have the same byte order as in STR
modes. Fortunately there are controllers that can swap the bytes back at
runtime, addressing the flash's endiannesses requirements.
If the controllers are not capable of swapping the bytes, the protocol is
downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the swapping
of the bytes is always done regardless if it's a data or register access,
so that we comply with the JESD216 requirements: "Byte order of 16-bit
words is swapped when read in 8D-8D-8D mode compared to 1-1-1".

Tested with atmel-quadspi driver, both in 8D-8D-8D mode and 1-1-1 mode
(via the protocol downgrade).

This patch set depends on the SPI MEM changes from linux-mtd/mtd/next and
also on some SPI NOR patches that are waiting for R-b tags. You can find
a branch if you want to test it at:

To github.com:ambarus/linux-0day.git
 * [new branch]                spi-nor/next-mx-byte-swap-v2 -> spi-nor/next-mx-byte-swap-v2

v2:
- all: s/bswap16/swab16 to comply with linux naming scheme
- SPI MEM: add dtr_swab16 cap to spi_controller_mem_caps and update
  spi_mem_default_supports_op() to check for it.
- SPI NOR: do not encode swab16 info in the SPI NOR protocol, use a helper
  instead
- SPI NOR: downgrade the SPI NOR protocol in case the SPI Controller does
  not support swab16
- add SPI_NOR_SOFT_RESET and support for mx66lm1g45g

Tudor Ambarus (6):
  spi: spi-mem: Allow specifying the byte order in DTR mode
  mtd: spi-nor: core: Allow specifying the byte order in DTR mode
  mtd: spi-nor: sfdp: Get the 8D-8D-8D byte order from BFPT
  mtd: spi-nor: core: Introduce SPI_NOR_DTR_BSWAP16 no_sfdp_flag
  mtd: spi-nor: core: Introduce SPI_NOR_SOFT_RESET flash_info fixup_flag
  mtd: spi-nor: macronix: Add support for mx66lm1g45g

 drivers/mtd/spi-nor/core.c     |  16 +++-
 drivers/mtd/spi-nor/core.h     |  10 ++-
 drivers/mtd/spi-nor/macronix.c | 130 +++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.c     |   4 +
 drivers/spi/spi-mem.c          |   4 +
 include/linux/spi/spi-mem.h    |   6 ++
 6 files changed, 168 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] spi: spi-mem: Allow specifying the byte order in DTR mode

[Tudor Ambarus]

There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
when configured in Octal DTR mode. The byte order of 16-bit words is
swapped when read or written in Octal Double Transfer Rate (DTR) mode
compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
read back D1 D0 D3 D2. Swapping the bytes is a bad design decision because
it may introduce some endianness problems. It can affect the boot sequence
if the entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1
mode. So we must swap the bytes back to have the same byte order as in STR
modes. Fortunately there are controllers that can swap the bytes back at
runtime, addressing the flash's endiannesses requirements. Provide a way
for the upper layers to specify the byte order in Octal DTR mode.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/spi/spi-mem.c       | 4 ++++
 include/linux/spi/spi-mem.h | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index f38ac31961c9..3b2e586bb6a6 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -171,6 +171,10 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 		if (!spi_mem_controller_is_capable(ctlr, dtr))
 			return false;
 
+		if (op->data.dtr_swab16 &&
+		    !(spi_mem_controller_is_capable(ctlr, dtr_swab16)))
+			return false;
+
 		if (op->cmd.nbytes != 2)
 			return false;
 	} else {
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 2ba044d0d5e5..a1bf51eda31f 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -89,6 +89,8 @@ enum spi_mem_data_dir {
  * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or not
  * @data.buswidth: number of IO lanes used to send/receive the data
  * @data.dtr: whether the data should be sent in DTR mode or not
+ * @data.dtr_swab16: whether the byte order of 16-bit words is swapped when read
+ *		     or written in Octal DTR mode compared to STR mode.
  * @data.ecc: whether error correction is required or not
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
@@ -120,6 +122,7 @@ struct spi_mem_op {
 	struct {
 		u8 buswidth;
 		u8 dtr : 1;
+		u8 dtr_swab16 : 1;
 		u8 ecc : 1;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
@@ -290,10 +293,13 @@ struct spi_controller_mem_ops {
 /**
  * struct spi_controller_mem_caps - SPI memory controller capabilities
  * @dtr: Supports DTR operations
+ * @dtr_swab16: Supports swapping bytes on a 16 bit boundary when configured in
+ *		Octal DTR
  * @ecc: Supports operations with error correction
  */
 struct spi_controller_mem_caps {
 	bool dtr;
+	bool dtr_swab16;
 	bool ecc;
 };
 
-- 
2.25.1

[PATCH v2 2/6] mtd: spi-nor: core: Allow specifying the byte order in DTR mode

[Tudor Ambarus]

Macronix swaps bytes on a 16-bit boundary when configured in Octal DTR.
The byte order of 16-bit words is swapped when read or written in 8D-8D-8D
mode compared to STR modes. Allow operations to specify the byte order in
DTR mode, so that controllers can swap the bytes back at run-time to
address the flash's endianness requirements, if they are capable. If the
controllers are not capable of swapping the bytes, the protocol is
downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the swapping
of the bytes is always done regardless if it's a data or register access,
so that we comply with the JESD216 requirements: "Byte order of 16-bit
words is swapped when read in 8D-8D-8D mode compared to 1-1-1".

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 8 ++++++++
 drivers/mtd/spi-nor/core.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 5de46a786cc5..a69c2813f6fc 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -70,6 +70,13 @@ static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor,
 	}
 }
 
+static inline bool spi_nor_is_octal_dtr_swab16(const struct spi_nor *nor,
+					       enum spi_nor_protocol proto)
+{
+	return (proto == SNOR_PROTO_8_8_8_DTR) &&
+		(nor->flags & SNOR_F_DTR_SWAB16);
+}
+
 /**
  * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op.
  * @nor:		pointer to a 'struct spi_nor'
@@ -105,6 +112,7 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 		op->addr.dtr = true;
 		op->dummy.dtr = true;
 		op->data.dtr = true;
+		op->data.dtr_swab16 = spi_nor_is_octal_dtr_swab16(nor, proto);
 
 		/* 2 bytes per clock cycle in DTR mode. */
 		op->dummy.nbytes *= 2;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index c83d5e75c563..0dcbc7a81e64 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -135,6 +135,7 @@ enum spi_nor_option_flags {
 	SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
 	SNOR_F_SOFT_RESET	= BIT(12),
 	SNOR_F_SWP_IS_VOLATILE	= BIT(13),
+	SNOR_F_DTR_SWAB16	= BIT(14),
 };
 
 struct spi_nor_read_command {
-- 
2.25.1

[PATCH v2 3/6] mtd: spi-nor: sfdp: Get the 8D-8D-8D byte order from BFPT

[Tudor Ambarus]

Parse BFPT in order to retrieve the byte order in 8D-8D-8D mode.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Michael Walle <michael@walle.cc>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/sfdp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index f5432cbd3daf..e049851a57a7 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -579,6 +579,7 @@ int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
 #define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
 #define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
 #define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
+#define BFPT_DWORD18_BYTE_ORDER_SWAPPED		BIT(31)
 
 /**
  * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
@@ -832,6 +833,9 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		return -EOPNOTSUPP;
 	}
 
+	if (bfpt.dwords[BFPT_DWORD(18)] & BFPT_DWORD18_BYTE_ORDER_SWAPPED)
+		nor->flags |= SNOR_F_DTR_SWAB16;
+
 	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
 }
 
-- 
2.25.1

[PATCH v2 4/6] mtd: spi-nor: core: Introduce SPI_NOR_DTR_BSWAP16 no_sfdp_flag

[Tudor Ambarus]

Introduce SPI_NOR_DTR_BSWAP16 flag for flashes that don't define the
mandatory BFPT table. When set it indicates that the byte order of 16-bit
words is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c | 5 ++++-
 drivers/mtd/spi-nor/core.h | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index a69c2813f6fc..d7eebbd01122 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2298,7 +2298,7 @@ static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter *params = nor->params;
 	struct spi_nor_erase_map *map = &params->erase_map;
-	const u8 no_sfdp_flags = nor->info->no_sfdp_flags;
+	const u16 no_sfdp_flags = nor->info->no_sfdp_flags;
 	u8 i, erase_mask;
 
 	if (no_sfdp_flags & SPI_NOR_DUAL_READ) {
@@ -2339,6 +2339,9 @@ static void spi_nor_no_sfdp_init_params(struct spi_nor *nor)
 					SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
 	}
 
+	if (no_sfdp_flags & SPI_NOR_DTR_SWAB16)
+		nor->flags |= SNOR_F_DTR_SWAB16;
+
 	/*
 	 * Sector Erase settings. Sort Erase Types in ascending order, with the
 	 * smallest erase size starting at BIT(0).
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 0dcbc7a81e64..4508bbea5df1 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -468,6 +468,9 @@ struct spi_nor_fixups {
  *   SPI_NOR_OCTAL_READ:      flash supports Octal Read.
  *   SPI_NOR_OCTAL_DTR_READ:  flash supports octal DTR Read.
  *   SPI_NOR_OCTAL_DTR_PP:    flash supports Octal DTR Page Program.
+ *   SPI_NOR_DTR_SWAB16:      the byte order of 16-bit words is swapped when
+ *			      read or written in Octal DTR mode compared to STR
+ *                            mode.
  *
  * @fixup_flags:    flags that indicate support that can be discovered via SFDP
  *                  ideally, but can not be discovered for this particular flash
@@ -507,7 +510,7 @@ struct flash_info {
 #define NO_CHIP_ERASE			BIT(7)
 #define SPI_NOR_NO_FR			BIT(8)
 
-	u8 no_sfdp_flags;
+	u16 no_sfdp_flags;
 #define SPI_NOR_SKIP_SFDP		BIT(0)
 #define SECT_4K				BIT(1)
 #define SECT_4K_PMC			BIT(2)
@@ -516,6 +519,7 @@ struct flash_info {
 #define SPI_NOR_OCTAL_READ		BIT(5)
 #define SPI_NOR_OCTAL_DTR_READ		BIT(6)
 #define SPI_NOR_OCTAL_DTR_PP		BIT(7)
+#define SPI_NOR_DTR_SWAB16		BIT(8)
 
 	u8 fixup_flags;
 #define SPI_NOR_4B_OPCODES		BIT(0)
-- 
2.25.1

[PATCH v2 5/6] mtd: spi-nor: core: Introduce SPI_NOR_SOFT_RESET flash_info fixup_flag

[Tudor Ambarus]

The Soft Reset and Rescue Sequence Support is defined in BFPT_DWORD(16)
starting with JESD216A. The first version of SFDP, JESD216 (April 2011),
defines just the first 9 BFPT DWORDS, thus it does not contain information
about the Software Reset and Rescue Support. Since this support can not
be discovered by parsing the first SFDP version, introduce a flash_info
fixup_flag that will be used either by flashes that define
JESD216 (April 2011) or by flashes that do not define SFDP at all.
In case a flash defines BFPT_DWORD(16) but with wrong values, one should
instead use a post_bfpt() hook and set SNOR_F_SOFT_RESET.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Link: https://lore.kernel.org/r/20220209133656.374903-7-tudor.ambarus@microchip.com
---
 drivers/mtd/spi-nor/core.c | 3 +++
 drivers/mtd/spi-nor/core.h | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index d7eebbd01122..7da8cf559dfd 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2417,6 +2417,9 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
 
 	if (fixup_flags & SPI_NOR_IO_MODE_EN_VOLATILE)
 		nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
+
+	if (fixup_flags & SPI_NOR_SOFT_RESET)
+		nor->flags |= SNOR_F_SOFT_RESET;
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 4508bbea5df1..fa716e467330 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -483,6 +483,8 @@ struct spi_nor_fixups {
  *                            memory size above 128Mib.
  *   SPI_NOR_IO_MODE_EN_VOLATILE: flash enables the best available I/O mode
  *                            via a volatile bit.
+ *   SPI_NOR_SOFT_RESET:      flash supports software reset enable, reset
+ *                            sequence.
  * @mfr_flags:      manufacturer private flags. Used in the manufacturer fixup
  *                  hooks to differentiate support between flashes of the same
  *                  manufacturer.
@@ -524,6 +526,7 @@ struct flash_info {
 	u8 fixup_flags;
 #define SPI_NOR_4B_OPCODES		BIT(0)
 #define SPI_NOR_IO_MODE_EN_VOLATILE	BIT(1)
+#define SPI_NOR_SOFT_RESET		BIT(2)
 
 	u8 mfr_flags;
 
-- 
2.25.1

[PATCH v2 6/6] mtd: spi-nor: macronix: Add support for mx66lm1g45g

[Tudor Ambarus]

mx66lm1g45g supports just 1-1-1, 8-8-8 and 8D-8D-8D modes. There are
versions of mx66lm1g45g which do not support SFDP, thus use
SPI_NOR_SKIP_SFDP. The RDID command issued through the octal peripheral
interface outputs data always in STR mode for whatever reason. Since
8D-8D-8S is not common, avoid reading the ID when enabling the octal dtr
mode. Instead, read back the CR2 to check if the switch was successful.
Tested in 1-1-1 and 8d-8d-8d modes using sama7g5 QSPI IP.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Link: https://lore.kernel.org/r/20220209133656.374903-8-tudor.ambarus@microchip.com
---
 drivers/mtd/spi-nor/macronix.c | 130 +++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index c267cbcc7f1d..c9b9536f93f2 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,128 @@
 
 #include "core.h"
 
+#define MACRONIX_NOR_OP_WRITE_CR2		0x72	/* Write Configuration Register 2 */
+#define MACRONIX_NOR_OP_DTR_RD			0xee	/* Octa DTR Read Opcode */
+
+#define MACRONIX_NOR_REG_CR2_MODE_ADDR		0	/* Address of Mode Enable in CR2 */
+#define MACRONIX_NOR_REG_CR2_DTR_OPI_ENABLE	BIT(1)	/* DTR OPI Enable */
+#define MACRONIX_NOR_REG_CR2_SPI		0	/* SPI Enable */
+
+/* Macronix SPI NOR flash operations. */
+#define MACRONIX_NOR_WRITE_CR2_OP(addr, buf, ndata)			\
+	SPI_MEM_OP(SPI_MEM_OP_CMD(MACRONIX_NOR_OP_WRITE_CR2, 0),	\
+		   SPI_MEM_OP_ADDR(4, addr, 0),				\
+		   SPI_MEM_OP_NO_DUMMY,					\
+		   SPI_MEM_OP_DATA_OUT(ndata, buf, 0))
+
+static int macronix_nor_write_cr2(struct spi_nor *nor, u64 addr, u8 val)
+{
+	u8 *buf = nor->bouncebuf;
+	struct spi_mem_op op;
+	unsigned int nbytes;
+	int ret;
+
+	if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
+		/*
+		 * When the flash is configured in 8D-8D-8D mode, halfwords are
+		 * send/received instead of bytes. One byte transactions are not
+		 * allowed in 8D-8D-8D mode, so we're going to send two bytes to
+		 * the flash to pass the 8D-8D-8D sanity checks. Macronix
+		 * ignores the second byte value in case of a one byte register
+		 * writes, we don't care of the value of the second byte.
+		 * When in 8D-8D-8D mode endianness must be configured according
+		 * to the flash requirements. Macronix swaps data bytes on a
+		 * 16-bit boundary, so the SPI NOR subsystem instructs the SPI
+		 * controllers to swap the bytes back to keep compatibility with
+		 * the STR modes. The swap back is always done regardless if
+		 * it's a register or data access. Since Macronix ignores the
+		 * second byte value on register writes and the SPI controllers
+		 * will swap the bytes, we actually have to set the value on the
+		 * second byte and ignore the first.
+		 */
+		buf[1] = val;
+		nbytes = 2;
+
+	} else {
+		buf[0] = val;
+		nbytes = 1;
+	}
+
+	op = (struct spi_mem_op)MACRONIX_NOR_WRITE_CR2_OP(addr, buf, nbytes);
+	spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+	return spi_mem_exec_op(nor->spimem, &op);
+}
+
+static int macronix_nor_octal_dtr_en(struct spi_nor *nor)
+{
+	u8 *buf = nor->bouncebuf;
+	int i, ret;
+
+	ret = macronix_nor_write_cr2(nor, MACRONIX_NOR_REG_CR2_MODE_ADDR,
+				     MACRONIX_NOR_REG_CR2_DTR_OPI_ENABLE);
+	if (ret)
+		return ret;
+
+	/* Read flash ID to make sure the switch was successful. */
+	ret = spi_nor_read_id(nor, 4, 4, buf, SNOR_PROTO_8_8_8_DTR);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < nor->info->id_len; i++)
+		if (buf[i * 2] != nor->info->id[i])
+			return -EINVAL;
+	return 0;
+}
+
+static int macronix_nor_octal_dtr_dis(struct spi_nor *nor)
+{
+	u8 *buf = nor->bouncebuf;
+	int ret;
+
+	ret = macronix_nor_write_cr2(nor, MACRONIX_NOR_REG_CR2_MODE_ADDR,
+				     MACRONIX_NOR_REG_CR2_SPI);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_read_id(nor, 0, 0, buf, SNOR_PROTO_1_1_1);
+	if (ret)
+		return ret;
+
+	if (memcmp(buf, nor->info->id, nor->info->id_len)) {
+		dev_dbg(nor->dev, "Failed to disable 8D-8D-8D mode.\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int macronix_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	return enable ? macronix_nor_octal_dtr_en(nor) :
+			macronix_nor_octal_dtr_dis(nor);
+}
+
+static void mx66lm1g45g_late_init(struct spi_nor *nor)
+{
+	nor->params->octal_dtr_enable = macronix_nor_octal_dtr_enable;
+
+	/* Set the Fast Read settings. */
+	nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+	spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
+				  0, 20, MACRONIX_NOR_OP_DTR_RD,
+				  SNOR_PROTO_8_8_8_DTR);
+
+	nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
+	nor->params->rdsr_dummy = 4;
+	nor->params->rdsr_addr_nbytes = 4;
+}
+
+static struct spi_nor_fixups mx66lm1g45g_fixups = {
+	.late_init = mx66lm1g45g_late_init,
+};
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -100,6 +222,14 @@ static const struct flash_info macronix_nor_parts[] = {
 	{ "mx66u2g45g",	 INFO(0xc2253c, 0, 64 * 1024, 4096)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
 		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+	{ "mx66lm1g45g", INFO(0xc2853b, 0, 64 * 1024, 2048)
+		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SECT_4K |
+			      SPI_NOR_OCTAL_DTR_READ | SPI_NOR_OCTAL_DTR_PP |
+			      SPI_NOR_DTR_SWAB16)
+		FIXUP_FLAGS(SPI_NOR_4B_OPCODES | SPI_NOR_IO_MODE_EN_VOLATILE |
+			    SPI_NOR_SOFT_RESET)
+		.fixups = &mx66lm1g45g_fixups,
+	},
 };
 
 static void macronix_nor_default_init(struct spi_nor *nor)
-- 
2.25.1

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Vignesh Raghavendra]

Hi,

On 11/03/22 1:31 pm, Tudor Ambarus wrote:
> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
> when configured in Octal DTR mode. The byte order of 16-bit words is
> swapped when read or written in Octal Double Transfer Rate (DTR) mode
> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision because
> it may introduce some endianness problems. It can affect the boot sequence
> if the entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1
> mode. So we must swap the bytes back to have the same byte order as in STR
> modes. Fortunately there are controllers that can swap the bytes back at
> runtime, addressing the flash's endiannesses requirements.
> If the controllers are not capable of swapping the bytes, the protocol is
> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the swapping
> of the bytes is always done regardless if it's a data or register access,
> so that we comply with the JESD216 requirements: "Byte order of 16-bit
> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
> 

Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is
quite restrictive IMO.

AFAIK, SFDP standard does not dictate how data should be stored in flash
or how SW should interpret after reading data from flash. It merely
indicates endian-ness compared to 1-1-1 mode.

So, its up to various system SWs like bootloader/Linux to work according
to pre-aligned layout as there is no rule that data needs to be stored
in byte order.

We have two types of controllers:

1. SPI controllers supporting swapping endian-ness on the fly:
-> For such flashes, better choice is to have SWAP option always
enabled. So that data written in 8D-8D-8D mode can be read correctly in
1-1-1 mode and vice-versa.
( I am assuming SWAP option of controller is only effective in 8D-8D-8D
mode and is NOP in 1-1-1 or other modes)

But, its possible that "ROM" or other non-upgradable SWs may choose not
make to use of this SWAP option of HW to keep things simple in which
case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
don't always have knowledge of flash and cannot be forced to have a
constraint to enable byte swap on read.

So, IMO, its best left to system integrators to specify whether or not
SWAP option needs to be enabled (perhaps via DT as its per flash
specific property?)

2.  SPI controllers don't support endian-ness SWAP on the fly:
It is still possible to reliably read and write data as long as its
written and read back in same mode.

De-rating speeds because of absence of this support would mean reduction
of speed by **16 times** (maybe even higher as 8D mode tends to support
higher bus freqs). Swapping bytes in Linux before writing or after
reading is not an option either as it negatively impacts performance.

Asking ROM/bootloaders to swap bytes based on SFDP indication is
restrictive too as it involves boot time penalty and most systems with
OSPI flashes are using them to achieve super fast boot times.

One more case to consider is flashes that dont have SFDP table to
indicate byte order but follow Macronix's convention. In such cases, its
better for SPI NOR layer to be as dumb as possible and not really do any
byte swapping, leaving it up to user space to handle/interpret data
appropriately.

Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
providing flash agnostic way of switching to 8D mode and reading data in
8D mode) would not care about SFDP bit indicating byteorder and its up
to flasher programs to take care of the same

IMO, kernel device drivers should just provide access to underlying HW
and not have too much intelligence to interpret data/take decisions

So, simpler constraint to put is:
Flasher programs should program data in the same mode in which
ROM/bootloder/Linux is expected to read the data on that system.

For Macronix like flashes, if one has a ROM/bootloader that only
supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
please generate a byte-swapped image offline and flash it. Don't impose
penalty on systems that do best to handle this messy situation.
I see this as the only option with least performance penalty.

Regards
Vignesh

[...]

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Tudor.Ambarus]

On 3/15/22 08:08, Vignesh Raghavendra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi,

Hi,

> 
> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
>> when configured in Octal DTR mode. The byte order of 16-bit words is
>> swapped when read or written in Octal Double Transfer Rate (DTR) mode
>> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
>> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
>> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision because
>> it may introduce some endianness problems. It can affect the boot sequence
>> if the entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1
>> mode. So we must swap the bytes back to have the same byte order as in STR
>> modes. Fortunately there are controllers that can swap the bytes back at
>> runtime, addressing the flash's endiannesses requirements.
>> If the controllers are not capable of swapping the bytes, the protocol is
>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the swapping
>> of the bytes is always done regardless if it's a data or register access,
>> so that we comply with the JESD216 requirements: "Byte order of 16-bit
>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>>
> 
> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is

no worries

> quite restrictive IMO.
> 
> AFAIK, SFDP standard does not dictate how data should be stored in flash
> or how SW should interpret after reading data from flash. It merely
> indicates endian-ness compared to 1-1-1 mode.
> 
> So, its up to various system SWs like bootloader/Linux to work according
> to pre-aligned layout as there is no rule that data needs to be stored
> in byte order.
> 
> We have two types of controllers:
> 
> 1. SPI controllers supporting swapping endian-ness on the fly:
> -> For such flashes, better choice is to have SWAP option always
> enabled. So that data written in 8D-8D-8D mode can be read correctly in
> 1-1-1 mode and vice-versa.
> ( I am assuming SWAP option of controller is only effective in 8D-8D-8D
> mode and is NOP in 1-1-1 or other modes)
> 
> But, its possible that "ROM" or other non-upgradable SWs may choose not
> make to use of this SWAP option of HW to keep things simple in which
> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
> don't always have knowledge of flash and cannot be forced to have a
> constraint to enable byte swap on read.
> 
> So, IMO, its best left to system integrators to specify whether or not
> SWAP option needs to be enabled (perhaps via DT as its per flash
> specific property?)

we can't use DT for configuration, maybe a Kconfig instead. Are there any
other options?

> 
> 2.  SPI controllers don't support endian-ness SWAP on the fly:
> It is still possible to reliably read and write data as long as its
> written and read back in same mode.
> 
> De-rating speeds because of absence of this support would mean reduction
> of speed by **16 times** (maybe even higher as 8D mode tends to support
> higher bus freqs). Swapping bytes in Linux before writing or after
> reading is not an option either as it negatively impacts performance.
> 
> Asking ROM/bootloaders to swap bytes based on SFDP indication is
> restrictive too as it involves boot time penalty and most systems with
> OSPI flashes are using them to achieve super fast boot times.
> 
> One more case to consider is flashes that dont have SFDP table to
> indicate byte order but follow Macronix's convention. In such cases, its
> better for SPI NOR layer to be as dumb as possible and not really do any
> byte swapping, leaving it up to user space to handle/interpret data
> appropriately.
> 
> Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
> providing flash agnostic way of switching to 8D mode and reading data in
> 8D mode) would not care about SFDP bit indicating byteorder and its up
> to flasher programs to take care of the same

This is a bit in contradiction, because if the ROMs follow xSPI, thus little
endian byte order, they should swap the bytes.

> 
> IMO, kernel device drivers should just provide access to underlying HW
> and not have too much intelligence to interpret data/take decisions
> 
> So, simpler constraint to put is:
> Flasher programs should program data in the same mode in which
> ROM/bootloder/Linux is expected to read the data on that system.

No, this constraint doesn't cover all possible cases: take a 1-1-1 ROMcode,
8D-8D-8D for other bootloaders and kernel. You need to dynamically change modes
in the flasher program in order to address this use case, which is a no go.
 
> 
> For Macronix like flashes, if one has a ROM/bootloader that only
> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
> please generate a byte-swapped image offline and flash it. Don't impose

we can't do that, see the example from above.

> penalty on systems that do best to handle this messy situation.
> I see this as the only option with least performance penalty.
> 

I take from this that we should let the byte swap be user-configurable,
thus a Kconfig. Which I'm not against it, but it will give users
headaches to sync all the software components. Making such a decision
implies that users know SPI NOR internal details, which also is a bit
stretched. Let's sync so that we move forward with this. Opinions? 

Cheers,
ta

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Michael Walle]

Hi,

Am 2022-03-15 07:08, schrieb Vignesh Raghavendra:
> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit 
>> boundary
>> when configured in Octal DTR mode. The byte order of 16-bit words is
>> swapped when read or written in Octal Double Transfer Rate (DTR) mode
>> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 
>> D3
>> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it 
>> will
>> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision 
>> because
>> it may introduce some endianness problems. It can affect the boot 
>> sequence
>> if the entire boot sequence is not handled in either 8D-8D-8D mode or 
>> 1-1-1
>> mode. So we must swap the bytes back to have the same byte order as in 
>> STR
>> modes. Fortunately there are controllers that can swap the bytes back 
>> at
>> runtime, addressing the flash's endiannesses requirements.
>> If the controllers are not capable of swapping the bytes, the protocol 
>> is
>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the 
>> swapping
>> of the bytes is always done regardless if it's a data or register 
>> access,
>> so that we comply with the JESD216 requirements: "Byte order of 16-bit
>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>> 
> 
> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is
> quite restrictive IMO.
> 
> AFAIK, SFDP standard does not dictate how data should be stored in 
> flash
> or how SW should interpret after reading data from flash. It merely
> indicates endian-ness compared to 1-1-1 mode.

Mh, but below you are saying that Micronix is violating the standard
and is swapping the bytes. So, the standard is actually specifying the
byte order.

> So, its up to various system SWs like bootloader/Linux to work 
> according
> to pre-aligned layout as there is no rule that data needs to be stored
> in byte order.
> 
> We have two types of controllers:
> 
> 1. SPI controllers supporting swapping endian-ness on the fly:
> -> For such flashes, better choice is to have SWAP option always
> enabled. So that data written in 8D-8D-8D mode can be read correctly in
> 1-1-1 mode and vice-versa.
> ( I am assuming SWAP option of controller is only effective in 8D-8D-8D
> mode and is NOP in 1-1-1 or other modes)

Why should it be always enabled? You can also say it should always
be disabled. It doesn't matter if the byte order isn't important.

But I say the byte order *is* important. We need one reference.

> But, its possible that "ROM" or other non-upgradable SWs may choose not
> make to use of this SWAP option of HW to keep things simple in which
> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
> don't always have knowledge of flash and cannot be forced to have a
> constraint to enable byte swap on read.
> 
> So, IMO, its best left to system integrators to specify whether or not
> SWAP option needs to be enabled (perhaps via DT as its per flash
> specific property?)

Agreed, but I don't think we have to do it now. If someone cares,
he can make a patch. Also we have to consider non-DT platforms.

> 2.  SPI controllers don't support endian-ness SWAP on the fly:
> It is still possible to reliably read and write data as long as its
> written and read back in same mode.
> 
> De-rating speeds because of absence of this support would mean 
> reduction
> of speed by **16 times** (maybe even higher as 8D mode tends to support
> higher bus freqs).

You can also fall back to a quad mode. Again, can be implemented if
someone cares.

> Swapping bytes in Linux before writing or after
> reading is not an option either as it negatively impacts performance.
> 
> Asking ROM/bootloaders to swap bytes based on SFDP indication is
> restrictive too as it involves boot time penalty and most systems with
> OSPI flashes are using them to achieve super fast boot times.

No we are talking about what? ms? to read the sfdp? If that is really
a use case, the the bootloader can hardcode it there.

> One more case to consider is flashes that dont have SFDP table to
> indicate byte order but follow Macronix's convention. In such cases, 
> its
> better for SPI NOR layer to be as dumb as possible and not really do 
> any
> byte swapping, leaving it up to user space to handle/interpret data
> appropriately.
> 
> Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
> providing flash agnostic way of switching to 8D mode and reading data 
> in
> 8D mode) would not care about SFDP bit indicating byteorder and its up
> to flasher programs to take care of the same
> 
> IMO, kernel device drivers should just provide access to underlying HW
> and not have too much intelligence to interpret data/take decisions

I strongly disagree here. The kernel should provide a consistent
view of the flash content, regardless if its read in 1-1-1 or 8d-8d-8d.
Imagine you are forced to switch away from 8d-8d-8d mode (for whatever
reason) after some time. How would you know how to read the contents?

JESD216 is a standard, too. There is a reason this bit ended up in
there. If I had to guess, someone messed up the byte order in 8d-8d-8d
but it was too late. And now the standard is giving you a hint that
you are using a flash with that messed up byte ordering.

I want to avoid having flash contents where the byte are swapped.
The sooner we care about that, the better. You cannot undo that
later on.

> So, simpler constraint to put is:
> Flasher programs should program data in the same mode in which
> ROM/bootloder/Linux is expected to read the data on that system.

So what if your flasher only has 1 pin? With this you are just
shifting the problem elsewhere.

> For Macronix like flashes, if one has a ROM/bootloader that only
> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
> please generate a byte-swapped image offline and flash it. Don't impose
> penalty on systems that do best to handle this messy situation.
> I see this as the only option with least performance penalty.

I certainly have nothing against an option to turn this all off
to improve speed. But the swapping (if asked to do so) and the
degradation should be an *opt-out*. Not an opt-in. Nobody will
do the opt-in and we end up with 'corrupted' flash contents.

-michael

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Vignesh Raghavendra]

Hi Michael,

On 15/03/22 12:49 pm, Michael Walle wrote:
> Hi,
> 
> Am 2022-03-15 07:08, schrieb Vignesh Raghavendra:
>> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit
>>> boundary
>>> when configured in Octal DTR mode. The byte order of 16-bit words is
>>> swapped when read or written in Octal Double Transfer Rate (DTR) mode
>>> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
>>> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
>>> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision
>>> because
>>> it may introduce some endianness problems. It can affect the boot
>>> sequence
>>> if the entire boot sequence is not handled in either 8D-8D-8D mode or
>>> 1-1-1
>>> mode. So we must swap the bytes back to have the same byte order as
>>> in STR
>>> modes. Fortunately there are controllers that can swap the bytes back at
>>> runtime, addressing the flash's endiannesses requirements.
>>> If the controllers are not capable of swapping the bytes, the
>>> protocol is
>>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the
>>> swapping
>>> of the bytes is always done regardless if it's a data or register
>>> access,
>>> so that we comply with the JESD216 requirements: "Byte order of 16-bit
>>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>>>
>>
>> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is
>> quite restrictive IMO.
>>
>> AFAIK, SFDP standard does not dictate how data should be stored in flash
>> or how SW should interpret after reading data from flash. It merely
>> indicates endian-ness compared to 1-1-1 mode.
> 
> Mh, but below you are saying that Micronix is violating the standard
> and is swapping the bytes. So, the standard is actually specifying the
> byte order.
> 

As I understand, SFDP spec(JESD216) is way of describing flash details,
it does not enforce any rules on how flash should "behave". It just
provides info for drivers to discover flash parameters at runtime.
OTOH, xSPI spec (JESD251) lays down guidelines for SW interface,
electrical, and mechanical interfaces. Macronix flash deviates from xSPI
spec but no SFDP per say (unless it claims xSPI compliance elsewhere in
SFDP tables).

>> So, its up to various system SWs like bootloader/Linux to work according
>> to pre-aligned layout as there is no rule that data needs to be stored
>> in byte order.
>>
>> We have two types of controllers:
>>
>> 1. SPI controllers supporting swapping endian-ness on the fly:
>> -> For such flashes, better choice is to have SWAP option always
>> enabled. So that data written in 8D-8D-8D mode can be read correctly in
>> 1-1-1 mode and vice-versa.
>> ( I am assuming SWAP option of controller is only effective in 8D-8D-8D
>> mode and is NOP in 1-1-1 or other modes)
> 
> Why should it be always enabled? You can also say it should always
> be disabled. It doesn't matter if the byte order isn't important.
> 
> But I say the byte order *is* important. We need one reference.
> 
>> But, its possible that "ROM" or other non-upgradable SWs may choose not
>> make to use of this SWAP option of HW to keep things simple in which
>> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
>> don't always have knowledge of flash and cannot be forced to have a
>> constraint to enable byte swap on read.
>>
>> So, IMO, its best left to system integrators to specify whether or not
>> SWAP option needs to be enabled (perhaps via DT as its per flash
>> specific property?)
> 
> Agreed, but I don't think we have to do it now. If someone cares,

I am fine with that, but I want to make sure we are not saying
controllers w/o ability to swap byte order can never support Macronix
like flash in 8D-8D-8D mode.

> he can make a patch. Also we have to consider non-DT platforms.
> 

non DT platforms also have a way to pass HW related data

>> 2.  SPI controllers don't support endian-ness SWAP on the fly:
>> It is still possible to reliably read and write data as long as its
>> written and read back in same mode.
>>
>> De-rating speeds because of absence of this support would mean reduction
>> of speed by **16 times** (maybe even higher as 8D mode tends to support
>> higher bus freqs).
> 
> You can also fall back to a quad mode. Again, can be implemented if
> someone cares.

Actually I have not come across OSPI flash that also supports quad mode.
So, fallback is 1-1-1 for majority of these flashes

> 
>> Swapping bytes in Linux before writing or after
>> reading is not an option either as it negatively impacts performance.
>>
>> Asking ROM/bootloaders to swap bytes based on SFDP indication is
>> restrictive too as it involves boot time penalty and most systems with
>> OSPI flashes are using them to achieve super fast boot times.
> 
> No we are talking about what? ms? to read the sfdp? If that is really
> a use case, the the bootloader can hardcode it there.
> 

Reading SFDP is not the issue but swapping entire image is the problem.
Takes several tens of ms depending on boot image size which is not
acceptable to systems looking at < 100 - 200msms start up times for
quick bootup usecases involving early Display / CAN output etc

>> One more case to consider is flashes that dont have SFDP table to
>> indicate byte order but follow Macronix's convention. In such cases, its
>> better for SPI NOR layer to be as dumb as possible and not really do any
>> byte swapping, leaving it up to user space to handle/interpret data
>> appropriately.
>>
>> Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
>> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
>> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
>> providing flash agnostic way of switching to 8D mode and reading data in
>> 8D mode) would not care about SFDP bit indicating byteorder and its up
>> to flasher programs to take care of the same
>>
>> IMO, kernel device drivers should just provide access to underlying HW
>> and not have too much intelligence to interpret data/take decisions
> 
> I strongly disagree here. The kernel should provide a consistent
> view of the flash content, regardless if its read in 1-1-1 or 8d-8d-8d.
> Imagine you are forced to switch away from 8d-8d-8d mode (for whatever
> reason) after some time. How would you know how to read the contents?
> 

Agree with need for consistent view, although if we need to fall back to
1-1-1 mode for a while and switch back to 8D-8D-8D mode then there is
something fundamentally broken.

> JESD216 is a standard, too. There is a reason this bit ended up in
> there. If I had to guess, someone messed up the byte order in 8d-8d-8d
> but it was too late. And now the standard is giving you a hint that
> you are using a flash with that messed up byte ordering.
> 

Yes, but note JESD216 is all about giving hint to softwares  on how
flash behaves, it does not dictate what softwares need to do with it.
So another OS is free to leave these Macronix flashes using native byte
ordering and not swapping on read / write

> I want to avoid having flash contents where the byte are swapped.
> The sooner we care about that, the better. You cannot undo that
> later on.
> 
>> So, simpler constraint to put is:
>> Flasher programs should program data in the same mode in which
>> ROM/bootloder/Linux is expected to read the data on that system.
> 
> So what if your flasher only has 1 pin? With this you are just
> shifting the problem elsewhere.
> 

flasher program would have to use byte swapped image if read is expected
to be in 8D-8D mode

>> For Macronix like flashes, if one has a ROM/bootloader that only
>> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
>> please generate a byte-swapped image offline and flash it. Don't impose
>> penalty on systems that do best to handle this messy situation.
>> I see this as the only option with least performance penalty.
> 
> I certainly have nothing against an option to turn this all off
> to improve speed. But the swapping (if asked to do so) and the
> degradation should be an *opt-out*. Not an opt-in. Nobody will
> do the opt-in and we end up with 'corrupted' flash contents.
> 

Sounds good to me. We should note this in commit message disabling
8D-8D-8D mode for these flashes.

Regards
Vignesh

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Vignesh Raghavendra]

On 15/03/22 12:28 pm, Tudor.Ambarus@microchip.com wrote:
> On 3/15/22 08:08, Vignesh Raghavendra wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi,
> 
> Hi,
> 
>>
>> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
>>> when configured in Octal DTR mode. The byte order of 16-bit words is
>>> swapped when read or written in Octal Double Transfer Rate (DTR) mode
>>> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
>>> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
>>> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision because
>>> it may introduce some endianness problems. It can affect the boot sequence
>>> if the entire boot sequence is not handled in either 8D-8D-8D mode or 1-1-1
>>> mode. So we must swap the bytes back to have the same byte order as in STR
>>> modes. Fortunately there are controllers that can swap the bytes back at
>>> runtime, addressing the flash's endiannesses requirements.
>>> If the controllers are not capable of swapping the bytes, the protocol is
>>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the swapping
>>> of the bytes is always done regardless if it's a data or register access,
>>> so that we comply with the JESD216 requirements: "Byte order of 16-bit
>>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>>>
>>
>> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is
> 
> no worries
> 
>> quite restrictive IMO.
>>
>> AFAIK, SFDP standard does not dictate how data should be stored in flash
>> or how SW should interpret after reading data from flash. It merely
>> indicates endian-ness compared to 1-1-1 mode.
>>
>> So, its up to various system SWs like bootloader/Linux to work according
>> to pre-aligned layout as there is no rule that data needs to be stored
>> in byte order.
>>
>> We have two types of controllers:
>>
>> 1. SPI controllers supporting swapping endian-ness on the fly:
>> -> For such flashes, better choice is to have SWAP option always
>> enabled. So that data written in 8D-8D-8D mode can be read correctly in
>> 1-1-1 mode and vice-versa.
>> ( I am assuming SWAP option of controller is only effective in 8D-8D-8D
>> mode and is NOP in 1-1-1 or other modes)
>>
>> But, its possible that "ROM" or other non-upgradable SWs may choose not
>> make to use of this SWAP option of HW to keep things simple in which
>> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
>> don't always have knowledge of flash and cannot be forced to have a
>> constraint to enable byte swap on read.
>>
>> So, IMO, its best left to system integrators to specify whether or not
>> SWAP option needs to be enabled (perhaps via DT as its per flash
>> specific property?)
> 
> we can't use DT for configuration, maybe a Kconfig instead. Are there any
> other options?
> 

Problem with Kconfig is that it cannot be used when there are multiple
flash instances on the board and both behave differently.

This is similar to big-endian vs little-endian property used by CFI
physmap driver and other places in kernel[1]

>>
>> 2.  SPI controllers don't support endian-ness SWAP on the fly:
>> It is still possible to reliably read and write data as long as its
>> written and read back in same mode.
>>
>> De-rating speeds because of absence of this support would mean reduction
>> of speed by **16 times** (maybe even higher as 8D mode tends to support
>> higher bus freqs). Swapping bytes in Linux before writing or after
>> reading is not an option either as it negatively impacts performance.
>>
>> Asking ROM/bootloaders to swap bytes based on SFDP indication is
>> restrictive too as it involves boot time penalty and most systems with
>> OSPI flashes are using them to achieve super fast boot times.
>>
>> One more case to consider is flashes that dont have SFDP table to
>> indicate byte order but follow Macronix's convention. In such cases, its
>> better for SPI NOR layer to be as dumb as possible and not really do any
>> byte swapping, leaving it up to user space to handle/interpret data
>> appropriately.
>>
>> Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
>> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
>> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
>> providing flash agnostic way of switching to 8D mode and reading data in
>> 8D mode) would not care about SFDP bit indicating byteorder and its up
>> to flasher programs to take care of the same
> 
> This is a bit in contradiction, because if the ROMs follow xSPI, thus little
> endian byte order, they should swap the bytes.
> 

As I indicated below xSPI (JESD251) spec seems to align on little endian
order. So, don't really need to swap the bytes on read. Anyways impact
on performance prohibits such support and thus most ROM designs will
expect flashing program to take care of it (especially in case where
controller does not support on the fly swapping).

>>
>> IMO, kernel device drivers should just provide access to underlying HW
>> and not have too much intelligence to interpret data/take decisions
>>
>> So, simpler constraint to put is:
>> Flasher programs should program data in the same mode in which
>> ROM/bootloder/Linux is expected to read the data on that system.
> 
> No, this constraint doesn't cover all possible cases: take a 1-1-1 ROMcode,
> 8D-8D-8D for other bootloaders and kernel. You need to dynamically change modes
> in the flasher program in order to address this use case, which is a no go.
> 

Flash programmer need not change the mode, but needs to generate a byte
swapped image and write it knowing that image is being flashed in
8D-8D-8D mode and read in 1-1-1 mode or vice versa.

>>
>> For Macronix like flashes, if one has a ROM/bootloader that only
>> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
>> please generate a byte-swapped image offline and flash it. Don't impose
> 
> we can't do that, see the example from above.
> 

See above, no need to change mode dynamically.

>> penalty on systems that do best to handle this messy situation.
>> I see this as the only option with least performance penalty.
>>
> 
> I take from this that we should let the byte swap be user-configurable,
> thus a Kconfig. Which I'm not against it, but it will give users
> headaches to sync all the software components. Making such a decision
> implies that users know SPI NOR internal details, which also is a bit
> stretched. Let's sync so that we move forward with this. Opinions? 
> 

I am fine with things being configurable

Only issues with Kconfig: it does not help systems with multiple OSPIs
with different endian-ness.


[1] Documentation/devicetree/bindings/mtd/mtd-physmap.yaml


Regards
Vignesh

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Tudor.Ambarus]

On 3/16/22 09:05, Vignesh Raghavendra wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Michael,
> 
> On 15/03/22 12:49 pm, Michael Walle wrote:
>> Hi,
>>
>> Am 2022-03-15 07:08, schrieb Vignesh Raghavendra:
>>> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>>>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit
>>>> boundary
>>>> when configured in Octal DTR mode. The byte order of 16-bit words is
>>>> swapped when read or written in Octal Double Transfer Rate (DTR) mode
>>>> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 D3
>>>> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it will
>>>> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision
>>>> because
>>>> it may introduce some endianness problems. It can affect the boot
>>>> sequence
>>>> if the entire boot sequence is not handled in either 8D-8D-8D mode or
>>>> 1-1-1
>>>> mode. So we must swap the bytes back to have the same byte order as
>>>> in STR
>>>> modes. Fortunately there are controllers that can swap the bytes back at
>>>> runtime, addressing the flash's endiannesses requirements.
>>>> If the controllers are not capable of swapping the bytes, the
>>>> protocol is
>>>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the
>>>> swapping
>>>> of the bytes is always done regardless if it's a data or register
>>>> access,
>>>> so that we comply with the JESD216 requirements: "Byte order of 16-bit
>>>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>>>>
>>>
>>> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is
>>> quite restrictive IMO.
>>>
>>> AFAIK, SFDP standard does not dictate how data should be stored in flash
>>> or how SW should interpret after reading data from flash. It merely
>>> indicates endian-ness compared to 1-1-1 mode.
>>
>> Mh, but below you are saying that Micronix is violating the standard
>> and is swapping the bytes. So, the standard is actually specifying the
>> byte order.
>>
> 
> As I understand, SFDP spec(JESD216) is way of describing flash details,
> it does not enforce any rules on how flash should "behave". It just
> provides info for drivers to discover flash parameters at runtime.
> OTOH, xSPI spec (JESD251) lays down guidelines for SW interface,
> electrical, and mechanical interfaces. Macronix flash deviates from xSPI
> spec but no SFDP per say (unless it claims xSPI compliance elsewhere in
> SFDP tables).
> 
>>> So, its up to various system SWs like bootloader/Linux to work according
>>> to pre-aligned layout as there is no rule that data needs to be stored
>>> in byte order.
>>>
>>> We have two types of controllers:
>>>
>>> 1. SPI controllers supporting swapping endian-ness on the fly:
>>> -> For such flashes, better choice is to have SWAP option always
>>> enabled. So that data written in 8D-8D-8D mode can be read correctly in
>>> 1-1-1 mode and vice-versa.
>>> ( I am assuming SWAP option of controller is only effective in 8D-8D-8D
>>> mode and is NOP in 1-1-1 or other modes)
>>
>> Why should it be always enabled? You can also say it should always
>> be disabled. It doesn't matter if the byte order isn't important.
>>
>> But I say the byte order *is* important. We need one reference.
>>
>>> But, its possible that "ROM" or other non-upgradable SWs may choose not
>>> make to use of this SWAP option of HW to keep things simple in which
>>> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
>>> don't always have knowledge of flash and cannot be forced to have a
>>> constraint to enable byte swap on read.
>>>
>>> So, IMO, its best left to system integrators to specify whether or not
>>> SWAP option needs to be enabled (perhaps via DT as its per flash
>>> specific property?)
>>
>> Agreed, but I don't think we have to do it now. If someone cares,
> 
> I am fine with that, but I want to make sure we are not saying
> controllers w/o ability to swap byte order can never support Macronix
> like flash in 8D-8D-8D mode.
> 
>> he can make a patch. Also we have to consider non-DT platforms.
>>
> 
> non DT platforms also have a way to pass HW related data
> 
>>> 2.  SPI controllers don't support endian-ness SWAP on the fly:
>>> It is still possible to reliably read and write data as long as its
>>> written and read back in same mode.
>>>
>>> De-rating speeds because of absence of this support would mean reduction
>>> of speed by **16 times** (maybe even higher as 8D mode tends to support
>>> higher bus freqs).
>>
>> You can also fall back to a quad mode. Again, can be implemented if
>> someone cares.
> 
> Actually I have not come across OSPI flash that also supports quad mode.
> So, fallback is 1-1-1 for majority of these flashes

8-8-8 is typically supported where 8d-8d-8d is supported. But downgrading to
8-8-8 still comes with a performance penalty.

> 
>>
>>> Swapping bytes in Linux before writing or after
>>> reading is not an option either as it negatively impacts performance.
>>>
>>> Asking ROM/bootloaders to swap bytes based on SFDP indication is
>>> restrictive too as it involves boot time penalty and most systems with
>>> OSPI flashes are using them to achieve super fast boot times.
>>
>> No we are talking about what? ms? to read the sfdp? If that is really
>> a use case, the the bootloader can hardcode it there.
>>
> 
> Reading SFDP is not the issue but swapping entire image is the problem.
> Takes several tens of ms depending on boot image size which is not
> acceptable to systems looking at < 100 - 200msms start up times for
> quick bootup usecases involving early Display / CAN output etc
> 
>>> One more case to consider is flashes that dont have SFDP table to
>>> indicate byte order but follow Macronix's convention. In such cases, its
>>> better for SPI NOR layer to be as dumb as possible and not really do any
>>> byte swapping, leaving it up to user space to handle/interpret data
>>> appropriately.
>>>
>>> Also, Macronix is probably in violation of xSPI spec (JESD251A 6.9.5.2
>>> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should be
>>> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
>>> providing flash agnostic way of switching to 8D mode and reading data in
>>> 8D mode) would not care about SFDP bit indicating byteorder and its up
>>> to flasher programs to take care of the same
>>>
>>> IMO, kernel device drivers should just provide access to underlying HW
>>> and not have too much intelligence to interpret data/take decisions
>>
>> I strongly disagree here. The kernel should provide a consistent
>> view of the flash content, regardless if its read in 1-1-1 or 8d-8d-8d.
>> Imagine you are forced to switch away from 8d-8d-8d mode (for whatever
>> reason) after some time. How would you know how to read the contents?
>>
> 
> Agree with need for consistent view, although if we need to fall back to
> 1-1-1 mode for a while and switch back to 8D-8D-8D mode then there is
> something fundamentally broken.
> 
>> JESD216 is a standard, too. There is a reason this bit ended up in
>> there. If I had to guess, someone messed up the byte order in 8d-8d-8d
>> but it was too late. And now the standard is giving you a hint that
>> you are using a flash with that messed up byte ordering.
>>
> 
> Yes, but note JESD216 is all about giving hint to softwares  on how
> flash behaves, it does not dictate what softwares need to do with it.
> So another OS is free to leave these Macronix flashes using native byte
> ordering and not swapping on read / write
> 
>> I want to avoid having flash contents where the byte are swapped.
>> The sooner we care about that, the better. You cannot undo that
>> later on.
>>
>>> So, simpler constraint to put is:
>>> Flasher programs should program data in the same mode in which
>>> ROM/bootloder/Linux is expected to read the data on that system.
>>
>> So what if your flasher only has 1 pin? With this you are just
>> shifting the problem elsewhere.
>>
> 
> flasher program would have to use byte swapped image if read is expected
> to be in 8D-8D mode

:) 

> 
>>> For Macronix like flashes, if one has a ROM/bootloader that only
>>> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, then
>>> please generate a byte-swapped image offline and flash it. Don't impose
>>> penalty on systems that do best to handle this messy situation.
>>> I see this as the only option with least performance penalty.
>>
>> I certainly have nothing against an option to turn this all off
>> to improve speed. But the swapping (if asked to do so) and the
>> degradation should be an *opt-out*. Not an opt-in. Nobody will
>> do the opt-in and we end up with 'corrupted' flash contents.
>>
> 
> Sounds good to me. We should note this in commit message disabling
> 8D-8D-8D mode for these flashes.
> 

I like Michael's *opt-out* idea too. But I'm not convinced with a dt property
used to configure when to swap the bytes and when not. DT should be used to
describe the hardware, not to configure it. So we can add a "dtr-swab16"
property but IMO it should specify just that the bytes are swapped in 8D-8D-8D
mode, just as SFDP does, and not to configure the logic to swab16 or not. And
if it just describes the hardware and not configuring it, then the entire dt
idea diminishes because it is equivalent with setting a SNOR_F flag either
by a flash-info flag or at the SFDP parsing time.

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Michael Walle]

Am 2022-03-16 08:08, schrieb Vignesh Raghavendra:
> On 15/03/22 12:28 pm, Tudor.Ambarus@microchip.com wrote:
>> On 3/15/22 08:08, Vignesh Raghavendra wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>> know the content is safe
>>> 
>>> Hi,
>> 
>> Hi,
>> 
>>> 
>>> On 11/03/22 1:31 pm, Tudor Ambarus wrote:
>>>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit 
>>>> boundary
>>>> when configured in Octal DTR mode. The byte order of 16-bit words is
>>>> swapped when read or written in Octal Double Transfer Rate (DTR) 
>>>> mode
>>>> compared to Single Transfer Rate (STR) modes. If one writes D0 D1 D2 
>>>> D3
>>>> bytes using 1-1-1 mode, and uses 8D-8D-8D SPI mode for reading, it 
>>>> will
>>>> read back D1 D0 D3 D2. Swapping the bytes is a bad design decision 
>>>> because
>>>> it may introduce some endianness problems. It can affect the boot 
>>>> sequence
>>>> if the entire boot sequence is not handled in either 8D-8D-8D mode 
>>>> or 1-1-1
>>>> mode. So we must swap the bytes back to have the same byte order as 
>>>> in STR
>>>> modes. Fortunately there are controllers that can swap the bytes 
>>>> back at
>>>> runtime, addressing the flash's endiannesses requirements.
>>>> If the controllers are not capable of swapping the bytes, the 
>>>> protocol is
>>>> downgraded via spi_nor_spimem_adjust_hwcaps(). When available, the 
>>>> swapping
>>>> of the bytes is always done regardless if it's a data or register 
>>>> access,
>>>> so that we comply with the JESD216 requirements: "Byte order of 
>>>> 16-bit
>>>> words is swapped when read in 8D-8D-8D mode compared to 1-1-1".
>>>> 
>>> 
>>> Sorry, bit late to the thread. But, dropping 8D-8D-8D mode support is
>> 
>> no worries
>> 
>>> quite restrictive IMO.
>>> 
>>> AFAIK, SFDP standard does not dictate how data should be stored in 
>>> flash
>>> or how SW should interpret after reading data from flash. It merely
>>> indicates endian-ness compared to 1-1-1 mode.
>>> 
>>> So, its up to various system SWs like bootloader/Linux to work 
>>> according
>>> to pre-aligned layout as there is no rule that data needs to be 
>>> stored
>>> in byte order.
>>> 
>>> We have two types of controllers:
>>> 
>>> 1. SPI controllers supporting swapping endian-ness on the fly:
>>> -> For such flashes, better choice is to have SWAP option always
>>> enabled. So that data written in 8D-8D-8D mode can be read correctly 
>>> in
>>> 1-1-1 mode and vice-versa.
>>> ( I am assuming SWAP option of controller is only effective in 
>>> 8D-8D-8D
>>> mode and is NOP in 1-1-1 or other modes)
>>> 
>>> But, its possible that "ROM" or other non-upgradable SWs may choose 
>>> not
>>> make to use of this SWAP option of HW to keep things simple in which
>>> case, they cannot boot from 8D-8D-8D mode with above setting. Such SW
>>> don't always have knowledge of flash and cannot be forced to have a
>>> constraint to enable byte swap on read.
>>> 
>>> So, IMO, its best left to system integrators to specify whether or 
>>> not
>>> SWAP option needs to be enabled (perhaps via DT as its per flash
>>> specific property?)
>> 
>> we can't use DT for configuration, maybe a Kconfig instead. Are there 
>> any
>> other options?
>> 
> 
> Problem with Kconfig is that it cannot be used when there are multiple
> flash instances on the board and both behave differently.
> 
> This is similar to big-endian vs little-endian property used by CFI
> physmap driver and other places in kernel[1]

On more thing to consider is that if we are using Kconfig things
cannot be changed during runtime. While this might be ok when you
are building your typical embedded distribution, there are efforts
to be able to boot standard distributions on embedded platforms (see
ARM SystemReady). A Kconfig option won't work there.

-michael

>>> 
>>> 2.  SPI controllers don't support endian-ness SWAP on the fly:
>>> It is still possible to reliably read and write data as long as its
>>> written and read back in same mode.
>>> 
>>> De-rating speeds because of absence of this support would mean 
>>> reduction
>>> of speed by **16 times** (maybe even higher as 8D mode tends to 
>>> support
>>> higher bus freqs). Swapping bytes in Linux before writing or after
>>> reading is not an option either as it negatively impacts performance.
>>> 
>>> Asking ROM/bootloaders to swap bytes based on SFDP indication is
>>> restrictive too as it involves boot time penalty and most systems 
>>> with
>>> OSPI flashes are using them to achieve super fast boot times.
>>> 
>>> One more case to consider is flashes that dont have SFDP table to
>>> indicate byte order but follow Macronix's convention. In such cases, 
>>> its
>>> better for SPI NOR layer to be as dumb as possible and not really do 
>>> any
>>> byte swapping, leaving it up to user space to handle/interpret data
>>> appropriately.
>>> 
>>> Also, Macronix is probably in violation of xSPI spec (JESD251A 
>>> 6.9.5.2
>>> 8D-8D-8D Profile 1.0) where diagrams clearly show data output should 
>>> be
>>> D0 D1 D2 D3... So ROMs following xSPI spec (which is the only spec
>>> providing flash agnostic way of switching to 8D mode and reading data 
>>> in
>>> 8D mode) would not care about SFDP bit indicating byteorder and its 
>>> up
>>> to flasher programs to take care of the same
>> 
>> This is a bit in contradiction, because if the ROMs follow xSPI, thus 
>> little
>> endian byte order, they should swap the bytes.
>> 
> 
> As I indicated below xSPI (JESD251) spec seems to align on little 
> endian
> order. So, don't really need to swap the bytes on read. Anyways impact
> on performance prohibits such support and thus most ROM designs will
> expect flashing program to take care of it (especially in case where
> controller does not support on the fly swapping).
> 
>>> 
>>> IMO, kernel device drivers should just provide access to underlying 
>>> HW
>>> and not have too much intelligence to interpret data/take decisions
>>> 
>>> So, simpler constraint to put is:
>>> Flasher programs should program data in the same mode in which
>>> ROM/bootloder/Linux is expected to read the data on that system.
>> 
>> No, this constraint doesn't cover all possible cases: take a 1-1-1 
>> ROMcode,
>> 8D-8D-8D for other bootloaders and kernel. You need to dynamically 
>> change modes
>> in the flasher program in order to address this use case, which is a 
>> no go.
>> 
> 
> Flash programmer need not change the mode, but needs to generate a byte
> swapped image and write it knowing that image is being flashed in
> 8D-8D-8D mode and read in 1-1-1 mode or vice versa.
> 
>>> 
>>> For Macronix like flashes, if one has a ROM/bootloader that only
>>> supports 1-1-1 mode and flashing data in 8D-8D-8D mode with Linux, 
>>> then
>>> please generate a byte-swapped image offline and flash it. Don't 
>>> impose
>> 
>> we can't do that, see the example from above.
>> 
> 
> See above, no need to change mode dynamically.
> 
>>> penalty on systems that do best to handle this messy situation.
>>> I see this as the only option with least performance penalty.
>>> 
>> 
>> I take from this that we should let the byte swap be 
>> user-configurable,
>> thus a Kconfig. Which I'm not against it, but it will give users
>> headaches to sync all the software components. Making such a decision
>> implies that users know SPI NOR internal details, which also is a bit
>> stretched. Let's sync so that we move forward with this. Opinions?
>> 
> 
> I am fine with things being configurable
> 
> Only issues with Kconfig: it does not help systems with multiple OSPIs
> with different endian-ness.
> 
> 
> [1] Documentation/devicetree/bindings/mtd/mtd-physmap.yaml
> 
> 
> Regards
> Vignesh

-- 
-michael

RE: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[David Laight]

Thought...

Can you read the device in STR mode until you get a suitable
non-palindromic value, then read it in DTR mode and dynamically
determine the byte order?

Clearly this won't work if the device is erased to all 0xff.
But a check could be done on/after the first write.

I suspect write times are actually dominated by the time spent
waiting for the write to complete?
(Never mind the earlier block erase time.)
So always writing in STR mode probably makes little difference?
Writes really ought to be uncommon as well.

Speeding up reads is a different matter - and probably useful.

Of course, if you've got hardware reading the spi memory in DTR
mode for config data you might need to byteswap it (compared
to the STR writes) - but that is probably a 2nd order problem.

I've got some bespoke logic on an PCIe fpga for accessing spi memory.
Uses address bits for the control signals and converts a 32bit
read/write into 8 nibble transfers to the chip.
(uses byte enables - don't an odd number of clocks.)
mmapp()ed to userspace for updating the 6MB fpga image.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Michael Walle]

Hi,

Am 2022-03-16 14:55, schrieb David Laight:
> Thought...

Thank you for your proposal.

> Can you read the device in STR mode until you get a suitable
> non-palindromic value, then read it in DTR mode and dynamically
> determine the byte order?
> 
> Clearly this won't work if the device is erased to all 0xff.
> But a check could be done on/after the first write.
> 
> I suspect write times are actually dominated by the time spent
> waiting for the write to complete?
> (Never mind the earlier block erase time.)
> So always writing in STR mode probably makes little difference?
> Writes really ought to be uncommon as well.
> 
> Speeding up reads is a different matter - and probably useful.
> 
> Of course, if you've got hardware reading the spi memory in DTR
> mode for config data you might need to byteswap it (compared
> to the STR writes) - but that is probably a 2nd order problem.
> 
> I've got some bespoke logic on an PCIe fpga for accessing spi memory.
> Uses address bits for the control signals and converts a 32bit
> read/write into 8 nibble transfers to the chip.
> (uses byte enables - don't an odd number of clocks.)
> mmapp()ed to userspace for updating the 6MB fpga image.

Our problem is not how to detect that we have to swap it, but
rather what we do when we have to do it.

If we have a controller which can swap the bytes for us on the
fly, we are lucky and can enable swapping if we need it. We are
also lucky when we don't have to swap the flash contents, obviously.

But what do we do when we need to swap it and the controller
doesn't support it. We could do it in software which will slow
things down. So depending on the use case this might or might not
work. We can degrade it to a speed which doesn't have this issue;
which might be 1-1-1 in the worst case. We could also do just
nothing special; but this will lead to inconsistencies between
reading in 1-1-1 and 8d-8d-8d.

-michael

RE: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[David Laight]

From: Michael Walle
> Sent: 17 March 2022 09:40
> 
> Am 2022-03-16 14:55, schrieb David Laight:
> > Thought...
> 
> Thank you for your proposal.
> 
> > Can you read the device in STR mode until you get a suitable
> > non-palindromic value, then read it in DTR mode and dynamically
> > determine the byte order?
> >
> > Clearly this won't work if the device is erased to all 0xff.
> > But a check could be done on/after the first write.
> >
> > I suspect write times are actually dominated by the time spent
> > waiting for the write to complete?
> > (Never mind the earlier block erase time.)
> > So always writing in STR mode probably makes little difference?
> > Writes really ought to be uncommon as well.
> >
> > Speeding up reads is a different matter - and probably useful.
> >
> > Of course, if you've got hardware reading the spi memory in DTR
> > mode for config data you might need to byteswap it (compared
> > to the STR writes) - but that is probably a 2nd order problem.
> >
> > I've got some bespoke logic on an PCIe fpga for accessing spi memory.
> > Uses address bits for the control signals and converts a 32bit
> > read/write into 8 nibble transfers to the chip.
> > (uses byte enables - don't an odd number of clocks.)
> > mmapp()ed to userspace for updating the 6MB fpga image.
> 
> Our problem is not how to detect that we have to swap it, but
> rather what we do when we have to do it.
> 
> If we have a controller which can swap the bytes for us on the
> fly, we are lucky and can enable swapping if we need it. We are
> also lucky when we don't have to swap the flash contents, obviously.
> 
> But what do we do when we need to swap it and the controller
> doesn't support it. We could do it in software which will slow
> things down. So depending on the use case this might or might not
> work. We can degrade it to a speed which doesn't have this issue;
> which might be 1-1-1 in the worst case. We could also do just
> nothing special; but this will lead to inconsistencies between
> reading in 1-1-1 and 8d-8d-8d.

I really doubt you'll notice the effects of a software byteswap
compared to the actual time taken to do an spi read.

What's the maximum clock rate for spi memory?
Something like 50MHz ?
If the spi controller isn't doing dma then the cpu pio reads
to get the data are very likely to be even slower than that.
(Especially if they are PCIe reads.)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Vignesh Raghavendra]

On 17/03/22 3:44 pm, David Laight wrote:
> From: Michael Walle
>> Sent: 17 March 2022 09:40
>>
>> Am 2022-03-16 14:55, schrieb David Laight:
>>> Thought...
>>
>> Thank you for your proposal.
>>
>>> Can you read the device in STR mode until you get a suitable
>>> non-palindromic value, then read it in DTR mode and dynamically
>>> determine the byte order?
>>>
>>> Clearly this won't work if the device is erased to all 0xff.
>>> But a check could be done on/after the first write.
>>>
>>> I suspect write times are actually dominated by the time spent
>>> waiting for the write to complete?
>>> (Never mind the earlier block erase time.)
>>> So always writing in STR mode probably makes little difference?
>>> Writes really ought to be uncommon as well.
>>>
>>> Speeding up reads is a different matter - and probably useful.
>>>
>>> Of course, if you've got hardware reading the spi memory in DTR
>>> mode for config data you might need to byteswap it (compared
>>> to the STR writes) - but that is probably a 2nd order problem.
>>>
>>> I've got some bespoke logic on an PCIe fpga for accessing spi memory.
>>> Uses address bits for the control signals and converts a 32bit
>>> read/write into 8 nibble transfers to the chip.
>>> (uses byte enables - don't an odd number of clocks.)
>>> mmapp()ed to userspace for updating the 6MB fpga image.
>>
>> Our problem is not how to detect that we have to swap it, but
>> rather what we do when we have to do it.
>>
>> If we have a controller which can swap the bytes for us on the
>> fly, we are lucky and can enable swapping if we need it. We are
>> also lucky when we don't have to swap the flash contents, obviously.
>>
>> But what do we do when we need to swap it and the controller
>> doesn't support it. We could do it in software which will slow
>> things down. So depending on the use case this might or might not
>> work. We can degrade it to a speed which doesn't have this issue;
>> which might be 1-1-1 in the worst case. We could also do just
>> nothing special; but this will lead to inconsistencies between
>> reading in 1-1-1 and 8d-8d-8d.
> 
> I really doubt you'll notice the effects of a software byteswap
> compared to the actual time taken to do an spi read.
> 
> What's the maximum clock rate for spi memory?
> Something like 50MHz ?

We have Octal SPI flashes running at upwards of 200MHz clock (400MB/s)
so SW byteswap will add significant overhead.


> If the spi controller isn't doing dma then the cpu pio reads
> to get the data are very likely to be even slower than that.
> (Especially if they are PCIe reads.)
> 

Modern OSPI/QSPI flash controllers provide MMIO interface to read from
flash where DMA can pull data as if though you are reading from On chip RAM

Regards
Vignesh

RE: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[David Laight]

From: Vignesh Raghavendra
> Sent: 17 March 2022 10:24
...
> Modern OSPI/QSPI flash controllers provide MMIO interface to read from
> flash where DMA can pull data as if though you are reading from On chip RAM

So the cpu does an MMIO read cycle to the controller which doesn't
complete until (for the nibble-mode spi device I have):
1) Chipselect is asserted.
2) The 8-bit command has been clocked out.
3) The 32bit address have been clocked out (8 clocks in nibbles).
4) A few (probably 4) extra delay clocks are added.
5) The data is read - 8 clocks for 32bits in nibble mode.
6) Chipselect is removed.

Now you can do long sequential reads without all the red tape.
But a random read in nibble mode is about 30 clocks.
16 bit mode saves 6 clocks for the data and maybe 6 for the address?

The controller could do 'clever stuff' for sequential reads.
At a cost of slowing down random reads.

So even at 400MHz it isn't that fast.

If the MMIO interface to the flash controller is PCIe you can
add in a load of extra latency for the cpu read itself.

While PCIe allows multiple read requests to be outstanding,
the Intel cpu I've looked at serialise the reads from each
cpu core (each cpu always uses the same TLP tag).

Now longer read TLP help a lot (IIRC max is 256 bytes).
But the x86 cpu will only generate read TLP for register reads.
You need to use AVX512 registers (or cache line fetches) to
get better throughput!

The alternative is getting the flash controller to issue
the read/write TLP for memory transfers.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 0/6] spi-mem: Allow specifying the byte order in DTR mode

[Vignesh Raghavendra]

On 17/03/22 4:40 pm, David Laight wrote:
> From: Vignesh Raghavendra
>> Sent: 17 March 2022 10:24
> ...
>> Modern OSPI/QSPI flash controllers provide MMIO interface to read from
>> flash where DMA can pull data as if though you are reading from On chip RAM
> 
> So the cpu does an MMIO read cycle to the controller which doesn't
> complete until (for the nibble-mode spi device I have):
> 1) Chipselect is asserted.
> 2) The 8-bit command has been clocked out.
> 3) The 32bit address have been clocked out (8 clocks in nibbles).
> 4) A few (probably 4) extra delay clocks are added.
> 5) The data is read - 8 clocks for 32bits in nibble mode.
> 6) Chipselect is removed.
> 
> Now you can do long sequential reads without all the red tape.
> But a random read in nibble mode is about 30 clocks.
> 16 bit mode saves 6 clocks for the data and maybe 6 for the address?
> 
> The controller could do 'clever stuff' for sequential reads.
> At a cost of slowing down random reads.
> 
> So even at 400MHz it isn't that fast.

Random CPU reads would be inherently slow, its just how HW is.

But, there are cases like image load from flash and Filesystem over
flash which would use DMA to maximize performance, such cases would be
greatly affected if we do SW byte swap

> 
> If the MMIO interface to the flash controller is PCIe you can
> add in a load of extra latency for the cpu read itself.
> 
> While PCIe allows multiple read requests to be outstanding,
> the Intel cpu I've looked at serialise the reads from each
> cpu core (each cpu always uses the same TLP tag).
> 
> Now longer read TLP help a lot (IIRC max is 256 bytes).
> But the x86 cpu will only generate read TLP for register reads.
> You need to use AVX512 registers (or cache line fetches) to
> get better throughput!
> 

Direct CPU fetch from SPI would not be able to make use of full
Bandwidth for high speed flashes and its not the only usecase.

Regards
Vignesh