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.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? >
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 = ¶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, >>> -- Regards Vignesh _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot