> -----Original Message----- > From: Vignesh R <vigne...@ti.com> > Sent: Thursday, November 15, 2018 2:52 PM > To: Rajat Srivastava <rajat.srivast...@nxp.com>; u-boot@lists.denx.de > Cc: ja...@openedev.com > Subject: Re: [U-Boot] [PATCH v2 1/2] mtd/spi: Add JEDEC SFDP support in SPI > framework > > 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.srivast...@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? > 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 > > 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 = ¶m_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, > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot