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

[PATCH 0/4] 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 DTR mode. The byte order of 16-bit words is swapped
when read or written in Double Transfer Rate (DTR) mode compared to
Single Transfer Rate (STR) mode. 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 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. Fortunately there are controllers
that can swap back the bytes at runtime, fixing the endiannesses. Provide
a way for the upper layers to specify the byte order in DTR mode.

Tested with atmel-quadspi and mx66lm1g45g.

Tudor Ambarus (4):
  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

 drivers/mtd/spi-nor/core.c  | 36 +++++++++++++++++++++++++++++-------
 drivers/mtd/spi-nor/core.h  |  6 +++++-
 drivers/mtd/spi-nor/sfdp.c  |  3 +++
 drivers/mtd/spi-nor/sfdp.h  |  1 +
 include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
 include/linux/spi/spi-mem.h |  3 +++
 6 files changed, 58 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/4] 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 DTR mode. The byte order of 16-bit words is swapped
when read or written in Double Transfer Rate (DTR) mode compared to
Single Transfer Rate (STR) mode. 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 this 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.
Fortunately there are controllers that can swap back the bytes at runtime,
fixing the endiannesses. Provide a way for the upper layers to specify the
byte order in DTR mode.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 include/linux/spi/spi-mem.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 85e2ff7b840d..e1878417420c 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_bswap16: whether the byte order of 16-bit words is swapped when
+ *		      read or written in DTR mode compared to STR mode.
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
@@ -119,6 +121,7 @@ struct spi_mem_op {
 	struct {
 		u8 buswidth;
 		u8 dtr : 1;
+		u8 dtr_bswap16 : 1;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
 		union {
-- 
2.25.1

[PATCH 2/4] 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 write written in
8D-8D-8D mode compared to STR modes. Swapping the bytes is a bad design
decision because this may affect the boot sequence if the entire boot
sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode. Allow
operations to specify the byte order in DTR mode, so that controllers can
swap the bytes back at run-time to fix the endianness, if they are capable.

The byte order in 8D-8D-8D mode can be retrieved at run-time by checking
BFPT[DWORD(18)] BIT(31). When set to one, the "Byte order of 16-bit words
is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.". It doesn't
specify if this applies to both register and data operations. Macronix is
the single user of this byte swap and it doesn't have clear rules, as it
contains register operations that require data swap (RDPASS, WRPASS,
PASSULK, RDSFDP) and register operations that don't require data swap
(WRFBR). All these are not common and can be handled in 1-1-1 mode, so we
can ignore them for now. All the other register operations are done on one
byte length. The read register operations are actually in 8D-8D-8S mode,
as they send the data value twice, on each half of the clock cycle. In case
of a register write of one byte, the memory supports receiving the register
value only on the first byte, thus it discards the value of the byte on the
second half of the clock cycle. Swapping the bytes for one byte register
writes is not required, and for one byte register reads it doesn't matter.
Thus swap the bytes only for read or page program operations.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c  | 31 +++++++++++++++++++++++++------
 drivers/mtd/spi-nor/core.h  |  1 +
 include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
 3 files changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 04ea180118e3..453d8c54d062 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -106,6 +106,9 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 		op->dummy.dtr = true;
 		op->data.dtr = true;
 
+		if (spi_nor_protocol_is_dtr_bswap16(proto))
+			op->data.dtr_bswap16 = true;
+
 		/* 2 bytes per clock cycle in DTR mode. */
 		op->dummy.nbytes *= 2;
 
@@ -388,7 +391,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, sr, 0));
 
-		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
+		if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
 			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
 			op.dummy.nbytes = nor->params->rdsr_dummy;
 			/*
@@ -432,7 +435,7 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, fsr, 0));
 
-		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
+		if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
 			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
 			op.dummy.nbytes = nor->params->rdsr_dummy;
 			/*
@@ -2488,7 +2491,7 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
 {
 	if (nor->addr_width) {
 		/* already configured from SFDP */
-	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
+	} else if (spi_nor_protocol_is_octal_dtr(nor->read_proto)) {
 		/*
 		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
 		 * in this protocol an odd address width cannot be used because
@@ -2701,6 +2704,19 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
 		nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
 }
 
+static void spi_nor_set_dtr_bswap16_ops(struct spi_nor *nor)
+{
+	struct spi_nor_flash_parameter *params = nor->params;
+	u32 mask = SNOR_HWCAPS_READ_8_8_8_DTR | SNOR_HWCAPS_PP_8_8_8_DTR;
+
+	if ((params->hwcaps.mask & mask) == mask) {
+		params->reads[SNOR_CMD_READ_8_8_8_DTR].proto |=
+			SNOR_PROTO_IS_DTR_BSWAP16;
+		params->page_programs[SNOR_CMD_PP_8_8_8_DTR].proto |=
+			SNOR_PROTO_IS_DTR_BSWAP16;
+	}
+}
+
 /**
  * spi_nor_late_init_params() - Late initialization of default flash parameters.
  * @nor:	pointer to a 'struct spi_nor'
@@ -2721,6 +2737,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
 	spi_nor_init_flags(nor);
 	spi_nor_init_fixup_flags(nor);
 
+	if (nor->flags & SNOR_F_DTR_BSWAP16)
+		spi_nor_set_dtr_bswap16_ops(nor);
+
 	/*
 	 * NOR protection support. When locking_ops are not provided, we pick
 	 * the default ones.
@@ -2899,8 +2918,8 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
 	if (!nor->params->octal_dtr_enable)
 		return 0;
 
-	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
-	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
+	if (!(spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
+	      spi_nor_protocol_is_octal_dtr(nor->write_proto)))
 		return 0;
 
 	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
@@ -2968,7 +2987,7 @@ static int spi_nor_init(struct spi_nor *nor)
 		spi_nor_try_unlock_all(nor);
 
 	if (nor->addr_width == 4 &&
-	    nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
+	    !spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
 	    !(nor->flags & SNOR_F_4B_OPCODES)) {
 		/*
 		 * If the RESET# pin isn't hooked up properly, or the system
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 2afb610853a9..7c077d41c335 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -29,6 +29,7 @@ enum spi_nor_option_flags {
 	SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
 	SNOR_F_SOFT_RESET	= BIT(15),
 	SNOR_F_SWP_IS_VOLATILE	= BIT(16),
+	SNOR_F_DTR_BSWAP16	= BIT(17),
 };
 
 struct spi_nor_read_command {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index fc90fce26e33..6e9660475c5b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -168,6 +168,11 @@
 	 SNOR_PROTO_DATA_MASK)
 
 #define SNOR_PROTO_IS_DTR	BIT(24)	/* Double Transfer Rate */
+/*
+ * Byte order of 16-bit words is swapped when read or written in DTR mode
+ * compared to STR mode.
+ */
+#define SNOR_PROTO_IS_DTR_BSWAP16	BIT(25)
 
 #define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits)	\
 	(SNOR_PROTO_INST(_inst_nbits) |				\
@@ -201,6 +206,18 @@ static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
 	return !!(proto & SNOR_PROTO_IS_DTR);
 }
 
+static inline bool spi_nor_protocol_is_octal_dtr(enum spi_nor_protocol proto)
+{
+	return ((proto & SNOR_PROTO_8_8_8_DTR) == SNOR_PROTO_8_8_8_DTR);
+}
+
+static inline bool spi_nor_protocol_is_dtr_bswap16(enum spi_nor_protocol proto)
+{
+	u32 mask = SNOR_PROTO_IS_DTR | SNOR_PROTO_IS_DTR_BSWAP16;
+
+	return ((proto & mask) == mask);
+}
+
 static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol proto)
 {
 	return ((unsigned long)(proto & SNOR_PROTO_INST_MASK)) >>
-- 
2.25.1

[PATCH 3/4] 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>
---
 drivers/mtd/spi-nor/sfdp.c | 3 +++
 drivers/mtd/spi-nor/sfdp.h | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index a5211543d30d..551edbb039f0 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -633,6 +633,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_BSWAP16;
+
 	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
 }
 
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index bbf80d2990ab..9a834ea31c16 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -97,6 +97,7 @@ struct sfdp_bfpt {
 #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)
 
 struct sfdp_parameter_header {
 	u8		id_lsb;
-- 
2.25.1

[PATCH 4/4] 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>
---
 drivers/mtd/spi-nor/core.c | 5 ++++-
 drivers/mtd/spi-nor/core.h | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 453d8c54d062..c3128a8e1544 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2572,7 +2572,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) {
@@ -2613,6 +2613,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_BSWAP16)
+		nor->flags |= SNOR_F_DTR_BSWAP16;
+
 	/*
 	 * 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 7c077d41c335..1cb887437193 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -362,6 +362,8 @@ 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_BSWAP16:     the byte order of 16-bit words is swapped when
+ *			      read or written in 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
@@ -404,7 +406,7 @@ struct flash_info {
 #define USE_FSR				BIT(10)
 #define SPI_NOR_XSR_RDY			BIT(11)
 
-	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)
@@ -413,6 +415,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_BSWAP16		BIT(8)
 
 	u8 fixup_flags;
 #define SPI_NOR_4B_OPCODES		BIT(0)
-- 
2.25.1

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

[Michael Walle]

Am 2022-02-18 15:58, schrieb 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 write written in
> 8D-8D-8D mode compared to STR modes. Swapping the bytes is a bad design
> decision because this may affect the boot sequence if the entire boot
> sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode. Allow
> operations to specify the byte order in DTR mode, so that controllers 
> can
> swap the bytes back at run-time to fix the endianness, if they are 
> capable.
> 
> The byte order in 8D-8D-8D mode can be retrieved at run-time by 
> checking
> BFPT[DWORD(18)] BIT(31). When set to one, the "Byte order of 16-bit 
> words
> is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.". It 
> doesn't
> specify if this applies to both register and data operations. Macronix 
> is
> the single user of this byte swap and it doesn't have clear rules, as 
> it
> contains register operations that require data swap (RDPASS, WRPASS,
> PASSULK, RDSFDP) and register operations that don't require data swap
> (WRFBR). All these are not common and can be handled in 1-1-1 mode, so 
> we
> can ignore them for now. All the other register operations are done on 
> one
> byte length. The read register operations are actually in 8D-8D-8S 
> mode,
> as they send the data value twice, on each half of the clock cycle. In 
> case
> of a register write of one byte, the memory supports receiving the 
> register
> value only on the first byte, thus it discards the value of the byte on 
> the
> second half of the clock cycle. Swapping the bytes for one byte 
> register
> writes is not required, and for one byte register reads it doesn't 
> matter.
> Thus swap the bytes only for read or page program operations.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 31 +++++++++++++++++++++++++------
>  drivers/mtd/spi-nor/core.h  |  1 +
>  include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
>  3 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 04ea180118e3..453d8c54d062 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -106,6 +106,9 @@ void spi_nor_spimem_setup_op(const struct spi_nor 
> *nor,
>  		op->dummy.dtr = true;
>  		op->data.dtr = true;
> 
> +		if (spi_nor_protocol_is_dtr_bswap16(proto))
> +			op->data.dtr_bswap16 = true;
> +
>  		/* 2 bytes per clock cycle in DTR mode. */
>  		op->dummy.nbytes *= 2;
> 
> @@ -388,7 +391,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_DATA_IN(1, sr, 0));
> 
> -		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +		if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>  			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>  			op.dummy.nbytes = nor->params->rdsr_dummy;
>  			/*
> @@ -432,7 +435,7 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 
> *fsr)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_DATA_IN(1, fsr, 0));
> 
> -		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +		if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>  			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>  			op.dummy.nbytes = nor->params->rdsr_dummy;
>  			/*
> @@ -2488,7 +2491,7 @@ static int spi_nor_set_addr_width(struct spi_nor 
> *nor)
>  {
>  	if (nor->addr_width) {
>  		/* already configured from SFDP */
> -	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> +	} else if (spi_nor_protocol_is_octal_dtr(nor->read_proto)) {
>  		/*
>  		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
>  		 * in this protocol an odd address width cannot be used because
> @@ -2701,6 +2704,19 @@ static void spi_nor_init_fixup_flags(struct 
> spi_nor *nor)
>  		nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>  }
> 
> +static void spi_nor_set_dtr_bswap16_ops(struct spi_nor *nor)
> +{
> +	struct spi_nor_flash_parameter *params = nor->params;
> +	u32 mask = SNOR_HWCAPS_READ_8_8_8_DTR | SNOR_HWCAPS_PP_8_8_8_DTR;
> +
> +	if ((params->hwcaps.mask & mask) == mask) {
> +		params->reads[SNOR_CMD_READ_8_8_8_DTR].proto |=
> +			SNOR_PROTO_IS_DTR_BSWAP16;
> +		params->page_programs[SNOR_CMD_PP_8_8_8_DTR].proto |=
> +			SNOR_PROTO_IS_DTR_BSWAP16;
> +	}
> +}
> +
>  /**
>   * spi_nor_late_init_params() - Late initialization of default flash
> parameters.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -2721,6 +2737,9 @@ static void spi_nor_late_init_params(struct 
> spi_nor *nor)
>  	spi_nor_init_flags(nor);
>  	spi_nor_init_fixup_flags(nor);
> 
> +	if (nor->flags & SNOR_F_DTR_BSWAP16)
> +		spi_nor_set_dtr_bswap16_ops(nor);
> +
>  	/*
>  	 * NOR protection support. When locking_ops are not provided, we pick
>  	 * the default ones.
> @@ -2899,8 +2918,8 @@ static int spi_nor_octal_dtr_enable(struct
> spi_nor *nor, bool enable)
>  	if (!nor->params->octal_dtr_enable)
>  		return 0;
> 
> -	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
> -	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
> +	if (!(spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
> +	      spi_nor_protocol_is_octal_dtr(nor->write_proto)))
>  		return 0;
> 
>  	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
> @@ -2968,7 +2987,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  		spi_nor_try_unlock_all(nor);
> 
>  	if (nor->addr_width == 4 &&
> -	    nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
> +	    !spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>  	    !(nor->flags & SNOR_F_4B_OPCODES)) {
>  		/*
>  		 * If the RESET# pin isn't hooked up properly, or the system
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 2afb610853a9..7c077d41c335 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -29,6 +29,7 @@ enum spi_nor_option_flags {
>  	SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
>  	SNOR_F_SOFT_RESET	= BIT(15),
>  	SNOR_F_SWP_IS_VOLATILE	= BIT(16),
> +	SNOR_F_DTR_BSWAP16	= BIT(17),
>  };
> 
>  struct spi_nor_read_command {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index fc90fce26e33..6e9660475c5b 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -168,6 +168,11 @@
>  	 SNOR_PROTO_DATA_MASK)
> 
>  #define SNOR_PROTO_IS_DTR	BIT(24)	/* Double Transfer Rate */
> +/*
> + * Byte order of 16-bit words is swapped when read or written in DTR 
> mode
> + * compared to STR mode.
> + */
> +#define SNOR_PROTO_IS_DTR_BSWAP16	BIT(25)
> 
>  #define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits)	\
>  	(SNOR_PROTO_INST(_inst_nbits) |				\
> @@ -201,6 +206,18 @@ static inline bool spi_nor_protocol_is_dtr(enum
> spi_nor_protocol proto)
>  	return !!(proto & SNOR_PROTO_IS_DTR);
>  }
> 
> +static inline bool spi_nor_protocol_is_octal_dtr(enum spi_nor_protocol 
> proto)
> +{
> +	return ((proto & SNOR_PROTO_8_8_8_DTR) == SNOR_PROTO_8_8_8_DTR);

This looks wrong what if there are 0's in SNOR_PROTO_8_8_8_DTR? If this
happens to be the same as SNOR_PROTO_MASK (which doesn't exist) this
deserves a comment.

> +}
> +
> +static inline bool spi_nor_protocol_is_dtr_bswap16(enum 
> spi_nor_protocol proto)
> +{
> +	u32 mask = SNOR_PROTO_IS_DTR | SNOR_PROTO_IS_DTR_BSWAP16;
> +
> +	return ((proto & mask) == mask);

isn't "return proto & SNOR_PROTO_IS_DTR_BSWAP16;" enough here?

> +}
> +
>  static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol 
> proto)
>  {
>  	return ((unsigned long)(proto & SNOR_PROTO_INST_MASK)) >>

-michael

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

[Michael Walle]

Am 2022-02-18 15:58, schrieb 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>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 3 +++
>  drivers/mtd/spi-nor/sfdp.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..551edbb039f0 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -633,6 +633,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_BSWAP16;
> +
>  	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
>  }
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index bbf80d2990ab..9a834ea31c16 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -97,6 +97,7 @@ struct sfdp_bfpt {
>  #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)
> 
>  struct sfdp_parameter_header {
>  	u8		id_lsb;

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

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

[Michael Walle]

Am 2022-02-18 15:59, schrieb 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>
> ---
>  drivers/mtd/spi-nor/core.c | 5 ++++-
>  drivers/mtd/spi-nor/core.h | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 453d8c54d062..c3128a8e1544 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2572,7 +2572,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) {
> @@ -2613,6 +2613,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_BSWAP16)
> +		nor->flags |= SNOR_F_DTR_BSWAP16;
> +
>  	/*
>  	 * 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 7c077d41c335..1cb887437193 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -362,6 +362,8 @@ 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_BSWAP16:     the byte order of 16-bit words is 
> swapped when
> + *			      read or written in 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
> @@ -404,7 +406,7 @@ struct flash_info {
>  #define USE_FSR				BIT(10)
>  #define SPI_NOR_XSR_RDY			BIT(11)
> 
> -	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)
> @@ -413,6 +415,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_BSWAP16		BIT(8)
> 
>  	u8 fixup_flags;
>  #define SPI_NOR_4B_OPCODES		BIT(0)

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

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

[Michael Walle]

Am 2022-02-18 15:58, schrieb Tudor Ambarus:
> Fortunately there are controllers
> that can swap back the bytes at runtime, fixing the endiannesses. 
> Provide
> a way for the upper layers to specify the byte order in DTR mode.

Are there any patches for the atmel-quadspi yet? What happens if
the controller doesn't support it? Will there be a software fallback?

-michael

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

[Tudor.Ambarus]

On 2/21/22 09:44, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>> Fortunately there are controllers
>> that can swap back the bytes at runtime, fixing the endiannesses.
>> Provide
>> a way for the upper layers to specify the byte order in DTR mode.
> 
> Are there any patches for the atmel-quadspi yet? What happens if

not public, but will publish them these days.

> the controller doesn't support it? Will there be a software fallback?

no need for a fallback, the controller can ignore op->data.dtr_bswap16 if
it can't swap bytes.

Here's the changes that enable this on atmel-quadspi:

Author: Tudor Ambarus <tudor.ambarus@microchip.com>
Date:   Thu Feb 17 10:48:10 2022 +0200

    spi: atmel-quadspi: Set endianness on 8D-8D-8D mode according to the flash requirements
    
    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 write written in
    8D-8D-8D mode compared to STR modes. Set the endianness flash requirements
    to avoid endianness problems during boot stages.
    
    Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index a4ba94ce84f1..c4a3963f7c84 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -697,6 +697,8 @@ static int atmel_qspi_sama7g5_set_cfg(struct atmel_qspi *aq,
                ifr |= QSPI_IFR_DDREN;
                if (op->cmd.dtr)
                        ifr |= QSPI_IFR_DDRCMDEN;
+               if (op->data.dtr_bswap16)
+                       ifr |= QSPI_IFR_END;
 
                ifr |= QSPI_IFR_DQSEN;
        }

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

[Tudor.Ambarus]

On 2/21/22 09:36, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-18 15:58, schrieb 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 write written in
>> 8D-8D-8D mode compared to STR modes. Swapping the bytes is a bad design
>> decision because this may affect the boot sequence if the entire boot
>> sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode. Allow
>> operations to specify the byte order in DTR mode, so that controllers
>> can
>> swap the bytes back at run-time to fix the endianness, if they are
>> capable.
>>
>> The byte order in 8D-8D-8D mode can be retrieved at run-time by
>> checking
>> BFPT[DWORD(18)] BIT(31). When set to one, the "Byte order of 16-bit
>> words
>> is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.". It
>> doesn't
>> specify if this applies to both register and data operations. Macronix
>> is
>> the single user of this byte swap and it doesn't have clear rules, as
>> it
>> contains register operations that require data swap (RDPASS, WRPASS,
>> PASSULK, RDSFDP) and register operations that don't require data swap
>> (WRFBR). All these are not common and can be handled in 1-1-1 mode, so
>> we
>> can ignore them for now. All the other register operations are done on
>> one
>> byte length. The read register operations are actually in 8D-8D-8S
>> mode,
>> as they send the data value twice, on each half of the clock cycle. In
>> case
>> of a register write of one byte, the memory supports receiving the
>> register
>> value only on the first byte, thus it discards the value of the byte on
>> the
>> second half of the clock cycle. Swapping the bytes for one byte
>> register
>> writes is not required, and for one byte register reads it doesn't
>> matter.
>> Thus swap the bytes only for read or page program operations.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/core.c  | 31 +++++++++++++++++++++++++------
>>  drivers/mtd/spi-nor/core.h  |  1 +
>>  include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
>>  3 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 04ea180118e3..453d8c54d062 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -106,6 +106,9 @@ void spi_nor_spimem_setup_op(const struct spi_nor
>> *nor,
>>               op->dummy.dtr = true;
>>               op->data.dtr = true;
>>
>> +             if (spi_nor_protocol_is_dtr_bswap16(proto))
>> +                     op->data.dtr_bswap16 = true;
>> +
>>               /* 2 bytes per clock cycle in DTR mode. */
>>               op->dummy.nbytes *= 2;
>>
>> @@ -388,7 +391,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_DATA_IN(1, sr, 0));
>>
>> -             if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>> +             if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>>                       op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>>                       op.dummy.nbytes = nor->params->rdsr_dummy;
>>                       /*
>> @@ -432,7 +435,7 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8
>> *fsr)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_DATA_IN(1, fsr, 0));
>>
>> -             if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>> +             if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>>                       op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>>                       op.dummy.nbytes = nor->params->rdsr_dummy;
>>                       /*
>> @@ -2488,7 +2491,7 @@ static int spi_nor_set_addr_width(struct spi_nor
>> *nor)
>>  {
>>       if (nor->addr_width) {
>>               /* already configured from SFDP */
>> -     } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>> +     } else if (spi_nor_protocol_is_octal_dtr(nor->read_proto)) {
>>               /*
>>                * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
>>                * in this protocol an odd address width cannot be used because
>> @@ -2701,6 +2704,19 @@ static void spi_nor_init_fixup_flags(struct
>> spi_nor *nor)
>>               nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>>  }
>>
>> +static void spi_nor_set_dtr_bswap16_ops(struct spi_nor *nor)
>> +{
>> +     struct spi_nor_flash_parameter *params = nor->params;
>> +     u32 mask = SNOR_HWCAPS_READ_8_8_8_DTR | SNOR_HWCAPS_PP_8_8_8_DTR;
>> +
>> +     if ((params->hwcaps.mask & mask) == mask) {
>> +             params->reads[SNOR_CMD_READ_8_8_8_DTR].proto |=
>> +                     SNOR_PROTO_IS_DTR_BSWAP16;
>> +             params->page_programs[SNOR_CMD_PP_8_8_8_DTR].proto |=
>> +                     SNOR_PROTO_IS_DTR_BSWAP16;
>> +     }
>> +}
>> +
>>  /**
>>   * spi_nor_late_init_params() - Late initialization of default flash
>> parameters.
>>   * @nor:     pointer to a 'struct spi_nor'
>> @@ -2721,6 +2737,9 @@ static void spi_nor_late_init_params(struct
>> spi_nor *nor)
>>       spi_nor_init_flags(nor);
>>       spi_nor_init_fixup_flags(nor);
>>
>> +     if (nor->flags & SNOR_F_DTR_BSWAP16)
>> +             spi_nor_set_dtr_bswap16_ops(nor);
>> +
>>       /*
>>        * NOR protection support. When locking_ops are not provided, we pick
>>        * the default ones.
>> @@ -2899,8 +2918,8 @@ static int spi_nor_octal_dtr_enable(struct
>> spi_nor *nor, bool enable)
>>       if (!nor->params->octal_dtr_enable)
>>               return 0;
>>
>> -     if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
>> -           nor->write_proto == SNOR_PROTO_8_8_8_DTR))
>> +     if (!(spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>> +           spi_nor_protocol_is_octal_dtr(nor->write_proto)))
>>               return 0;
>>
>>       if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
>> @@ -2968,7 +2987,7 @@ static int spi_nor_init(struct spi_nor *nor)
>>               spi_nor_try_unlock_all(nor);
>>
>>       if (nor->addr_width == 4 &&
>> -         nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
>> +         !spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>>           !(nor->flags & SNOR_F_4B_OPCODES)) {
>>               /*
>>                * If the RESET# pin isn't hooked up properly, or the system
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 2afb610853a9..7c077d41c335 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -29,6 +29,7 @@ enum spi_nor_option_flags {
>>       SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
>>       SNOR_F_SOFT_RESET       = BIT(15),
>>       SNOR_F_SWP_IS_VOLATILE  = BIT(16),
>> +     SNOR_F_DTR_BSWAP16      = BIT(17),
>>  };
>>
>>  struct spi_nor_read_command {
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index fc90fce26e33..6e9660475c5b 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -168,6 +168,11 @@
>>        SNOR_PROTO_DATA_MASK)
>>
>>  #define SNOR_PROTO_IS_DTR    BIT(24) /* Double Transfer Rate */
>> +/*
>> + * Byte order of 16-bit words is swapped when read or written in DTR
>> mode
>> + * compared to STR mode.
>> + */
>> +#define SNOR_PROTO_IS_DTR_BSWAP16    BIT(25)
>>
>>  #define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits)        \
>>       (SNOR_PROTO_INST(_inst_nbits) |                         \
>> @@ -201,6 +206,18 @@ static inline bool spi_nor_protocol_is_dtr(enum
>> spi_nor_protocol proto)
>>       return !!(proto & SNOR_PROTO_IS_DTR);
>>  }
>>
>> +static inline bool spi_nor_protocol_is_octal_dtr(enum spi_nor_protocol
>> proto)
>> +{
>> +     return ((proto & SNOR_PROTO_8_8_8_DTR) == SNOR_PROTO_8_8_8_DTR);
> 
> This looks wrong what if there are 0's in SNOR_PROTO_8_8_8_DTR? If this
> happens to be the same as SNOR_PROTO_MASK (which doesn't exist) this
> deserves a comment.

I'm not sure I understand the comment. SNOR_PROTO_8_8_8_DTR has value 0x80808.
This method is added to cover the classical 8D-8D-8D mode and the 8D-8D-8D mode
with bytes swapped. This method will return true for both cases.

> 
>> +}
>> +
>> +static inline bool spi_nor_protocol_is_dtr_bswap16(enum
>> spi_nor_protocol proto)
>> +{
>> +     u32 mask = SNOR_PROTO_IS_DTR | SNOR_PROTO_IS_DTR_BSWAP16;
>> +
>> +     return ((proto & mask) == mask);
> 
> isn't "return proto & SNOR_PROTO_IS_DTR_BSWAP16;" enough here?

Byte swap can be done only on DTR modes. SNOR_PROTO_IS_DTR_BSWAP16
without SNOR_PROTO_IS_DTR doesn't make sense. This method includes
this sanity check.

Cheers,
ta
> 
>> +}
>> +
>>  static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol
>> proto)
>>  {
>>       return ((unsigned long)(proto & SNOR_PROTO_INST_MASK)) >>
> 
> -michael

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

[Michael Walle]

Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
> On 2/21/22 09:44, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>> Fortunately there are controllers
>>> that can swap back the bytes at runtime, fixing the endiannesses.
>>> Provide
>>> a way for the upper layers to specify the byte order in DTR mode.
>> 
>> Are there any patches for the atmel-quadspi yet? What happens if
> 
> not public, but will publish them these days.
> 
>> the controller doesn't support it? Will there be a software fallback?
> 
> no need for a fallback, the controller can ignore op->data.dtr_bswap16 
> if
> it can't swap bytes.

I don't understand. If the controller doesn't swap the 16bit values,
you will read the wrong content, no?

-michael

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

[Tudor.Ambarus]

On 2/22/22 16:13, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>> On 2/21/22 09:44, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>> Fortunately there are controllers
>>>> that can swap back the bytes at runtime, fixing the endiannesses.
>>>> Provide
>>>> a way for the upper layers to specify the byte order in DTR mode.
>>>
>>> Are there any patches for the atmel-quadspi yet? What happens if
>>
>> not public, but will publish them these days.
>>
>>> the controller doesn't support it? Will there be a software fallback?
>>
>> no need for a fallback, the controller can ignore op->data.dtr_bswap16
>> if
>> it can't swap bytes.
> 
> I don't understand. If the controller doesn't swap the 16bit values,
> you will read the wrong content, no?
> 

In linux no, because macronix swaps bytes on a 2 byte boundary both on
reads and on page program. The problem is when you mix 8D-8D-8D mode and
1-1-1 mode along the boot stages. Let's assume you write all boot binaries
in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, when u-boot
will try to get the kernel it will fail, as the flash swaps the bytes compared
to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 1-1-1 mode and
when reaching u-boot you will read D1 D0 D3 D2 and it will mess the kernel image.

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

[Michael Walle]

Am 2022-02-22 15:02, schrieb Tudor.Ambarus@microchip.com:
> On 2/21/22 09:36, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-02-18 15:58, schrieb 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 write written 
>>> in
>>> 8D-8D-8D mode compared to STR modes. Swapping the bytes is a bad 
>>> design
>>> decision because this may affect the boot sequence if the entire boot
>>> sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode. Allow
>>> operations to specify the byte order in DTR mode, so that controllers
>>> can
>>> swap the bytes back at run-time to fix the endianness, if they are
>>> capable.
>>> 
>>> The byte order in 8D-8D-8D mode can be retrieved at run-time by
>>> checking
>>> BFPT[DWORD(18)] BIT(31). When set to one, the "Byte order of 16-bit
>>> words
>>> is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.". It
>>> doesn't
>>> specify if this applies to both register and data operations. 
>>> Macronix
>>> is
>>> the single user of this byte swap and it doesn't have clear rules, as
>>> it
>>> contains register operations that require data swap (RDPASS, WRPASS,
>>> PASSULK, RDSFDP) and register operations that don't require data swap
>>> (WRFBR). All these are not common and can be handled in 1-1-1 mode, 
>>> so
>>> we
>>> can ignore them for now. All the other register operations are done 
>>> on
>>> one
>>> byte length. The read register operations are actually in 8D-8D-8S
>>> mode,
>>> as they send the data value twice, on each half of the clock cycle. 
>>> In
>>> case
>>> of a register write of one byte, the memory supports receiving the
>>> register
>>> value only on the first byte, thus it discards the value of the byte 
>>> on
>>> the
>>> second half of the clock cycle. Swapping the bytes for one byte
>>> register
>>> writes is not required, and for one byte register reads it doesn't
>>> matter.
>>> Thus swap the bytes only for read or page program operations.
>>> 
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> ---
>>>  drivers/mtd/spi-nor/core.c  | 31 +++++++++++++++++++++++++------
>>>  drivers/mtd/spi-nor/core.h  |  1 +
>>>  include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
>>>  3 files changed, 43 insertions(+), 6 deletions(-)
>>> 
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index 04ea180118e3..453d8c54d062 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -106,6 +106,9 @@ void spi_nor_spimem_setup_op(const struct spi_nor
>>> *nor,
>>>               op->dummy.dtr = true;
>>>               op->data.dtr = true;
>>> 
>>> +             if (spi_nor_protocol_is_dtr_bswap16(proto))
>>> +                     op->data.dtr_bswap16 = true;
>>> +
>>>               /* 2 bytes per clock cycle in DTR mode. */
>>>               op->dummy.nbytes *= 2;
>>> 
>>> @@ -388,7 +391,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>>>                                  SPI_MEM_OP_NO_DUMMY,
>>>                                  SPI_MEM_OP_DATA_IN(1, sr, 0));
>>> 
>>> -             if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>>> +             if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>>>                       op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>>>                       op.dummy.nbytes = nor->params->rdsr_dummy;
>>>                       /*
>>> @@ -432,7 +435,7 @@ static int spi_nor_read_fsr(struct spi_nor *nor, 
>>> u8
>>> *fsr)
>>>                                  SPI_MEM_OP_NO_DUMMY,
>>>                                  SPI_MEM_OP_DATA_IN(1, fsr, 0));
>>> 
>>> -             if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>>> +             if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>>>                       op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>>>                       op.dummy.nbytes = nor->params->rdsr_dummy;
>>>                       /*
>>> @@ -2488,7 +2491,7 @@ static int spi_nor_set_addr_width(struct 
>>> spi_nor
>>> *nor)
>>>  {
>>>       if (nor->addr_width) {
>>>               /* already configured from SFDP */
>>> -     } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>>> +     } else if (spi_nor_protocol_is_octal_dtr(nor->read_proto)) {
>>>               /*
>>>                * In 8D-8D-8D mode, one byte takes half a cycle to 
>>> transfer. So
>>>                * in this protocol an odd address width cannot be used 
>>> because
>>> @@ -2701,6 +2704,19 @@ static void spi_nor_init_fixup_flags(struct
>>> spi_nor *nor)
>>>               nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>>>  }
>>> 
>>> +static void spi_nor_set_dtr_bswap16_ops(struct spi_nor *nor)
>>> +{
>>> +     struct spi_nor_flash_parameter *params = nor->params;
>>> +     u32 mask = SNOR_HWCAPS_READ_8_8_8_DTR | 
>>> SNOR_HWCAPS_PP_8_8_8_DTR;
>>> +
>>> +     if ((params->hwcaps.mask & mask) == mask) {
>>> +             params->reads[SNOR_CMD_READ_8_8_8_DTR].proto |=
>>> +                     SNOR_PROTO_IS_DTR_BSWAP16;
>>> +             params->page_programs[SNOR_CMD_PP_8_8_8_DTR].proto |=
>>> +                     SNOR_PROTO_IS_DTR_BSWAP16;
>>> +     }
>>> +}
>>> +
>>>  /**
>>>   * spi_nor_late_init_params() - Late initialization of default flash
>>> parameters.
>>>   * @nor:     pointer to a 'struct spi_nor'
>>> @@ -2721,6 +2737,9 @@ static void spi_nor_late_init_params(struct
>>> spi_nor *nor)
>>>       spi_nor_init_flags(nor);
>>>       spi_nor_init_fixup_flags(nor);
>>> 
>>> +     if (nor->flags & SNOR_F_DTR_BSWAP16)
>>> +             spi_nor_set_dtr_bswap16_ops(nor);
>>> +
>>>       /*
>>>        * NOR protection support. When locking_ops are not provided, 
>>> we pick
>>>        * the default ones.
>>> @@ -2899,8 +2918,8 @@ static int spi_nor_octal_dtr_enable(struct
>>> spi_nor *nor, bool enable)
>>>       if (!nor->params->octal_dtr_enable)
>>>               return 0;
>>> 
>>> -     if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
>>> -           nor->write_proto == SNOR_PROTO_8_8_8_DTR))
>>> +     if (!(spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>>> +           spi_nor_protocol_is_octal_dtr(nor->write_proto)))
>>>               return 0;
>>> 
>>>       if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
>>> @@ -2968,7 +2987,7 @@ static int spi_nor_init(struct spi_nor *nor)
>>>               spi_nor_try_unlock_all(nor);
>>> 
>>>       if (nor->addr_width == 4 &&
>>> -         nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
>>> +         !spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>>>           !(nor->flags & SNOR_F_4B_OPCODES)) {
>>>               /*
>>>                * If the RESET# pin isn't hooked up properly, or the 
>>> system
>>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>>> index 2afb610853a9..7c077d41c335 100644
>>> --- a/drivers/mtd/spi-nor/core.h
>>> +++ b/drivers/mtd/spi-nor/core.h
>>> @@ -29,6 +29,7 @@ enum spi_nor_option_flags {
>>>       SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
>>>       SNOR_F_SOFT_RESET       = BIT(15),
>>>       SNOR_F_SWP_IS_VOLATILE  = BIT(16),
>>> +     SNOR_F_DTR_BSWAP16      = BIT(17),
>>>  };
>>> 
>>>  struct spi_nor_read_command {
>>> diff --git a/include/linux/mtd/spi-nor.h 
>>> b/include/linux/mtd/spi-nor.h
>>> index fc90fce26e33..6e9660475c5b 100644
>>> --- a/include/linux/mtd/spi-nor.h
>>> +++ b/include/linux/mtd/spi-nor.h
>>> @@ -168,6 +168,11 @@
>>>        SNOR_PROTO_DATA_MASK)
>>> 
>>>  #define SNOR_PROTO_IS_DTR    BIT(24) /* Double Transfer Rate */
>>> +/*
>>> + * Byte order of 16-bit words is swapped when read or written in DTR
>>> mode
>>> + * compared to STR mode.
>>> + */
>>> +#define SNOR_PROTO_IS_DTR_BSWAP16    BIT(25)
>>> 
>>>  #define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits)        
>>> \
>>>       (SNOR_PROTO_INST(_inst_nbits) |                         \
>>> @@ -201,6 +206,18 @@ static inline bool spi_nor_protocol_is_dtr(enum
>>> spi_nor_protocol proto)
>>>       return !!(proto & SNOR_PROTO_IS_DTR);
>>>  }
>>> 
>>> +static inline bool spi_nor_protocol_is_octal_dtr(enum 
>>> spi_nor_protocol
>>> proto)
>>> +{
>>> +     return ((proto & SNOR_PROTO_8_8_8_DTR) == 
>>> SNOR_PROTO_8_8_8_DTR);
>> 
>> This looks wrong what if there are 0's in SNOR_PROTO_8_8_8_DTR? If 
>> this
>> happens to be the same as SNOR_PROTO_MASK (which doesn't exist) this
>> deserves a comment.
> 
> I'm not sure I understand the comment. SNOR_PROTO_8_8_8_DTR has value 
> 0x80808.
> This method is added to cover the classical 8D-8D-8D mode and the 
> 8D-8D-8D mode
> with bytes swapped. This method will return true for both cases.

I know it should cover both cases, or that is what I dedcuded because
you moved the simple compare into a helper. It works in this case,
because all values just have mutually exclusive bits, thus I think this
deserves a comment.

Usually, you'd mask a field and then compare it with a value. So I'd
have expected sth like:

#define MASK (SNOR_PROTO_INST_MASK | SNOR_PROTO_ADDR_MASK | 
SNOR_PROTO_DATA_MASK)
return proto & (MASK | SNOR_PROTO_IS_DTR) == SNOR_PROTO_8_8_8_DTR;


>>> +}
>>> +
>>> +static inline bool spi_nor_protocol_is_dtr_bswap16(enum
>>> spi_nor_protocol proto)
>>> +{
>>> +     u32 mask = SNOR_PROTO_IS_DTR | SNOR_PROTO_IS_DTR_BSWAP16;
>>> +
>>> +     return ((proto & mask) == mask);
>> 
>> isn't "return proto & SNOR_PROTO_IS_DTR_BSWAP16;" enough here?
> 
> Byte swap can be done only on DTR modes. SNOR_PROTO_IS_DTR_BSWAP16
> without SNOR_PROTO_IS_DTR doesn't make sense. This method includes
> this sanity check.

I don't think this is the best place for sanity checks TBH.

-michael

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

[Michael Walle]

Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
> On 2/22/22 16:13, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>> On 2/21/22 09:44, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>>> Fortunately there are controllers
>>>>> that can swap back the bytes at runtime, fixing the endiannesses.
>>>>> Provide
>>>>> a way for the upper layers to specify the byte order in DTR mode.
>>>> 
>>>> Are there any patches for the atmel-quadspi yet? What happens if
>>> 
>>> not public, but will publish them these days.
>>> 
>>>> the controller doesn't support it? Will there be a software 
>>>> fallback?
>>> 
>>> no need for a fallback, the controller can ignore 
>>> op->data.dtr_bswap16
>>> if
>>> it can't swap bytes.
>> 
>> I don't understand. If the controller doesn't swap the 16bit values,
>> you will read the wrong content, no?
>> 
> 
> In linux no, because macronix swaps bytes on a 2 byte boundary both on
> reads and on page program. The problem is when you mix 8D-8D-8D mode 
> and
> 1-1-1 mode along the boot stages. Let's assume you write all boot 
> binaries
> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, when 
> u-boot
> will try to get the kernel it will fail, as the flash swaps the bytes 
> compared
> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 1-1-1 
> mode and
> when reaching u-boot you will read D1 D0 D3 D2 and it will mess the
> kernel image.

But you have to consider also 3rd parties, like an external programmer 
or
another OS. So, there has to be *one correct* way of writing/reading 
these
bytes.

-michael

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

[Tudor.Ambarus]

On 2/22/22 16:27, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>> On 2/22/22 16:13, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>>> On 2/21/22 09:44, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>>>> Fortunately there are controllers
>>>>>> that can swap back the bytes at runtime, fixing the endiannesses.
>>>>>> Provide
>>>>>> a way for the upper layers to specify the byte order in DTR mode.
>>>>>
>>>>> Are there any patches for the atmel-quadspi yet? What happens if
>>>>
>>>> not public, but will publish them these days.
>>>>
>>>>> the controller doesn't support it? Will there be a software
>>>>> fallback?
>>>>
>>>> no need for a fallback, the controller can ignore
>>>> op->data.dtr_bswap16
>>>> if
>>>> it can't swap bytes.
>>>
>>> I don't understand. If the controller doesn't swap the 16bit values,
>>> you will read the wrong content, no?
>>>
>>
>> In linux no, because macronix swaps bytes on a 2 byte boundary both on
>> reads and on page program. The problem is when you mix 8D-8D-8D mode
>> and
>> 1-1-1 mode along the boot stages. Let's assume you write all boot
>> binaries
>> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, when
>> u-boot
>> will try to get the kernel it will fail, as the flash swaps the bytes
>> compared
>> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 1-1-1
>> mode and
>> when reaching u-boot you will read D1 D0 D3 D2 and it will mess the
>> kernel image.
> 
> But you have to consider also 3rd parties, like an external programmer
> or

Why? If you use the same mode when reading and writing, everything is fine.
I'm not sure what's your suggestion here.

> another OS. So, there has to be *one correct* way of writing/reading
> these
> bytes.
> 

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

[Pratyush Yadav]

Hi Tudor,

On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
> On 2/22/22 16:27, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
> >> On 2/22/22 16:13, Michael Walle wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> >>> the content is safe
> >>>
> >>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
> >>>> On 2/21/22 09:44, Michael Walle wrote:
> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>>>> know
> >>>>> the content is safe
> >>>>>
> >>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
> >>>>>> Fortunately there are controllers
> >>>>>> that can swap back the bytes at runtime, fixing the endiannesses.
> >>>>>> Provide
> >>>>>> a way for the upper layers to specify the byte order in DTR mode.
> >>>>>
> >>>>> Are there any patches for the atmel-quadspi yet? What happens if
> >>>>
> >>>> not public, but will publish them these days.
> >>>>
> >>>>> the controller doesn't support it? Will there be a software
> >>>>> fallback?
> >>>>
> >>>> no need for a fallback, the controller can ignore
> >>>> op->data.dtr_bswap16
> >>>> if
> >>>> it can't swap bytes.
> >>>
> >>> I don't understand. If the controller doesn't swap the 16bit values,
> >>> you will read the wrong content, no?
> >>>
> >>
> >> In linux no, because macronix swaps bytes on a 2 byte boundary both on
> >> reads and on page program. The problem is when you mix 8D-8D-8D mode
> >> and
> >> 1-1-1 mode along the boot stages. Let's assume you write all boot
> >> binaries
> >> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, when
> >> u-boot
> >> will try to get the kernel it will fail, as the flash swaps the bytes
> >> compared
> >> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 1-1-1
> >> mode and
> >> when reaching u-boot you will read D1 D0 D3 D2 and it will mess the
> >> kernel image.
> > 
> > But you have to consider also 3rd parties, like an external programmer
> > or
> 
> Why? If you use the same mode when reading and writing, everything is fine.
> I'm not sure what's your suggestion here.

So our stance here is that we don't care about external programs?

If that is the case then why bother with all this anyway? Since the swap 
happens at both page program and read, what you write is what you read 
back. Who cares the order stored in the actual flash memory as long as 
the data read is correct?

If we do care about external programs, then what would happen if the
external program writes data in 8D-8D-8D mode _without_ swapping the 
bytes? This would also cause data corruption. You can't control what 
they mode they use, and you can't detect it later either.

I think there is no winning here. You just have to say that external 
programs should write in 8D-8D-8D mode or it won't boot.

> 
> > another OS. So, there has to be *one correct* way of writing/reading
> > these
> > bytes.
> > 
> 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Tudor.Ambarus]

On 2/23/22 20:38, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
>> On 2/22/22 16:27, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>>>> On 2/22/22 16:13, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>>> the content is safe
>>>>>
>>>>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>>>>> On 2/21/22 09:44, Michael Walle wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>>> know
>>>>>>> the content is safe
>>>>>>>
>>>>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>>>>>> Fortunately there are controllers
>>>>>>>> that can swap back the bytes at runtime, fixing the endiannesses.
>>>>>>>> Provide
>>>>>>>> a way for the upper layers to specify the byte order in DTR mode.
>>>>>>>
>>>>>>> Are there any patches for the atmel-quadspi yet? What happens if
>>>>>>
>>>>>> not public, but will publish them these days.
>>>>>>
>>>>>>> the controller doesn't support it? Will there be a software
>>>>>>> fallback?
>>>>>>
>>>>>> no need for a fallback, the controller can ignore
>>>>>> op->data.dtr_bswap16
>>>>>> if
>>>>>> it can't swap bytes.
>>>>>
>>>>> I don't understand. If the controller doesn't swap the 16bit values,
>>>>> you will read the wrong content, no?
>>>>>
>>>>
>>>> In linux no, because macronix swaps bytes on a 2 byte boundary both on
>>>> reads and on page program. The problem is when you mix 8D-8D-8D mode
>>>> and
>>>> 1-1-1 mode along the boot stages. Let's assume you write all boot
>>>> binaries
>>>> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, when
>>>> u-boot
>>>> will try to get the kernel it will fail, as the flash swaps the bytes
>>>> compared
>>>> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 1-1-1
>>>> mode and
>>>> when reaching u-boot you will read D1 D0 D3 D2 and it will mess the
>>>> kernel image.
>>>
>>> But you have to consider also 3rd parties, like an external programmer
>>> or
>>
>> Why? If you use the same mode when reading and writing, everything is fine.
>> I'm not sure what's your suggestion here.
> 
> So our stance here is that we don't care about external programs?> 
> If that is the case then why bother with all this anyway? Since the swap
> happens at both page program and read, what you write is what you read
> back. Who cares the order stored in the actual flash memory as long as
> the data read is correct?
> 
> If we do care about external programs, then what would happen if the
> external program writes data in 8D-8D-8D mode _without_ swapping the
> bytes? This would also cause data corruption. You can't control what
> they mode they use, and you can't detect it later either.
> 
> I think there is no winning here. You just have to say that external
> programs should write in 8D-8D-8D mode or it won't boot.
> 

How about swapping the bytes just at user request? Maybe with a Kconfig
option.

>>
>>> another OS. So, there has to be *one correct* way of writing/reading
>>> these
>>> bytes.
>>>
>>
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.

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

[Tudor.Ambarus]

On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 2/23/22 20:38, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Hi Tudor,
>>
>> On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
>>> On 2/22/22 16:27, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>>>>> On 2/22/22 16:13, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>>>>> the content is safe
>>>>>>
>>>>>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>>>>>> On 2/21/22 09:44, Michael Walle wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>>>> know
>>>>>>>> the content is safe
>>>>>>>>
>>>>>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>>>>>>> Fortunately there are controllers
>>>>>>>>> that can swap back the bytes at runtime, fixing the endiannesses.
>>>>>>>>> Provide
>>>>>>>>> a way for the upper layers to specify the byte order in DTR mode.
>>>>>>>>
>>>>>>>> Are there any patches for the atmel-quadspi yet? What happens if
>>>>>>>
>>>>>>> not public, but will publish them these days.
>>>>>>>
>>>>>>>> the controller doesn't support it? Will there be a software
>>>>>>>> fallback?
>>>>>>>
>>>>>>> no need for a fallback, the controller can ignore
>>>>>>> op->data.dtr_bswap16
>>>>>>> if
>>>>>>> it can't swap bytes.
>>>>>>
>>>>>> I don't understand. If the controller doesn't swap the 16bit values,
>>>>>> you will read the wrong content, no?
>>>>>>
>>>>>
>>>>> In linux no, because macronix swaps bytes on a 2 byte boundary both on
>>>>> reads and on page program. The problem is when you mix 8D-8D-8D mode
>>>>> and
>>>>> 1-1-1 mode along the boot stages. Let's assume you write all boot
>>>>> binaries
>>>>> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, when
>>>>> u-boot
>>>>> will try to get the kernel it will fail, as the flash swaps the bytes
>>>>> compared
>>>>> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 1-1-1
>>>>> mode and
>>>>> when reaching u-boot you will read D1 D0 D3 D2 and it will mess the
>>>>> kernel image.
>>>>
>>>> But you have to consider also 3rd parties, like an external programmer
>>>> or
>>>
>>> Why? If you use the same mode when reading and writing, everything is fine.
>>> I'm not sure what's your suggestion here.
>>
>> So our stance here is that we don't care about external programs?>
>> If that is the case then why bother with all this anyway? Since the swap
>> happens at both page program and read, what you write is what you read
>> back. Who cares the order stored in the actual flash memory as long as
>> the data read is correct?
>>
>> If we do care about external programs, then what would happen if the
>> external program writes data in 8D-8D-8D mode _without_ swapping the
>> bytes? This would also cause data corruption. You can't control what
>> they mode they use, and you can't detect it later either.
>>
>> I think there is no winning here. You just have to say that external
>> programs should write in 8D-8D-8D mode or it won't boot.
>>
> 
> How about swapping the bytes just at user request? Maybe with a Kconfig
> option.

Michael has suggested on #irc to always swap the bytes: if the SPI controller
can't do it, to do it in software at SPI NOR level. I don't know what to say
about this, because JEDEC216 just informs the reader I guess:
"Byte order of 16-bit words is swapped when read in 8D-8D-8D mode compared to
1-1-1 mode.", this doesn't look like a hard request. The downside to doing
the swapping in software is performance penalty which will make macronix
users have second thoughts. I don't have a strong opinion, but I lean towards
doing the swap just at user request, regardless if I do it via the SPI controller
or in software.

I would love to hear Macronix's opinion.

Cheers,
ta

> 
>>>
>>>> another OS. So, there has to be *one correct* way of writing/reading
>>>> these
>>>> bytes.
>>>>
>>>
>>>
>>
>> --
>> Regards,
>> Pratyush Yadav
>> Texas Instruments Inc.
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Michael Walle]

Am 2022-02-24 07:37, schrieb Tudor.Ambarus@microchip.com:
> On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> On 2/23/22 20:38, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>> know the content is safe
>>> 
>>> Hi Tudor,
>>> 
>>> On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
>>>> On 2/22/22 16:27, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>>> know the content is safe
>>>>> 
>>>>> Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>>>>>> On 2/22/22 16:13, Michael Walle wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>>>>> know
>>>>>>> the content is safe
>>>>>>> 
>>>>>>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>>>>>>> On 2/21/22 09:44, Michael Walle wrote:
>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless 
>>>>>>>>> you
>>>>>>>>> know
>>>>>>>>> the content is safe
>>>>>>>>> 
>>>>>>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>>>>>>>> Fortunately there are controllers
>>>>>>>>>> that can swap back the bytes at runtime, fixing the 
>>>>>>>>>> endiannesses.
>>>>>>>>>> Provide
>>>>>>>>>> a way for the upper layers to specify the byte order in DTR 
>>>>>>>>>> mode.
>>>>>>>>> 
>>>>>>>>> Are there any patches for the atmel-quadspi yet? What happens 
>>>>>>>>> if
>>>>>>>> 
>>>>>>>> not public, but will publish them these days.
>>>>>>>> 
>>>>>>>>> the controller doesn't support it? Will there be a software
>>>>>>>>> fallback?
>>>>>>>> 
>>>>>>>> no need for a fallback, the controller can ignore
>>>>>>>> op->data.dtr_bswap16
>>>>>>>> if
>>>>>>>> it can't swap bytes.
>>>>>>> 
>>>>>>> I don't understand. If the controller doesn't swap the 16bit 
>>>>>>> values,
>>>>>>> you will read the wrong content, no?
>>>>>>> 
>>>>>> 
>>>>>> In linux no, because macronix swaps bytes on a 2 byte boundary 
>>>>>> both on
>>>>>> reads and on page program. The problem is when you mix 8D-8D-8D 
>>>>>> mode
>>>>>> and
>>>>>> 1-1-1 mode along the boot stages. Let's assume you write all boot
>>>>>> binaries
>>>>>> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode, 
>>>>>> when
>>>>>> u-boot
>>>>>> will try to get the kernel it will fail, as the flash swaps the 
>>>>>> bytes
>>>>>> compared
>>>>>> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in 
>>>>>> 1-1-1
>>>>>> mode and
>>>>>> when reaching u-boot you will read D1 D0 D3 D2 and it will mess 
>>>>>> the
>>>>>> kernel image.
>>>>> 
>>>>> But you have to consider also 3rd parties, like an external 
>>>>> programmer
>>>>> or
>>>> 
>>>> Why? If you use the same mode when reading and writing, everything 
>>>> is fine.
>>>> I'm not sure what's your suggestion here.
>>> 
>>> So our stance here is that we don't care about external programs?>
>>> If that is the case then why bother with all this anyway? Since the 
>>> swap
>>> happens at both page program and read, what you write is what you 
>>> read
>>> back. Who cares the order stored in the actual flash memory as long 
>>> as
>>> the data read is correct?
>>> 
>>> If we do care about external programs, then what would happen if the
>>> external program writes data in 8D-8D-8D mode _without_ swapping the
>>> bytes? This would also cause data corruption. You can't control what
>>> they mode they use, and you can't detect it later either.
>>> 
>>> I think there is no winning here. You just have to say that external
>>> programs should write in 8D-8D-8D mode or it won't boot.

IMHO it should just work that you can use 1S-1S-1S mode and 8D-8D-8D on 
the
same flash. After all, that is Tudor's use case. The ROM access the 
flash
in single bit mode and linux in 8D-8D-8D mode. Maybe u-boot will use 
quad
mode in between. All of these accesses should return the same flash
content.

>> How about swapping the bytes just at user request? Maybe with a 
>> Kconfig
>> option.
> 
> Michael has suggested on #irc to always swap the bytes: if the SPI 
> controller
> can't do it, to do it in software at SPI NOR level. I don't know what 
> to say
> about this, because JEDEC216 just informs the reader I guess:
> "Byte order of 16-bit words is swapped when read in 8D-8D-8D mode 
> compared to
> 1-1-1 mode.", this doesn't look like a hard request. The downside to 
> doing
> the swapping in software is performance penalty which will make 
> macronix
> users have second thoughts. I don't have a strong opinion, but I lean 
> towards
> doing the swap just at user request, regardless if I do it via the SPI
> controller
> or in software.

Just having and opt-in will be a mess in the future with flashes 
containing
byte swapped content and we can't even fix it and we will have to live 
with
that forever. IMHO right now is the best time to circumvent that 
scenario.
I don't have anything against make it user configurable, but it should 
be
an opt-out.

I haven't looked at any controllers who can do 8D-8D-8D accesses, maybe 
most
of them can do the swapping on their own? So if you don't want to 
support a
software fallback, then we should just say this mode isn't supported if
the controller can't do the byte swapping and we fall back to a slower 
mode.

-michael

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

[Tudor.Ambarus]

On 2/24/22 11:37, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-24 07:37, schrieb Tudor.Ambarus@microchip.com:
>> On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> On 2/23/22 20:38, Pratyush Yadav wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>> know the content is safe
>>>>
>>>> Hi Tudor,
>>>>
>>>> On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
>>>>> On 2/22/22 16:27, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>> know the content is safe
>>>>>>
>>>>>> Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>>>>>>> On 2/22/22 16:13, Michael Walle wrote:
>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>>>> know
>>>>>>>> the content is safe
>>>>>>>>
>>>>>>>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>>>>>>>> On 2/21/22 09:44, Michael Walle wrote:
>>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless
>>>>>>>>>> you
>>>>>>>>>> know
>>>>>>>>>> the content is safe
>>>>>>>>>>
>>>>>>>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>>>>>>>>>> Fortunately there are controllers
>>>>>>>>>>> that can swap back the bytes at runtime, fixing the
>>>>>>>>>>> endiannesses.
>>>>>>>>>>> Provide
>>>>>>>>>>> a way for the upper layers to specify the byte order in DTR
>>>>>>>>>>> mode.
>>>>>>>>>>
>>>>>>>>>> Are there any patches for the atmel-quadspi yet? What happens
>>>>>>>>>> if
>>>>>>>>>
>>>>>>>>> not public, but will publish them these days.
>>>>>>>>>
>>>>>>>>>> the controller doesn't support it? Will there be a software
>>>>>>>>>> fallback?
>>>>>>>>>
>>>>>>>>> no need for a fallback, the controller can ignore
>>>>>>>>> op->data.dtr_bswap16
>>>>>>>>> if
>>>>>>>>> it can't swap bytes.
>>>>>>>>
>>>>>>>> I don't understand. If the controller doesn't swap the 16bit
>>>>>>>> values,
>>>>>>>> you will read the wrong content, no?
>>>>>>>>
>>>>>>>
>>>>>>> In linux no, because macronix swaps bytes on a 2 byte boundary
>>>>>>> both on
>>>>>>> reads and on page program. The problem is when you mix 8D-8D-8D
>>>>>>> mode
>>>>>>> and
>>>>>>> 1-1-1 mode along the boot stages. Let's assume you write all boot
>>>>>>> binaries
>>>>>>> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode,
>>>>>>> when
>>>>>>> u-boot
>>>>>>> will try to get the kernel it will fail, as the flash swaps the
>>>>>>> bytes
>>>>>>> compared
>>>>>>> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in
>>>>>>> 1-1-1
>>>>>>> mode and
>>>>>>> when reaching u-boot you will read D1 D0 D3 D2 and it will mess
>>>>>>> the
>>>>>>> kernel image.
>>>>>>
>>>>>> But you have to consider also 3rd parties, like an external
>>>>>> programmer
>>>>>> or
>>>>>
>>>>> Why? If you use the same mode when reading and writing, everything
>>>>> is fine.
>>>>> I'm not sure what's your suggestion here.
>>>>
>>>> So our stance here is that we don't care about external programs?>
>>>> If that is the case then why bother with all this anyway? Since the
>>>> swap
>>>> happens at both page program and read, what you write is what you
>>>> read
>>>> back. Who cares the order stored in the actual flash memory as long
>>>> as
>>>> the data read is correct?
>>>>
>>>> If we do care about external programs, then what would happen if the
>>>> external program writes data in 8D-8D-8D mode _without_ swapping the
>>>> bytes? This would also cause data corruption. You can't control what
>>>> they mode they use, and you can't detect it later either.
>>>>
>>>> I think there is no winning here. You just have to say that external
>>>> programs should write in 8D-8D-8D mode or it won't boot.
> 
> IMHO it should just work that you can use 1S-1S-1S mode and 8D-8D-8D on
> the
> same flash. After all, that is Tudor's use case. The ROM access the
> flash
> in single bit mode and linux in 8D-8D-8D mode. Maybe u-boot will use
> quad
> mode in between. All of these accesses should return the same flash
> content.
> 
>>> How about swapping the bytes just at user request? Maybe with a
>>> Kconfig
>>> option.
>>
>> Michael has suggested on #irc to always swap the bytes: if the SPI
>> controller
>> can't do it, to do it in software at SPI NOR level. I don't know what
>> to say
>> about this, because JEDEC216 just informs the reader I guess:
>> "Byte order of 16-bit words is swapped when read in 8D-8D-8D mode
>> compared to
>> 1-1-1 mode.", this doesn't look like a hard request. The downside to
>> doing
>> the swapping in software is performance penalty which will make
>> macronix
>> users have second thoughts. I don't have a strong opinion, but I lean
>> towards
>> doing the swap just at user request, regardless if I do it via the SPI
>> controller
>> or in software.
> 
> Just having and opt-in will be a mess in the future with flashes
> containing
> byte swapped content and we can't even fix it and we will have to live
> with
> that forever. IMHO right now is the best time to circumvent that
> scenario.
> I don't have anything against make it user configurable, but it should
> be
> an opt-out.
> 

sounds good to me

> I haven't looked at any controllers who can do 8D-8D-8D accesses, maybe
> most
> of them can do the swapping on their own? So if you don't want to
> support a
> software fallback, then we should just say this mode isn't supported if
> the controller can't do the byte swapping and we fall back to a slower
> mode.

Software fallback or mode downgrade - both are good ideas.
Pratyush, can your Octal SPI controller swap bytes on a 16 bit boundary?

The only debate that we have is whether to always swap (or downgrade),
thus to have the same byte order as in 1-1-1, or to introduce a Kconfig option
that will opt-out the swap, isn't it? Kconfig is a bit uglier, but more flexible,
and we still don't know for sure if the swap is mandatory or not. Can someone from
macronix shed some light on this topic?

Cheers,
ta

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

[Pratyush Yadav]

On 24/02/22 10:37AM, Michael Walle wrote:
> Am 2022-02-24 07:37, schrieb Tudor.Ambarus@microchip.com:
> > On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > On 2/23/22 20:38, Pratyush Yadav wrote:
> > > > EXTERNAL EMAIL: Do not click links or open attachments unless
> > > > you know the content is safe
> > > > 
> > > > Hi Tudor,
> > > > 
> > > > On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
> > > > > On 2/22/22 16:27, Michael Walle wrote:
> > > > > > EXTERNAL EMAIL: Do not click links or open attachments
> > > > > > unless you know the content is safe
> > > > > > 
> > > > > > Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
> > > > > > > On 2/22/22 16:13, Michael Walle wrote:
> > > > > > > > EXTERNAL EMAIL: Do not click links or open
> > > > > > > > attachments unless you know
> > > > > > > > the content is safe
> > > > > > > > 
> > > > > > > > Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
> > > > > > > > > On 2/21/22 09:44, Michael Walle wrote:
> > > > > > > > > > EXTERNAL EMAIL: Do not click links or
> > > > > > > > > > open attachments unless you
> > > > > > > > > > know
> > > > > > > > > > the content is safe
> > > > > > > > > > 
> > > > > > > > > > Am 2022-02-18 15:58, schrieb Tudor Ambarus:
> > > > > > > > > > > Fortunately there are controllers
> > > > > > > > > > > that can swap back the bytes at
> > > > > > > > > > > runtime, fixing the endiannesses.
> > > > > > > > > > > Provide
> > > > > > > > > > > a way for the upper layers to
> > > > > > > > > > > specify the byte order in DTR mode.
> > > > > > > > > > 
> > > > > > > > > > Are there any patches for the
> > > > > > > > > > atmel-quadspi yet? What happens if
> > > > > > > > > 
> > > > > > > > > not public, but will publish them these days.
> > > > > > > > > 
> > > > > > > > > > the controller doesn't support it? Will there be a software
> > > > > > > > > > fallback?
> > > > > > > > > 
> > > > > > > > > no need for a fallback, the controller can ignore
> > > > > > > > > op->data.dtr_bswap16
> > > > > > > > > if
> > > > > > > > > it can't swap bytes.
> > > > > > > > 
> > > > > > > > I don't understand. If the controller doesn't
> > > > > > > > swap the 16bit values,
> > > > > > > > you will read the wrong content, no?
> > > > > > > > 
> > > > > > > 
> > > > > > > In linux no, because macronix swaps bytes on a 2
> > > > > > > byte boundary both on
> > > > > > > reads and on page program. The problem is when you
> > > > > > > mix 8D-8D-8D mode
> > > > > > > and
> > > > > > > 1-1-1 mode along the boot stages. Let's assume you write all boot
> > > > > > > binaries
> > > > > > > in 1-1-1 mode. When reaching u-boot if you enable
> > > > > > > 8D-8D-8D mode, when
> > > > > > > u-boot
> > > > > > > will try to get the kernel it will fail, as the
> > > > > > > flash swaps the bytes
> > > > > > > compared
> > > > > > > to what was written with 1-1-1 mode. You write D0 D1
> > > > > > > D2 D3 in 1-1-1
> > > > > > > mode and
> > > > > > > when reaching u-boot you will read D1 D0 D3 D2 and
> > > > > > > it will mess the
> > > > > > > kernel image.
> > > > > > 
> > > > > > But you have to consider also 3rd parties, like an
> > > > > > external programmer
> > > > > > or
> > > > > 
> > > > > Why? If you use the same mode when reading and writing,
> > > > > everything is fine.
> > > > > I'm not sure what's your suggestion here.
> > > > 
> > > > So our stance here is that we don't care about external programs?>
> > > > If that is the case then why bother with all this anyway? Since
> > > > the swap
> > > > happens at both page program and read, what you write is what
> > > > you read
> > > > back. Who cares the order stored in the actual flash memory as
> > > > long as
> > > > the data read is correct?
> > > > 
> > > > If we do care about external programs, then what would happen if the
> > > > external program writes data in 8D-8D-8D mode _without_ swapping the
> > > > bytes? This would also cause data corruption. You can't control what
> > > > they mode they use, and you can't detect it later either.
> > > > 
> > > > I think there is no winning here. You just have to say that external
> > > > programs should write in 8D-8D-8D mode or it won't boot.
> 
> IMHO it should just work that you can use 1S-1S-1S mode and 8D-8D-8D on the
> same flash. After all, that is Tudor's use case. The ROM access the flash
> in single bit mode and linux in 8D-8D-8D mode. Maybe u-boot will use quad

But you don't know that ROM will always access the flash in single bit 
mode. For example, ROM on some TI SoC can read SFDP and use 8D-8D-8D 
mode for reading images from flash. If you want to flash data from 
Linux, and it byte swaps, ROM won't be able to read the images properly.

This can only work when everything that reads/writes in 8D mode does 
byte swapping. Otherwise it will lead to a mess where data is read 
correctly by some software but not by some other software. I don't know 
how practical it is to make this assumption.

> mode in between. All of these accesses should return the same flash
> content.
> 
> > > How about swapping the bytes just at user request? Maybe with a
> > > Kconfig
> > > option.
> > 
> > Michael has suggested on #irc to always swap the bytes: if the SPI
> > controller
> > can't do it, to do it in software at SPI NOR level. I don't know what to
> > say
> > about this, because JEDEC216 just informs the reader I guess:
> > "Byte order of 16-bit words is swapped when read in 8D-8D-8D mode
> > compared to
> > 1-1-1 mode.", this doesn't look like a hard request. The downside to
> > doing
> > the swapping in software is performance penalty which will make macronix
> > users have second thoughts. I don't have a strong opinion, but I lean
> > towards
> > doing the swap just at user request, regardless if I do it via the SPI
> > controller
> > or in software.
> 
> Just having and opt-in will be a mess in the future with flashes containing
> byte swapped content and we can't even fix it and we will have to live with
> that forever. IMHO right now is the best time to circumvent that scenario.
> I don't have anything against make it user configurable, but it should be
> an opt-out.
> 
> I haven't looked at any controllers who can do 8D-8D-8D accesses, maybe most
> of them can do the swapping on their own? So if you don't want to support a

I checked the datasheet of the Cadence Quadspi (spi-cadence-quadspi.c) 
controller. I don't see any such option.

> software fallback, then we should just say this mode isn't supported if
> the controller can't do the byte swapping and we fall back to a slower mode.

From all I understand of this, it looks to me that this can't really be 
solved completely. If you want to allow compatibility with 1S-1S-1S mode 
then you lose compatibility with 8D-8D-8D software that doesn't do this 
swap. So the question really is which one we consider "more important". 
In my eyes the choice is arbitrary.

But I am not convinced that adding a Kconfig option is the right thing 
to do. I think that would cause too much confusion. It is entirely 
possible that your data gets corrupted going from one kernel version to 
another depending on how it was compiled. Us SPI NOR developers know 
this tiny detail but other people won't, and it would be hard to explain 
this to them.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Michael Walle]

Am 2022-02-24 14:24, schrieb Pratyush Yadav:
> On 24/02/22 10:37AM, Michael Walle wrote:
>> Am 2022-02-24 07:37, schrieb Tudor.Ambarus@microchip.com:
>> > On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
>> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
>> > > know the content is safe
>> > >
>> > > On 2/23/22 20:38, Pratyush Yadav wrote:
>> > > > EXTERNAL EMAIL: Do not click links or open attachments unless
>> > > > you know the content is safe
>> > > >
>> > > > Hi Tudor,
>> > > >
>> > > > On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
>> > > > > On 2/22/22 16:27, Michael Walle wrote:
>> > > > > > EXTERNAL EMAIL: Do not click links or open attachments
>> > > > > > unless you know the content is safe
>> > > > > >
>> > > > > > Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>> > > > > > > On 2/22/22 16:13, Michael Walle wrote:
>> > > > > > > > EXTERNAL EMAIL: Do not click links or open
>> > > > > > > > attachments unless you know
>> > > > > > > > the content is safe
>> > > > > > > >
>> > > > > > > > Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>> > > > > > > > > On 2/21/22 09:44, Michael Walle wrote:
>> > > > > > > > > > EXTERNAL EMAIL: Do not click links or
>> > > > > > > > > > open attachments unless you
>> > > > > > > > > > know
>> > > > > > > > > > the content is safe
>> > > > > > > > > >
>> > > > > > > > > > Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>> > > > > > > > > > > Fortunately there are controllers
>> > > > > > > > > > > that can swap back the bytes at
>> > > > > > > > > > > runtime, fixing the endiannesses.
>> > > > > > > > > > > Provide
>> > > > > > > > > > > a way for the upper layers to
>> > > > > > > > > > > specify the byte order in DTR mode.
>> > > > > > > > > >
>> > > > > > > > > > Are there any patches for the
>> > > > > > > > > > atmel-quadspi yet? What happens if
>> > > > > > > > >
>> > > > > > > > > not public, but will publish them these days.
>> > > > > > > > >
>> > > > > > > > > > the controller doesn't support it? Will there be a software
>> > > > > > > > > > fallback?
>> > > > > > > > >
>> > > > > > > > > no need for a fallback, the controller can ignore
>> > > > > > > > > op->data.dtr_bswap16
>> > > > > > > > > if
>> > > > > > > > > it can't swap bytes.
>> > > > > > > >
>> > > > > > > > I don't understand. If the controller doesn't
>> > > > > > > > swap the 16bit values,
>> > > > > > > > you will read the wrong content, no?
>> > > > > > > >
>> > > > > > >
>> > > > > > > In linux no, because macronix swaps bytes on a 2
>> > > > > > > byte boundary both on
>> > > > > > > reads and on page program. The problem is when you
>> > > > > > > mix 8D-8D-8D mode
>> > > > > > > and
>> > > > > > > 1-1-1 mode along the boot stages. Let's assume you write all boot
>> > > > > > > binaries
>> > > > > > > in 1-1-1 mode. When reaching u-boot if you enable
>> > > > > > > 8D-8D-8D mode, when
>> > > > > > > u-boot
>> > > > > > > will try to get the kernel it will fail, as the
>> > > > > > > flash swaps the bytes
>> > > > > > > compared
>> > > > > > > to what was written with 1-1-1 mode. You write D0 D1
>> > > > > > > D2 D3 in 1-1-1
>> > > > > > > mode and
>> > > > > > > when reaching u-boot you will read D1 D0 D3 D2 and
>> > > > > > > it will mess the
>> > > > > > > kernel image.
>> > > > > >
>> > > > > > But you have to consider also 3rd parties, like an
>> > > > > > external programmer
>> > > > > > or
>> > > > >
>> > > > > Why? If you use the same mode when reading and writing,
>> > > > > everything is fine.
>> > > > > I'm not sure what's your suggestion here.
>> > > >
>> > > > So our stance here is that we don't care about external programs?>
>> > > > If that is the case then why bother with all this anyway? Since
>> > > > the swap
>> > > > happens at both page program and read, what you write is what
>> > > > you read
>> > > > back. Who cares the order stored in the actual flash memory as
>> > > > long as
>> > > > the data read is correct?
>> > > >
>> > > > If we do care about external programs, then what would happen if the
>> > > > external program writes data in 8D-8D-8D mode _without_ swapping the
>> > > > bytes? This would also cause data corruption. You can't control what
>> > > > they mode they use, and you can't detect it later either.
>> > > >
>> > > > I think there is no winning here. You just have to say that external
>> > > > programs should write in 8D-8D-8D mode or it won't boot.
>> 
>> IMHO it should just work that you can use 1S-1S-1S mode and 8D-8D-8D 
>> on the
>> same flash. After all, that is Tudor's use case. The ROM access the 
>> flash
>> in single bit mode and linux in 8D-8D-8D mode. Maybe u-boot will use 
>> quad
> 
> But you don't know that ROM will always access the flash in single bit
> mode. For example, ROM on some TI SoC can read SFDP and use 8D-8D-8D
> mode for reading images from flash. If you want to flash data from
> Linux, and it byte swaps, ROM won't be able to read the images 
> properly.

