Read SFDP parameters and access flash above 16MB

Add support for JESD216 rev B standard JEDEC Serial Flash Discoverable Parameters (SFDP) tables to dynamically initialize flash size, page size and address width of the flash. More parameters can be added as per requirement.
SFDP parsing is made default but already existing method for parsing these parameters are not deprecated. A flag is created to skip SFDP parsing for a particular flash, if required.

SFDP data lets us auto-detect the addressing mode supported by the flash which helps us access the flash using 4-byte address.

Add a new argument in spi_flash_addr() function to create commands with 3-byte or 4-byte address depending on the SFDP data read. Add pointer to struct spi_flash in struct spi_slave so that driver can have access to SFDP data.

Introduce new structures and functions to read and parse SFDP data. This is loosely based on Linux SFDP framework.

Signed-off-by Rajat Srivastava <[email protected]>

---
Changes in v2:
- Make SFDP parsing the default method.
- Change SPI_FLASH_USE_SFDP flag to SPI_FLASH_USE_SFDP to provide an option to skip SFDP parsing for a particular flash.
---

drivers/mtd/spi/spi_flash.c +36 -0

+static int spi_flash_parse_bfpt(struct spi_flash *flash,
+				const struct sfdp_parameter_header *bfpt_header)
+{
+	struct sfdp_bfpt bfpt;
+	size_t len;
+	int i, err;
+	u32 addr;
+
+	/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs. */
+	if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
+		return -EINVAL;
+
+	/* Read the Basic Flash Parameter Table. */
+	len = min_t(size_t, sizeof(bfpt),
+		    bfpt_header->length * sizeof(u32));
+	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
+	memset(&bfpt, 0, sizeof(bfpt));
+	err = spi_flash_read_sfdp(flash, addr, len, &bfpt);
+	if (err < 0)
+		return err;
+
+	/* Fix endianness of the BFPT DWORDs. */
+	for (i = 0; i < BFPT_DWORD_MAX; i++)
+		bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+
+	/* Number of address bytes. */
+	switch (bfpt.dwords[BFPT_DWORD(1)] & BFPT_DWORD1_ADDRESS_BYTES_MASK) {
+	case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
+		flash->addr_width = 3;
+		break;
+
+	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
+		printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+		printf("mode on command\n");
+		/*
+		 * By default, 4-byte addressing mode is set.

drivers/mtd/spi/spi_flash.c +151 -6

+	default:
+		break;
+	}
+
+	/* Flash Memory Density (in bits). */
+	flash->size = bfpt.dwords[BFPT_DWORD(2)];
+	if (flash->size & BIT(31)) {
+		flash->size &= ~BIT(31);
+
+		/*
+		 * Prevent overflows on flash->size. Anyway, a NOR of 2^64
+		 * bits is unlikely to exist so this error probably means
+		 * the BFPT we are reading is corrupted/wrong.
+		 */
+		if (flash->size > 63)
+			return -EINVAL;
+
+		flash->size = 1ULL << flash->size;
+	} else {
+		flash->size++;
+	}
+	flash->size >>= 3; /* Convert to bytes. */
+
+	/* Stop here if not JESD216 rev A or later. */
+	if (bfpt_header->length < BFPT_DWORD_MAX)
+		return 0;
+
+	/* Page size: this field specifies 'N' so the page size = 2^N bytes. */
+	flash->page_size = bfpt.dwords[BFPT_DWORD(11)];
+	flash->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+	flash->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+	flash->page_size = 1U << flash->page_size;
+
+	return 0;
+}
+
+/*
+ * spi_flash_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
+ * @flash:	pointer to a 'struct spi_flash'
+ *
+ * The Serial Flash Discoverable Parameters are described by the JEDEC JESD216
+ * specification. This is a standard which tends to supported by almost all
+ * (Q)SPI memory manufacturers. Those hard-coded tables allow us to learn at
+ * runtime the main parameters needed to perform basic SPI flash operations.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_flash_parse_sfdp(struct spi_flash *flash)
+{
+	const struct sfdp_parameter_header *param_header, *bfpt_header;
+	struct sfdp_parameter_header *param_headers = NULL;
+	struct sfdp_header header;
+	size_t psize;
+	int i, err;
+
+	/* Get the SFDP header. */
+	err = spi_flash_read_sfdp(flash, 0, sizeof(header), &header);
+	if (err < 0)
+		return err;
+
+	/* Check the SFDP header version. */
+	if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
+	    header.major != SFDP_JESD216_MAJOR)
+		return -EINVAL;
+
+	/*
+	 * Verify that the first and only mandatory parameter header is a
+	 * Basic Flash Parameter Table header as specified in JESD216.
+	 */
+	bfpt_header = &header.bfpt_header;
+	if (SFDP_PARAM_HEADER_ID(bfpt_header) != SFDP_BFPT_ID ||
+	    bfpt_header->major != SFDP_JESD216_MAJOR)
+		return -EINVAL;
+
+	/*
+	 * Allocate memory then read all parameter headers with a single
+	 * Read SFDP command. These parameter headers will actually be parsed
+	 * twice: a first time to get the latest revision of the basic flash
+	 * parameter table, then a second time to handle the supported optional
+	 * tables.
+	 * Hence we read the parameter headers once for all to reduce the
+	 * processing time
+	 */
+	if (header.nph) {
+		psize = header.nph * sizeof(*param_headers);
+
+		param_headers = malloc(psize);
+		if (!param_headers)
+			return -ENOMEM;
+
+		err = spi_flash_read_sfdp(flash, sizeof(header),
+					  psize, param_headers);
+		if (err < 0) {
+			dev_err(dev, "failed to read SFDP parameter headers\n");
+			goto exit;
+		}
+	}
+
+	/*
+	 * Check other parameter headers to get the latest revision of
+	 * the basic flash parameter table.
+	 */
+	for (i = 0; i < header.nph; i++) {
+		param_header = &param_headers[i];
+
+		if (SFDP_PARAM_HEADER_ID(param_header) == SFDP_BFPT_ID &&
+		    param_header->major == SFDP_JESD216_MAJOR &&
+		    (param_header->minor > bfpt_header->minor ||
+		     (param_header->minor == bfpt_header->minor &&
+		      param_header->length > bfpt_header->length)))
+			bfpt_header = param_header;
+	}
+
+	err = spi_flash_parse_bfpt(flash, bfpt_header);
+	if (err)
+		goto exit;
+
+exit:
+	free(param_headers);
+	return err;
+}
+
 static int set_quad_mode(struct spi_flash *flash,
 			 const struct spi_flash_info *info)
 {

@@ -1130,7 +1366,7 @@ int spi_flash_scan(struct spi_flash *flash)
 {
 	struct spi_slave *spi = flash->spi;
 	const struct spi_flash_info *info = NULL;
-	int ret;
+	int ret, sfdp = 0;
 
 	info = spi_flash_read_id(flash);
 	if (IS_ERR_OR_NULL(info))

@@ -1196,9 +1432,28 @@ int spi_flash_scan(struct spi_flash *flash)
 	}
 #endif
 
+	spi->flash = flash;
+	flash->addrwd_3_in_use = false;
+
+	/* Read Serial Flash Discoverable Parameters and initialize
+	 * the following parameters of flash:
+	 * 1. Flash size
+	 * 2. Page size
+	 * 3. Address width to be used for commands
+	 */
+	if (!(info->flags & SPI_FLASH_SKIP_SFDP)) {
+		flash->size = 0;
+		sfdp = spi_flash_parse_sfdp(flash);
+		if (sfdp < 0)
+			debug("Unable to get SFDP information\n");
+	}
+
 	/* Compute the flash size */
 	flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 : 0;
-	flash->page_size = info->page_size;
+	if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+		flash->page_size = info->page_size;
+		flash->addr_width = SPI_FLASH_3B_ADDR_LEN;
+	}
 	/*
 	 * The Spansion S25FS512S, S25FL032P and S25FL064P have 256b pages,
 	 * yet use the 0x4d00 Extended JEDEC code. The rest of the Spansion

@@ -1213,7 +1468,10 @@ int spi_flash_scan(struct spi_flash *flash)
 	}
 	flash->page_size <<= flash->shift;
 	flash->sector_size = info->sector_size << flash->shift;
-	flash->size = flash->sector_size * info->n_sectors << flash->shift;
+	if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+		flash->size = flash->sector_size * info->n_sectors <<
+			flash->shift;
+	}
 #ifdef CONFIG_SF_DUAL_FLASH
 	if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
 		flash->size <<= 1;

@@ -1312,9 +1570,10 @@ int spi_flash_scan(struct spi_flash *flash)
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR
-	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
-	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
+	if ((info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) &&
+	    (flash->dual_flash == SF_SINGLE_FLASH &&
+	     flash->size > SPI_FLASH_16MB_BOUN) ||
+	     (flash->dual_flash > SF_SINGLE_FLASH &&
 	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
 		puts("SF: Warning - Only lower 16MiB accessible,");
 		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");

include/spi.h +2 -0

index 938627bc01..7189e60581 100644
@@ -10,6 +10,7 @@
 #define _SPI_H_
 
 #include <common.h>
+#include <spi_flash.h>
 
 /* SPI mode flags */
 #define SPI_CPHA	BIT(0)			/* clock phase */

