[U-Boot] [PATCH v2 1/2] mtd/spi: Add JEDEC SFDP support in SPI framework

Vignesh R vigneshr at ti.com
Fri Nov 16 10:20:33 UTC 2018



On 16/11/18 3:10 PM, Rajat Srivastava wrote:
>> Hi Rajat,
>>
>> On 13/11/18 5:30 PM, Rajat Srivastava wrote:
>>> 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 <rajat.srivastava at nxp.com>
>>> ---
>>> 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.
>>> ---
>>
>> [...]
>>> +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.
>>> +		 * To enforce 3-byte addressing mode, set addrwd_3_in_use
>> flag
>>> +		 * 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?
> 

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

>> 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.
> 

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

> Regards
> Rajat
> 
>>
>> Regards
>> Vigensh
>>
>>
>>> +	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"); diff -
>> -git
>>> a/include/spi.h b/include/spi.h index 938627bc01..7189e60581 100644
>>> --- a/include/spi.h
>>> +++ b/include/spi.h
>>> @@ -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;
>>> diff --git a/include/spi_flash.h b/include/spi_flash.h index
>>> 0ec98fb55d..6558a4a1d5 100644
>>> --- a/include/spi_flash.h
>>> +++ b/include/spi_flash.h
>>> @@ -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,
>>>

-- 
Regards
Vignesh


More information about the U-Boot mailing list