Then I'd argue your ROM code is broken because it doesn't respect
the SFDP bit which tells you the data is swapped. I'm not implying
we should ignore that case.

> This can only work when everything that reads/writes in 8D mode does
> byte swapping. Otherwise it will lead to a mess where data is read
> correctly by some software but not by some other software. I don't know
> how practical it is to make this assumption.

What assumption, that everyone reads it the same way and swap the bytes
if necessary?

>> mode in between. All of these accesses should return the same flash
>> content.
>> 
>> > > How about swapping the bytes just at user request? Maybe with a
>> > > Kconfig
>> > > option.
>> >
>> > Michael has suggested on #irc to always swap the bytes: if the SPI
>> > controller
>> > can't do it, to do it in software at SPI NOR level. I don't know what to
>> > say
>> > about this, because JEDEC216 just informs the reader I guess:
>> > "Byte order of 16-bit words is swapped when read in 8D-8D-8D mode
>> > compared to
>> > 1-1-1 mode.", this doesn't look like a hard request. The downside to
>> > doing
>> > the swapping in software is performance penalty which will make macronix
>> > users have second thoughts. I don't have a strong opinion, but I lean
>> > towards
>> > doing the swap just at user request, regardless if I do it via the SPI
>> > controller
>> > or in software.
>> 
>> Just having and opt-in will be a mess in the future with flashes 
>> containing
>> byte swapped content and we can't even fix it and we will have to live 
>> with
>> that forever. IMHO right now is the best time to circumvent that 
>> scenario.
>> I don't have anything against make it user configurable, but it should 
>> be
>> an opt-out.
>> 
>> I haven't looked at any controllers who can do 8D-8D-8D accesses, 
>> maybe most
>> of them can do the swapping on their own? So if you don't want to 
>> support a
> 
> I checked the datasheet of the Cadence Quadspi (spi-cadence-quadspi.c)
> controller. I don't see any such option.

I've also checked the flexspi, doesn't have such an option either.

>> software fallback, then we should just say this mode isn't supported 
>> if
>> the controller can't do the byte swapping and we fall back to a slower 
>> mode.
> 
> From all I understand of this, it looks to me that this can't really be
> solved completely. If you want to allow compatibility with 1S-1S-1S 
> mode
> then you lose compatibility with 8D-8D-8D software that doesn't do this
> swap. So the question really is which one we consider "more important".
> In my eyes the choice is arbitrary.

We need a reference. And IMHO this reference is that if the SFDP
tells us the bytes are swapped, we need to swap em in 8D-8D-8D,
any software which deviates from that is broken; which doesn't
mean we should not try to be compatible with it. But we - as in the
SPI-NOR subsystem - should not be broken too and maybe we are
getting to be the reference..

Is there any sofware yet where we can lose compatibility with? This
patch series will break it anyway if you are using this combination
of atmel qspi controller and macronix flash. So apparently we don't
care about that. Yes there might be some fallout now, but if we just
ignore the problem now, the fallout later might be even bigger.

Imagine, someone with an SPI controller without swapping comes
along and want to use that macronix flash with a boot rom doing
single bit accesses. It doesn't work, does it? So, what we are
doing then?

> But I am not convinced that adding a Kconfig option is the right thing
> to do. I think that would cause too much confusion. It is entirely
> possible that your data gets corrupted going from one kernel version to
> another depending on how it was compiled. Us SPI NOR developers know
> this tiny detail but other people won't, and it would be hard to 
> explain
> this to them.

I don't think a Kconfig is the way to go here neither. What if you
have two flashes and you want one with and one without?

-michael

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

[Tudor.Ambarus]

On 2/24/22 16:02, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-24 14:24, schrieb Pratyush Yadav:
>> On 24/02/22 10:37AM, Michael Walle wrote:
>>> Am 2022-02-24 07:37, schrieb Tudor.Ambarus@microchip.com:
>>> > On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
>>> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
>>> > > know the content is safe
>>> > >
>>> > > On 2/23/22 20:38, Pratyush Yadav wrote:
>>> > > > EXTERNAL EMAIL: Do not click links or open attachments unless
>>> > > > you know the content is safe
>>> > > >
>>> > > > Hi Tudor,
>>> > > >
>>> > > > On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
>>> > > > > On 2/22/22 16:27, Michael Walle wrote:
>>> > > > > > EXTERNAL EMAIL: Do not click links or open attachments
>>> > > > > > unless you know the content is safe
>>> > > > > >
>>> > > > > > Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
>>> > > > > > > On 2/22/22 16:13, Michael Walle wrote:
>>> > > > > > > > EXTERNAL EMAIL: Do not click links or open
>>> > > > > > > > attachments unless you know
>>> > > > > > > > the content is safe
>>> > > > > > > >
>>> > > > > > > > Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
>>> > > > > > > > > On 2/21/22 09:44, Michael Walle wrote:
>>> > > > > > > > > > EXTERNAL EMAIL: Do not click links or
>>> > > > > > > > > > open attachments unless you
>>> > > > > > > > > > know
>>> > > > > > > > > > the content is safe
>>> > > > > > > > > >
>>> > > > > > > > > > Am 2022-02-18 15:58, schrieb Tudor Ambarus:
>>> > > > > > > > > > > Fortunately there are controllers
>>> > > > > > > > > > > that can swap back the bytes at
>>> > > > > > > > > > > runtime, fixing the endiannesses.
>>> > > > > > > > > > > Provide
>>> > > > > > > > > > > a way for the upper layers to
>>> > > > > > > > > > > specify the byte order in DTR mode.
>>> > > > > > > > > >
>>> > > > > > > > > > Are there any patches for the
>>> > > > > > > > > > atmel-quadspi yet? What happens if
>>> > > > > > > > >
>>> > > > > > > > > not public, but will publish them these days.
>>> > > > > > > > >
>>> > > > > > > > > > the controller doesn't support it? Will there be a software
>>> > > > > > > > > > fallback?
>>> > > > > > > > >
>>> > > > > > > > > no need for a fallback, the controller can ignore
>>> > > > > > > > > op->data.dtr_bswap16
>>> > > > > > > > > if
>>> > > > > > > > > it can't swap bytes.
>>> > > > > > > >
>>> > > > > > > > I don't understand. If the controller doesn't
>>> > > > > > > > swap the 16bit values,
>>> > > > > > > > you will read the wrong content, no?
>>> > > > > > > >
>>> > > > > > >
>>> > > > > > > In linux no, because macronix swaps bytes on a 2
>>> > > > > > > byte boundary both on
>>> > > > > > > reads and on page program. The problem is when you
>>> > > > > > > mix 8D-8D-8D mode
>>> > > > > > > and
>>> > > > > > > 1-1-1 mode along the boot stages. Let's assume you write all boot
>>> > > > > > > binaries
>>> > > > > > > in 1-1-1 mode. When reaching u-boot if you enable
>>> > > > > > > 8D-8D-8D mode, when
>>> > > > > > > u-boot
>>> > > > > > > will try to get the kernel it will fail, as the
>>> > > > > > > flash swaps the bytes
>>> > > > > > > compared
>>> > > > > > > to what was written with 1-1-1 mode. You write D0 D1
>>> > > > > > > D2 D3 in 1-1-1
>>> > > > > > > mode and
>>> > > > > > > when reaching u-boot you will read D1 D0 D3 D2 and
>>> > > > > > > it will mess the
>>> > > > > > > kernel image.
>>> > > > > >
>>> > > > > > But you have to consider also 3rd parties, like an
>>> > > > > > external programmer
>>> > > > > > or
>>> > > > >
>>> > > > > Why? If you use the same mode when reading and writing,
>>> > > > > everything is fine.
>>> > > > > I'm not sure what's your suggestion here.
>>> > > >
>>> > > > So our stance here is that we don't care about external programs?>
>>> > > > If that is the case then why bother with all this anyway? Since
>>> > > > the swap
>>> > > > happens at both page program and read, what you write is what
>>> > > > you read
>>> > > > back. Who cares the order stored in the actual flash memory as
>>> > > > long as
>>> > > > the data read is correct?
>>> > > >
>>> > > > If we do care about external programs, then what would happen if the
>>> > > > external program writes data in 8D-8D-8D mode _without_ swapping the
>>> > > > bytes? This would also cause data corruption. You can't control what
>>> > > > they mode they use, and you can't detect it later either.
>>> > > >
>>> > > > I think there is no winning here. You just have to say that external
>>> > > > programs should write in 8D-8D-8D mode or it won't boot.
>>>
>>> IMHO it should just work that you can use 1S-1S-1S mode and 8D-8D-8D
>>> on the
>>> same flash. After all, that is Tudor's use case. The ROM access the
>>> flash
>>> in single bit mode and linux in 8D-8D-8D mode. Maybe u-boot will use
>>> quad
>>
>> But you don't know that ROM will always access the flash in single bit
>> mode. For example, ROM on some TI SoC can read SFDP and use 8D-8D-8D
>> mode for reading images from flash. If you want to flash data from
>> Linux, and it byte swaps, ROM won't be able to read the images
>> properly.
> 
> Then I'd argue your ROM code is broken because it doesn't respect
> the SFDP bit which tells you the data is swapped. I'm not implying
> we should ignore that case.
> 
>> This can only work when everything that reads/writes in 8D mode does
>> byte swapping. Otherwise it will lead to a mess where data is read
>> correctly by some software but not by some other software. I don't know
>> how practical it is to make this assumption.
> 
> What assumption, that everyone reads it the same way and swap the bytes
> if necessary?
> 
>>> mode in between. All of these accesses should return the same flash
>>> content.
>>>
>>> > > How about swapping the bytes just at user request? Maybe with a
>>> > > Kconfig
>>> > > option.
>>> >
>>> > Michael has suggested on #irc to always swap the bytes: if the SPI
>>> > controller
>>> > can't do it, to do it in software at SPI NOR level. I don't know what to
>>> > say
>>> > about this, because JEDEC216 just informs the reader I guess:
>>> > "Byte order of 16-bit words is swapped when read in 8D-8D-8D mode
>>> > compared to
>>> > 1-1-1 mode.", this doesn't look like a hard request. The downside to
>>> > doing
>>> > the swapping in software is performance penalty which will make macronix
>>> > users have second thoughts. I don't have a strong opinion, but I lean
>>> > towards
>>> > doing the swap just at user request, regardless if I do it via the SPI
>>> > controller
>>> > or in software.
>>>
>>> Just having and opt-in will be a mess in the future with flashes
>>> containing
>>> byte swapped content and we can't even fix it and we will have to live
>>> with
>>> that forever. IMHO right now is the best time to circumvent that
>>> scenario.
>>> I don't have anything against make it user configurable, but it should
>>> be
>>> an opt-out.
>>>
>>> I haven't looked at any controllers who can do 8D-8D-8D accesses,
>>> maybe most
>>> of them can do the swapping on their own? So if you don't want to
>>> support a
>>
>> I checked the datasheet of the Cadence Quadspi (spi-cadence-quadspi.c)
>> controller. I don't see any such option.
> 
> I've also checked the flexspi, doesn't have such an option either.
> 
>>> software fallback, then we should just say this mode isn't supported
>>> if
>>> the controller can't do the byte swapping and we fall back to a slower
>>> mode.
>>
>> From all I understand of this, it looks to me that this can't really be
>> solved completely. If you want to allow compatibility with 1S-1S-1S
>> mode
>> then you lose compatibility with 8D-8D-8D software that doesn't do this
>> swap. So the question really is which one we consider "more important".
>> In my eyes the choice is arbitrary.
> 
> We need a reference. And IMHO this reference is that if the SFDP
> tells us the bytes are swapped, we need to swap em in 8D-8D-8D,
> any software which deviates from that is broken; which doesn't
> mean we should not try to be compatible with it. But we - as in the
> SPI-NOR subsystem - should not be broken too and maybe we are
> getting to be the reference..
> 
> Is there any sofware yet where we can lose compatibility with? This
> patch series will break it anyway if you are using this combination
> of atmel qspi controller and macronix flash. So apparently we don't
> care about that. Yes there might be some fallout now, but if we just

I know an example of RomCode supporting 1-1-1 and the other boot stages
handling the flash in either 1-1-1 or 8D-8D-8D. The problem is real
and I do care.

> ignore the problem now, the fallout later might be even bigger.
> 
> Imagine, someone with an SPI controller without swapping comes
> along and want to use that macronix flash with a boot rom doing
> single bit accesses. It doesn't work, does it? So, what we are

It won't work, sure.

> doing then?

That's what we're trying to address.

> 
>> But I am not convinced that adding a Kconfig option is the right thing
>> to do. I think that would cause too much confusion. It is entirely
>> possible that your data gets corrupted going from one kernel version to
>> another depending on how it was compiled. Us SPI NOR developers know
>> this tiny detail but other people won't, and it would be hard to
>> explain
>> this to them.
> 
> I don't think a Kconfig is the way to go here neither. What if you
> have two flashes and you want one with and one without?
> 

Is this use case real?
The first thing to answer is whether we want to introduce a configuration
option that allows users to choose whether to swap the bytes or not.
If we want to make it configurable, we can't use dt properties as those
should describe the hw and not configure it. What other options do we have?

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

[zhengxunli]

Hi all,

<Tudor.Ambarus@microchip.com> wrote on 2022/02/24 下午 06:27:57:

