linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
@ 2016-12-06 16:52 Cyrille Pitchen
  2016-12-06 16:52 ` [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes Cyrille Pitchen
  2016-12-06 16:52 ` [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib Cyrille Pitchen
  0 siblings, 2 replies; 10+ messages in thread
From: Cyrille Pitchen @ 2016-12-06 16:52 UTC (permalink / raw)
  To: marek.vasut-Re5JQEeQqe8AvxtiuMwx3w
  Cc: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	richard-/L3Ra7n9ekc, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-spi-u79uwXL29TY76Z2rM5mHXA, broonie-DgEjT+Ai2ygdnm+yROfE0A,
	Cyrille Pitchen

Hi all,

this series of patches is based on next-20161206 and can also be applied
to the git://github.com/spi-nor/linux.git tree.
It has been extracted from the SFDP series and is resent as a standalone
series.

This series fixes compatibility issue between Linux and many bootloaders
when using SPI flash with size greater than 128Mib.

Indeed, before this series, Linux used to make the SPI flash memory enter
its 4-byte address mode when its size is greater than 128Mib: The very
same Fast Read 1-1-z op code is used, for instance Fast Read 1-1-4 (6Bh)
but once in 4-byte address mode, the SPI flash memory now expects a 4-byte
address following the op code instead of a 3-byte address.
This solution is statefull: it changes the internal state of the memory.

Hence, when the CPU is reset but not the memory, many bootloaders are not
aware of this internal state change at the memory side, don't know how to
handle this and still send one Fast Read op code followed by a 3-byte
address. So the bootloader fails to read data from the SPI flash memory.

Cyrille Pitchen (2):
  mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes
  mtd: spi-nor: add a stateless method to support memory size above
    128Mib

 drivers/mtd/devices/serial_flash_cmds.h |   7 --
 drivers/mtd/devices/st_spi_fsm.c        |  28 ++++----
 drivers/mtd/spi-nor/spi-nor.c           | 114 ++++++++++++++++++++++++++------
 drivers/spi/spi-bcm-qspi.c              |   6 +-
 include/linux/mtd/spi-nor.h             |  22 ++++--
 5 files changed, 126 insertions(+), 51 deletions(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes
  2016-12-06 16:52 [PATCH 0/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib Cyrille Pitchen
@ 2016-12-06 16:52 ` Cyrille Pitchen
  2016-12-07  9:51   ` Mark Brown
  2016-12-06 16:52 ` [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib Cyrille Pitchen
  1 sibling, 1 reply; 10+ messages in thread
From: Cyrille Pitchen @ 2016-12-06 16:52 UTC (permalink / raw)
  To: marek.vasut
  Cc: computersforpeace, boris.brezillon, richard, linux-mtd,
	linux-kernel, linux-spi, broonie, Cyrille Pitchen

This patch renames the SPINOR_OP_* macros of the 4-byte address
instruction set so the new names all share a common pattern: the 4-byte
address name is built from the 3-byte address name appending the "_4B"
suffix.

The patch also introduces new op codes to support other SPI protocols such
as SPI 1-4-4 and SPI 1-2-2.

This is a transitional patch and will help a later patch of spi-nor.c
to automate the translation from the 3-byte address op codes into their
4-byte address version.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/mtd/devices/serial_flash_cmds.h |  7 -------
 drivers/mtd/devices/st_spi_fsm.c        | 28 ++++++++++++++--------------
 drivers/mtd/spi-nor/spi-nor.c           |  8 ++++----
 drivers/spi/spi-bcm-qspi.c              |  6 +++---
 include/linux/mtd/spi-nor.h             | 22 ++++++++++++++++------
 5 files changed, 37 insertions(+), 34 deletions(-)

diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h
index f59a125295d0..8b81e15105dd 100644
--- a/drivers/mtd/devices/serial_flash_cmds.h
+++ b/drivers/mtd/devices/serial_flash_cmds.h
@@ -18,19 +18,12 @@
 #define SPINOR_OP_RDVCR		0x85
 
 /* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */
-#define SPINOR_OP_READ_1_2_2	0xbb	/* DUAL I/O READ */
-#define SPINOR_OP_READ_1_4_4	0xeb	/* QUAD I/O READ */
-
 #define SPINOR_OP_WRITE		0x02	/* PAGE PROGRAM */
 #define SPINOR_OP_WRITE_1_1_2	0xa2	/* DUAL INPUT PROGRAM */
 #define SPINOR_OP_WRITE_1_2_2	0xd2	/* DUAL INPUT EXT PROGRAM */
 #define SPINOR_OP_WRITE_1_1_4	0x32	/* QUAD INPUT PROGRAM */
 #define SPINOR_OP_WRITE_1_4_4	0x12	/* QUAD INPUT EXT PROGRAM */
 
-/* READ commands with 32-bit addressing */
-#define SPINOR_OP_READ4_1_2_2	0xbc
-#define SPINOR_OP_READ4_1_4_4	0xec
-
 /* Configuration flags */
 #define FLASH_FLAG_SINGLE	0x000000ff
 #define FLASH_FLAG_READ_WRITE	0x00000001
diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 5454b4113589..804313a33f2b 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -507,13 +507,13 @@ static struct seq_rw_config n25q_read3_configs[] = {
  *	- 'FAST' variants configured for 8 dummy cycles (see note above.)
  */
 static struct seq_rw_config n25q_read4_configs[] = {
-	{FLASH_FLAG_READ_1_4_4, SPINOR_OP_READ4_1_4_4,	0, 4, 4, 0x00, 0, 8},
-	{FLASH_FLAG_READ_1_1_4, SPINOR_OP_READ4_1_1_4,	0, 1, 4, 0x00, 0, 8},
-	{FLASH_FLAG_READ_1_2_2, SPINOR_OP_READ4_1_2_2,	0, 2, 2, 0x00, 0, 8},
-	{FLASH_FLAG_READ_1_1_2, SPINOR_OP_READ4_1_1_2,	0, 1, 2, 0x00, 0, 8},
-	{FLASH_FLAG_READ_FAST,	SPINOR_OP_READ4_FAST,	0, 1, 1, 0x00, 0, 8},
-	{FLASH_FLAG_READ_WRITE, SPINOR_OP_READ4,	0, 1, 1, 0x00, 0, 0},
-	{0x00,			0,			0, 0, 0, 0x00, 0, 0},
+	{FLASH_FLAG_READ_1_4_4, SPINOR_OP_READ_1_4_4_4B, 0, 4, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_1_4, SPINOR_OP_READ_1_1_4_4B, 0, 1, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_2_2, SPINOR_OP_READ_1_2_2_4B, 0, 2, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_1_2, SPINOR_OP_READ_1_1_2_4B, 0, 1, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_FAST,	SPINOR_OP_READ_FAST_4B,  0, 1, 1, 0x00, 0, 8},
+	{FLASH_FLAG_READ_WRITE, SPINOR_OP_READ_4B,       0, 1, 1, 0x00, 0, 0},
+	{0x00,			0,                       0, 0, 0, 0x00, 0, 0},
 };
 
 /*
@@ -553,13 +553,13 @@ static int stfsm_mx25_en_32bit_addr_seq(struct stfsm_seq *seq)
  * entering a state that is incompatible with the SPIBoot Controller.
  */
 static struct seq_rw_config stfsm_s25fl_read4_configs[] = {
-	{FLASH_FLAG_READ_1_4_4,  SPINOR_OP_READ4_1_4_4,  0, 4, 4, 0x00, 2, 4},
-	{FLASH_FLAG_READ_1_1_4,  SPINOR_OP_READ4_1_1_4,  0, 1, 4, 0x00, 0, 8},
-	{FLASH_FLAG_READ_1_2_2,  SPINOR_OP_READ4_1_2_2,  0, 2, 2, 0x00, 4, 0},
-	{FLASH_FLAG_READ_1_1_2,  SPINOR_OP_READ4_1_1_2,  0, 1, 2, 0x00, 0, 8},
-	{FLASH_FLAG_READ_FAST,   SPINOR_OP_READ4_FAST,   0, 1, 1, 0x00, 0, 8},
-	{FLASH_FLAG_READ_WRITE,  SPINOR_OP_READ4,        0, 1, 1, 0x00, 0, 0},
-	{0x00,                   0,                      0, 0, 0, 0x00, 0, 0},
+	{FLASH_FLAG_READ_1_4_4,  SPINOR_OP_READ_1_4_4_4B,  0, 4, 4, 0x00, 2, 4},
+	{FLASH_FLAG_READ_1_1_4,  SPINOR_OP_READ_1_1_4_4B,  0, 1, 4, 0x00, 0, 8},
+	{FLASH_FLAG_READ_1_2_2,  SPINOR_OP_READ_1_2_2_4B,  0, 2, 2, 0x00, 4, 0},
+	{FLASH_FLAG_READ_1_1_2,  SPINOR_OP_READ_1_1_2_4B,  0, 1, 2, 0x00, 0, 8},
+	{FLASH_FLAG_READ_FAST,   SPINOR_OP_READ_FAST_4B,   0, 1, 1, 0x00, 0, 8},
+	{FLASH_FLAG_READ_WRITE,  SPINOR_OP_READ_4B,        0, 1, 1, 0x00, 0, 0},
+	{0x00,                   0,                        0, 0, 0, 0x00, 0, 0},
 };
 
 static struct seq_rw_config stfsm_s25fl_write4_configs[] = {
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 1fd32b991eb7..8abe134e174a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1490,16 +1490,16 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 			/* Dedicated 4-byte command set */
 			switch (nor->flash_read) {
 			case SPI_NOR_QUAD:
-				nor->read_opcode = SPINOR_OP_READ4_1_1_4;
+				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
 				break;
 			case SPI_NOR_DUAL:
-				nor->read_opcode = SPINOR_OP_READ4_1_1_2;
+				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
 				break;
 			case SPI_NOR_FAST:
-				nor->read_opcode = SPINOR_OP_READ4_FAST;
+				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
 				break;
 			case SPI_NOR_NORMAL:
-				nor->read_opcode = SPINOR_OP_READ4;
+				nor->read_opcode = SPINOR_OP_READ_4B;
 				break;
 			}
 			nor->program_opcode = SPINOR_OP_PP_4B;
diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 14f9dea3173f..d7843fd8c610 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -371,7 +371,7 @@ static int bcm_qspi_bspi_set_flex_mode(struct bcm_qspi *qspi, int width,
 			/* default mode, does not need flex_cmd */
 			flex_mode = 0;
 		else
-			command = SPINOR_OP_READ4_FAST;
+			command = SPINOR_OP_READ_FAST_4B;
 		break;
 	case SPI_NBITS_DUAL:
 		bpc = 0x00000001;
@@ -384,7 +384,7 @@ static int bcm_qspi_bspi_set_flex_mode(struct bcm_qspi *qspi, int width,
 		} else {
 			command = SPINOR_OP_READ_1_1_2;
 			if (spans_4byte)
-				command = SPINOR_OP_READ4_1_1_2;
+				command = SPINOR_OP_READ_1_1_2_4B;
 		}
 		break;
 	case SPI_NBITS_QUAD:
@@ -399,7 +399,7 @@ static int bcm_qspi_bspi_set_flex_mode(struct bcm_qspi *qspi, int width,
 		} else {
 			command = SPINOR_OP_READ_1_1_4;
 			if (spans_4byte)
-				command = SPINOR_OP_READ4_1_1_4;
+				command = SPINOR_OP_READ_1_1_4_4B;
 		}
 		break;
 	default:
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index c425c7b4c2a0..8b02fd7864d0 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -43,9 +43,13 @@
 #define SPINOR_OP_WRSR		0x01	/* Write status register 1 byte */
 #define SPINOR_OP_READ		0x03	/* Read data bytes (low frequency) */
 #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
-#define SPINOR_OP_READ_1_1_2	0x3b	/* Read data bytes (Dual SPI) */
-#define SPINOR_OP_READ_1_1_4	0x6b	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ_1_1_2	0x3b	/* Read data bytes (Dual Output SPI) */
+#define SPINOR_OP_READ_1_2_2	0xbb	/* Read data bytes (Dual I/O SPI) */
+#define SPINOR_OP_READ_1_1_4	0x6b	/* Read data bytes (Quad Output SPI) */
+#define SPINOR_OP_READ_1_4_4	0xeb	/* Read data bytes (Quad I/O SPI) */
 #define SPINOR_OP_PP		0x02	/* Page program (up to 256 bytes) */
+#define SPINOR_OP_PP_1_1_4	0x32	/* Quad page program */
+#define SPINOR_OP_PP_1_4_4	0x38	/* Quad page program */
 #define SPINOR_OP_BE_4K		0x20	/* Erase 4KiB block */
 #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
 #define SPINOR_OP_BE_32K	0x52	/* Erase 32KiB block */
@@ -56,11 +60,17 @@
 #define SPINOR_OP_RDFSR		0x70	/* Read flag status register */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
-#define SPINOR_OP_READ4		0x13	/* Read data bytes (low frequency) */
-#define SPINOR_OP_READ4_FAST	0x0c	/* Read data bytes (high frequency) */
-#define SPINOR_OP_READ4_1_1_2	0x3c	/* Read data bytes (Dual SPI) */
-#define SPINOR_OP_READ4_1_1_4	0x6c	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */
+#define SPINOR_OP_READ_FAST_4B	0x0c	/* Read data bytes (high frequency) */
+#define SPINOR_OP_READ_1_1_2_4B	0x3c	/* Read data bytes (Dual Output SPI) */
+#define SPINOR_OP_READ_1_2_2_4B	0xbc	/* Read data bytes (Dual I/O SPI) */
+#define SPINOR_OP_READ_1_1_4_4B	0x6c	/* Read data bytes (Quad Output SPI) */
+#define SPINOR_OP_READ_1_4_4_4B	0xec	/* Read data bytes (Quad I/O SPI) */
 #define SPINOR_OP_PP_4B		0x12	/* Page program (up to 256 bytes) */
+#define SPINOR_OP_PP_1_1_4_4B	0x34	/* Quad page program */
+#define SPINOR_OP_PP_1_4_4_4B	0x3e	/* Quad page program */
+#define SPINOR_OP_BE_4K_4B	0x21	/* Erase 4KiB block */
+#define SPINOR_OP_BE_32K_4B	0x5c	/* Erase 32KiB block */
 #define SPINOR_OP_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
 
 /* Used for SST flashes only. */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
  2016-12-06 16:52 [PATCH 0/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib Cyrille Pitchen
  2016-12-06 16:52 ` [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes Cyrille Pitchen
@ 2016-12-06 16:52 ` Cyrille Pitchen
       [not found]   ` <97e02fd9d6328bc591f98d5ae3da98b0ad9051d2.1481041702.git.cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
  1 sibling, 1 reply; 10+ messages in thread
From: Cyrille Pitchen @ 2016-12-06 16:52 UTC (permalink / raw)
  To: marek.vasut
  Cc: computersforpeace, boris.brezillon, richard, linux-mtd,
	linux-kernel, linux-spi, broonie, Cyrille Pitchen

This patch provides an alternative mean to support memory above 16MiB
(128Mib) by replacing 3byte address op codes by their associated 4byte
address versions.

Using the dedicated 4byte address op codes doesn't change the internal
state of the SPI NOR memory as opposed to using other means such as
updating a Base Address Register (BAR) and sending command to enter/leave
the 4byte mode.

Hence when a CPU reset occurs, early bootloaders don't need to be aware
of BAR value or 4byte mode being enabled: they can still access the first
16MiB of the SPI NOR memory using the regular 3byte address op codes.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Tested-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 93 insertions(+), 21 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 8abe134e174a..606c030c566d 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -75,6 +75,10 @@ struct flash_info {
 					 * bit. Must be used with
 					 * SPI_NOR_HAS_LOCK.
 					 */
+#define SPI_NOR_4B_OPCODES	BIT(10)	/*
+					 * Use dedicated 4byte address op codes
+					 * to support memory size above 128Mib.
+					 */
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 	return mtd->priv;
 }
 
+
+struct spi_nor_address_entry {
+	u8	src_opcode;
+	u8	dst_opcode;
+};
+
+static u8 spi_nor_convert_opcode(u8 opcode,
+				 const struct spi_nor_address_entry *entries,
+				 size_t num_entries)
+{
+	int min, max;
+
+	/*
+	 * This function implements a dichotomic search in the entries[]
+	 * array indexed by src_opcode. Hence we assume that the entries[]
+	 * array is sorted by src_opcode.
+	 * The dichotomic search has a logarithmic complexity as opposed
+	 * to a simple loop on all entires, which has a linear complexity:
+	 * it means that when n is the number of entries in the input array,
+	 * the dichotomic search performs O(log2(n)) comparisons whereas
+	 * a simple loop performs O(n) comparisons.
+	 */
+	min = 0;
+	max = num_entries - 1;
+	while (min <= max) {
+		int mid = (min + max) >> 1;
+		const struct spi_nor_address_entry *entry = &entries[mid];
+
+		if (opcode == entry->src_opcode)
+			return entry->dst_opcode;
+
+		if (opcode < entry->src_opcode)
+			max = mid - 1;
+		else
+			min = mid + 1;
+	}
+
+	/* No conversion found */
+	return opcode;
+}
+
+static u8 spi_nor_3to4_opcode(u8 opcode)
+{
+	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
+#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
+	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {
+		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
+		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
+		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
+		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
+		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
+		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
+		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
+		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
+		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
+		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
+		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
+		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */
+	};
+#undef ENTRY_3TO4
+
+	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
+				      ARRAY_SIZE(spi_nor_3to4_table));
+}
+
+static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
+				      const struct flash_info *info)
+{
+	/* Do some manufacturer fixups first */
+	switch (JEDEC_MFR(info)) {
+	case SNOR_MFR_SPANSION:
+		/* No small sector erase for 4-byte command set */
+		nor->erase_opcode = SPINOR_OP_SE;
+		nor->mtd.erasesize = info->sector_size;
+		break;
+
+	default:
+		break;
+	}
+
+	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
+	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
+	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
+}
+
 /* Enable/disable 4-byte addressing mode. */
 static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
 			    int enable)
@@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
 	else if (mtd->size > 0x1000000) {
 		/* enable 4-byte addressing if the device exceeds 16MiB */
 		nor->addr_width = 4;
-		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
-			/* Dedicated 4-byte command set */
-			switch (nor->flash_read) {
-			case SPI_NOR_QUAD:
-				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
-				break;
-			case SPI_NOR_DUAL:
-				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
-				break;
-			case SPI_NOR_FAST:
-				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
-				break;
-			case SPI_NOR_NORMAL:
-				nor->read_opcode = SPINOR_OP_READ_4B;
-				break;
-			}
-			nor->program_opcode = SPINOR_OP_PP_4B;
-			/* No small sector erase for 4-byte command set */
-			nor->erase_opcode = SPINOR_OP_SE_4B;
-			mtd->erasesize = info->sector_size;
-		} else
+		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
+		    info->flags & SPI_NOR_4B_OPCODES)
+			spi_nor_set_4byte_opcodes(nor, info);
+		else
 			set_4byte(nor, info, 1);
 	} else {
 		nor->addr_width = 3;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes
  2016-12-06 16:52 ` [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes Cyrille Pitchen
@ 2016-12-07  9:51   ` Mark Brown
  2016-12-07 16:08     ` Marek Vasut
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Brown @ 2016-12-07  9:51 UTC (permalink / raw)
  To: Cyrille Pitchen
  Cc: boris.brezillon, richard, linux-kernel, linux-spi, marek.vasut,
	linux-mtd, computersforpeace


[-- Attachment #1.1: Type: text/plain, Size: 331 bytes --]

On Tue, Dec 06, 2016 at 05:52:31PM +0100, Cyrille Pitchen wrote:
> This patch renames the SPINOR_OP_* macros of the 4-byte address
> instruction set so the new names all share a common pattern: the 4-byte
> address name is built from the 3-byte address name appending the "_4B"
> suffix.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes
  2016-12-07  9:51   ` Mark Brown
@ 2016-12-07 16:08     ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2016-12-07 16:08 UTC (permalink / raw)
  To: Mark Brown, Cyrille Pitchen
  Cc: computersforpeace, boris.brezillon, richard, linux-mtd,
	linux-kernel, linux-spi

On 12/07/2016 10:51 AM, Mark Brown wrote:
> On Tue, Dec 06, 2016 at 05:52:31PM +0100, Cyrille Pitchen wrote:
>> This patch renames the SPINOR_OP_* macros of the 4-byte address
>> instruction set so the new names all share a common pattern: the 4-byte
>> address name is built from the 3-byte address name appending the "_4B"
>> suffix.
> 
> Acked-by: Mark Brown <broonie@kernel.org>
> 
It's certainly less confusing naming, so

Acked-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
       [not found]   ` <97e02fd9d6328bc591f98d5ae3da98b0ad9051d2.1481041702.git.cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
@ 2016-12-07 16:20     ` Marek Vasut
       [not found]       ` <e1f9652e-4dda-0420-69d3-6fef606bc5dd-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2016-12-07 16:20 UTC (permalink / raw)
  To: Cyrille Pitchen
  Cc: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	richard-/L3Ra7n9ekc, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-spi-u79uwXL29TY76Z2rM5mHXA, broonie-DgEjT+Ai2ygdnm+yROfE0A

On 12/06/2016 05:52 PM, Cyrille Pitchen wrote:
> This patch provides an alternative mean to support memory above 16MiB
> (128Mib) by replacing 3byte address op codes by their associated 4byte
> address versions.
> 
> Using the dedicated 4byte address op codes doesn't change the internal
> state of the SPI NOR memory as opposed to using other means such as
> updating a Base Address Register (BAR) and sending command to enter/leave
> the 4byte mode.
> 
> Hence when a CPU reset occurs, early bootloaders don't need to be aware
> of BAR value or 4byte mode being enabled: they can still access the first
> 16MiB of the SPI NOR memory using the regular 3byte address op codes.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Tested-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 93 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 8abe134e174a..606c030c566d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -75,6 +75,10 @@ struct flash_info {
>  					 * bit. Must be used with
>  					 * SPI_NOR_HAS_LOCK.
>  					 */
> +#define SPI_NOR_4B_OPCODES	BIT(10)	/*
> +					 * Use dedicated 4byte address op codes
> +					 * to support memory size above 128Mib.
> +					 */
>  };
>  
>  #define JEDEC_MFR(info)	((info)->id[0])
> @@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  	return mtd->priv;
>  }
>  
> +
> +struct spi_nor_address_entry {
> +	u8	src_opcode;
> +	u8	dst_opcode;
> +};
> +
> +static u8 spi_nor_convert_opcode(u8 opcode,
> +				 const struct spi_nor_address_entry *entries,
> +				 size_t num_entries)
> +{
> +	int min, max;
> +
> +	/*
> +	 * This function implements a dichotomic search in the entries[]
> +	 * array indexed by src_opcode. Hence we assume that the entries[]
> +	 * array is sorted by src_opcode.
> +	 * The dichotomic search has a logarithmic complexity as opposed
> +	 * to a simple loop on all entires, which has a linear complexity:
> +	 * it means that when n is the number of entries in the input array,
> +	 * the dichotomic search performs O(log2(n)) comparisons whereas
> +	 * a simple loop performs O(n) comparisons.
> +	 */
> +	min = 0;
> +	max = num_entries - 1;
> +	while (min <= max) {
> +		int mid = (min + max) >> 1;
> +		const struct spi_nor_address_entry *entry = &entries[mid];
> +
> +		if (opcode == entry->src_opcode)
> +			return entry->dst_opcode;
> +
> +		if (opcode < entry->src_opcode)
> +			max = mid - 1;
> +		else
> +			min = mid + 1;
> +	}

You have like 16 entries in that table, just do a linear search, this is
only complex for no benefit.

> +	/* No conversion found */
> +	return opcode;
> +}
> +
> +static u8 spi_nor_3to4_opcode(u8 opcode)
> +{
> +	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
> +#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
> +	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {

You can make this static const struct const for extra constness :-)

> +		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
> +		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
> +		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
> +		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
> +		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
> +		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
> +		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
> +		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
> +		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
> +		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */

I'd probably break this into three smaller tables, read/program/erase
and then call something like:

spi_nor_3to4_opcode(nor->read_opcode, read_opcode_table,
                    ARRAY_SIZE(read_opcode_table));

This would further reduce the table size (heck, it'd probably fit into a
cacheline), so linear search would be more than enough.

> +	};
> +#undef ENTRY_3TO4
> +
> +	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
> +				      ARRAY_SIZE(spi_nor_3to4_table));
> +}
> +
> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
> +				      const struct flash_info *info)
> +{
> +	/* Do some manufacturer fixups first */
> +	switch (JEDEC_MFR(info)) {
> +	case SNOR_MFR_SPANSION:
> +		/* No small sector erase for 4-byte command set */
> +		nor->erase_opcode = SPINOR_OP_SE;
> +		nor->mtd.erasesize = info->sector_size;
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
> +	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
> +	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
> +}
> +
>  /* Enable/disable 4-byte addressing mode. */
>  static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>  			    int enable)
> @@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>  	else if (mtd->size > 0x1000000) {
>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>  		nor->addr_width = 4;
> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
> -			/* Dedicated 4-byte command set */
> -			switch (nor->flash_read) {
> -			case SPI_NOR_QUAD:
> -				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
> -				break;
> -			case SPI_NOR_DUAL:
> -				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
> -				break;
> -			case SPI_NOR_FAST:
> -				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
> -				break;
> -			case SPI_NOR_NORMAL:
> -				nor->read_opcode = SPINOR_OP_READ_4B;
> -				break;
> -			}
> -			nor->program_opcode = SPINOR_OP_PP_4B;
> -			/* No small sector erase for 4-byte command set */
> -			nor->erase_opcode = SPINOR_OP_SE_4B;
> -			mtd->erasesize = info->sector_size;
> -		} else
> +		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
> +		    info->flags & SPI_NOR_4B_OPCODES)
> +			spi_nor_set_4byte_opcodes(nor, info);
> +		else
>  			set_4byte(nor, info, 1);
>  	} else {
>  		nor->addr_width = 3;
> 


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
       [not found]       ` <e1f9652e-4dda-0420-69d3-6fef606bc5dd-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-12-07 16:29         ` Cyrille Pitchen
  2016-12-07 16:32           ` Marek Vasut
  0 siblings, 1 reply; 10+ messages in thread
From: Cyrille Pitchen @ 2016-12-07 16:29 UTC (permalink / raw)
  To: Marek Vasut
  Cc: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	richard-/L3Ra7n9ekc, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-spi-u79uwXL29TY76Z2rM5mHXA, broonie-DgEjT+Ai2ygdnm+yROfE0A

Le 07/12/2016 à 17:20, Marek Vasut a écrit :
> On 12/06/2016 05:52 PM, Cyrille Pitchen wrote:
>> This patch provides an alternative mean to support memory above 16MiB
>> (128Mib) by replacing 3byte address op codes by their associated 4byte
>> address versions.
>>
>> Using the dedicated 4byte address op codes doesn't change the internal
>> state of the SPI NOR memory as opposed to using other means such as
>> updating a Base Address Register (BAR) and sending command to enter/leave
>> the 4byte mode.
>>
>> Hence when a CPU reset occurs, early bootloaders don't need to be aware
>> of BAR value or 4byte mode being enabled: they can still access the first
>> 16MiB of the SPI NOR memory using the regular 3byte address op codes.
>>
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>> Tested-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
>>  1 file changed, 93 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 8abe134e174a..606c030c566d 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -75,6 +75,10 @@ struct flash_info {
>>  					 * bit. Must be used with
>>  					 * SPI_NOR_HAS_LOCK.
>>  					 */
>> +#define SPI_NOR_4B_OPCODES	BIT(10)	/*
>> +					 * Use dedicated 4byte address op codes
>> +					 * to support memory size above 128Mib.
>> +					 */
>>  };
>>  
>>  #define JEDEC_MFR(info)	((info)->id[0])
>> @@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>>  	return mtd->priv;
>>  }
>>  
>> +
>> +struct spi_nor_address_entry {
>> +	u8	src_opcode;
>> +	u8	dst_opcode;
>> +};
>> +
>> +static u8 spi_nor_convert_opcode(u8 opcode,
>> +				 const struct spi_nor_address_entry *entries,
>> +				 size_t num_entries)
>> +{
>> +	int min, max;
>> +
>> +	/*
>> +	 * This function implements a dichotomic search in the entries[]
>> +	 * array indexed by src_opcode. Hence we assume that the entries[]
>> +	 * array is sorted by src_opcode.
>> +	 * The dichotomic search has a logarithmic complexity as opposed
>> +	 * to a simple loop on all entires, which has a linear complexity:
>> +	 * it means that when n is the number of entries in the input array,
>> +	 * the dichotomic search performs O(log2(n)) comparisons whereas
>> +	 * a simple loop performs O(n) comparisons.
>> +	 */
>> +	min = 0;
>> +	max = num_entries - 1;
>> +	while (min <= max) {
>> +		int mid = (min + max) >> 1;
>> +		const struct spi_nor_address_entry *entry = &entries[mid];
>> +
>> +		if (opcode == entry->src_opcode)
>> +			return entry->dst_opcode;
>> +
>> +		if (opcode < entry->src_opcode)
>> +			max = mid - 1;
>> +		else
>> +			min = mid + 1;
>> +	}
> 
> You have like 16 entries in that table, just do a linear search, this is
> only complex for no benefit.

Well ok, I agree with you, it's too much overkill for what it does.
For readiness purpose, what about a simple and straight forward switch()
statement? Let's forget about cache-line and other memory/time optimizations :)

> 
>> +	/* No conversion found */
>> +	return opcode;
>> +}
>> +
>> +static u8 spi_nor_3to4_opcode(u8 opcode)
>> +{
>> +	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
>> +#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
>> +	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {
> 
> You can make this static const struct const for extra constness :-)
> 
>> +		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
>> +		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
>> +		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
>> +		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
>> +		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
>> +		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
>> +		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
>> +		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
>> +		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
>> +		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */
> 
> I'd probably break this into three smaller tables, read/program/erase
> and then call something like:
> 
> spi_nor_3to4_opcode(nor->read_opcode, read_opcode_table,
>                     ARRAY_SIZE(read_opcode_table));
> 
> This would further reduce the table size (heck, it'd probably fit into a
> cacheline), so linear search would be more than enough.
> 
>> +	};
>> +#undef ENTRY_3TO4
>> +
>> +	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
>> +				      ARRAY_SIZE(spi_nor_3to4_table));
>> +}
>> +
>> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>> +				      const struct flash_info *info)
>> +{
>> +	/* Do some manufacturer fixups first */
>> +	switch (JEDEC_MFR(info)) {
>> +	case SNOR_MFR_SPANSION:
>> +		/* No small sector erase for 4-byte command set */
>> +		nor->erase_opcode = SPINOR_OP_SE;
>> +		nor->mtd.erasesize = info->sector_size;
>> +		break;
>> +
>> +	default:
>> +		break;
>> +	}
>> +
>> +	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
>> +	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
>> +	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
>> +}
>> +
>>  /* Enable/disable 4-byte addressing mode. */
>>  static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>  			    int enable)
>> @@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>  	else if (mtd->size > 0x1000000) {
>>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>>  		nor->addr_width = 4;
>> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
>> -			/* Dedicated 4-byte command set */
>> -			switch (nor->flash_read) {
>> -			case SPI_NOR_QUAD:
>> -				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
>> -				break;
>> -			case SPI_NOR_DUAL:
>> -				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
>> -				break;
>> -			case SPI_NOR_FAST:
>> -				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
>> -				break;
>> -			case SPI_NOR_NORMAL:
>> -				nor->read_opcode = SPINOR_OP_READ_4B;
>> -				break;
>> -			}
>> -			nor->program_opcode = SPINOR_OP_PP_4B;
>> -			/* No small sector erase for 4-byte command set */
>> -			nor->erase_opcode = SPINOR_OP_SE_4B;
>> -			mtd->erasesize = info->sector_size;
>> -		} else
>> +		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>> +		    info->flags & SPI_NOR_4B_OPCODES)
>> +			spi_nor_set_4byte_opcodes(nor, info);
>> +		else
>>  			set_4byte(nor, info, 1);
>>  	} else {
>>  		nor->addr_width = 3;
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
  2016-12-07 16:29         ` Cyrille Pitchen
@ 2016-12-07 16:32           ` Marek Vasut
       [not found]             ` <6443cfb4-2d6d-c929-0d8a-053082bda506-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Vasut @ 2016-12-07 16:32 UTC (permalink / raw)
  To: Cyrille Pitchen
  Cc: computersforpeace, boris.brezillon, richard, linux-mtd,
	linux-kernel, linux-spi, broonie

On 12/07/2016 05:29 PM, Cyrille Pitchen wrote:
> Le 07/12/2016 à 17:20, Marek Vasut a écrit :
>> On 12/06/2016 05:52 PM, Cyrille Pitchen wrote:
>>> This patch provides an alternative mean to support memory above 16MiB
>>> (128Mib) by replacing 3byte address op codes by their associated 4byte
>>> address versions.
>>>
>>> Using the dedicated 4byte address op codes doesn't change the internal
>>> state of the SPI NOR memory as opposed to using other means such as
>>> updating a Base Address Register (BAR) and sending command to enter/leave
>>> the 4byte mode.
>>>
>>> Hence when a CPU reset occurs, early bootloaders don't need to be aware
>>> of BAR value or 4byte mode being enabled: they can still access the first
>>> 16MiB of the SPI NOR memory using the regular 3byte address op codes.
>>>
>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>> Tested-by: Vignesh R <vigneshr@ti.com>
>>> ---
>>>  drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 93 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>> index 8abe134e174a..606c030c566d 100644
>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>> @@ -75,6 +75,10 @@ struct flash_info {
>>>  					 * bit. Must be used with
>>>  					 * SPI_NOR_HAS_LOCK.
>>>  					 */
>>> +#define SPI_NOR_4B_OPCODES	BIT(10)	/*
>>> +					 * Use dedicated 4byte address op codes
>>> +					 * to support memory size above 128Mib.
>>> +					 */
>>>  };
>>>  
>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>> @@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>>>  	return mtd->priv;
>>>  }
>>>  
>>> +
>>> +struct spi_nor_address_entry {
>>> +	u8	src_opcode;
>>> +	u8	dst_opcode;
>>> +};
>>> +
>>> +static u8 spi_nor_convert_opcode(u8 opcode,
>>> +				 const struct spi_nor_address_entry *entries,
>>> +				 size_t num_entries)
>>> +{
>>> +	int min, max;
>>> +
>>> +	/*
>>> +	 * This function implements a dichotomic search in the entries[]
>>> +	 * array indexed by src_opcode. Hence we assume that the entries[]
>>> +	 * array is sorted by src_opcode.
>>> +	 * The dichotomic search has a logarithmic complexity as opposed
>>> +	 * to a simple loop on all entires, which has a linear complexity:
>>> +	 * it means that when n is the number of entries in the input array,
>>> +	 * the dichotomic search performs O(log2(n)) comparisons whereas
>>> +	 * a simple loop performs O(n) comparisons.
>>> +	 */
>>> +	min = 0;
>>> +	max = num_entries - 1;
>>> +	while (min <= max) {
>>> +		int mid = (min + max) >> 1;
>>> +		const struct spi_nor_address_entry *entry = &entries[mid];
>>> +
>>> +		if (opcode == entry->src_opcode)
>>> +			return entry->dst_opcode;
>>> +
>>> +		if (opcode < entry->src_opcode)
>>> +			max = mid - 1;
>>> +		else
>>> +			min = mid + 1;
>>> +	}
>>
>> You have like 16 entries in that table, just do a linear search, this is
>> only complex for no benefit.
> 
> Well ok, I agree with you, it's too much overkill for what it does.
> For readiness purpose, what about a simple and straight forward switch()
> statement? Let's forget about cache-line and other memory/time optimizations :)

If you do a switch, you cannot walk a table which you pass in, see below.

>>> +	/* No conversion found */
>>> +	return opcode;
>>> +}
>>> +
>>> +static u8 spi_nor_3to4_opcode(u8 opcode)
>>> +{
>>> +	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
>>> +#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
>>> +	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {
>>
>> You can make this static const struct const for extra constness :-)
>>
>>> +		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
>>> +		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
>>> +		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
>>> +		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
>>> +		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
>>> +		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */
>>
>> I'd probably break this into three smaller tables, read/program/erase
>> and then call something like:
>>
>> spi_nor_3to4_opcode(nor->read_opcode, read_opcode_table,
>>                     ARRAY_SIZE(read_opcode_table));
>>
>> This would further reduce the table size (heck, it'd probably fit into a
>> cacheline), so linear search would be more than enough.
>>
>>> +	};
>>> +#undef ENTRY_3TO4
>>> +
>>> +	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
>>> +				      ARRAY_SIZE(spi_nor_3to4_table));
>>> +}
>>> +
>>> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>>> +				      const struct flash_info *info)
>>> +{
>>> +	/* Do some manufacturer fixups first */
>>> +	switch (JEDEC_MFR(info)) {
>>> +	case SNOR_MFR_SPANSION:
>>> +		/* No small sector erase for 4-byte command set */
>>> +		nor->erase_opcode = SPINOR_OP_SE;
>>> +		nor->mtd.erasesize = info->sector_size;
>>> +		break;
>>> +
>>> +	default:
>>> +		break;
>>> +	}
>>> +
>>> +	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
>>> +	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
>>> +	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
>>> +}
>>> +
>>>  /* Enable/disable 4-byte addressing mode. */
>>>  static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>>  			    int enable)
>>> @@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>  	else if (mtd->size > 0x1000000) {
>>>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>>>  		nor->addr_width = 4;
>>> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
>>> -			/* Dedicated 4-byte command set */
>>> -			switch (nor->flash_read) {
>>> -			case SPI_NOR_QUAD:
>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
>>> -				break;
>>> -			case SPI_NOR_DUAL:
>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
>>> -				break;
>>> -			case SPI_NOR_FAST:
>>> -				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
>>> -				break;
>>> -			case SPI_NOR_NORMAL:
>>> -				nor->read_opcode = SPINOR_OP_READ_4B;
>>> -				break;
>>> -			}
>>> -			nor->program_opcode = SPINOR_OP_PP_4B;
>>> -			/* No small sector erase for 4-byte command set */
>>> -			nor->erase_opcode = SPINOR_OP_SE_4B;
>>> -			mtd->erasesize = info->sector_size;
>>> -		} else
>>> +		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>>> +		    info->flags & SPI_NOR_4B_OPCODES)
>>> +			spi_nor_set_4byte_opcodes(nor, info);
>>> +		else
>>>  			set_4byte(nor, info, 1);
>>>  	} else {
>>>  		nor->addr_width = 3;
>>>
>>
>>
> 


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
       [not found]             ` <6443cfb4-2d6d-c929-0d8a-053082bda506-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-12-07 16:59               ` Cyrille Pitchen
       [not found]                 ` <679d301c-363a-e8fe-abed-c3ad00ea1fad-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 10+ messages in thread
From: Cyrille Pitchen @ 2016-12-07 16:59 UTC (permalink / raw)
  To: Marek Vasut
  Cc: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	richard-/L3Ra7n9ekc, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-spi-u79uwXL29TY76Z2rM5mHXA, broonie-DgEjT+Ai2ygdnm+yROfE0A

Le 07/12/2016 à 17:32, Marek Vasut a écrit :
> On 12/07/2016 05:29 PM, Cyrille Pitchen wrote:
>> Le 07/12/2016 à 17:20, Marek Vasut a écrit :
>>> On 12/06/2016 05:52 PM, Cyrille Pitchen wrote:
>>>> This patch provides an alternative mean to support memory above 16MiB
>>>> (128Mib) by replacing 3byte address op codes by their associated 4byte
>>>> address versions.
>>>>
>>>> Using the dedicated 4byte address op codes doesn't change the internal
>>>> state of the SPI NOR memory as opposed to using other means such as
>>>> updating a Base Address Register (BAR) and sending command to enter/leave
>>>> the 4byte mode.
>>>>
>>>> Hence when a CPU reset occurs, early bootloaders don't need to be aware
>>>> of BAR value or 4byte mode being enabled: they can still access the first
>>>> 16MiB of the SPI NOR memory using the regular 3byte address op codes.
>>>>
>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>>> Tested-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>>>> ---
>>>>  drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
>>>>  1 file changed, 93 insertions(+), 21 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>> index 8abe134e174a..606c030c566d 100644
>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>> @@ -75,6 +75,10 @@ struct flash_info {
>>>>  					 * bit. Must be used with
>>>>  					 * SPI_NOR_HAS_LOCK.
>>>>  					 */
>>>> +#define SPI_NOR_4B_OPCODES	BIT(10)	/*
>>>> +					 * Use dedicated 4byte address op codes
>>>> +					 * to support memory size above 128Mib.
>>>> +					 */
>>>>  };
>>>>  
>>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>>> @@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>>>>  	return mtd->priv;
>>>>  }
>>>>  
>>>> +
>>>> +struct spi_nor_address_entry {
>>>> +	u8	src_opcode;
>>>> +	u8	dst_opcode;
>>>> +};
>>>> +
>>>> +static u8 spi_nor_convert_opcode(u8 opcode,
>>>> +				 const struct spi_nor_address_entry *entries,
>>>> +				 size_t num_entries)
>>>> +{
>>>> +	int min, max;
>>>> +
>>>> +	/*
>>>> +	 * This function implements a dichotomic search in the entries[]
>>>> +	 * array indexed by src_opcode. Hence we assume that the entries[]
>>>> +	 * array is sorted by src_opcode.
>>>> +	 * The dichotomic search has a logarithmic complexity as opposed
>>>> +	 * to a simple loop on all entires, which has a linear complexity:
>>>> +	 * it means that when n is the number of entries in the input array,
>>>> +	 * the dichotomic search performs O(log2(n)) comparisons whereas
>>>> +	 * a simple loop performs O(n) comparisons.
>>>> +	 */
>>>> +	min = 0;
>>>> +	max = num_entries - 1;
>>>> +	while (min <= max) {
>>>> +		int mid = (min + max) >> 1;
>>>> +		const struct spi_nor_address_entry *entry = &entries[mid];
>>>> +
>>>> +		if (opcode == entry->src_opcode)
>>>> +			return entry->dst_opcode;
>>>> +
>>>> +		if (opcode < entry->src_opcode)
>>>> +			max = mid - 1;
>>>> +		else
>>>> +			min = mid + 1;
>>>> +	}
>>>
>>> You have like 16 entries in that table, just do a linear search, this is
>>> only complex for no benefit.
>>
>> Well ok, I agree with you, it's too much overkill for what it does.
>> For readiness purpose, what about a simple and straight forward switch()
>> statement? Let's forget about cache-line and other memory/time optimizations :)
> 
> If you do a switch, you cannot walk a table which you pass in, see below.
> 

I meant I remove the table too, a hidden "table" would still be generated
by the compiler in the assembly code to translate the switch statement:

static u8 spi_nor_3to4_opcode(u8 3byte_address_opcode)
{
	#define CASE_3TO4(_opcode)	case _opcode: return _opcode##_4B
	
	switch (3_byte_address_opcode) {
	/* Without macro. */
	case SPINOR_OP_PP:
		return SPINOR_OP_PP_4B;

	case SPINOR_OP_READ:
		return SPINOR_OP_READ_4B;

	case SPINOR_OP_READ_FAST:
		return SPINOR_OP_READ_FAST_4B;

	/* With macro. */
	CASE_3TO4(SPINOR_OP_READ_FAST);

	default:
		break;
	}

	/* No conversion found */
	return opcode;
}

For readiness, I don't whether people prefer the version with macro or the
version without. Just tell me :)

Of course with the switch statement I no longer need the struct
spi_nor_address_entry.

Also for now I don't need a 4TO3 translation.


>>>> +	/* No conversion found */
>>>> +	return opcode;
>>>> +}
>>>> +
>>>> +static u8 spi_nor_3to4_opcode(u8 opcode)
>>>> +{
>>>> +	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
>>>> +#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
>>>> +	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {
>>>
>>> You can make this static const struct const for extra constness :-)
>>>
>>>> +		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
>>>> +		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
>>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
>>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
>>>> +		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
>>>> +		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */
>>>
>>> I'd probably break this into three smaller tables, read/program/erase
>>> and then call something like:
>>>
>>> spi_nor_3to4_opcode(nor->read_opcode, read_opcode_table,
>>>                     ARRAY_SIZE(read_opcode_table));
>>>
>>> This would further reduce the table size (heck, it'd probably fit into a
>>> cacheline), so linear search would be more than enough.
>>>
>>>> +	};
>>>> +#undef ENTRY_3TO4
>>>> +
>>>> +	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
>>>> +				      ARRAY_SIZE(spi_nor_3to4_table));
>>>> +}
>>>> +
>>>> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>>>> +				      const struct flash_info *info)
>>>> +{
>>>> +	/* Do some manufacturer fixups first */
>>>> +	switch (JEDEC_MFR(info)) {
>>>> +	case SNOR_MFR_SPANSION:
>>>> +		/* No small sector erase for 4-byte command set */
>>>> +		nor->erase_opcode = SPINOR_OP_SE;
>>>> +		nor->mtd.erasesize = info->sector_size;
>>>> +		break;
>>>> +
>>>> +	default:
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
>>>> +	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
>>>> +	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
>>>> +}
>>>> +
>>>>  /* Enable/disable 4-byte addressing mode. */
>>>>  static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>>>  			    int enable)
>>>> @@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>  	else if (mtd->size > 0x1000000) {
>>>>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>>>>  		nor->addr_width = 4;
>>>> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
>>>> -			/* Dedicated 4-byte command set */
>>>> -			switch (nor->flash_read) {
>>>> -			case SPI_NOR_QUAD:
>>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
>>>> -				break;
>>>> -			case SPI_NOR_DUAL:
>>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
>>>> -				break;
>>>> -			case SPI_NOR_FAST:
>>>> -				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
>>>> -				break;
>>>> -			case SPI_NOR_NORMAL:
>>>> -				nor->read_opcode = SPINOR_OP_READ_4B;
>>>> -				break;
>>>> -			}
>>>> -			nor->program_opcode = SPINOR_OP_PP_4B;
>>>> -			/* No small sector erase for 4-byte command set */
>>>> -			nor->erase_opcode = SPINOR_OP_SE_4B;
>>>> -			mtd->erasesize = info->sector_size;
>>>> -		} else
>>>> +		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>>>> +		    info->flags & SPI_NOR_4B_OPCODES)
>>>> +			spi_nor_set_4byte_opcodes(nor, info);
>>>> +		else
>>>>  			set_4byte(nor, info, 1);
>>>>  	} else {
>>>>  		nor->addr_width = 3;
>>>>
>>>
>>>
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib
       [not found]                 ` <679d301c-363a-e8fe-abed-c3ad00ea1fad-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
@ 2016-12-08  3:50                   ` Marek Vasut
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Vasut @ 2016-12-08  3:50 UTC (permalink / raw)
  To: Cyrille Pitchen
  Cc: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w,
	boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
	richard-/L3Ra7n9ekc, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-spi-u79uwXL29TY76Z2rM5mHXA, broonie-DgEjT+Ai2ygdnm+yROfE0A

On 12/07/2016 05:59 PM, Cyrille Pitchen wrote:
> Le 07/12/2016 à 17:32, Marek Vasut a écrit :
>> On 12/07/2016 05:29 PM, Cyrille Pitchen wrote:
>>> Le 07/12/2016 à 17:20, Marek Vasut a écrit :
>>>> On 12/06/2016 05:52 PM, Cyrille Pitchen wrote:
>>>>> This patch provides an alternative mean to support memory above 16MiB
>>>>> (128Mib) by replacing 3byte address op codes by their associated 4byte
>>>>> address versions.
>>>>>
>>>>> Using the dedicated 4byte address op codes doesn't change the internal
>>>>> state of the SPI NOR memory as opposed to using other means such as
>>>>> updating a Base Address Register (BAR) and sending command to enter/leave
>>>>> the 4byte mode.
>>>>>
>>>>> Hence when a CPU reset occurs, early bootloaders don't need to be aware
>>>>> of BAR value or 4byte mode being enabled: they can still access the first
>>>>> 16MiB of the SPI NOR memory using the regular 3byte address op codes.
>>>>>
>>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
>>>>> Tested-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/spi-nor.c | 114 ++++++++++++++++++++++++++++++++++--------
>>>>>  1 file changed, 93 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>>>>> index 8abe134e174a..606c030c566d 100644
>>>>> --- a/drivers/mtd/spi-nor/spi-nor.c
>>>>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>>>>> @@ -75,6 +75,10 @@ struct flash_info {
>>>>>  					 * bit. Must be used with
>>>>>  					 * SPI_NOR_HAS_LOCK.
>>>>>  					 */
>>>>> +#define SPI_NOR_4B_OPCODES	BIT(10)	/*
>>>>> +					 * Use dedicated 4byte address op codes
>>>>> +					 * to support memory size above 128Mib.
>>>>> +					 */
>>>>>  };
>>>>>  
>>>>>  #define JEDEC_MFR(info)	((info)->id[0])
>>>>> @@ -188,6 +192,91 @@ static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>>>>>  	return mtd->priv;
>>>>>  }
>>>>>  
>>>>> +
>>>>> +struct spi_nor_address_entry {
>>>>> +	u8	src_opcode;
>>>>> +	u8	dst_opcode;
>>>>> +};
>>>>> +
>>>>> +static u8 spi_nor_convert_opcode(u8 opcode,
>>>>> +				 const struct spi_nor_address_entry *entries,
>>>>> +				 size_t num_entries)
>>>>> +{
>>>>> +	int min, max;
>>>>> +
>>>>> +	/*
>>>>> +	 * This function implements a dichotomic search in the entries[]
>>>>> +	 * array indexed by src_opcode. Hence we assume that the entries[]
>>>>> +	 * array is sorted by src_opcode.
>>>>> +	 * The dichotomic search has a logarithmic complexity as opposed
>>>>> +	 * to a simple loop on all entires, which has a linear complexity:
>>>>> +	 * it means that when n is the number of entries in the input array,
>>>>> +	 * the dichotomic search performs O(log2(n)) comparisons whereas
>>>>> +	 * a simple loop performs O(n) comparisons.
>>>>> +	 */
>>>>> +	min = 0;
>>>>> +	max = num_entries - 1;
>>>>> +	while (min <= max) {
>>>>> +		int mid = (min + max) >> 1;
>>>>> +		const struct spi_nor_address_entry *entry = &entries[mid];
>>>>> +
>>>>> +		if (opcode == entry->src_opcode)
>>>>> +			return entry->dst_opcode;
>>>>> +
>>>>> +		if (opcode < entry->src_opcode)
>>>>> +			max = mid - 1;
>>>>> +		else
>>>>> +			min = mid + 1;
>>>>> +	}
>>>>
>>>> You have like 16 entries in that table, just do a linear search, this is
>>>> only complex for no benefit.
>>>
>>> Well ok, I agree with you, it's too much overkill for what it does.
>>> For readiness purpose, what about a simple and straight forward switch()
>>> statement? Let's forget about cache-line and other memory/time optimizations :)
>>
>> If you do a switch, you cannot walk a table which you pass in, see below.
>>
> 
> I meant I remove the table too, a hidden "table" would still be generated
> by the compiler in the assembly code to translate the switch statement:
> 
> static u8 spi_nor_3to4_opcode(u8 3byte_address_opcode)
> {
> 	#define CASE_3TO4(_opcode)	case _opcode: return _opcode##_4B
> 	
> 	switch (3_byte_address_opcode) {
> 	/* Without macro. */
> 	case SPINOR_OP_PP:
> 		return SPINOR_OP_PP_4B;
> 
> 	case SPINOR_OP_READ:
> 		return SPINOR_OP_READ_4B;
> 
> 	case SPINOR_OP_READ_FAST:
> 		return SPINOR_OP_READ_FAST_4B;
> 
> 	/* With macro. */
> 	CASE_3TO4(SPINOR_OP_READ_FAST);

Geh, the macro stuff is something I don't really like, it feels like
it's obfuscating the code for little benefit. But that might be a matter
of my personal taste.

> 	default:
> 		break;
> 	}
> 
> 	/* No conversion found */
> 	return opcode;
> }
> 
> For readiness, I don't whether people prefer the version with macro or the
> version without. Just tell me :)

Well, you asked for it ... I demand that you implement it with a table! ;-)

I can see something like:

static const u8 spi_read_opcodes[][2] = {
 { SPINOR_OP_READ,      SPINOR_OP_READ_4B },
 { SPINOR_OP_READ_FAST, SPINOR_OP_READ_4B_FAST },
}
...
u8 convert_opcode(u8 opcodes[][2], opcodesize) {

for (i = 0; i < opcodesize; i++)
    if (opcodes[i][0] == opcode)
        return opcodes[i][1];

return opcode;
}

It feels a bit more compact and explicit, but it might be just my taste.

> Of course with the switch statement I no longer need the struct
> spi_nor_address_entry.
> 
> Also for now I don't need a 4TO3 translation.
> 
> 
>>>>> +	/* No conversion found */
>>>>> +	return opcode;
>>>>> +}
>>>>> +
>>>>> +static u8 spi_nor_3to4_opcode(u8 opcode)
>>>>> +{
>>>>> +	/* MUST be sorted by 3byte opcode (cf spi_nor_convert_opcode). */
>>>>> +#define ENTRY_3TO4(_opcode)	{ _opcode, _opcode##_4B }
>>>>> +	static const struct spi_nor_address_entry spi_nor_3to4_table[] = {
>>>>
>>>> You can make this static const struct const for extra constness :-)
>>>>
>>>>> +		ENTRY_3TO4(SPINOR_OP_PP),		/* 0x02 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_READ),		/* 0x03 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_READ_FAST),	/* 0x0b */
>>>>> +		ENTRY_3TO4(SPINOR_OP_BE_4K),		/* 0x20 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_1_4),		/* 0x32 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_PP_1_4_4),		/* 0x38 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_2),	/* 0x3b */
>>>>> +		ENTRY_3TO4(SPINOR_OP_BE_32K),		/* 0x52 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_1_4),	/* 0x6b */
>>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_2_2),	/* 0xbb */
>>>>> +		ENTRY_3TO4(SPINOR_OP_SE),		/* 0xd8 */
>>>>> +		ENTRY_3TO4(SPINOR_OP_READ_1_4_4),	/* 0xeb */
>>>>
>>>> I'd probably break this into three smaller tables, read/program/erase
>>>> and then call something like:
>>>>
>>>> spi_nor_3to4_opcode(nor->read_opcode, read_opcode_table,
>>>>                     ARRAY_SIZE(read_opcode_table));
>>>>
>>>> This would further reduce the table size (heck, it'd probably fit into a
>>>> cacheline), so linear search would be more than enough.
>>>>
>>>>> +	};
>>>>> +#undef ENTRY_3TO4
>>>>> +
>>>>> +	return spi_nor_convert_opcode(opcode, spi_nor_3to4_table,
>>>>> +				      ARRAY_SIZE(spi_nor_3to4_table));
>>>>> +}
>>>>> +
>>>>> +static void spi_nor_set_4byte_opcodes(struct spi_nor *nor,
>>>>> +				      const struct flash_info *info)
>>>>> +{
>>>>> +	/* Do some manufacturer fixups first */
>>>>> +	switch (JEDEC_MFR(info)) {
>>>>> +	case SNOR_MFR_SPANSION:
>>>>> +		/* No small sector erase for 4-byte command set */
>>>>> +		nor->erase_opcode = SPINOR_OP_SE;
>>>>> +		nor->mtd.erasesize = info->sector_size;
>>>>> +		break;
>>>>> +
>>>>> +	default:
>>>>> +		break;
>>>>> +	}
>>>>> +
>>>>> +	nor->read_opcode	= spi_nor_3to4_opcode(nor->read_opcode);
>>>>> +	nor->program_opcode	= spi_nor_3to4_opcode(nor->program_opcode);
>>>>> +	nor->erase_opcode	= spi_nor_3to4_opcode(nor->erase_opcode);
>>>>> +}
>>>>> +
>>>>>  /* Enable/disable 4-byte addressing mode. */
>>>>>  static inline int set_4byte(struct spi_nor *nor, const struct flash_info *info,
>>>>>  			    int enable)
>>>>> @@ -1486,27 +1575,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name, enum read_mode mode)
>>>>>  	else if (mtd->size > 0x1000000) {
>>>>>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>>>>>  		nor->addr_width = 4;
>>>>> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION) {
>>>>> -			/* Dedicated 4-byte command set */
>>>>> -			switch (nor->flash_read) {
>>>>> -			case SPI_NOR_QUAD:
>>>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_4_4B;
>>>>> -				break;
>>>>> -			case SPI_NOR_DUAL:
>>>>> -				nor->read_opcode = SPINOR_OP_READ_1_1_2_4B;
>>>>> -				break;
>>>>> -			case SPI_NOR_FAST:
>>>>> -				nor->read_opcode = SPINOR_OP_READ_FAST_4B;
>>>>> -				break;
>>>>> -			case SPI_NOR_NORMAL:
>>>>> -				nor->read_opcode = SPINOR_OP_READ_4B;
>>>>> -				break;
>>>>> -			}
>>>>> -			nor->program_opcode = SPINOR_OP_PP_4B;
>>>>> -			/* No small sector erase for 4-byte command set */
>>>>> -			nor->erase_opcode = SPINOR_OP_SE_4B;
>>>>> -			mtd->erasesize = info->sector_size;
>>>>> -		} else
>>>>> +		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
>>>>> +		    info->flags & SPI_NOR_4B_OPCODES)
>>>>> +			spi_nor_set_4byte_opcodes(nor, info);
>>>>> +		else
>>>>>  			set_4byte(nor, info, 1);
>>>>>  	} else {
>>>>>  		nor->addr_width = 3;
>>>>>
>>>>
>>>>
>>>
>>
>>
> 


-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-12-08  3:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-06 16:52 [PATCH 0/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib Cyrille Pitchen
2016-12-06 16:52 ` [PATCH 1/2] mtd: spi-nor: rename SPINOR_OP_* macros of the 4-byte address op codes Cyrille Pitchen
2016-12-07  9:51   ` Mark Brown
2016-12-07 16:08     ` Marek Vasut
2016-12-06 16:52 ` [PATCH 2/2] mtd: spi-nor: add a stateless method to support memory size above 128Mib Cyrille Pitchen
     [not found]   ` <97e02fd9d6328bc591f98d5ae3da98b0ad9051d2.1481041702.git.cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
2016-12-07 16:20     ` Marek Vasut
     [not found]       ` <e1f9652e-4dda-0420-69d3-6fef606bc5dd-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-12-07 16:29         ` Cyrille Pitchen
2016-12-07 16:32           ` Marek Vasut
     [not found]             ` <6443cfb4-2d6d-c929-0d8a-053082bda506-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-12-07 16:59               ` Cyrille Pitchen
     [not found]                 ` <679d301c-363a-e8fe-abed-c3ad00ea1fad-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
2016-12-08  3:50                   ` Marek Vasut

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).