@@ -103,6 +104,7 @@ struct spi_slave {
 	unsigned int bus;
 	unsigned int cs;
 #endif
+	struct spi_flash *flash;
 	uint mode;
 	unsigned int wordlen;
 	unsigned int max_read_size;

include/spi_flash.h +120 -0

index 0ec98fb55d..6558a4a1d5 100644
@@ -47,6 +47,9 @@ struct spi_slave;
  * @read_cmd:		Read cmd - Array Fast, Extn read and quad read.
  * @write_cmd:		Write cmd - page and quad program.
  * @dummy_byte:		Dummy cycles for read operation.
+ * @cmd_len:		Total length of command.
+ * @addr_width:		Number of address bytes.
+ * @addrwd_3_in_use:	Flag to send command in 3-byte address mode.
  * @memory_map:		Address of read-only SPI flash access
  * @flash_lock:		lock a region of the SPI Flash
  * @flash_unlock:	unlock a region of the SPI Flash

@@ -82,6 +85,9 @@ struct spi_flash {
 	u8 read_cmd;
 	u8 write_cmd;
 	u8 dummy_byte;
+	u8 cmd_len;
+	u8 addr_width;
+	bool addrwd_3_in_use;
 
 	void *memory_map;
 

@@ -107,6 +113,120 @@ struct spi_flash {
 #endif
 };
 
+/*
+ * Serial Flash Discoverable Parameter Headers
+ */
+struct sfdp_parameter_header {
+	u8	id_lsb;
+	u8	minor;
+	u8	major;
+	u8	length; /* in double words */
+	u8	parameter_table_pointer[3]; /* byte address */
+	u8	id_msb;
+};
+
+struct sfdp_header {
+	u32	signature; /* Ox50444653U <=> "SFDP" */
+	u8	minor;
+	u8	major;
+	u8	nph; /* 0-base number of parameter headers */
+	u8	unused;
+
+	/* Basic Flash Parameter Table. */
+	struct sfdp_parameter_header	bfpt_header;
+};
+
+#define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) | (p)->id_lsb)
+#define SFDP_PARAM_HEADER_PTP(p) \
+	(((p)->parameter_table_pointer[2] << 16) | \
+	 ((p)->parameter_table_pointer[1] <<  8) | \
+	 ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID		0xff00  /* Basic Flash Parameter Table */
+#define SFDP_SECTOR_MAP_ID	0xff81  /* Sector Map Table */
+
+#define SFDP_SIGNATURE		0x50444653U
+#define SFDP_JESD216_MAJOR	1
+#define SFDP_JESD216_MINOR	0
+#define SFDP_JESD216A_MINOR	5
+#define SFDP_JESD216B_MINOR	6
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.
+ */
+#define BFPT_DWORD(i)		((i) - 1)
+#define BFPT_DWORD_MAX		16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216			9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK		GENMASK(18, 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
+#define BFPT_DWORD1_DTR				BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)
+#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
+#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7, 4)
+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
+ *         reads based on instruction. DQ3/HOLD# functions are hold during
+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect of
+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK			GENMASK(22, 20)
+#define BFPT_DWORD15_QER_NONE			(0x0UL << 20) /* Micron */
+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /* Macronix */
+#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
+
+struct sfdp_bfpt {
+	u32	dwords[BFPT_DWORD_MAX];
+};
+
 struct dm_spi_flash_ops {
 	int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
 	int (*write)(struct udevice *dev, u32 offset, size_t len,

-----Original Message-----
From: Vignesh R <[email protected]>
Sent: Thursday, November 15, 2018 2:52 PM
To: Rajat Srivastava <[email protected]>; u-boot at lists.denx.de
Cc: jagan at openedev.com
Subject: Re: [U-Boot] [PATCH v2 1/2] mtd/spi: Add JEDEC SFDP support in SPI framework

Hi Rajat,

Quoted 27 lines · Rajat Srivastava

Add support for JESD216 rev B standard JEDEC Serial Flash Discoverable Parameters (SFDP) tables to dynamically initialize flash size, page size and address width of the flash. More parameters can be added as per requirement.
SFDP parsing is made default but already existing method for parsing these parameters are not deprecated.
A flag is created to skip SFDP parsing for a particular flash, if required.

SFDP data lets us auto-detect the addressing mode supported by the flash which helps us access the flash using 4-byte address.

Add a new argument in spi_flash_addr() function to create commands with 3-byte or 4-byte address depending on the SFDP data read. Add pointer to struct spi_flash in struct spi_slave so that driver can have access to SFDP data.

Introduce new structures and functions to read and parse SFDP data. This is loosely based on Linux SFDP framework.

Signed-off-by Rajat Srivastava <[email protected]>

---
Changes in v2:
- Make SFDP parsing the default method.
- Change SPI_FLASH_USE_SFDP flag to SPI_FLASH_USE_SFDP to provide an option to skip SFDP parsing for a particular flash.
---

[...]

Review

Quoted 2 lines

(quoted diff) +2 -0

+static int spi_flash_parse_bfpt(struct spi_flash *flash,
+				const struct sfdp_parameter_header

*bfpt_header) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 6 lines

drivers/mtd/spi/spi_flash.c +6 -0

+	struct sfdp_bfpt bfpt;
+	size_t len;
+	int i, err;
+	u32 addr;
+
+	/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs.

*/

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+	if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
+		return -EINVAL;
+
+	/* Read the Basic Flash Parameter Table. */
+	len = min_t(size_t, sizeof(bfpt),
+		    bfpt_header->length * sizeof(u32));
+	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
+	memset(&bfpt, 0, sizeof(bfpt));

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	err = spi_flash_read_sfdp(flash, addr, len, &bfpt);
+	if (err < 0)
+		return err;
+
+	/* Fix endianness of the BFPT DWORDs. */
+	for (i = 0; i < BFPT_DWORD_MAX; i++)
+		bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+
+	/* Number of address bytes. */
+	switch (bfpt.dwords[BFPT_DWORD(1)] &

BFPT_DWORD1_ADDRESS_BYTES_MASK) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
+		flash->addr_width = 3;
+		break;
+
+	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
+		printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+		printf("mode on command\n");
+		/*
+		 * By default, 4-byte addressing mode is set.
+		 * To enforce 3-byte addressing mode, set addrwd_3_in_use

flag

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash) · line 1192

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+		 * in struct spi_flash for every command.
+		 */
+		flash->addr_width = 4;
+		break;
+
+	case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+		flash->addr_width = 4;
+		break;
+

I see your are updating flash->addr_width to 4 bytes here. But I don't see any code in this patch that changes flash->read_cmd (and write/erase cmds) to use 4B addressing opcodes. Also, I dont see any code that bypasses BAR register configuration when CONFIG_SPI_FLASH_BAR is defined and SFDP is available.

This patch will most certainly break SPI controller drivers(like cadence_qspi.c) that expect sf layer to provide correct read opcode and address width information, since there will be mismatch wrt opcode used and number of address byte sent.
Not sure how this patch was tested, but I guess fsl_qspi modifies read opcode internal to the driver and gets away with it.
Please modify the patch to update read_cmd/erase_cmd/write_cmd to use 4 byte addressing opcodes

Regards
Vigensh

Quoted 28 lines

drivers/mtd/spi/spi_flash.c +28 -0

+	default:
+		break;
+	}
+
+	/* Flash Memory Density (in bits). */
+	flash->size = bfpt.dwords[BFPT_DWORD(2)];
+	if (flash->size & BIT(31)) {
+		flash->size &= ~BIT(31);
+
+		/*
+		 * Prevent overflows on flash->size. Anyway, a NOR of 2^64
+		 * bits is unlikely to exist so this error probably means
+		 * the BFPT we are reading is corrupted/wrong.
+		 */
+		if (flash->size > 63)
+			return -EINVAL;
+
+		flash->size = 1ULL << flash->size;
+	} else {
+		flash->size++;
+	}
+	flash->size >>= 3; /* Convert to bytes. */
+
+	/* Stop here if not JESD216 rev A or later. */
+	if (bfpt_header->length < BFPT_DWORD_MAX)
+		return 0;
+
+	/* Page size: this field specifies 'N' so the page size = 2^N bytes. */

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	flash->page_size = bfpt.dwords[BFPT_DWORD(11)];
+	flash->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+	flash->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+	flash->page_size = 1U << flash->page_size;
+
+	return 0;
+}
+
+/*
+ * spi_flash_parse_sfdp() - parse the Serial Flash Discoverable

Parameters.

Review

Quoted 9 lines

+ * @flash: pointer to a 'struct spi_flash'
+ *
+ * The Serial Flash Discoverable Parameters are described by the

(quoted diff) +6 -0

+JEDEC JESD216
+ * specification. This is a standard which tends to supported by
+almost all
+ * (Q)SPI memory manufacturers. Those hard-coded tables allow us to
+learn at
+ * runtime the main parameters needed to perform basic SPI flash

operations.

Review

Quoted 5 lines

+ *
+ * Return: 0 on success, -errno otherwise.
+ */

(quoted diff) +2 -0

+static int spi_flash_parse_sfdp(struct spi_flash *flash) {
+	const struct sfdp_parameter_header *param_header,

*bfpt_header;

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +17 -0

+	struct sfdp_parameter_header *param_headers = NULL;
+	struct sfdp_header header;
+	size_t psize;
+	int i, err;
+
+	/* Get the SFDP header. */
+	err = spi_flash_read_sfdp(flash, 0, sizeof(header), &header);
+	if (err < 0)
+		return err;
+
+	/* Check the SFDP header version. */
+	if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
+	    header.major != SFDP_JESD216_MAJOR)
+		return -EINVAL;
+
+	/*
+	 * Verify that the first and only mandatory parameter header is a

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	 * Basic Flash Parameter Table header as specified in JESD216.
+	 */
+	bfpt_header = &header.bfpt_header;
+	if (SFDP_PARAM_HEADER_ID(bfpt_header) != SFDP_BFPT_ID ||
+	    bfpt_header->major != SFDP_JESD216_MAJOR)
+		return -EINVAL;
+
+	/*
+	 * Allocate memory then read all parameter headers with a single
+	 * Read SFDP command. These parameter headers will actually be

parsed

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 * twice: a first time to get the latest revision of the basic flash
+	 * parameter table, then a second time to handle the supported

optional

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +5 -0

+	 * tables.
+	 * Hence we read the parameter headers once for all to reduce the
+	 * processing time
+	 */
+	if (header.nph) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+		psize = header.nph * sizeof(*param_headers);
+
+		param_headers = malloc(psize);
+		if (!param_headers)
+			return -ENOMEM;
+
+		err = spi_flash_read_sfdp(flash, sizeof(header),
+					  psize, param_headers);
+		if (err < 0) {
+			dev_err(dev, "failed to read SFDP parameter

headers\n");

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 12 lines

drivers/mtd/spi/spi_flash.c +12 -0

+			goto exit;
+		}
+	}
+
+	/*
+	 * Check other parameter headers to get the latest revision of
+	 * the basic flash parameter table.
+	 */
+	for (i = 0; i < header.nph; i++) {
+		param_header = &param_headers[i];
+
+		if (SFDP_PARAM_HEADER_ID(param_header) ==

SFDP_BFPT_ID &&

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+		    param_header->major == SFDP_JESD216_MAJOR &&
+		    (param_header->minor > bfpt_header->minor ||
+		     (param_header->minor == bfpt_header->minor &&
+		      param_header->length > bfpt_header->length)))
+			bfpt_header = param_header;
+	}
+
+	err = spi_flash_parse_bfpt(flash, bfpt_header);

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+	if (err)
+		goto exit;
+
+exit:
+	free(param_headers);
+	return err;
+}
+
 static int set_quad_mode(struct spi_flash *flash,
 			 const struct spi_flash_info *info)  { @@ -1130,7

+1366,7 @@ int

Quoted 20 lines

spi_flash_scan(struct spi_flash *flash) {

drivers/mtd/spi/spi_flash.c +9 -1

 	struct spi_slave *spi = flash->spi;
 	const struct spi_flash_info *info = NULL;
-	int ret;
+	int ret, sfdp = 0;
 
 	info = spi_flash_read_id(flash);
 	if (IS_ERR_OR_NULL(info))

@@ -1196,9 +1432,28 @@ int spi_flash_scan(struct spi_flash *flash)
 	}
 #endif
 
+	spi->flash = flash;
+	flash->addrwd_3_in_use = false;
+
+	/* Read Serial Flash Discoverable Parameters and initialize
+	 * the following parameters of flash:
+	 * 1. Flash size
+	 * 2. Page size
+	 * 3. Address width to be used for commands

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1452

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+	 */
+	if (!(info->flags & SPI_FLASH_SKIP_SFDP)) {
+		flash->size = 0;
+		sfdp = spi_flash_parse_sfdp(flash);
+		if (sfdp < 0)
+			debug("Unable to get SFDP information\n");
+	}
+
 	/* Compute the flash size */
 	flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :

0;

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash)

Quoted 7 lines

drivers/mtd/spi/spi_flash.c +4 -1

-	flash->page_size = info->page_size;
+	if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+		flash->page_size = info->page_size;
+		flash->addr_width = SPI_FLASH_3B_ADDR_LEN;
+	}
 	/*
 	 * The Spansion S25FS512S, S25FL032P and S25FL064P have 256b

pages,

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +4 -1

 	 * yet use the 0x4d00 Extended JEDEC code. The rest of the Spansion

@@ -1213,7 +1468,10 @@ int spi_flash_scan(struct spi_flash *flash)
 	}
 	flash->page_size <<= flash->shift;
 	flash->sector_size = info->sector_size << flash->shift;
-	flash->size = flash->sector_size * info->n_sectors << flash->shift;
+	if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+		flash->size = flash->sector_size * info->n_sectors <<
+			flash->shift;
+	}
 #ifdef CONFIG_SF_DUAL_FLASH
 	if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
 		flash->size <<= 1;

@@ -1312,9 +1570,10 @@ int spi_flash_scan(struct spi_flash *flash)
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1579

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +4 -3

-	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
-	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
+	if ((info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) &&
+	    (flash->dual_flash == SF_SINGLE_FLASH &&
+	     flash->size > SPI_FLASH_16MB_BOUN) ||
+	     (flash->dual_flash > SF_SINGLE_FLASH &&
 	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
 		puts("SF: Warning - Only lower 16MiB accessible,");
 		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n"); diff -

-git

Quoted 24 lines

a/include/spi.h b/include/spi.h index 938627bc01..7189e60581 100644

include/spi.h +2 -0

@@ -10,6 +10,7 @@
 #define _SPI_H_
 
 #include <common.h>
+#include <spi_flash.h>
 
 /* SPI mode flags */
 #define SPI_CPHA	BIT(0)			/* clock phase */

@@ -103,6 +104,7 @@ struct spi_slave {
 	unsigned int bus;
 	unsigned int cs;
 #endif
+	struct spi_flash *flash;
 	uint mode;
 	unsigned int wordlen;
 	unsigned int max_read_size;

include/spi_flash.h index renamed from include/spi_flash.h +0 -0

0ec98fb55d..6558a4a1d5 100644

include/spi_flash.h struct spi_slave; · line 47

Quoted 1 line

include/spi_flash.h +0 -0

@@ -47,6 +47,9 @@ struct spi_slave;
  * @read_cmd:		Read cmd - Array Fast, Extn read and quad

read.

include/spi_flash.h struct spi_slave;

Quoted 5 lines

include/spi_flash.h +3 -0

  * @write_cmd:		Write cmd - page and quad program.
  * @dummy_byte:		Dummy cycles for read operation.
+ * @cmd_len:		Total length of command.
+ * @addr_width:		Number of address bytes.
+ * @addrwd_3_in_use:	Flag to send command in 3-byte address

mode.

Quoted 34 lines

include/spi_flash.h +22 -0

  * @memory_map:		Address of read-only SPI flash access
  * @flash_lock:		lock a region of the SPI Flash
  * @flash_unlock:	unlock a region of the SPI Flash

@@ -82,6 +85,9 @@ struct spi_flash {
 	u8 read_cmd;
 	u8 write_cmd;
 	u8 dummy_byte;
+	u8 cmd_len;
+	u8 addr_width;
+	bool addrwd_3_in_use;
 
 	void *memory_map;
 

@@ -107,6 +113,120 @@ struct spi_flash {  #endif  };
 
+/*
+ * Serial Flash Discoverable Parameter Headers  */ struct
+sfdp_parameter_header {
+	u8	id_lsb;
+	u8	minor;
+	u8	major;
+	u8	length; /* in double words */
+	u8	parameter_table_pointer[3]; /* byte address */
+	u8	id_msb;
+};
+
+struct sfdp_header {
+	u32	signature; /* Ox50444653U <=> "SFDP" */
+	u8	minor;
+	u8	major;
+	u8	nph; /* 0-base number of parameter headers */
+	u8	unused;
+
+	/* Basic Flash Parameter Table. */

include/spi_flash.h struct spi_flash { #endif }; · line 142

Quoted 10 lines

include/spi_flash.h +10 -0

+	struct sfdp_parameter_header	bfpt_header;
+};
+
+#define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) | (p)->id_lsb)
+#define SFDP_PARAM_HEADER_PTP(p) \
+	(((p)->parameter_table_pointer[2] << 16) | \
+	 ((p)->parameter_table_pointer[1] <<  8) | \
+	 ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID		0xff00  /* Basic Flash Parameter Table

*/

Quoted 13 lines

include/spi_flash.h +13 -0

+#define SFDP_SECTOR_MAP_ID	0xff81  /* Sector Map Table */
+
+#define SFDP_SIGNATURE		0x50444653U
+#define SFDP_JESD216_MAJOR	1
+#define SFDP_JESD216_MINOR	0
+#define SFDP_JESD216A_MINOR	5
+#define SFDP_JESD216B_MINOR	6
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ */
+#define BFPT_DWORD(i)		((i) - 1)
+#define BFPT_DWORD_MAX		16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216			9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK

	GENMASK(18, 17)

Quoted 5 lines

include/spi_flash.h +5 -0

+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
+#define BFPT_DWORD1_DTR				BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
+#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7,

4)

Quoted 25 lines

include/spi_flash.h +25 -0

+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
+ *         reads based on instruction. DQ3/HOLD# functions are hold during
+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect of
+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK

	GENMASK(22, 20)

include/spi_flash.h struct spi_flash { #endif }; · line 216

Quoted 1 line

include/spi_flash.h +1 -0

+#define BFPT_DWORD15_QER_NONE			(0x0UL << 20)

/* Micron */

include/spi_flash.h struct spi_flash { #endif }; · line 218

Quoted 2 lines

include/spi_flash.h +2 -0

+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /*

Macronix */

include/spi_flash.h struct spi_flash {

Quoted 3 lines

include/spi_flash.h +3 -0

+#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /*

Spansion */

Quoted 9 lines

include/spi_flash.h +5 -0

+
+struct sfdp_bfpt {
+	u32	dwords[BFPT_DWORD_MAX];
+};
+
 struct dm_spi_flash_ops {
 	int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
 	int (*write)(struct udevice *dev, u32 offset, size_t len,
 

Quoted 72 lines

Hi Rajat,

Quoted 27 lines · Rajat Srivastava

Add support for JESD216 rev B standard JEDEC Serial Flash Discoverable Parameters (SFDP) tables to dynamically initialize flash size, page size and address width of the flash. More parameters can be added as per requirement.
SFDP parsing is made default but already existing method for parsing these parameters are not deprecated.
A flag is created to skip SFDP parsing for a particular flash, if required.

SFDP data lets us auto-detect the addressing mode supported by the flash which helps us access the flash using 4-byte address.

Add a new argument in spi_flash_addr() function to create commands with 3-byte or 4-byte address depending on the SFDP data read. Add pointer to struct spi_flash in struct spi_slave so that driver can have access to SFDP data.

Introduce new structures and functions to read and parse SFDP data. This is loosely based on Linux SFDP framework.

Signed-off-by Rajat Srivastava <[email protected]>

---
Changes in v2:
- Make SFDP parsing the default method.
- Change SPI_FLASH_USE_SFDP flag to SPI_FLASH_USE_SFDP to provide an option to skip SFDP parsing for a particular flash.
---

[...]

Review

Quoted 2 lines

(quoted diff) +2 -0

+static int spi_flash_parse_bfpt(struct spi_flash *flash,
+				const struct sfdp_parameter_header

*bfpt_header) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 6 lines

drivers/mtd/spi/spi_flash.c +6 -0

+	struct sfdp_bfpt bfpt;
+	size_t len;
+	int i, err;
+	u32 addr;
+
+	/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs.

*/

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+	if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
+		return -EINVAL;
+
+	/* Read the Basic Flash Parameter Table. */
+	len = min_t(size_t, sizeof(bfpt),
+		    bfpt_header->length * sizeof(u32));
+	addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
+	memset(&bfpt, 0, sizeof(bfpt));

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	err = spi_flash_read_sfdp(flash, addr, len, &bfpt);
+	if (err < 0)
+		return err;
+
+	/* Fix endianness of the BFPT DWORDs. */
+	for (i = 0; i < BFPT_DWORD_MAX; i++)
+		bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+
+	/* Number of address bytes. */
+	switch (bfpt.dwords[BFPT_DWORD(1)] &

BFPT_DWORD1_ADDRESS_BYTES_MASK) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
+		flash->addr_width = 3;
+		break;
+
+	case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
+		printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+		printf("mode on command\n");
+		/*
+		 * By default, 4-byte addressing mode is set.
+		 * To enforce 3-byte addressing mode, set addrwd_3_in_use

flag

Quoted 15 lines

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash) · line 1192

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+		 * in struct spi_flash for every command.
+		 */
+		flash->addr_width = 4;
+		break;
+
+	case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+		flash->addr_width = 4;
+		break;
+

I see your are updating flash->addr_width to 4 bytes here. But I don't see any code in this patch that changes flash->read_cmd (and write/erase cmds) to use 4B addressing opcodes. Also, I dont see any code that bypasses BAR register configuration when CONFIG_SPI_FLASH_BAR is defined and SFDP is available.

We don't have any flash that supports CONFIG_SPI_FLASH_BAR. What do you suggest, shall we skip SFDP parsing in case CONFIG_SPI_FLASH_BAR is defined?

Quoted 8 lines

This patch will most certainly break SPI controller drivers(like cadence_qspi.c) that expect sf layer to provide correct read opcode and address width information, since there will be mismatch wrt opcode used and number of address byte sent.
Not sure how this patch was tested, but I guess fsl_qspi modifies read opcode internal to the driver and gets away with it.
Please modify the patch to update read_cmd/erase_cmd/write_cmd to use 4 byte addressing opcodes

If the flash supports only supports 4-byte opcodes then there will be no mismatch wrt opcode used and number of address bytes sent since all of the opcodes that the flash supports will be 4-bytes opcodes.
But if the flash supports both 3-byte as well as 4-byte address widths (which is a separate case) then this code will default the address mode to 4-byte but gives an option to use 3-byte by setting addrwd_3_in_use flag.

Regards
Rajat

Quoted 377 lines

Regards
Vigensh

Quoted 28 lines

drivers/mtd/spi/spi_flash.c +28 -0

+	default:
+		break;
+	}
+
+	/* Flash Memory Density (in bits). */
+	flash->size = bfpt.dwords[BFPT_DWORD(2)];
+	if (flash->size & BIT(31)) {
+		flash->size &= ~BIT(31);
+
+		/*
+		 * Prevent overflows on flash->size. Anyway, a NOR of 2^64
+		 * bits is unlikely to exist so this error probably means
+		 * the BFPT we are reading is corrupted/wrong.
+		 */
+		if (flash->size > 63)
+			return -EINVAL;
+
+		flash->size = 1ULL << flash->size;
+	} else {
+		flash->size++;
+	}
+	flash->size >>= 3; /* Convert to bytes. */
+
+	/* Stop here if not JESD216 rev A or later. */
+	if (bfpt_header->length < BFPT_DWORD_MAX)
+		return 0;
+
+	/* Page size: this field specifies 'N' so the page size = 2^N bytes. */

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	flash->page_size = bfpt.dwords[BFPT_DWORD(11)];
+	flash->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+	flash->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+	flash->page_size = 1U << flash->page_size;
+
+	return 0;
+}
+
+/*
+ * spi_flash_parse_sfdp() - parse the Serial Flash Discoverable

Parameters.

Review

Quoted 9 lines

+ * @flash: pointer to a 'struct spi_flash'
+ *
+ * The Serial Flash Discoverable Parameters are described by the

(quoted diff) +6 -0

+JEDEC JESD216
+ * specification. This is a standard which tends to supported by
+almost all
+ * (Q)SPI memory manufacturers. Those hard-coded tables allow us to
+learn at
+ * runtime the main parameters needed to perform basic SPI flash

operations.

Review

Quoted 5 lines

+ *
+ * Return: 0 on success, -errno otherwise.
+ */

(quoted diff) +2 -0

+static int spi_flash_parse_sfdp(struct spi_flash *flash) {
+	const struct sfdp_parameter_header *param_header,

*bfpt_header;

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +17 -0

+	struct sfdp_parameter_header *param_headers = NULL;
+	struct sfdp_header header;
+	size_t psize;
+	int i, err;
+
+	/* Get the SFDP header. */
+	err = spi_flash_read_sfdp(flash, 0, sizeof(header), &header);
+	if (err < 0)
+		return err;
+
+	/* Check the SFDP header version. */
+	if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
+	    header.major != SFDP_JESD216_MAJOR)
+		return -EINVAL;
+
+	/*
+	 * Verify that the first and only mandatory parameter header is a

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+	 * Basic Flash Parameter Table header as specified in JESD216.
+	 */
+	bfpt_header = &header.bfpt_header;
+	if (SFDP_PARAM_HEADER_ID(bfpt_header) != SFDP_BFPT_ID ||
+	    bfpt_header->major != SFDP_JESD216_MAJOR)
+		return -EINVAL;
+
+	/*
+	 * Allocate memory then read all parameter headers with a single
+	 * Read SFDP command. These parameter headers will actually be

parsed

Review

Quoted 2 lines

(quoted diff) +2 -0

+	 * twice: a first time to get the latest revision of the basic flash
+	 * parameter table, then a second time to handle the supported

optional

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +5 -0

+	 * tables.
+	 * Hence we read the parameter headers once for all to reduce the
+	 * processing time
+	 */
+	if (header.nph) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+		psize = header.nph * sizeof(*param_headers);
+
+		param_headers = malloc(psize);
+		if (!param_headers)
+			return -ENOMEM;
+
+		err = spi_flash_read_sfdp(flash, sizeof(header),
+					  psize, param_headers);
+		if (err < 0) {
+			dev_err(dev, "failed to read SFDP parameter

headers\n");

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 12 lines

drivers/mtd/spi/spi_flash.c +12 -0

+			goto exit;
+		}
+	}
+
+	/*
+	 * Check other parameter headers to get the latest revision of
+	 * the basic flash parameter table.
+	 */
+	for (i = 0; i < header.nph; i++) {
+		param_header = &param_headers[i];
+
+		if (SFDP_PARAM_HEADER_ID(param_header) ==

SFDP_BFPT_ID &&

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+		    param_header->major == SFDP_JESD216_MAJOR &&
+		    (param_header->minor > bfpt_header->minor ||
+		     (param_header->minor == bfpt_header->minor &&
+		      param_header->length > bfpt_header->length)))
+			bfpt_header = param_header;
+	}
+
+	err = spi_flash_parse_bfpt(flash, bfpt_header);

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+	if (err)
+		goto exit;
+
+exit:
+	free(param_headers);
+	return err;
+}
+
 static int set_quad_mode(struct spi_flash *flash,
 			 const struct spi_flash_info *info)  { @@ -1130,7

+1366,7 @@ int

Quoted 20 lines

spi_flash_scan(struct spi_flash *flash) {

drivers/mtd/spi/spi_flash.c +9 -1

 	struct spi_slave *spi = flash->spi;
 	const struct spi_flash_info *info = NULL;
-	int ret;
+	int ret, sfdp = 0;
 
 	info = spi_flash_read_id(flash);
 	if (IS_ERR_OR_NULL(info))

@@ -1196,9 +1432,28 @@ int spi_flash_scan(struct spi_flash *flash)
 	}
 #endif
 
+	spi->flash = flash;
+	flash->addrwd_3_in_use = false;
+
+	/* Read Serial Flash Discoverable Parameters and initialize
+	 * the following parameters of flash:
+	 * 1. Flash size
+	 * 2. Page size
+	 * 3. Address width to be used for commands

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1452

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+	 */
+	if (!(info->flags & SPI_FLASH_SKIP_SFDP)) {
+		flash->size = 0;
+		sfdp = spi_flash_parse_sfdp(flash);
+		if (sfdp < 0)
+			debug("Unable to get SFDP information\n");
+	}
+
 	/* Compute the flash size */
 	flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :

0;

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash)

Quoted 7 lines

drivers/mtd/spi/spi_flash.c +4 -1

-	flash->page_size = info->page_size;
+	if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+		flash->page_size = info->page_size;
+		flash->addr_width = SPI_FLASH_3B_ADDR_LEN;
+	}
 	/*
 	 * The Spansion S25FS512S, S25FL032P and S25FL064P have 256b

pages,

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +4 -1

 	 * yet use the 0x4d00 Extended JEDEC code. The rest of the Spansion

@@ -1213,7 +1468,10 @@ int spi_flash_scan(struct spi_flash *flash)
 	}
 	flash->page_size <<= flash->shift;
 	flash->sector_size = info->sector_size << flash->shift;
-	flash->size = flash->sector_size * info->n_sectors << flash->shift;
+	if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+		flash->size = flash->sector_size * info->n_sectors <<
+			flash->shift;
+	}
 #ifdef CONFIG_SF_DUAL_FLASH
 	if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
 		flash->size <<= 1;

@@ -1312,9 +1570,10 @@ int spi_flash_scan(struct spi_flash *flash)
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1579

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +4 -3

-	if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-	     (flash->size > SPI_FLASH_16MB_BOUN)) ||
-	     ((flash->dual_flash > SF_SINGLE_FLASH) &&
+	if ((info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) &&
+	    (flash->dual_flash == SF_SINGLE_FLASH &&
+	     flash->size > SPI_FLASH_16MB_BOUN) ||
+	     (flash->dual_flash > SF_SINGLE_FLASH &&
 	     (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
 		puts("SF: Warning - Only lower 16MiB accessible,");
 		puts(" Full access #define CONFIG_SPI_FLASH_BAR\n"); diff -

-git

Quoted 24 lines

a/include/spi.h b/include/spi.h index 938627bc01..7189e60581 100644

include/spi.h +2 -0

@@ -10,6 +10,7 @@
 #define _SPI_H_
 
 #include <common.h>
+#include <spi_flash.h>
 
 /* SPI mode flags */
 #define SPI_CPHA	BIT(0)			/* clock phase */

@@ -103,6 +104,7 @@ struct spi_slave {
 	unsigned int bus;
 	unsigned int cs;
 #endif
+	struct spi_flash *flash;
 	uint mode;
 	unsigned int wordlen;
 	unsigned int max_read_size;

include/spi_flash.h index renamed from include/spi_flash.h +0 -0

0ec98fb55d..6558a4a1d5 100644

include/spi_flash.h struct spi_slave; · line 47

Quoted 1 line

include/spi_flash.h +0 -0

@@ -47,6 +47,9 @@ struct spi_slave;
  * @read_cmd:		Read cmd - Array Fast, Extn read and quad

read.

include/spi_flash.h struct spi_slave;

Quoted 5 lines

include/spi_flash.h +3 -0

  * @write_cmd:		Write cmd - page and quad program.
  * @dummy_byte:		Dummy cycles for read operation.
+ * @cmd_len:		Total length of command.
+ * @addr_width:		Number of address bytes.
+ * @addrwd_3_in_use:	Flag to send command in 3-byte address

mode.

Quoted 34 lines

include/spi_flash.h +22 -0

  * @memory_map:		Address of read-only SPI flash access
  * @flash_lock:		lock a region of the SPI Flash
  * @flash_unlock:	unlock a region of the SPI Flash

@@ -82,6 +85,9 @@ struct spi_flash {
 	u8 read_cmd;
 	u8 write_cmd;
 	u8 dummy_byte;
+	u8 cmd_len;
+	u8 addr_width;
+	bool addrwd_3_in_use;
 
 	void *memory_map;
 

@@ -107,6 +113,120 @@ struct spi_flash {  #endif  };
 
+/*
+ * Serial Flash Discoverable Parameter Headers  */ struct
+sfdp_parameter_header {
+	u8	id_lsb;
+	u8	minor;
+	u8	major;
+	u8	length; /* in double words */
+	u8	parameter_table_pointer[3]; /* byte address */
+	u8	id_msb;
+};
+
+struct sfdp_header {
+	u32	signature; /* Ox50444653U <=> "SFDP" */
+	u8	minor;
+	u8	major;
+	u8	nph; /* 0-base number of parameter headers */
+	u8	unused;
+
+	/* Basic Flash Parameter Table. */

include/spi_flash.h struct spi_flash { #endif }; · line 142

Quoted 10 lines

include/spi_flash.h +10 -0

+	struct sfdp_parameter_header	bfpt_header;
+};
+
+#define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) | (p)->id_lsb)
+#define SFDP_PARAM_HEADER_PTP(p) \
+	(((p)->parameter_table_pointer[2] << 16) | \
+	 ((p)->parameter_table_pointer[1] <<  8) | \
+	 ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID		0xff00  /* Basic Flash Parameter Table

*/

Quoted 13 lines

include/spi_flash.h +13 -0

+#define SFDP_SECTOR_MAP_ID	0xff81  /* Sector Map Table */
+
+#define SFDP_SIGNATURE		0x50444653U
+#define SFDP_JESD216_MAJOR	1
+#define SFDP_JESD216_MINOR	0
+#define SFDP_JESD216A_MINOR	5
+#define SFDP_JESD216B_MINOR	6
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ */
+#define BFPT_DWORD(i)		((i) - 1)
+#define BFPT_DWORD_MAX		16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216			9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK

	GENMASK(18, 17)

Quoted 5 lines

include/spi_flash.h +5 -0

+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
+#define BFPT_DWORD1_DTR				BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
+#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7,

4)

Quoted 25 lines

include/spi_flash.h +25 -0

+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
+ *         reads based on instruction. DQ3/HOLD# functions are hold during
+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect of
+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK

	GENMASK(22, 20)

include/spi_flash.h struct spi_flash { #endif }; · line 216

Quoted 1 line

include/spi_flash.h +1 -0

+#define BFPT_DWORD15_QER_NONE			(0x0UL << 20)

/* Micron */

include/spi_flash.h struct spi_flash { #endif }; · line 218

Quoted 2 lines

include/spi_flash.h +2 -0

+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /*

Macronix */

include/spi_flash.h struct spi_flash {

Quoted 3 lines

include/spi_flash.h +3 -0

+#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /*

Spansion */

Quoted 9 lines

include/spi_flash.h +5 -0

+
+struct sfdp_bfpt {
+	u32	dwords[BFPT_DWORD_MAX];
+};
+
 struct dm_spi_flash_ops {
 	int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
 	int (*write)(struct udevice *dev, u32 offset, size_t len,
 

Quoted 88 lines · Rajat Srivastava

Quoted 72 lines

Hi Rajat,

Quoted 27 lines · Rajat Srivastava

Add support for JESD216 rev B standard JEDEC Serial Flash Discoverable Parameters (SFDP) tables to dynamically initialize flash size, page size and address width of the flash. More parameters can be added as per requirement.
SFDP parsing is made default but already existing method for parsing these parameters are not deprecated.
A flag is created to skip SFDP parsing for a particular flash, if required.

SFDP data lets us auto-detect the addressing mode supported by the flash which helps us access the flash using 4-byte address.

Add a new argument in spi_flash_addr() function to create commands with 3-byte or 4-byte address depending on the SFDP data read. Add pointer to struct spi_flash in struct spi_slave so that driver can have access to SFDP data.

Introduce new structures and functions to read and parse SFDP data. This is loosely based on Linux SFDP framework.

Signed-off-by Rajat Srivastava <[email protected]>

---
Changes in v2:
- Make SFDP parsing the default method.
- Change SPI_FLASH_USE_SFDP flag to SPI_FLASH_USE_SFDP to provide an option to skip SFDP parsing for a particular flash.
---

[...]

Review

Quoted 2 lines

(quoted diff) +2 -0

+static int spi_flash_parse_bfpt(struct spi_flash *flash,
+                           const struct sfdp_parameter_header

*bfpt_header) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 6 lines

drivers/mtd/spi/spi_flash.c +6 -0

+   struct sfdp_bfpt bfpt;
+   size_t len;
+   int i, err;
+   u32 addr;
+
+   /* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs.

*/

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+   if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
+           return -EINVAL;
+
+   /* Read the Basic Flash Parameter Table. */
+   len = min_t(size_t, sizeof(bfpt),
+               bfpt_header->length * sizeof(u32));
+   addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
+   memset(&bfpt, 0, sizeof(bfpt));

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   err = spi_flash_read_sfdp(flash, addr, len, &bfpt);
+   if (err < 0)
+           return err;
+
+   /* Fix endianness of the BFPT DWORDs. */
+   for (i = 0; i < BFPT_DWORD_MAX; i++)
+           bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+
+   /* Number of address bytes. */
+   switch (bfpt.dwords[BFPT_DWORD(1)] &

BFPT_DWORD1_ADDRESS_BYTES_MASK) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
+           flash->addr_width = 3;
+           break;
+
+   case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
+           printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+           printf("mode on command\n");
+           /*
+            * By default, 4-byte addressing mode is set.
+            * To enforce 3-byte addressing mode, set addrwd_3_in_use

flag

Quoted 15 lines

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash) · line 1192

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+            * in struct spi_flash for every command.
+            */
+           flash->addr_width = 4;
+           break;
+
+   case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+           flash->addr_width = 4;
+           break;
+

I see your are updating flash->addr_width to 4 bytes here. But I don't see any code in this patch that changes flash->read_cmd (and write/erase cmds) to use 4B addressing opcodes. Also, I dont see any code that bypasses BAR register configuration when CONFIG_SPI_FLASH_BAR is defined and SFDP is available.

Quoted 2 lines

We don't have any flash that supports CONFIG_SPI_FLASH_BAR. What do you suggest, shall we skip SFDP parsing in case CONFIG_SPI_FLASH_BAR is defined?

I suggest skipping BAR configuration completely if flash supports address width of 4 as per SFDP. BAR configuration is only needed if flash does not support 4 byte addressing and flash size is > 16MB

Quoted 9 lines

Quoted 8 lines

This patch will most certainly break SPI controller drivers(like cadence_qspi.c) that expect sf layer to provide correct read opcode and address width information, since there will be mismatch wrt opcode used and number of address byte sent.
Not sure how this patch was tested, but I guess fsl_qspi modifies read opcode internal to the driver and gets away with it.
Please modify the patch to update read_cmd/erase_cmd/write_cmd to use 4 byte addressing opcodes

Quoted 7 lines

If the flash supports only supports 4-byte opcodes then there will be no mismatch wrt opcode used and number of address bytes sent since all of the opcodes that the flash supports will be 4-bytes opcodes.
But if the flash supports both 3-byte as well as 4-byte address widths (which is a separate case) then this code will default the address mode to 4-byte but gives an option to use 3-byte by setting addrwd_3_in_use flag.

Lets assume flash supports both 3 and 4 byte addressing (AFAIK, majority of flash parts do that). In this case, this patch is defaulting to 4 byte address width but flash->read_cmd/write_cmd/erase_cmd are still set to 3 byte addressing opcodes.
So, SPI controller would end up send 3 addressing byte opcode but 4 byte of address. The flash would interpret last byte of address as first byte of data and thus lead to data corruption in case of write. Isn't that a problem?

In particular you are not parsing entire bfpt like in [1] and will break flash operations

[1]https://elixir.bootlin.com/linux/latest/source/drivers/mtd/spi-nor/spi-nor.c#L2259

Regards
Vignesh

Quoted 371 lines

Regards
Rajat

Quoted 368 lines

Regards
Vigensh

Quoted 28 lines

drivers/mtd/spi/spi_flash.c +28 -0

+   default:
+           break;
+   }
+
+   /* Flash Memory Density (in bits). */
+   flash->size = bfpt.dwords[BFPT_DWORD(2)];
+   if (flash->size & BIT(31)) {
+           flash->size &= ~BIT(31);
+
+           /*
+            * Prevent overflows on flash->size. Anyway, a NOR of 2^64
+            * bits is unlikely to exist so this error probably means
+            * the BFPT we are reading is corrupted/wrong.
+            */
+           if (flash->size > 63)
+                   return -EINVAL;
+
+           flash->size = 1ULL << flash->size;
+   } else {
+           flash->size++;
+   }
+   flash->size >>= 3; /* Convert to bytes. */
+
+   /* Stop here if not JESD216 rev A or later. */
+   if (bfpt_header->length < BFPT_DWORD_MAX)
+           return 0;
+
+   /* Page size: this field specifies 'N' so the page size = 2^N bytes. */

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   flash->page_size = bfpt.dwords[BFPT_DWORD(11)];
+   flash->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+   flash->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+   flash->page_size = 1U << flash->page_size;
+
+   return 0;
+}
+
+/*
+ * spi_flash_parse_sfdp() - parse the Serial Flash Discoverable

Parameters.

Review

Quoted 9 lines

+ * @flash: pointer to a 'struct spi_flash'
+ *
+ * The Serial Flash Discoverable Parameters are described by the

(quoted diff) +6 -0

+JEDEC JESD216
+ * specification. This is a standard which tends to supported by
+almost all
+ * (Q)SPI memory manufacturers. Those hard-coded tables allow us to
+learn at
+ * runtime the main parameters needed to perform basic SPI flash

operations.

Review

Quoted 5 lines

+ *
+ * Return: 0 on success, -errno otherwise.
+ */

(quoted diff) +2 -0

+static int spi_flash_parse_sfdp(struct spi_flash *flash) {
+   const struct sfdp_parameter_header *param_header,

*bfpt_header;

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +17 -0

+   struct sfdp_parameter_header *param_headers = NULL;
+   struct sfdp_header header;
+   size_t psize;
+   int i, err;
+
+   /* Get the SFDP header. */
+   err = spi_flash_read_sfdp(flash, 0, sizeof(header), &header);
+   if (err < 0)
+           return err;
+
+   /* Check the SFDP header version. */
+   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
+       header.major != SFDP_JESD216_MAJOR)
+           return -EINVAL;
+
+   /*
+    * Verify that the first and only mandatory parameter header is a

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+    * Basic Flash Parameter Table header as specified in JESD216.
+    */
+   bfpt_header = &header.bfpt_header;
+   if (SFDP_PARAM_HEADER_ID(bfpt_header) != SFDP_BFPT_ID ||
+       bfpt_header->major != SFDP_JESD216_MAJOR)
+           return -EINVAL;
+
+   /*
+    * Allocate memory then read all parameter headers with a single
+    * Read SFDP command. These parameter headers will actually be

parsed

Review

Quoted 2 lines

(quoted diff) +2 -0

+    * twice: a first time to get the latest revision of the basic flash
+    * parameter table, then a second time to handle the supported

optional

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +5 -0

+    * tables.
+    * Hence we read the parameter headers once for all to reduce the
+    * processing time
+    */
+   if (header.nph) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+           psize = header.nph * sizeof(*param_headers);
+
+           param_headers = malloc(psize);
+           if (!param_headers)
+                   return -ENOMEM;
+
+           err = spi_flash_read_sfdp(flash, sizeof(header),
+                                     psize, param_headers);
+           if (err < 0) {
+                   dev_err(dev, "failed to read SFDP parameter

headers\n");

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 12 lines

drivers/mtd/spi/spi_flash.c +12 -0

+                   goto exit;
+           }
+   }
+
+   /*
+    * Check other parameter headers to get the latest revision of
+    * the basic flash parameter table.
+    */
+   for (i = 0; i < header.nph; i++) {
+           param_header = &param_headers[i];
+
+           if (SFDP_PARAM_HEADER_ID(param_header) ==

SFDP_BFPT_ID &&

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+               param_header->major == SFDP_JESD216_MAJOR &&
+               (param_header->minor > bfpt_header->minor ||
+                (param_header->minor == bfpt_header->minor &&
+                 param_header->length > bfpt_header->length)))
+                   bfpt_header = param_header;
+   }
+
+   err = spi_flash_parse_bfpt(flash, bfpt_header);

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+   if (err)
+           goto exit;
+
+exit:
+   free(param_headers);
+   return err;
+}
+
 static int set_quad_mode(struct spi_flash *flash,
                     const struct spi_flash_info *info)  { @@ -1130,7

+1366,7 @@ int

Quoted 20 lines

spi_flash_scan(struct spi_flash *flash)  {
    struct spi_slave *spi = flash->spi;
    const struct spi_flash_info *info = NULL;

drivers/mtd/spi/spi_flash.c +9 -1

-   int ret;
+   int ret, sfdp = 0;
 
    info = spi_flash_read_id(flash);
    if (IS_ERR_OR_NULL(info))

@@ -1196,9 +1432,28 @@ int spi_flash_scan(struct spi_flash *flash)
    }
 #endif
 
+   spi->flash = flash;
+   flash->addrwd_3_in_use = false;
+
+   /* Read Serial Flash Discoverable Parameters and initialize
+    * the following parameters of flash:
+    * 1. Flash size
+    * 2. Page size
+    * 3. Address width to be used for commands

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1452

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+    */
+   if (!(info->flags & SPI_FLASH_SKIP_SFDP)) {
+           flash->size = 0;
+           sfdp = spi_flash_parse_sfdp(flash);
+           if (sfdp < 0)
+                   debug("Unable to get SFDP information\n");
+   }
+
    /* Compute the flash size */
    flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :

0;

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash)

Quoted 7 lines

drivers/mtd/spi/spi_flash.c +4 -1

-   flash->page_size = info->page_size;
+   if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+           flash->page_size = info->page_size;
+           flash->addr_width = SPI_FLASH_3B_ADDR_LEN;
+   }
    /*
     * The Spansion S25FS512S, S25FL032P and S25FL064P have 256b

pages,

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +4 -1

     * yet use the 0x4d00 Extended JEDEC code. The rest of the Spansion

@@ -1213,7 +1468,10 @@ int spi_flash_scan(struct spi_flash *flash)
    }
    flash->page_size <<= flash->shift;
    flash->sector_size = info->sector_size << flash->shift;
-   flash->size = flash->sector_size * info->n_sectors << flash->shift;
+   if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+           flash->size = flash->sector_size * info->n_sectors <<
+                   flash->shift;
+   }
 #ifdef CONFIG_SF_DUAL_FLASH
    if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
            flash->size <<= 1;

@@ -1312,9 +1570,10 @@ int spi_flash_scan(struct spi_flash *flash)
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1579

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +4 -3

-   if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-        (flash->size > SPI_FLASH_16MB_BOUN)) ||
-        ((flash->dual_flash > SF_SINGLE_FLASH) &&
+   if ((info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) &&
+       (flash->dual_flash == SF_SINGLE_FLASH &&
+        flash->size > SPI_FLASH_16MB_BOUN) ||
+        (flash->dual_flash > SF_SINGLE_FLASH &&
         (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
            puts("SF: Warning - Only lower 16MiB accessible,");
            puts(" Full access #define CONFIG_SPI_FLASH_BAR\n"); diff -

-git

Quoted 24 lines

a/include/spi.h b/include/spi.h index 938627bc01..7189e60581 100644

include/spi.h +2 -0

@@ -10,6 +10,7 @@
 #define _SPI_H_
 
 #include <common.h>
+#include <spi_flash.h>
 
 /* SPI mode flags */
 #define SPI_CPHA   BIT(0)                  /* clock phase */

@@ -103,6 +104,7 @@ struct spi_slave {
    unsigned int bus;
    unsigned int cs;
 #endif
+   struct spi_flash *flash;
    uint mode;
    unsigned int wordlen;
    unsigned int max_read_size;

include/spi_flash.h index renamed from include/spi_flash.h +0 -0

0ec98fb55d..6558a4a1d5 100644

include/spi_flash.h struct spi_slave; · line 47

Quoted 1 line

include/spi_flash.h +0 -0

@@ -47,6 +47,9 @@ struct spi_slave;
  * @read_cmd:              Read cmd - Array Fast, Extn read and quad

read.

include/spi_flash.h struct spi_slave;

Quoted 5 lines

include/spi_flash.h +3 -0

  * @write_cmd:             Write cmd - page and quad program.
  * @dummy_byte:            Dummy cycles for read operation.
+ * @cmd_len:               Total length of command.
+ * @addr_width:            Number of address bytes.
+ * @addrwd_3_in_use:       Flag to send command in 3-byte address

mode.

Quoted 34 lines

include/spi_flash.h +22 -0

  * @memory_map:            Address of read-only SPI flash access
  * @flash_lock:            lock a region of the SPI Flash
  * @flash_unlock:  unlock a region of the SPI Flash

@@ -82,6 +85,9 @@ struct spi_flash {
    u8 read_cmd;
    u8 write_cmd;
    u8 dummy_byte;
+   u8 cmd_len;
+   u8 addr_width;
+   bool addrwd_3_in_use;
 
    void *memory_map;
 

@@ -107,6 +113,120 @@ struct spi_flash {  #endif  };
 
+/*
+ * Serial Flash Discoverable Parameter Headers  */ struct
+sfdp_parameter_header {
+   u8      id_lsb;
+   u8      minor;
+   u8      major;
+   u8      length; /* in double words */
+   u8      parameter_table_pointer[3]; /* byte address */
+   u8      id_msb;
+};
+
+struct sfdp_header {
+   u32     signature; /* Ox50444653U <=> "SFDP" */
+   u8      minor;
+   u8      major;
+   u8      nph; /* 0-base number of parameter headers */
+   u8      unused;
+
+   /* Basic Flash Parameter Table. */

include/spi_flash.h struct spi_flash { #endif }; · line 142

Quoted 10 lines

include/spi_flash.h +10 -0

+   struct sfdp_parameter_header    bfpt_header;
+};
+
+#define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) | (p)->id_lsb)
+#define SFDP_PARAM_HEADER_PTP(p) \
+   (((p)->parameter_table_pointer[2] << 16) | \
+    ((p)->parameter_table_pointer[1] <<  8) | \
+    ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID               0xff00  /* Basic Flash Parameter Table

*/

Quoted 13 lines

include/spi_flash.h +13 -0

+#define SFDP_SECTOR_MAP_ID 0xff81  /* Sector Map Table */
+
+#define SFDP_SIGNATURE             0x50444653U
+#define SFDP_JESD216_MAJOR 1
+#define SFDP_JESD216_MINOR 0
+#define SFDP_JESD216A_MINOR        5
+#define SFDP_JESD216B_MINOR        6
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ */
+#define BFPT_DWORD(i)              ((i) - 1)
+#define BFPT_DWORD_MAX             16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216                     9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2                BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK

     GENMASK(18, 17)

Quoted 5 lines

include/spi_flash.h +5 -0

+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY   (0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4   (0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY   (0x2UL << 17)
+#define BFPT_DWORD1_DTR                            BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2                BIT(20)

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+#define BFPT_DWORD1_FAST_READ_1_4_4                BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4                BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2                BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4                BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT               4
+#define BFPT_DWORD11_PAGE_SIZE_MASK                GENMASK(7,

4)

Quoted 25 lines

include/spi_flash.h +25 -0

+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
+ *         reads based on instruction. DQ3/HOLD# functions are hold during
+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect of
+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK

     GENMASK(22, 20)

include/spi_flash.h struct spi_flash { #endif }; · line 216

Quoted 1 line

include/spi_flash.h +1 -0

+#define BFPT_DWORD15_QER_NONE                      (0x0UL << 20)

/* Micron */

include/spi_flash.h struct spi_flash { #endif }; · line 218

Quoted 2 lines

include/spi_flash.h +2 -0

+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY            (0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6          (0x2UL << 20) /*

Macronix */

include/spi_flash.h struct spi_flash {

Quoted 3 lines

include/spi_flash.h +3 -0

+#define BFPT_DWORD15_QER_SR2_BIT7          (0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD            (0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1          (0x5UL << 20) /*

Spansion */

Quoted 9 lines

Quoted 9 lines

include/spi_flash.h struct spi_flash {

Quoted 9 lines

include/spi_flash.h +5 -0

+
+struct sfdp_bfpt {
+   u32     dwords[BFPT_DWORD_MAX];
+};
+
 struct dm_spi_flash_ops {
    int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
    int (*write)(struct udevice *dev, u32 offset, size_t len,
 

--
Regards
Vignesh
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
https://lists.denx.de/listinfo/u-boot

-----Original Message-----
From: Simon Goldschmidt <[email protected]>
Sent: Friday, November 16, 2018 3:53 PM
To: vigneshr at ti.com
Cc: Rajat Srivastava <[email protected]>; U-Boot Mailing List <u- boot at lists.denx.de>; Jagan Teki <[email protected]>
Subject: Re: [U-Boot] [PATCH v2 1/2] mtd/spi: Add JEDEC SFDP support in SPI framework

Quoted 24 lines · Vignesh R

Quoted 20 lines · Rajat Srivastava

Quoted 20 lines

Hi Rajat,

Quoted 17 lines · Rajat Srivastava

Add support for JESD216 rev B standard JEDEC Serial Flash Discoverable Parameters (SFDP) tables to dynamically initialize flash size, page size and address width of the flash. More parameters can be added as per requirement.
SFDP parsing is made default but already existing method for parsing these parameters are not deprecated.
A flag is created to skip SFDP parsing for a particular flash, if required.

SFDP data lets us auto-detect the addressing mode supported by the flash which helps us access the flash using 4-byte address.

Add a new argument in spi_flash_addr() function to create commands with 3-byte or 4-byte address depending on the SFDP data read. Add pointer to struct spi_flash in struct spi_slave so that driver can have access to SFDP data.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Introduce new structures and functions to read and parse SFDP data. This is loosely based on Linux SFDP framework.

Signed-off-by Rajat Srivastava <[email protected]>

---
Changes in v2:
- Make SFDP parsing the default method.
- Change SPI_FLASH_USE_SFDP flag to SPI_FLASH_USE_SFDP to

provide

Quoted 62 lines

Quoted 62 lines

Quoted 61 lines

Quoted 2 lines

an option to skip SFDP parsing for a particular flash.
---

[...]

Review

Quoted 2 lines

(quoted diff) +2 -0

+static int spi_flash_parse_bfpt(struct spi_flash *flash,
+                           const struct sfdp_parameter_header

*bfpt_header) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 6 lines

drivers/mtd/spi/spi_flash.c +6 -0

+   struct sfdp_bfpt bfpt;
+   size_t len;
+   int i, err;
+   u32 addr;
+
+   /* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs.

*/

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+   if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
+           return -EINVAL;
+
+   /* Read the Basic Flash Parameter Table. */
+   len = min_t(size_t, sizeof(bfpt),
+               bfpt_header->length * sizeof(u32));
+   addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
+   memset(&bfpt, 0, sizeof(bfpt));

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   err = spi_flash_read_sfdp(flash, addr, len, &bfpt);
+   if (err < 0)
+           return err;
+
+   /* Fix endianness of the BFPT DWORDs. */
+   for (i = 0; i < BFPT_DWORD_MAX; i++)
+           bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+
+   /* Number of address bytes. */
+   switch (bfpt.dwords[BFPT_DWORD(1)] &

BFPT_DWORD1_ADDRESS_BYTES_MASK) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 11 lines

drivers/mtd/spi/spi_flash.c +11 -0

+   case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
+           flash->addr_width = 3;
+           break;
+
+   case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
+           printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+           printf("mode on command\n");
+           /*
+            * By default, 4-byte addressing mode is set.
+            * To enforce 3-byte addressing mode, set
+ addrwd_3_in_use

flag

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash) · line 1192

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+            * in struct spi_flash for every command.
+            */
+           flash->addr_width = 4;
+           break;
+
+   case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+           flash->addr_width = 4;
+           break;
+

I see your are updating flash->addr_width to 4 bytes here. But I don't see any code in this patch that changes flash->read_cmd (and write/erase
cmds) to use 4B addressing opcodes. Also, I dont see any code that bypasses BAR register configuration when CONFIG_SPI_FLASH_BAR is defined and SFDP is available.

Quoted 1 line

Quoted 1 line

We don't have any flash that supports CONFIG_SPI_FLASH_BAR. What do

you suggest, shall we skip SFDP parsing in case CONFIG_SPI_FLASH_BAR is defined?

Quoted 22 lines

Quoted 1 line

I suggest skipping BAR configuration completely if flash supports address width of 4 as per SFDP. BAR configuration is only needed if flash does not support 4 byte addressing and flash size is > 16MB

Quoted 16 lines

Quoted 9 lines

This patch will most certainly break SPI controller drivers(like cadence_qspi.c) that expect sf layer to provide correct read opcode and address width information, since there will be mismatch wrt opcode used and number of address byte sent.
Not sure how this patch was tested, but I guess fsl_qspi modifies read opcode internal to the driver and gets away with it. Please modify the patch to update read_cmd/erase_cmd/write_cmd to use
4 byte addressing opcodes

If the flash supports only supports 4-byte opcodes then there will be no mismatch wrt opcode used and number of address bytes sent since all of the opcodes that the flash supports will be 4-bytes opcodes. But if the flash supports both 3-byte as well as 4-byte address widths (which is a separate case) then this code will default the address mode to 4-byte but gives an option to use 3-byte by setting

addrwd_3_in_use flag.

Quoted 6 lines

Quoted 1 line

Lets assume flash supports both 3 and 4 byte addressing (AFAIK, majority of flash parts do that). In this case, this patch is defaulting to 4 byte address width but flash->read_cmd/write_cmd/erase_cmd are still set to 3 byte addressing

opcodes.

Quoted 4 lines

So, SPI controller would end up send 3 addressing byte opcode but 4 byte of address. The flash would interpret last byte of address as first byte of data and thus lead to data corruption in case of write. Isn't that a problem?

Additionally, there are flash chips with a 3-/4-byte mode change instruction. But using 4-byte opcodes should be preferred.

Simon

Quoted 5 lines

In particular you are not parsing entire bfpt like in [1] and will break flash operations

[1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F

Quoted 1 line

elixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fmtd%2Fspi-

nor

Quoted 1 line

%2Fspi-

nor.c%23L2259&amp;data=02%7C01%7Crajat.srivastava%40nxp.com%7Ca

Quoted 1 line

cee153f50194bba71c808d64bad8d2e%7C686ea1d3bc2b4c6fa92cd99c5c301635 %7C0

Quoted 1 line

%7C0%7C636779606107591743&amp;sdata=UACl919%2BMQmdtBo5GUTAy M428u1bT1bU

Quoted 189 lines

rXuX2GsSjb0%3D&amp;reserved=0

Regards
Vignesh

Quoted 184 lines

Regards
Rajat

Quoted 181 lines

Regards
Vigensh

Quoted 28 lines

drivers/mtd/spi/spi_flash.c +28 -0

+   default:
+           break;
+   }
+
+   /* Flash Memory Density (in bits). */
+   flash->size = bfpt.dwords[BFPT_DWORD(2)];
+   if (flash->size & BIT(31)) {
+           flash->size &= ~BIT(31);
+
+           /*
+            * Prevent overflows on flash->size. Anyway, a NOR of 2^64
+            * bits is unlikely to exist so this error probably means
+            * the BFPT we are reading is corrupted/wrong.
+            */
+           if (flash->size > 63)
+                   return -EINVAL;
+
+           flash->size = 1ULL << flash->size;
+   } else {
+           flash->size++;
+   }
+   flash->size >>= 3; /* Convert to bytes. */
+
+   /* Stop here if not JESD216 rev A or later. */
+   if (bfpt_header->length < BFPT_DWORD_MAX)
+           return 0;
+
+   /* Page size: this field specifies 'N' so the page size = 2^N bytes. */

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   flash->page_size = bfpt.dwords[BFPT_DWORD(11)];
+   flash->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+   flash->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+   flash->page_size = 1U << flash->page_size;
+
+   return 0;
+}
+
+/*
+ * spi_flash_parse_sfdp() - parse the Serial Flash Discoverable

Parameters.

Review

Quoted 9 lines

+ * @flash: pointer to a 'struct spi_flash'
+ *
+ * The Serial Flash Discoverable Parameters are described by the

(quoted diff) +6 -0

+JEDEC JESD216
+ * specification. This is a standard which tends to supported by
+almost all
+ * (Q)SPI memory manufacturers. Those hard-coded tables allow us
+to learn at
+ * runtime the main parameters needed to perform basic SPI flash

operations.

Review

Quoted 5 lines

+ *
+ * Return: 0 on success, -errno otherwise.
+ */

(quoted diff) +2 -0

+static int spi_flash_parse_sfdp(struct spi_flash *flash) {
+   const struct sfdp_parameter_header *param_header,

*bfpt_header;

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +17 -0

+   struct sfdp_parameter_header *param_headers = NULL;
+   struct sfdp_header header;
+   size_t psize;
+   int i, err;
+
+   /* Get the SFDP header. */
+   err = spi_flash_read_sfdp(flash, 0, sizeof(header), &header);
+   if (err < 0)
+           return err;
+
+   /* Check the SFDP header version. */
+   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
+       header.major != SFDP_JESD216_MAJOR)
+           return -EINVAL;
+
+   /*
+    * Verify that the first and only mandatory parameter header is a

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+    * Basic Flash Parameter Table header as specified in JESD216.
+    */
+   bfpt_header = &header.bfpt_header;
+   if (SFDP_PARAM_HEADER_ID(bfpt_header) != SFDP_BFPT_ID ||
+       bfpt_header->major != SFDP_JESD216_MAJOR)
+           return -EINVAL;
+
+   /*
+    * Allocate memory then read all parameter headers with a single
+    * Read SFDP command. These parameter headers will actually be

parsed

Review

Quoted 2 lines

(quoted diff) +2 -0

+    * twice: a first time to get the latest revision of the basic flash
+    * parameter table, then a second time to handle the supported

optional

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +5 -0

+    * tables.
+    * Hence we read the parameter headers once for all to reduce the
+    * processing time
+    */
+   if (header.nph) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+           psize = header.nph * sizeof(*param_headers);
+
+           param_headers = malloc(psize);
+           if (!param_headers)
+                   return -ENOMEM;
+
+           err = spi_flash_read_sfdp(flash, sizeof(header),
+                                     psize, param_headers);
+           if (err < 0) {
+                   dev_err(dev, "failed to read SFDP parameter

headers\n");

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 12 lines

drivers/mtd/spi/spi_flash.c +12 -0

+                   goto exit;
+           }
+   }
+
+   /*
+    * Check other parameter headers to get the latest revision of
+    * the basic flash parameter table.
+    */
+   for (i = 0; i < header.nph; i++) {
+           param_header = &param_headers[i];
+
+           if (SFDP_PARAM_HEADER_ID(param_header) ==

SFDP_BFPT_ID &&

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+               param_header->major == SFDP_JESD216_MAJOR &&
+               (param_header->minor > bfpt_header->minor ||
+                (param_header->minor == bfpt_header->minor &&
+                 param_header->length > bfpt_header->length)))
+                   bfpt_header = param_header;
+   }
+
+   err = spi_flash_parse_bfpt(flash, bfpt_header);
+   if (err)

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +7 -1

+           goto exit;
+
+exit:
+   free(param_headers);
+   return err;
+}
+
 static int set_quad_mode(struct spi_flash *flash,
                     const struct spi_flash_info *info)  { @@
-1130,7

+1366,7 @@ int

Quoted 20 lines

spi_flash_scan(struct spi_flash *flash)  {
    struct spi_slave *spi = flash->spi;
    const struct spi_flash_info *info = NULL;

drivers/mtd/spi/spi_flash.c +9 -1

-   int ret;
+   int ret, sfdp = 0;
 
    info = spi_flash_read_id(flash);
    if (IS_ERR_OR_NULL(info))

@@ -1196,9 +1432,28 @@ int spi_flash_scan(struct spi_flash *flash)
    }
 #endif
 
+   spi->flash = flash;
+   flash->addrwd_3_in_use = false;
+
+   /* Read Serial Flash Discoverable Parameters and initialize
+    * the following parameters of flash:
+    * 1. Flash size
+    * 2. Page size
+    * 3. Address width to be used for commands

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1452

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+    */
+   if (!(info->flags & SPI_FLASH_SKIP_SFDP)) {
+           flash->size = 0;
+           sfdp = spi_flash_parse_sfdp(flash);
+           if (sfdp < 0)
+                   debug("Unable to get SFDP information\n");
+   }
+
    /* Compute the flash size */
    flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :

0;

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash)

Quoted 7 lines

drivers/mtd/spi/spi_flash.c +4 -1

-   flash->page_size = info->page_size;
+   if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+           flash->page_size = info->page_size;
+           flash->addr_width = SPI_FLASH_3B_ADDR_LEN;
+   }
    /*
     * The Spansion S25FS512S, S25FL032P and S25FL064P have 256b

pages,

Quoted 2 lines

(quoted diff) +0 -0

     * yet use the 0x4d00 Extended JEDEC code. The rest of the

Spansion @@ -1213,7 +1468,10 @@ int spi_flash_scan(struct spi_flash

*flash)

Quoted 145 lines

Quoted 145 lines

Quoted 145 lines

Quoted 25 lines

    }
    flash->page_size <<= flash->shift;
    flash->sector_size = info->sector_size << flash->shift;

drivers/mtd/spi/spi_flash.c +8 -4

-   flash->size = flash->sector_size * info->n_sectors << flash->shift;
+   if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+           flash->size = flash->sector_size * info->n_sectors <<
+                   flash->shift;
+   }
 #ifdef CONFIG_SF_DUAL_FLASH
    if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
            flash->size <<= 1;

@@ -1312,9 +1570,10 @@ int spi_flash_scan(struct spi_flash *flash)
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR
-   if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-        (flash->size > SPI_FLASH_16MB_BOUN)) ||
-        ((flash->dual_flash > SF_SINGLE_FLASH) &&
+   if ((info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) &&
+       (flash->dual_flash == SF_SINGLE_FLASH &&
+        flash->size > SPI_FLASH_16MB_BOUN) ||
+        (flash->dual_flash > SF_SINGLE_FLASH &&
         (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
            puts("SF: Warning - Only lower 16MiB accessible,");
            puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");

Quoted 1 line

diff -

-git

Quoted 25 lines

a/include/spi.h b/include/spi.h index 938627bc01..7189e60581 100644

include/spi.h +2 -0

@@ -10,6 +10,7 @@
 #define _SPI_H_
 
 #include <common.h>
+#include <spi_flash.h>
 
 /* SPI mode flags */
 #define SPI_CPHA   BIT(0)                  /* clock phase */

@@ -103,6 +104,7 @@ struct spi_slave {
    unsigned int bus;
    unsigned int cs;
 #endif
+   struct spi_flash *flash;
    uint mode;
    unsigned int wordlen;
    unsigned int max_read_size;

include/spi_flash.h index renamed from include/spi_flash.h +0 -0

0ec98fb55d..6558a4a1d5 100644

include/spi_flash.h struct spi_slave; · line 47

Quoted 1 line

include/spi_flash.h +0 -0

@@ -47,6 +47,9 @@ struct spi_slave;
  * @read_cmd:              Read cmd - Array Fast, Extn read and quad

read.

include/spi_flash.h struct spi_slave;

Quoted 5 lines

include/spi_flash.h +3 -0

  * @write_cmd:             Write cmd - page and quad program.
  * @dummy_byte:            Dummy cycles for read operation.
+ * @cmd_len:               Total length of command.
+ * @addr_width:            Number of address bytes.
+ * @addrwd_3_in_use:       Flag to send command in 3-byte address

mode.

Quoted 34 lines

include/spi_flash.h +23 -0

  * @memory_map:            Address of read-only SPI flash access
  * @flash_lock:            lock a region of the SPI Flash
  * @flash_unlock:  unlock a region of the SPI Flash @@ -82,6
+85,9 @@ struct spi_flash {
    u8 read_cmd;
    u8 write_cmd;
    u8 dummy_byte;
+   u8 cmd_len;
+   u8 addr_width;
+   bool addrwd_3_in_use;
 
    void *memory_map;

@@ -107,6 +113,120 @@ struct spi_flash {  #endif  };
 
+/*
+ * Serial Flash Discoverable Parameter Headers  */ struct
+sfdp_parameter_header {
+   u8      id_lsb;
+   u8      minor;
+   u8      major;
+   u8      length; /* in double words */
+   u8      parameter_table_pointer[3]; /* byte address */
+   u8      id_msb;
+};
+
+struct sfdp_header {
+   u32     signature; /* Ox50444653U <=> "SFDP" */
+   u8      minor;
+   u8      major;
+   u8      nph; /* 0-base number of parameter headers */
+   u8      unused;
+
+   /* Basic Flash Parameter Table. */

include/spi_flash.h struct spi_flash { #endif }; · line 142

Quoted 10 lines

include/spi_flash.h +10 -0

+   struct sfdp_parameter_header    bfpt_header;
+};
+
+#define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) |
+(p)->id_lsb) #define SFDP_PARAM_HEADER_PTP(p) \
+   (((p)->parameter_table_pointer[2] << 16) | \
+    ((p)->parameter_table_pointer[1] <<  8) | \
+    ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID               0xff00  /* Basic Flash Parameter Table

*/

Quoted 13 lines

include/spi_flash.h +13 -0

+#define SFDP_SECTOR_MAP_ID 0xff81  /* Sector Map Table */
+
+#define SFDP_SIGNATURE             0x50444653U
+#define SFDP_JESD216_MAJOR 1
+#define SFDP_JESD216_MINOR 0
+#define SFDP_JESD216A_MINOR        5
+#define SFDP_JESD216B_MINOR        6
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ */
+#define BFPT_DWORD(i)              ((i) - 1)
+#define BFPT_DWORD_MAX             16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216                     9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2                BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK

     GENMASK(18, 17)

Quoted 5 lines

include/spi_flash.h +5 -0

+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY   (0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4   (0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY   (0x2UL << 17)
+#define BFPT_DWORD1_DTR                            BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2                BIT(20)

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+#define BFPT_DWORD1_FAST_READ_1_4_4                BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4                BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2                BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4                BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT               4
+#define BFPT_DWORD11_PAGE_SIZE_MASK                GENMASK(7,

4)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

include/spi_flash.h struct spi_flash {

Quoted 7 lines

include/spi_flash.h +7 -0

+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-

4

Quoted 1 line

Quoted 1 line

Quoted 1 line

include/spi_flash.h struct spi_flash { #endif }; · line 188

Quoted 1 line

include/spi_flash.h +1 -0

+ *         reads based on instruction. DQ3/HOLD# functions are hold

during

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

include/spi_flash.h struct spi_flash {

Quoted 5 lines

include/spi_flash.h +5 -0

+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect

of

Quoted 32 lines

Quoted 32 lines

Quoted 32 lines

Quoted 12 lines

include/spi_flash.h +12 -0

+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK

     GENMASK(22, 20)

include/spi_flash.h struct spi_flash { #endif }; · line 216

Quoted 1 line

include/spi_flash.h +1 -0

+#define BFPT_DWORD15_QER_NONE                      (0x0UL << 20)

/* Micron */

include/spi_flash.h struct spi_flash { #endif }; · line 218

Quoted 2 lines

include/spi_flash.h +2 -0

+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY            (0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6          (0x2UL << 20) /*

Macronix */

include/spi_flash.h struct spi_flash {

Quoted 3 lines

include/spi_flash.h +3 -0

+#define BFPT_DWORD15_QER_SR2_BIT7          (0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD            (0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1          (0x5UL << 20) /*

Spansion */

Quoted 17 lines

Quoted 9 lines

Quoted 9 lines

include/spi_flash.h struct spi_flash {

Quoted 9 lines

include/spi_flash.h +5 -0

+
+struct sfdp_bfpt {
+   u32     dwords[BFPT_DWORD_MAX];
+};
+
 struct dm_spi_flash_ops {
    int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
    int (*write)(struct udevice *dev, u32 offset, size_t len,
 

--
Regards
Vignesh
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de

https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis

Quoted 1 line

ts.denx.de%2Flistinfo%2Fu-

boot&amp;data=02%7C01%7Crajat.srivastava%40n

Quoted 1 line

xp.com%7Cacee153f50194bba71c808d64bad8d2e%7C686ea1d3bc2b4c6fa92c d99c5c

Quoted 1 line

301635%7C0%7C0%7C636779606107591743&amp;sdata=u9H5PMJiRNVKyyFI7 y5eCmDx

Quoted 1 line

0KPmAHXvF8kusaJVUdM%3D&amp;reserved=0

Quoted 133 lines

-----Original Message-----
From: Simon Goldschmidt <[email protected]>
Sent: Friday, November 16, 2018 3:53 PM
To: vigneshr at ti.com
Cc: Rajat Srivastava <[email protected]>; U-Boot Mailing List <u- boot at lists.denx.de>; Jagan Teki <[email protected]>
Subject: Re: [U-Boot] [PATCH v2 1/2] mtd/spi: Add JEDEC SFDP support in SPI framework

Quoted 24 lines · Vignesh R

Quoted 20 lines · Rajat Srivastava

Quoted 20 lines

Hi Rajat,

Quoted 17 lines · Rajat Srivastava

Add support for JESD216 rev B standard JEDEC Serial Flash Discoverable Parameters (SFDP) tables to dynamically initialize flash size, page size and address width of the flash. More parameters can be added as per requirement.
SFDP parsing is made default but already existing method for parsing these parameters are not deprecated.
A flag is created to skip SFDP parsing for a particular flash, if required.

SFDP data lets us auto-detect the addressing mode supported by the flash which helps us access the flash using 4-byte address.

Add a new argument in spi_flash_addr() function to create commands with 3-byte or 4-byte address depending on the SFDP data read. Add pointer to struct spi_flash in struct spi_slave so that driver can have access to SFDP data.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Introduce new structures and functions to read and parse SFDP data. This is loosely based on Linux SFDP framework.

Signed-off-by Rajat Srivastava <[email protected]>

---
Changes in v2:
- Make SFDP parsing the default method.
- Change SPI_FLASH_USE_SFDP flag to SPI_FLASH_USE_SFDP to

provide

Quoted 62 lines

Quoted 62 lines

Quoted 61 lines

Quoted 2 lines

an option to skip SFDP parsing for a particular flash.
---

[...]

Review

Quoted 2 lines

(quoted diff) +2 -0

+static int spi_flash_parse_bfpt(struct spi_flash *flash,
+                           const struct sfdp_parameter_header

*bfpt_header) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 6 lines

drivers/mtd/spi/spi_flash.c +6 -0

+   struct sfdp_bfpt bfpt;
+   size_t len;
+   int i, err;
+   u32 addr;
+
+   /* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs.

*/

Quoted 8 lines

drivers/mtd/spi/spi_flash.c +8 -0

+   if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
+           return -EINVAL;
+
+   /* Read the Basic Flash Parameter Table. */
+   len = min_t(size_t, sizeof(bfpt),
+               bfpt_header->length * sizeof(u32));
+   addr = SFDP_PARAM_HEADER_PTP(bfpt_header);
+   memset(&bfpt, 0, sizeof(bfpt));

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   err = spi_flash_read_sfdp(flash, addr, len, &bfpt);
+   if (err < 0)
+           return err;
+
+   /* Fix endianness of the BFPT DWORDs. */
+   for (i = 0; i < BFPT_DWORD_MAX; i++)
+           bfpt.dwords[i] = le32_to_cpu(bfpt.dwords[i]);
+
+   /* Number of address bytes. */
+   switch (bfpt.dwords[BFPT_DWORD(1)] &

BFPT_DWORD1_ADDRESS_BYTES_MASK) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 11 lines

drivers/mtd/spi/spi_flash.c +11 -0

+   case BFPT_DWORD1_ADDRESS_BYTES_3_ONLY:
+           flash->addr_width = 3;
+           break;
+
+   case BFPT_DWORD1_ADDRESS_BYTES_3_OR_4:
+           printf("SF: Flash defaults to 3-Byte mode; enters 4-Byte ");
+           printf("mode on command\n");
+           /*
+            * By default, 4-byte addressing mode is set.
+            * To enforce 3-byte addressing mode, set
+ addrwd_3_in_use

flag

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash) · line 1192

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+            * in struct spi_flash for every command.
+            */
+           flash->addr_width = 4;
+           break;
+
+   case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+           flash->addr_width = 4;
+           break;
+

I see your are updating flash->addr_width to 4 bytes here. But I don't see any code in this patch that changes flash->read_cmd (and write/erase
cmds) to use 4B addressing opcodes. Also, I dont see any code that bypasses BAR register configuration when CONFIG_SPI_FLASH_BAR is defined and SFDP is available.

Quoted 1 line

Quoted 1 line

We don't have any flash that supports CONFIG_SPI_FLASH_BAR. What do

you suggest, shall we skip SFDP parsing in case CONFIG_SPI_FLASH_BAR is defined?

Quoted 22 lines

Quoted 1 line

I suggest skipping BAR configuration completely if flash supports address width of 4 as per SFDP. BAR configuration is only needed if flash does not support 4 byte addressing and flash size is > 16MB

Quoted 16 lines

Quoted 9 lines

This patch will most certainly break SPI controller drivers(like cadence_qspi.c) that expect sf layer to provide correct read opcode and address width information, since there will be mismatch wrt opcode used and number of address byte sent.
Not sure how this patch was tested, but I guess fsl_qspi modifies read opcode internal to the driver and gets away with it. Please modify the patch to update read_cmd/erase_cmd/write_cmd to use
4 byte addressing opcodes

If the flash supports only supports 4-byte opcodes then there will be no mismatch wrt opcode used and number of address bytes sent since all of the opcodes that the flash supports will be 4-bytes opcodes. But if the flash supports both 3-byte as well as 4-byte address widths (which is a separate case) then this code will default the address mode to 4-byte but gives an option to use 3-byte by setting

Quoted 12 lines

addrwd_3_in_use flag.

Quoted 6 lines

Quoted 1 line

Lets assume flash supports both 3 and 4 byte addressing (AFAIK, majority of flash parts do that). In this case, this patch is defaulting to 4 byte address width but flash->read_cmd/write_cmd/erase_cmd are still set to 3 byte addressing

opcodes.

Quoted 4 lines

So, SPI controller would end up send 3 addressing byte opcode but 4 byte of address. The flash would interpret last byte of address as first byte of data and thus lead to data corruption in case of write. Isn't that a problem?

Ok. But I'm curious if a flash supports both 3 and 4 byte address widths (and it does not have BAR support), then why would one use 3-byte read/write/erase commands? How would one access >16 MB in such case?

Also, initially I wanted to make SFDP optional so that none of the drivers are affected and one could enable it using a flag as per requirement. Shall we get back to making this optional (similar to v1 of this patch)?

Regards
Rajat

Quoted 432 lines

Additionally, there are flash chips with a 3-/4-byte mode change instruction. But using 4-byte opcodes should be preferred.

Simon

Quoted 5 lines

In particular you are not parsing entire bfpt like in [1] and will break flash operations

[1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F

Quoted 1 line

elixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Fmtd%2Fspi-

nor

Quoted 1 line

%2Fspi-

nor.c%23L2259&amp;data=02%7C01%7Crajat.srivastava%40nxp.com%7Ca

Quoted 1 line

cee153f50194bba71c808d64bad8d2e%7C686ea1d3bc2b4c6fa92cd99c5c301635 %7C0

Quoted 1 line

%7C0%7C636779606107591743&amp;sdata=UACl919%2BMQmdtBo5GUTAy M428u1bT1bU

Quoted 189 lines

rXuX2GsSjb0%3D&amp;reserved=0

Regards
Vignesh

Quoted 184 lines

Regards
Rajat

Quoted 181 lines

Regards
Vigensh

Quoted 28 lines

drivers/mtd/spi/spi_flash.c +28 -0

+   default:
+           break;
+   }
+
+   /* Flash Memory Density (in bits). */
+   flash->size = bfpt.dwords[BFPT_DWORD(2)];
+   if (flash->size & BIT(31)) {
+           flash->size &= ~BIT(31);
+
+           /*
+            * Prevent overflows on flash->size. Anyway, a NOR of 2^64
+            * bits is unlikely to exist so this error probably means
+            * the BFPT we are reading is corrupted/wrong.
+            */
+           if (flash->size > 63)
+                   return -EINVAL;
+
+           flash->size = 1ULL << flash->size;
+   } else {
+           flash->size++;
+   }
+   flash->size >>= 3; /* Convert to bytes. */
+
+   /* Stop here if not JESD216 rev A or later. */
+   if (bfpt_header->length < BFPT_DWORD_MAX)
+           return 0;
+
+   /* Page size: this field specifies 'N' so the page size = 2^N bytes. */

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+   flash->page_size = bfpt.dwords[BFPT_DWORD(11)];
+   flash->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+   flash->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+   flash->page_size = 1U << flash->page_size;
+
+   return 0;
+}
+
+/*
+ * spi_flash_parse_sfdp() - parse the Serial Flash Discoverable

Parameters.

Review

Quoted 9 lines

+ * @flash: pointer to a 'struct spi_flash'
+ *
+ * The Serial Flash Discoverable Parameters are described by the

(quoted diff) +6 -0

+JEDEC JESD216
+ * specification. This is a standard which tends to supported by
+almost all
+ * (Q)SPI memory manufacturers. Those hard-coded tables allow us
+to learn at
+ * runtime the main parameters needed to perform basic SPI flash

operations.

Review

Quoted 5 lines

+ *
+ * Return: 0 on success, -errno otherwise.
+ */

(quoted diff) +2 -0

+static int spi_flash_parse_sfdp(struct spi_flash *flash) {
+   const struct sfdp_parameter_header *param_header,

*bfpt_header;

Quoted 17 lines

drivers/mtd/spi/spi_flash.c +17 -0

+   struct sfdp_parameter_header *param_headers = NULL;
+   struct sfdp_header header;
+   size_t psize;
+   int i, err;
+
+   /* Get the SFDP header. */
+   err = spi_flash_read_sfdp(flash, 0, sizeof(header), &header);
+   if (err < 0)
+           return err;
+
+   /* Check the SFDP header version. */
+   if (le32_to_cpu(header.signature) != SFDP_SIGNATURE ||
+       header.major != SFDP_JESD216_MAJOR)
+           return -EINVAL;
+
+   /*
+    * Verify that the first and only mandatory parameter header is a

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+    * Basic Flash Parameter Table header as specified in JESD216.
+    */
+   bfpt_header = &header.bfpt_header;
+   if (SFDP_PARAM_HEADER_ID(bfpt_header) != SFDP_BFPT_ID ||
+       bfpt_header->major != SFDP_JESD216_MAJOR)
+           return -EINVAL;
+
+   /*
+    * Allocate memory then read all parameter headers with a single
+    * Read SFDP command. These parameter headers will actually be

parsed

Review

Quoted 2 lines

(quoted diff) +2 -0

+    * twice: a first time to get the latest revision of the basic flash
+    * parameter table, then a second time to handle the supported

optional

Quoted 5 lines

drivers/mtd/spi/spi_flash.c +5 -0

+    * tables.
+    * Hence we read the parameter headers once for all to reduce the
+    * processing time
+    */
+   if (header.nph) {

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +10 -0

+           psize = header.nph * sizeof(*param_headers);
+
+           param_headers = malloc(psize);
+           if (!param_headers)
+                   return -ENOMEM;
+
+           err = spi_flash_read_sfdp(flash, sizeof(header),
+                                     psize, param_headers);
+           if (err < 0) {
+                   dev_err(dev, "failed to read SFDP parameter

headers\n");

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 12 lines

drivers/mtd/spi/spi_flash.c +12 -0

+                   goto exit;
+           }
+   }
+
+   /*
+    * Check other parameter headers to get the latest revision of
+    * the basic flash parameter table.
+    */
+   for (i = 0; i < header.nph; i++) {
+           param_header = &param_headers[i];
+
+           if (SFDP_PARAM_HEADER_ID(param_header) ==

SFDP_BFPT_ID &&

Quoted 9 lines

drivers/mtd/spi/spi_flash.c +9 -0

+               param_header->major == SFDP_JESD216_MAJOR &&
+               (param_header->minor > bfpt_header->minor ||
+                (param_header->minor == bfpt_header->minor &&
+                 param_header->length > bfpt_header->length)))
+                   bfpt_header = param_header;
+   }
+
+   err = spi_flash_parse_bfpt(flash, bfpt_header);
+   if (err)

drivers/mtd/spi/spi_flash.c static const struct spi_flash_info *spi_flash_read_id(struct spi_flash *flash)

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +7 -1

+           goto exit;
+
+exit:
+   free(param_headers);
+   return err;
+}
+
 static int set_quad_mode(struct spi_flash *flash,
                     const struct spi_flash_info *info)  { @@
-1130,7

+1366,7 @@ int

Quoted 20 lines

spi_flash_scan(struct spi_flash *flash)  {
    struct spi_slave *spi = flash->spi;
    const struct spi_flash_info *info = NULL;

drivers/mtd/spi/spi_flash.c +9 -1

-   int ret;
+   int ret, sfdp = 0;
 
    info = spi_flash_read_id(flash);
    if (IS_ERR_OR_NULL(info))

@@ -1196,9 +1432,28 @@ int spi_flash_scan(struct spi_flash *flash)
    }
 #endif
 
+   spi->flash = flash;
+   flash->addrwd_3_in_use = false;
+
+   /* Read Serial Flash Discoverable Parameters and initialize
+    * the following parameters of flash:
+    * 1. Flash size
+    * 2. Page size
+    * 3. Address width to be used for commands

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash) · line 1452

Quoted 10 lines

drivers/mtd/spi/spi_flash.c +8 -0

+    */
+   if (!(info->flags & SPI_FLASH_SKIP_SFDP)) {
+           flash->size = 0;
+           sfdp = spi_flash_parse_sfdp(flash);
+           if (sfdp < 0)
+                   debug("Unable to get SFDP information\n");
+   }
+
    /* Compute the flash size */
    flash->shift = (flash->dual_flash & SF_DUAL_PARALLEL_FLASH) ? 1 :

0;

drivers/mtd/spi/spi_flash.c int spi_flash_scan(struct spi_flash *flash)

Quoted 7 lines

drivers/mtd/spi/spi_flash.c +4 -1

-   flash->page_size = info->page_size;
+   if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+           flash->page_size = info->page_size;
+           flash->addr_width = SPI_FLASH_3B_ADDR_LEN;
+   }
    /*
     * The Spansion S25FS512S, S25FL032P and S25FL064P have 256b

pages,

Quoted 2 lines

(quoted diff) +0 -0

     * yet use the 0x4d00 Extended JEDEC code. The rest of the

Spansion @@ -1213,7 +1468,10 @@ int spi_flash_scan(struct spi_flash

*flash)

Quoted 145 lines

Quoted 145 lines

Quoted 145 lines

Quoted 25 lines

    }
    flash->page_size <<= flash->shift;
    flash->sector_size = info->sector_size << flash->shift;

drivers/mtd/spi/spi_flash.c +8 -4

-   flash->size = flash->sector_size * info->n_sectors << flash->shift;
+   if (info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) {
+           flash->size = flash->sector_size * info->n_sectors <<
+                   flash->shift;
+   }
 #ifdef CONFIG_SF_DUAL_FLASH
    if (flash->dual_flash & SF_DUAL_STACKED_FLASH)
            flash->size <<= 1;

@@ -1312,9 +1570,10 @@ int spi_flash_scan(struct spi_flash *flash)
 #endif
 
 #ifndef CONFIG_SPI_FLASH_BAR
-   if (((flash->dual_flash == SF_SINGLE_FLASH) &&
-        (flash->size > SPI_FLASH_16MB_BOUN)) ||
-        ((flash->dual_flash > SF_SINGLE_FLASH) &&
+   if ((info->flags & SPI_FLASH_SKIP_SFDP || sfdp < 0) &&
+       (flash->dual_flash == SF_SINGLE_FLASH &&
+        flash->size > SPI_FLASH_16MB_BOUN) ||
+        (flash->dual_flash > SF_SINGLE_FLASH &&
         (flash->size > SPI_FLASH_16MB_BOUN << 1))) {
            puts("SF: Warning - Only lower 16MiB accessible,");
            puts(" Full access #define CONFIG_SPI_FLASH_BAR\n");

Quoted 1 line

diff -

-git

Quoted 25 lines

a/include/spi.h b/include/spi.h index 938627bc01..7189e60581 100644

include/spi.h +2 -0

@@ -10,6 +10,7 @@
 #define _SPI_H_
 
 #include <common.h>
+#include <spi_flash.h>
 
 /* SPI mode flags */
 #define SPI_CPHA   BIT(0)                  /* clock phase */

@@ -103,6 +104,7 @@ struct spi_slave {
    unsigned int bus;
    unsigned int cs;
 #endif
+   struct spi_flash *flash;
    uint mode;
    unsigned int wordlen;
    unsigned int max_read_size;

include/spi_flash.h index renamed from include/spi_flash.h +0 -0

0ec98fb55d..6558a4a1d5 100644

include/spi_flash.h struct spi_slave; · line 47

Quoted 1 line

include/spi_flash.h +0 -0

@@ -47,6 +47,9 @@ struct spi_slave;
  * @read_cmd:              Read cmd - Array Fast, Extn read and quad

read.

include/spi_flash.h struct spi_slave;

Quoted 5 lines

include/spi_flash.h +3 -0

  * @write_cmd:             Write cmd - page and quad program.
  * @dummy_byte:            Dummy cycles for read operation.
+ * @cmd_len:               Total length of command.
+ * @addr_width:            Number of address bytes.
+ * @addrwd_3_in_use:       Flag to send command in 3-byte address

mode.

Quoted 34 lines

include/spi_flash.h +23 -0

  * @memory_map:            Address of read-only SPI flash access
  * @flash_lock:            lock a region of the SPI Flash
  * @flash_unlock:  unlock a region of the SPI Flash @@ -82,6
+85,9 @@ struct spi_flash {
    u8 read_cmd;
    u8 write_cmd;
    u8 dummy_byte;
+   u8 cmd_len;
+   u8 addr_width;
+   bool addrwd_3_in_use;
 
    void *memory_map;

@@ -107,6 +113,120 @@ struct spi_flash {  #endif  };
 
+/*
+ * Serial Flash Discoverable Parameter Headers  */ struct
+sfdp_parameter_header {
+   u8      id_lsb;
+   u8      minor;
+   u8      major;
+   u8      length; /* in double words */
+   u8      parameter_table_pointer[3]; /* byte address */
+   u8      id_msb;
+};
+
+struct sfdp_header {
+   u32     signature; /* Ox50444653U <=> "SFDP" */
+   u8      minor;
+   u8      major;
+   u8      nph; /* 0-base number of parameter headers */
+   u8      unused;
+
+   /* Basic Flash Parameter Table. */

include/spi_flash.h struct spi_flash { #endif }; · line 142

Quoted 10 lines

include/spi_flash.h +10 -0

+   struct sfdp_parameter_header    bfpt_header;
+};
+
+#define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) |
+(p)->id_lsb) #define SFDP_PARAM_HEADER_PTP(p) \
+   (((p)->parameter_table_pointer[2] << 16) | \
+    ((p)->parameter_table_pointer[1] <<  8) | \
+    ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID               0xff00  /* Basic Flash Parameter Table

*/

Quoted 13 lines

include/spi_flash.h +13 -0

+#define SFDP_SECTOR_MAP_ID 0xff81  /* Sector Map Table */
+
+#define SFDP_SIGNATURE             0x50444653U
+#define SFDP_JESD216_MAJOR 1
+#define SFDP_JESD216_MINOR 0
+#define SFDP_JESD216A_MINOR        5
+#define SFDP_JESD216B_MINOR        6
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ */
+#define BFPT_DWORD(i)              ((i) - 1)
+#define BFPT_DWORD_MAX             16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216                     9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2                BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK

     GENMASK(18, 17)

Quoted 5 lines

include/spi_flash.h +5 -0

+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY   (0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4   (0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY   (0x2UL << 17)
+#define BFPT_DWORD1_DTR                            BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2                BIT(20)

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+#define BFPT_DWORD1_FAST_READ_1_4_4                BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4                BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2                BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4                BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT               4
+#define BFPT_DWORD11_PAGE_SIZE_MASK                GENMASK(7,

4)

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

include/spi_flash.h struct spi_flash {

Quoted 7 lines

include/spi_flash.h +7 -0

+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-

4

Quoted 1 line

Quoted 1 line

Quoted 1 line

include/spi_flash.h struct spi_flash { #endif }; · line 188

Quoted 1 line

include/spi_flash.h +1 -0

+ *         reads based on instruction. DQ3/HOLD# functions are hold

during

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

include/spi_flash.h struct spi_flash {

Quoted 5 lines

include/spi_flash.h +5 -0

+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect

of

Quoted 32 lines

Quoted 32 lines

Quoted 32 lines

Quoted 12 lines

include/spi_flash.h +12 -0

+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.

include/spi_flash.h struct spi_flash {

Quoted 10 lines

include/spi_flash.h +10 -0

+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK

     GENMASK(22, 20)

include/spi_flash.h struct spi_flash { #endif }; · line 216

Quoted 1 line

include/spi_flash.h +1 -0

+#define BFPT_DWORD15_QER_NONE                      (0x0UL << 20)

/* Micron */

include/spi_flash.h struct spi_flash { #endif }; · line 218

Quoted 2 lines

include/spi_flash.h +2 -0

+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY            (0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6          (0x2UL << 20) /*

Macronix */

include/spi_flash.h struct spi_flash {

Quoted 3 lines

include/spi_flash.h +3 -0

+#define BFPT_DWORD15_QER_SR2_BIT7          (0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD            (0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1          (0x5UL << 20) /*

Spansion */

Quoted 17 lines

Quoted 9 lines

Quoted 9 lines

include/spi_flash.h struct spi_flash {

Quoted 9 lines

include/spi_flash.h +5 -0

+
+struct sfdp_bfpt {
+   u32     dwords[BFPT_DWORD_MAX];
+};
+
 struct dm_spi_flash_ops {
    int (*read)(struct udevice *dev, u32 offset, size_t len, void *buf);
    int (*write)(struct udevice *dev, u32 offset, size_t len,
 

--
Regards
Vignesh
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de

https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis

Quoted 1 line

ts.denx.de%2Flistinfo%2Fu-

boot&amp;data=02%7C01%7Crajat.srivastava%40n

Quoted 1 line

xp.com%7Cacee153f50194bba71c808d64bad8d2e%7C686ea1d3bc2b4c6fa92c d99c5c

Quoted 1 line

301635%7C0%7C0%7C636779606107591743&amp;sdata=u9H5PMJiRNVKyyFI7 y5eCmDx

Quoted 1 line

0KPmAHXvF8kusaJVUdM%3D&amp;reserved=0