> <Tudor.Ambarus@microchip.com> 
> 2022/02/24 下午 06:28
> 
> To
> 
> <michael@walle.cc>, <p.yadav@ti.com>, 
> 
> cc
> 
> <p.yadav@ti.com>, <broonie@kernel.org>, <miquel.raynal@bootlin.com>,
> <richard@nod.at>, <vigneshr@ti.com>, <linux-
> mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>, <linux-
> spi@vger.kernel.org>, <Nicolas.Ferre@microchip.com>, 
> <zhengxunli@mxic.com.tw>, <jaimeliao@mxic.com.tw>
> 
> Subject
> 
> Re: [PATCH 0/4] spi-mem: Allow specifying the byte order in DTR mode
> 
> On 2/24/22 11:37, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you 
> know the content is safe
> > 
> > Am 2022-02-24 07:37, schrieb Tudor.Ambarus@microchip.com:
> >> On 2/24/22 08:08, Tudor.Ambarus@microchip.com wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
know
> >>> the content is safe
> >>>
> >>> On 2/23/22 20:38, Pratyush Yadav wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>>> know the content is safe
> >>>>
> >>>> Hi Tudor,
> >>>>
> >>>> On 22/02/22 02:43PM, Tudor.Ambarus@microchip.com wrote:
> >>>>> On 2/22/22 16:27, Michael Walle wrote:
> >>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> >>>>>> know the content is safe
> >>>>>>
> >>>>>> Am 2022-02-22 15:23, schrieb Tudor.Ambarus@microchip.com:
> >>>>>>> On 2/22/22 16:13, Michael Walle wrote:
> >>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless 
you
> >>>>>>>> know
> >>>>>>>> the content is safe
> >>>>>>>>
> >>>>>>>> Am 2022-02-22 14:54, schrieb Tudor.Ambarus@microchip.com:
> >>>>>>>>> On 2/21/22 09:44, Michael Walle wrote:
> >>>>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless
> >>>>>>>>>> you
> >>>>>>>>>> know
> >>>>>>>>>> the content is safe
> >>>>>>>>>>
> >>>>>>>>>> Am 2022-02-18 15:58, schrieb Tudor Ambarus:
> >>>>>>>>>>> Fortunately there are controllers
> >>>>>>>>>>> that can swap back the bytes at runtime, fixing the
> >>>>>>>>>>> endiannesses.
> >>>>>>>>>>> Provide
> >>>>>>>>>>> a way for the upper layers to specify the byte order in DTR
> >>>>>>>>>>> mode.
> >>>>>>>>>>
> >>>>>>>>>> Are there any patches for the atmel-quadspi yet? What happens
> >>>>>>>>>> if
> >>>>>>>>>
> >>>>>>>>> not public, but will publish them these days.
> >>>>>>>>>
> >>>>>>>>>> the controller doesn't support it? Will there be a software
> >>>>>>>>>> fallback?
> >>>>>>>>>
> >>>>>>>>> no need for a fallback, the controller can ignore
> >>>>>>>>> op->data.dtr_bswap16
> >>>>>>>>> if
> >>>>>>>>> it can't swap bytes.
> >>>>>>>>
> >>>>>>>> I don't understand. If the controller doesn't swap the 16bit
> >>>>>>>> values,
> >>>>>>>> you will read the wrong content, no?
> >>>>>>>>
> >>>>>>>
> >>>>>>> In linux no, because macronix swaps bytes on a 2 byte boundary
> >>>>>>> both on
> >>>>>>> reads and on page program. The problem is when you mix 8D-8D-8D
> >>>>>>> mode
> >>>>>>> and
> >>>>>>> 1-1-1 mode along the boot stages. Let's assume you write all 
boot
> >>>>>>> binaries
> >>>>>>> in 1-1-1 mode. When reaching u-boot if you enable 8D-8D-8D mode,
> >>>>>>> when
> >>>>>>> u-boot
> >>>>>>> will try to get the kernel it will fail, as the flash swaps the
> >>>>>>> bytes
> >>>>>>> compared
> >>>>>>> to what was written with 1-1-1 mode. You write D0 D1 D2 D3 in
> >>>>>>> 1-1-1
> >>>>>>> mode and
> >>>>>>> when reaching u-boot you will read D1 D0 D3 D2 and it will mess
> >>>>>>> the
> >>>>>>> kernel image.
> >>>>>>
> >>>>>> But you have to consider also 3rd parties, like an external
> >>>>>> programmer
> >>>>>> or
> >>>>>
> >>>>> Why? If you use the same mode when reading and writing, everything
> >>>>> is fine.
> >>>>> I'm not sure what's your suggestion here.
> >>>>
> >>>> So our stance here is that we don't care about external programs?>
> >>>> If that is the case then why bother with all this anyway? Since the
> >>>> swap
> >>>> happens at both page program and read, what you write is what you
> >>>> read
> >>>> back. Who cares the order stored in the actual flash memory as long
> >>>> as
> >>>> the data read is correct?
> >>>>
> >>>> If we do care about external programs, then what would happen if 
the
> >>>> external program writes data in 8D-8D-8D mode _without_ swapping 
the
> >>>> bytes? This would also cause data corruption. You can't control 
what
> >>>> they mode they use, and you can't detect it later either.
> >>>>
> >>>> I think there is no winning here. You just have to say that 
external
> >>>> programs should write in 8D-8D-8D mode or it won't boot.
> > 
> > IMHO it should just work that you can use 1S-1S-1S mode and 8D-8D-8D 
on
> > the
> > same flash. After all, that is Tudor's use case. The ROM access the
> > flash
> > in single bit mode and linux in 8D-8D-8D mode. Maybe u-boot will use
> > quad
> > mode in between. All of these accesses should return the same flash
> > content.
> > 
> >>> How about swapping the bytes just at user request? Maybe with a
> >>> Kconfig
> >>> option.
> >>
> >> Michael has suggested on #irc to always swap the bytes: if the SPI
> >> controller
> >> can't do it, to do it in software at SPI NOR level. I don't know what
> >> to say
> >> about this, because JEDEC216 just informs the reader I guess:
> >> "Byte order of 16-bit words is swapped when read in 8D-8D-8D mode
> >> compared to
> >> 1-1-1 mode.", this doesn't look like a hard request. The downside to
> >> doing
> >> the swapping in software is performance penalty which will make
> >> macronix
> >> users have second thoughts. I don't have a strong opinion, but I lean
> >> towards
> >> doing the swap just at user request, regardless if I do it via the 
SPI
> >> controller
> >> or in software.
> > 
> > Just having and opt-in will be a mess in the future with flashes
> > containing
> > byte swapped content and we can't even fix it and we will have to live
> > with
> > that forever. IMHO right now is the best time to circumvent that
> > scenario.
> > I don't have anything against make it user configurable, but it should
> > be
> > an opt-out.
> > 
> 
> sounds good to me
> 
> > I haven't looked at any controllers who can do 8D-8D-8D accesses, 
maybe
> > most
> > of them can do the swapping on their own? So if you don't want to
> > support a
> > software fallback, then we should just say this mode isn't supported 
if
> > the controller can't do the byte swapping and we fall back to a slower
> > mode.
> 
> Software fallback or mode downgrade - both are good ideas.
> Pratyush, can your Octal SPI controller swap bytes on a 16 bit boundary?
> 
> The only debate that we have is whether to always swap (or downgrade),
> thus to have the same byte order as in 1-1-1, or to introduce a Kconfig 
option
> that will opt-out the swap, isn't it? Kconfig is a bit uglier, but 
> more flexible,
> and we still don't know for sure if the swap is mandatory or not. 
> Can someone from
> macronix shed some light on this topic?

The macronix in 8D-8D-8D mode always has to swap data during read and 
program
operations. Unfortunately this is our limitation, swap data at the flash 
layer
reduces performance and does not support dirmap mode. If the SPI 
controllers
all support swap data, everything is fine, but as far as I know, this is 
rare.

All in all, your opinions and comments are valuable. Moreover the learned 
lessons
could be input to next generation of OctaFlash.

Thanks,
Zhengxun


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

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

[Pratyush Yadav]

Hi Tudor,

I'm reviewing the code here. I still have not thought through the 
discussion about Kconfig option yet.

On 18/02/22 04:58PM, Tudor Ambarus wrote:
> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
> when configured in DTR mode. The byte order of 16-bit words is swapped

s/DTR mode/ Octal DTR mode/

I don't think this would apply to a 4D-4D-4D flash since it would only 
transmit one byte per clock cycle.

> when read or written in Double Transfer Rate (DTR) mode compared to
> Single Transfer Rate (STR) mode. 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 this 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.
> Fortunately there are controllers that can swap back the bytes at runtime,
> fixing the endiannesses. Provide a way for the upper layers to specify the
> byte order in DTR mode.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  include/linux/spi/spi-mem.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 85e2ff7b840d..e1878417420c 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_bswap16: whether the byte order of 16-bit words is swapped when
> + *		      read or written in DTR mode compared to STR mode.
>   * @data.dir: direction of the transfer
>   * @data.nbytes: number of data bytes to send/receive. Can be zero if the
>   *		 operation does not involve transferring data
> @@ -119,6 +121,7 @@ struct spi_mem_op {
>  	struct {
>  		u8 buswidth;
>  		u8 dtr : 1;
> +		u8 dtr_bswap16 : 1;

You also need to add this capability to spi_controller_mem_caps and 
update spi_mem_default_supports_op() to check for it.

>  		enum spi_mem_data_dir dir;
>  		unsigned int nbytes;
>  		union {
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Pratyush Yadav]

Hi Tudor,

On 18/02/22 04:58PM, Tudor Ambarus wrote:
> 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 write written in
> 8D-8D-8D mode compared to STR modes. Swapping the bytes is a bad design
> decision because this may affect the boot sequence if the entire boot
> sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode. Allow
> operations to specify the byte order in DTR mode, so that controllers can
> swap the bytes back at run-time to fix the endianness, if they are capable.
> 
> The byte order in 8D-8D-8D mode can be retrieved at run-time by checking
> BFPT[DWORD(18)] BIT(31). When set to one, the "Byte order of 16-bit words
> is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.". It doesn't
> specify if this applies to both register and data operations. Macronix is
> the single user of this byte swap and it doesn't have clear rules, as it
> contains register operations that require data swap (RDPASS, WRPASS,
> PASSULK, RDSFDP) and register operations that don't require data swap
> (WRFBR). All these are not common and can be handled in 1-1-1 mode, so we
> can ignore them for now. All the other register operations are done on one
> byte length. The read register operations are actually in 8D-8D-8S mode,
> as they send the data value twice, on each half of the clock cycle. In case
> of a register write of one byte, the memory supports receiving the register
> value only on the first byte, thus it discards the value of the byte on the
> second half of the clock cycle. Swapping the bytes for one byte register
> writes is not required, and for one byte register reads it doesn't matter.
> Thus swap the bytes only for read or page program operations.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 31 +++++++++++++++++++++++++------
>  drivers/mtd/spi-nor/core.h  |  1 +
>  include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
>  3 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 04ea180118e3..453d8c54d062 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -106,6 +106,9 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>  		op->dummy.dtr = true;
>  		op->data.dtr = true;
>  
> +		if (spi_nor_protocol_is_dtr_bswap16(proto))
> +			op->data.dtr_bswap16 = true;
> +
>  		/* 2 bytes per clock cycle in DTR mode. */
>  		op->dummy.nbytes *= 2;
>  
> @@ -388,7 +391,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_DATA_IN(1, sr, 0));
>  
> -		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +		if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>  			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>  			op.dummy.nbytes = nor->params->rdsr_dummy;
>  			/*
> @@ -432,7 +435,7 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_DATA_IN(1, fsr, 0));
>  
> -		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +		if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>  			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>  			op.dummy.nbytes = nor->params->rdsr_dummy;
>  			/*
> @@ -2488,7 +2491,7 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
>  {
>  	if (nor->addr_width) {
>  		/* already configured from SFDP */
> -	} else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
> +	} else if (spi_nor_protocol_is_octal_dtr(nor->read_proto)) {
>  		/*
>  		 * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
>  		 * in this protocol an odd address width cannot be used because
> @@ -2701,6 +2704,19 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>  		nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>  }
>  
> +static void spi_nor_set_dtr_bswap16_ops(struct spi_nor *nor)
> +{
> +	struct spi_nor_flash_parameter *params = nor->params;
> +	u32 mask = SNOR_HWCAPS_READ_8_8_8_DTR | SNOR_HWCAPS_PP_8_8_8_DTR;
> +
> +	if ((params->hwcaps.mask & mask) == mask) {
> +		params->reads[SNOR_CMD_READ_8_8_8_DTR].proto |=
> +			SNOR_PROTO_IS_DTR_BSWAP16;
> +		params->page_programs[SNOR_CMD_PP_8_8_8_DTR].proto |=
> +			SNOR_PROTO_IS_DTR_BSWAP16;
> +	}
> +}
> +
>  /**
>   * spi_nor_late_init_params() - Late initialization of default flash parameters.
>   * @nor:	pointer to a 'struct spi_nor'
> @@ -2721,6 +2737,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>  	spi_nor_init_flags(nor);
>  	spi_nor_init_fixup_flags(nor);
>  
> +	if (nor->flags & SNOR_F_DTR_BSWAP16)
> +		spi_nor_set_dtr_bswap16_ops(nor);
> +
>  	/*
>  	 * NOR protection support. When locking_ops are not provided, we pick
>  	 * the default ones.
> @@ -2899,8 +2918,8 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
>  	if (!nor->params->octal_dtr_enable)
>  		return 0;
>  
> -	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
> -	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
> +	if (!(spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
> +	      spi_nor_protocol_is_octal_dtr(nor->write_proto)))
>  		return 0;
>  
>  	if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
> @@ -2968,7 +2987,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  		spi_nor_try_unlock_all(nor);
>  
>  	if (nor->addr_width == 4 &&
> -	    nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
> +	    !spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>  	    !(nor->flags & SNOR_F_4B_OPCODES)) {
>  		/*
>  		 * If the RESET# pin isn't hooked up properly, or the system
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 2afb610853a9..7c077d41c335 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -29,6 +29,7 @@ enum spi_nor_option_flags {
>  	SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
>  	SNOR_F_SOFT_RESET	= BIT(15),
>  	SNOR_F_SWP_IS_VOLATILE	= BIT(16),
> +	SNOR_F_DTR_BSWAP16	= BIT(17),
>  };
>  
>  struct spi_nor_read_command {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index fc90fce26e33..6e9660475c5b 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -168,6 +168,11 @@
>  	 SNOR_PROTO_DATA_MASK)
>  
>  #define SNOR_PROTO_IS_DTR	BIT(24)	/* Double Transfer Rate */
> +/*
> + * Byte order of 16-bit words is swapped when read or written in DTR mode
> + * compared to STR mode.
> + */
> +#define SNOR_PROTO_IS_DTR_BSWAP16	BIT(25)

I am not so sure if the protocol is the best place to encode this. The 
protocol stays the same regardless of the data organisation. Maybe all 
we need to do is add something like

static inline bool
spi_nor_needs_bswap16(struct spi_nor *nor, enum spi_nor_protocol proto)
{
	return (proto == SNOR_PROTO_8_8_8_DTR) && (nor->flags & SNOR_F_DTR_BSWAP16);
}

And then call it from spi_nor_spimem_setup_op(). Thoughts?

>  
>  #define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits)	\
>  	(SNOR_PROTO_INST(_inst_nbits) |				\
> @@ -201,6 +206,18 @@ static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
>  	return !!(proto & SNOR_PROTO_IS_DTR);
>  }
>  
> +static inline bool spi_nor_protocol_is_octal_dtr(enum spi_nor_protocol proto)
> +{
> +	return ((proto & SNOR_PROTO_8_8_8_DTR) == SNOR_PROTO_8_8_8_DTR);
> +}
> +
> +static inline bool spi_nor_protocol_is_dtr_bswap16(enum spi_nor_protocol proto)
> +{
> +	u32 mask = SNOR_PROTO_IS_DTR | SNOR_PROTO_IS_DTR_BSWAP16;
> +
> +	return ((proto & mask) == mask);
> +}
> +
>  static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol proto)
>  {
>  	return ((unsigned long)(proto & SNOR_PROTO_INST_MASK)) >>
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Pratyush Yadav]

On 18/02/22 04:58PM, Tudor Ambarus wrote:
> 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: Pratyush Yadav <p.yadav@ti.com>

> ---
>  drivers/mtd/spi-nor/sfdp.c | 3 +++
>  drivers/mtd/spi-nor/sfdp.h | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..551edbb039f0 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -633,6 +633,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_BSWAP16;
> +
>  	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt);
>  }
>  
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index bbf80d2990ab..9a834ea31c16 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -97,6 +97,7 @@ struct sfdp_bfpt {
>  #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)
>  
>  struct sfdp_parameter_header {
>  	u8		id_lsb;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Pratyush Yadav]

On 18/02/22 04:59PM, Tudor Ambarus wrote:
> 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.

Is there any flash that currently needs this flag but does not define 
BFPT? If there is no user, let's not add it. It can always be added 
later.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 5 ++++-
>  drivers/mtd/spi-nor/core.h | 5 ++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 453d8c54d062..c3128a8e1544 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2572,7 +2572,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) {
> @@ -2613,6 +2613,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_BSWAP16)
> +		nor->flags |= SNOR_F_DTR_BSWAP16;
> +
>  	/*
>  	 * 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 7c077d41c335..1cb887437193 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -362,6 +362,8 @@ 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_BSWAP16:     the byte order of 16-bit words is swapped when
> + *			      read or written in 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
> @@ -404,7 +406,7 @@ struct flash_info {
>  #define USE_FSR				BIT(10)
>  #define SPI_NOR_XSR_RDY			BIT(11)
>  
> -	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)
> @@ -413,6 +415,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_BSWAP16		BIT(8)
>  
>  	u8 fixup_flags;
>  #define SPI_NOR_4B_OPCODES		BIT(0)
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

[Tudor.Ambarus]

On 3/2/22 14:30, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 18/02/22 04:59PM, Tudor Ambarus wrote:
>> 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.
> 
> Is there any flash that currently needs this flag but does not define
> BFPT? If there is no user, let's not add it. It can always be added
> later.

it's needed by mx66lm1g45g, the flash that I'm currently working on.
It doesn't define SFDP tables, at least the one that I currently have.
Let me add support for it in the next version.

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

[Tudor.Ambarus]

On 3/2/22 12:02, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,

Hi, Pratyush,

> 
> I'm reviewing the code here. I still have not thought through the
> discussion about Kconfig option yet.
> 
> On 18/02/22 04:58PM, Tudor Ambarus wrote:
>> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
>> when configured in DTR mode. The byte order of 16-bit words is swapped
> 
> s/DTR mode/ Octal DTR mode/
> 
> I don't think this would apply to a 4D-4D-4D flash since it would only
> transmit one byte per clock cycle.

From what I see, flashes that claim "QPI DTR support" they actually support
4S-4D-4D. JESD251-1 talks about 4S-4D-4D too. So data is latched on both rising
and falling edges of the clock. But I'm ok with your proposal because we don't
have any proof if there are any QPI DTR flashes that swap bytes in DTR.

> 
>> when read or written in Double Transfer Rate (DTR) mode compared to
>> Single Transfer Rate (STR) mode. 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 this 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.
>> Fortunately there are controllers that can swap back the bytes at runtime,
>> fixing the endiannesses. Provide a way for the upper layers to specify the
>> byte order in DTR mode.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  include/linux/spi/spi-mem.h | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
>> index 85e2ff7b840d..e1878417420c 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_bswap16: whether the byte order of 16-bit words is swapped when
>> + *                 read or written in DTR mode compared to STR mode.
>>   * @data.dir: direction of the transfer
>>   * @data.nbytes: number of data bytes to send/receive. Can be zero if the
>>   *            operation does not involve transferring data
>> @@ -119,6 +121,7 @@ struct spi_mem_op {
>>       struct {
>>               u8 buswidth;
>>               u8 dtr : 1;
>> +             u8 dtr_bswap16 : 1;

but I would keep this name here as it is, without prepending octal.

> 
> You also need to add this capability to spi_controller_mem_caps and
> update spi_mem_default_supports_op() to check for it.

sure, will do.

Thanks!
ta
> 
>>               enum spi_mem_data_dir dir;
>>               unsigned int nbytes;
>>               union {
>> --
>> 2.25.1
>>
> 
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.

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

[Tudor.Ambarus]

On 3/2/22 13:34, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,

Hi, Pratyush,

> 
> On 18/02/22 04:58PM, Tudor Ambarus wrote:
>> 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 write written in
>> 8D-8D-8D mode compared to STR modes. Swapping the bytes is a bad design
>> decision because this may affect the boot sequence if the entire boot
>> sequence is not handled in either 8D-8D-8D mode or 1-1-1 mode. Allow
>> operations to specify the byte order in DTR mode, so that controllers can
>> swap the bytes back at run-time to fix the endianness, if they are capable.
>>
>> The byte order in 8D-8D-8D mode can be retrieved at run-time by checking
>> BFPT[DWORD(18)] BIT(31). When set to one, the "Byte order of 16-bit words
>> is swapped when read in 8D-8D-8D mode compared to 1-1-1 mode.". It doesn't
>> specify if this applies to both register and data operations. Macronix is
>> the single user of this byte swap and it doesn't have clear rules, as it
>> contains register operations that require data swap (RDPASS, WRPASS,
>> PASSULK, RDSFDP) and register operations that don't require data swap
>> (WRFBR). All these are not common and can be handled in 1-1-1 mode, so we
>> can ignore them for now. All the other register operations are done on one
>> byte length. The read register operations are actually in 8D-8D-8S mode,
>> as they send the data value twice, on each half of the clock cycle. In case
>> of a register write of one byte, the memory supports receiving the register
>> value only on the first byte, thus it discards the value of the byte on the
>> second half of the clock cycle. Swapping the bytes for one byte register
>> writes is not required, and for one byte register reads it doesn't matter.
>> Thus swap the bytes only for read or page program operations.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/core.c  | 31 +++++++++++++++++++++++++------
>>  drivers/mtd/spi-nor/core.h  |  1 +
>>  include/linux/mtd/spi-nor.h | 17 +++++++++++++++++
>>  3 files changed, 43 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 04ea180118e3..453d8c54d062 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -106,6 +106,9 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
>>               op->dummy.dtr = true;
>>               op->data.dtr = true;
>>
>> +             if (spi_nor_protocol_is_dtr_bswap16(proto))
>> +                     op->data.dtr_bswap16 = true;
>> +
>>               /* 2 bytes per clock cycle in DTR mode. */
>>               op->dummy.nbytes *= 2;
>>
>> @@ -388,7 +391,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_DATA_IN(1, sr, 0));
>>
>> -             if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>> +             if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>>                       op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>>                       op.dummy.nbytes = nor->params->rdsr_dummy;
>>                       /*
>> @@ -432,7 +435,7 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
>>                                  SPI_MEM_OP_NO_DUMMY,
>>                                  SPI_MEM_OP_DATA_IN(1, fsr, 0));
>>
>> -             if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
>> +             if (spi_nor_protocol_is_octal_dtr(nor->reg_proto)) {
>>                       op.addr.nbytes = nor->params->rdsr_addr_nbytes;
>>                       op.dummy.nbytes = nor->params->rdsr_dummy;
>>                       /*
>> @@ -2488,7 +2491,7 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
>>  {
>>       if (nor->addr_width) {
>>               /* already configured from SFDP */
>> -     } else if (nor->read_proto == SNOR_PROTO_8_8_8_DTR) {
>> +     } else if (spi_nor_protocol_is_octal_dtr(nor->read_proto)) {
>>               /*
>>                * In 8D-8D-8D mode, one byte takes half a cycle to transfer. So
>>                * in this protocol an odd address width cannot be used because
>> @@ -2701,6 +2704,19 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>>               nor->flags |= SNOR_F_IO_MODE_EN_VOLATILE;
>>  }
>>
>> +static void spi_nor_set_dtr_bswap16_ops(struct spi_nor *nor)
>> +{
>> +     struct spi_nor_flash_parameter *params = nor->params;
>> +     u32 mask = SNOR_HWCAPS_READ_8_8_8_DTR | SNOR_HWCAPS_PP_8_8_8_DTR;
>> +
>> +     if ((params->hwcaps.mask & mask) == mask) {
>> +             params->reads[SNOR_CMD_READ_8_8_8_DTR].proto |=
>> +                     SNOR_PROTO_IS_DTR_BSWAP16;
>> +             params->page_programs[SNOR_CMD_PP_8_8_8_DTR].proto |=
>> +                     SNOR_PROTO_IS_DTR_BSWAP16;
>> +     }
>> +}
>> +
>>  /**
>>   * spi_nor_late_init_params() - Late initialization of default flash parameters.
>>   * @nor:     pointer to a 'struct spi_nor'
>> @@ -2721,6 +2737,9 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>>       spi_nor_init_flags(nor);
>>       spi_nor_init_fixup_flags(nor);
>>
>> +     if (nor->flags & SNOR_F_DTR_BSWAP16)
>> +             spi_nor_set_dtr_bswap16_ops(nor);
>> +
>>       /*
>>        * NOR protection support. When locking_ops are not provided, we pick
>>        * the default ones.
>> @@ -2899,8 +2918,8 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
>>       if (!nor->params->octal_dtr_enable)
>>               return 0;
>>
>> -     if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
>> -           nor->write_proto == SNOR_PROTO_8_8_8_DTR))
>> +     if (!(spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>> +           spi_nor_protocol_is_octal_dtr(nor->write_proto)))
>>               return 0;
>>
>>       if (!(nor->flags & SNOR_F_IO_MODE_EN_VOLATILE))
>> @@ -2968,7 +2987,7 @@ static int spi_nor_init(struct spi_nor *nor)
>>               spi_nor_try_unlock_all(nor);
>>
>>       if (nor->addr_width == 4 &&
>> -         nor->read_proto != SNOR_PROTO_8_8_8_DTR &&
>> +         !spi_nor_protocol_is_octal_dtr(nor->read_proto) &&
>>           !(nor->flags & SNOR_F_4B_OPCODES)) {
>>               /*
>>                * If the RESET# pin isn't hooked up properly, or the system
>> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
>> index 2afb610853a9..7c077d41c335 100644
>> --- a/drivers/mtd/spi-nor/core.h
>> +++ b/drivers/mtd/spi-nor/core.h
>> @@ -29,6 +29,7 @@ enum spi_nor_option_flags {
>>       SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
>>       SNOR_F_SOFT_RESET       = BIT(15),
>>       SNOR_F_SWP_IS_VOLATILE  = BIT(16),
>> +     SNOR_F_DTR_BSWAP16      = BIT(17),
>>  };
>>
>>  struct spi_nor_read_command {
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index fc90fce26e33..6e9660475c5b 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -168,6 +168,11 @@
>>        SNOR_PROTO_DATA_MASK)
>>
>>  #define SNOR_PROTO_IS_DTR    BIT(24) /* Double Transfer Rate */
>> +/*
>> + * Byte order of 16-bit words is swapped when read or written in DTR mode
>> + * compared to STR mode.
>> + */
>> +#define SNOR_PROTO_IS_DTR_BSWAP16    BIT(25)
> 
> I am not so sure if the protocol is the best place to encode this. The
> protocol stays the same regardless of the data organisation. Maybe all
> we need to do is add something like
> 
> static inline bool
> spi_nor_needs_bswap16(struct spi_nor *nor, enum spi_nor_protocol proto)
> {
>         return (proto == SNOR_PROTO_8_8_8_DTR) && (nor->flags & SNOR_F_DTR_BSWAP16);
> }
> 
> And then call it from spi_nor_spimem_setup_op(). Thoughts?
> 

I find it better. There's no such thing as a 8D-8D-8D-bswap16 in
the standard terminology, so I'll drop the protocol handling.

Thanks!
ta

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

[Pratyush Yadav]

On 10/03/22 05:31AM, Tudor.Ambarus@microchip.com wrote:
> On 3/2/22 12:02, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Hi Tudor,
> 
> Hi, Pratyush,
> 
> > 
> > I'm reviewing the code here. I still have not thought through the
> > discussion about Kconfig option yet.
> > 
> > On 18/02/22 04:58PM, Tudor Ambarus wrote:
> >> There are NOR flashes (Macronix) that swap the bytes on a 16-bit boundary
> >> when configured in DTR mode. The byte order of 16-bit words is swapped
> > 
> > s/DTR mode/ Octal DTR mode/
> > 
> > I don't think this would apply to a 4D-4D-4D flash since it would only
> > transmit one byte per clock cycle.
> 
> From what I see, flashes that claim "QPI DTR support" they actually support
> 4S-4D-4D. JESD251-1 talks about 4S-4D-4D too. So data is latched on both rising
> and falling edges of the clock. But I'm ok with your proposal because we don't
> have any proof if there are any QPI DTR flashes that swap bytes in DTR.

I think this problem fundamentally applies to Octal DTR and above (if 
there is ever 16-line DTR (hexadecimal DTR?) in the future). In your 4D 
data phase, you can only send _one_ byte per cycle. So the byte order 
inter-cycle does not matter as it does in 8D mode. Similarly, for a 
16-line STR this would also apply, since that has 2 bytes per cycle. For 
a 16-line DTR there are now 4 bytes per cycle and so on.

And the BFPT bit that you use to enable this swap also says "Byte order 
in 8D-8D-8D mode". So I really don't think it makes sense for QPI DTR.

> 
> > 
> >> when read or written in Double Transfer Rate (DTR) mode compared to
> >> Single Transfer Rate (STR) mode. 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 this 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.
> >> Fortunately there are controllers that can swap back the bytes at runtime,
> >> fixing the endiannesses. Provide a way for the upper layers to specify the
> >> byte order in DTR mode.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >>  include/linux/spi/spi-mem.h | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> >> index 85e2ff7b840d..e1878417420c 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_bswap16: whether the byte order of 16-bit words is swapped when
> >> + *                 read or written in DTR mode compared to STR mode.
> >>   * @data.dir: direction of the transfer
> >>   * @data.nbytes: number of data bytes to send/receive. Can be zero if the
> >>   *            operation does not involve transferring data
> >> @@ -119,6 +121,7 @@ struct spi_mem_op {
> >>       struct {
> >>               u8 buswidth;
> >>               u8 dtr : 1;
> >> +             u8 dtr_bswap16 : 1;
> 
> but I would keep this name here as it is, without prepending octal.

I won't nitpick much on the member name as long as the comment 
describing its role is clear enough.

> 
> > 
> > You also need to add this capability to spi_controller_mem_caps and
> > update spi_mem_default_supports_op() to check for it.
> 
> sure, will do.
> 
> Thanks!
> ta
> > 
> >>               enum spi_mem_data_dir dir;
> >>               unsigned int nbytes;
> >>               union {
> >> --
> >> 2.25.1
> >>
> > 
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.