2017-06-26 13:10:06

by Cyrille Pitchen

[permalink] [raw]
Subject: [PATCH v3 0/2] mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

Hi all,

a new version of the SFDP patch based on next-20170626

tested on sama5d2 xplained with the following QSPI memories:
Macronix
- mx25l25673g

Spansion/Cypress
- s25fl164
- s25fl127
- s25fl512

Winbond
- w25q256
- w25m512

SST
- sst26vf064

Micron
- m25q128
- n25q128a
- m25ql512
- m25ql01g

For my tests, I used mtd_debug to erase, write then read back some areas
inside data array of the SPI NOR flash memory.
To verify the integrity of the data, I used sha1sum to compare the
original file with the one read from the SPI flash memory.

For memories with a non-uniform erase map (sst26vf064, s25fl512, ...), I
chose an offset in the data array so the sector size is the one set into
nor->mtd.erasesize.

CONFIG_MTD_SPI_NOR_USE_4K_SECTORS was defined in my .config file.
Older tests were also run with this macro undefined and with other memory
parts.
Here I'm only talking about tests performed within the last few days.

For sst26vf064, the Global Unlock Block Protection command was already
sent to the memory by the bootloader so once in Linux, the memory was in a
rw mode, otherwise Sector Erase and Page Program would fail.

Flash unlock (block protection) and non-uniform erase map are out of the
scope of this patch and would be addressed later in dedicated patches.

I added/updated few entries in the spi_nor_ids[] array so I could use all
the memory parts listed above with SFDP support.

My s25fl127 sample is buggy and I don't know whether the issue has been
fixed by Cypress with the later revisions of this part number (s25fl128s):
The SFDP data programmed in my sample claim that the memory is compliant
with JESD216 rev B (version 1.6) however DWORDs 10 to 16 of the Basic
Flash Parameter Table are all 0xFFFFFFFF, only the first 9 DWORDs are
programmed correctly as is the memory was only compliant with JESD216
(version 1.0). Hence when testing the QER bits in DWORD15, the reserved
value 111b is read. That why I've changed the default case for QER bits
so it now returns -EINVAL when an unexpected value is read.
So the SFDP data are reported as invalid and just ignored.
I didn't use the SPI_NOR_SKIP_SFDP info->flags because s25fl127 and
s25fl128s share the same JEDEC ID and I want to give a chance to use the
SFDP tables of the later revisions of this memory part if the above
issue has been fixed.

Best regards,

Cyrille


ChangeLog

v2 > v3:
- add a small patch to fix a conflict when the SPINOR_OP_RDSR2 macro was
defined twice: first in drivers/mtd/devices/serial_flash_cmds.h and
secondly in include/linux/mtd/spi-nor.h. It resulted in a build warning.
- add the missing () in the kernel-doc comments for functions
- add a Return: section in the kernel-doc comments for functions
- improve spi_nor_read_sfdp() to take into account the case where all SFDP
data can't be read in a single nor->read() call.

v1 -> v2:
- add kernel-doc to the main functions introduced by this patch.
- rename spansion_new_quad_enable() into spansion_read_cr_quad_enable().
- add spansion_no_read_cr_quad_enable(): the explanation is given in
the kernel-doc.
- take Marek's comments into account (add new lines, remove parenthesis,
keep new error messages on the same line as dev_err(), already exisiting
error messages are left unchanged, use u32 instead of int for >>
operand, use sizeof(u32) instead of sizeof(uint32_t)).
- propagate return code of spi_nor_read_sfdp().
- handle the default case of the QER bits differently: now returns -EINVAL


Cyrille Pitchen (2):
mtd: st_spi_fsm: remove SPINOR_OP_RDSR2 and use SPINOR_OP_RDCR instead
mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

drivers/mtd/devices/serial_flash_cmds.h | 1 -
drivers/mtd/devices/st_spi_fsm.c | 4 +-
drivers/mtd/spi-nor/spi-nor.c | 775 +++++++++++++++++++++++++++++++-
include/linux/mtd/spi-nor.h | 6 +
4 files changed, 770 insertions(+), 16 deletions(-)

--
2.7.4


2017-06-26 13:10:21

by Cyrille Pitchen

[permalink] [raw]
Subject: [PATCH v3 2/2] mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

This patch adds support to the JESD216 rev B standard and parses the SFDP
tables to dynamically initialize the 'struct spi_nor_flash_parameter'.

Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/mtd/spi-nor/spi-nor.c | 775 +++++++++++++++++++++++++++++++++++++++++-
include/linux/mtd/spi-nor.h | 6 +
2 files changed, 768 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 040ab5d7ac05..dac789ff70ea 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -17,6 +17,7 @@
#include <linux/mutex.h>
#include <linux/math64.h>
#include <linux/sizes.h>
+#include <linux/slab.h>

#include <linux/mtd/mtd.h>
#include <linux/of_platform.h>
@@ -86,6 +87,7 @@ struct flash_info {
* to support memory size above 128Mib.
*/
#define NO_CHIP_ERASE BIT(12) /* Chip does not support chip erase */
+#define SPI_NOR_SKIP_SFDP BIT(13) /* Skip parsing of SFDP tables */
};

#define JEDEC_MFR(info) ((info)->id[0])
@@ -1378,6 +1380,16 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
return ret;
}

+/**
+ * macronix_quad_enable() - set QE bit in Status Register.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * Set the Quad Enable (QE) bit in the Status Register.
+ *
+ * bit 6 of the Status Register is the QE bit for Macronix like QSPI memories.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
static int macronix_quad_enable(struct spi_nor *nor)
{
int ret, val;
@@ -1411,22 +1423,13 @@ static int macronix_quad_enable(struct spi_nor *nor)
* second byte will be written to the configuration register.
* Return negative if error occurred.
*/
-static int write_sr_cr(struct spi_nor *nor, u16 val)
-{
- nor->cmd_buf[0] = val & 0xff;
- nor->cmd_buf[1] = (val >> 8);
-
- return nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 2);
-}
-
-static int spansion_quad_enable(struct spi_nor *nor)
+static int write_sr_cr(struct spi_nor *nor, u8 *sr_cr)
{
int ret;
- int quad_en = CR_QUAD_EN_SPAN << 8;

write_enable(nor);

- ret = write_sr_cr(nor, quad_en);
+ ret = nor->write_reg(nor, SPINOR_OP_WRSR, sr_cr, 2);
if (ret < 0) {
dev_err(nor->dev,
"error while writing configuration register\n");
@@ -1440,6 +1443,41 @@ static int spansion_quad_enable(struct spi_nor *nor)
return ret;
}

+ return 0;
+}
+
+/**
+ * spansion_quad_enable() - set QE bit in Configuraiton Register.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * Set the Quad Enable (QE) bit in the Configuration Register.
+ * This function is kept for legacy purpose because it has been used for a
+ * long time without anybody complaining but it should be considered as
+ * deprecated and maybe buggy.
+ * First, this function doesn't care about the previous values of the Status
+ * and Configuration Registers when it sets the QE bit (bit 1) in the
+ * Configuration Register: all other bits are cleared, which may have unwanted
+ * side effects like removing some block protections.
+ * Secondly, it uses the Read Configuration Register (35h) instruction though
+ * some very old and few memories don't support this instruction. If a pull-up
+ * resistor is present on the MISO/IO1 line, we might still be able to pass the
+ * "read back" test because the QSPI memory doesn't recognize the command,
+ * so leaves the MISO/IO1 line state unchanged, hence read_cr() returns 0xFF.
+ *
+ * bit 1 of the Configuration Register is the QE bit for Spansion like QSPI
+ * memories.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_quad_enable(struct spi_nor *nor)
+{
+ u8 sr_cr[2] = {0, CR_QUAD_EN_SPAN};
+ int ret;
+
+ ret = write_sr_cr(nor, sr_cr);
+ if (ret)
+ return ret;
+
/* read back and check it */
ret = read_cr(nor);
if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
@@ -1450,6 +1488,140 @@ static int spansion_quad_enable(struct spi_nor *nor)
return 0;
}

+/**
+ * spansion_no_read_cr_quad_enable() - set QE bit in Configuration Register.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * Set the Quad Enable (QE) bit in the Configuration Register.
+ * This function should be used with QSPI memories not supporting the Read
+ * Configuration Register (35h) instruction.
+ *
+ * bit 1 of the Configuration Register is the QE bit for Spansion like QSPI
+ * memories.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_no_read_cr_quad_enable(struct spi_nor *nor)
+{
+ u8 sr_cr[2];
+ int ret;
+
+ /* Keep the current value of the Status Register. */
+ ret = read_sr(nor);
+ if (ret < 0) {
+ dev_err(nor->dev, "error while reading status register\n");
+ return -EINVAL;
+ }
+ sr_cr[0] = ret;
+ sr_cr[1] = CR_QUAD_EN_SPAN;
+
+ return write_sr_cr(nor, sr_cr);
+}
+
+/**
+ * spansion_read_cr_quad_enable() - set QE bit in Configuration Register.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * Set the Quad Enable (QE) bit in the Configuration Register.
+ * This function should be used with QSPI memories supporting the Read
+ * Configuration Register (35h) instruction.
+ *
+ * bit 1 of the Configuration Register is the QE bit for Spansion like QSPI
+ * memories.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spansion_read_cr_quad_enable(struct spi_nor *nor)
+{
+ struct device *dev = nor->dev;
+ u8 sr_cr[2];
+ int ret;
+
+ /* Check current Quad Enable bit value. */
+ ret = read_cr(nor);
+ if (ret < 0) {
+ dev_err(dev, "error while reading configuration register\n");
+ return -EINVAL;
+ }
+
+ if (ret & CR_QUAD_EN_SPAN)
+ return 0;
+
+ sr_cr[1] = ret | CR_QUAD_EN_SPAN;
+
+ /* Keep the current value of the Status Register. */
+ ret = read_sr(nor);
+ if (ret < 0) {
+ dev_err(dev, "error while reading status register\n");
+ return -EINVAL;
+ }
+ sr_cr[0] = ret;
+
+ ret = write_sr_cr(nor, sr_cr);
+ if (ret)
+ return ret;
+
+ /* Read back and check it. */
+ ret = read_cr(nor);
+ if (!(ret > 0 && (ret & CR_QUAD_EN_SPAN))) {
+ dev_err(nor->dev, "Spansion Quad bit not set\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * sr2_bit7_quad_enable() - set QE bit in Status Register 2.
+ * @nor: pointer to a 'struct spi_nor'
+ *
+ * Set the Quad Enable (QE) bit in the Status Register 2.
+ *
+ * This is one of the procedures to set the QE bit described in the SFDP
+ * (JESD216 rev B) specification but no manufacturer using this procedure has
+ * been identified yet, hence the name of the function.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int sr2_bit7_quad_enable(struct spi_nor *nor)
+{
+ u8 sr2;
+ int ret;
+
+ /* Check current Quad Enable bit value. */
+ ret = nor->read_reg(nor, SPINOR_OP_RDSR2, &sr2, 1);
+ if (ret)
+ return ret;
+ if (sr2 & SR2_QUAD_EN_BIT7)
+ return 0;
+
+ /* Update the Quad Enable bit. */
+ sr2 |= SR2_QUAD_EN_BIT7;
+
+ write_enable(nor);
+
+ ret = nor->write_reg(nor, SPINOR_OP_WRSR2, &sr2, 1);
+ if (ret < 0) {
+ dev_err(nor->dev, "error while writing status register 2\n");
+ return -EINVAL;
+ }
+
+ ret = spi_nor_wait_till_ready(nor);
+ if (ret < 0) {
+ dev_err(nor->dev, "timeout while writing status register 2\n");
+ return ret;
+ }
+
+ /* Read back and check it. */
+ ret = nor->read_reg(nor, SPINOR_OP_RDSR2, &sr2, 1);
+ if (!(ret > 0 && (sr2 & SR2_QUAD_EN_BIT7))) {
+ dev_err(nor->dev, "SR2 Quad bit not set\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int spi_nor_check(struct spi_nor *nor)
{
if (!nor->dev || !nor->read || !nor->write ||
@@ -1589,6 +1761,560 @@ spi_nor_set_pp_settings(struct spi_nor_pp_command *pp,
pp->proto = proto;
}

+/*
+ * Serial Flash Discoverable Parameters (SFDP) parsing.
+ */
+
+/**
+ * spi_nor_read_sfdp() - read Serial Flash Discoverable Parameters.
+ * @nor: pointer to a 'struct spi_nor'
+ * @addr: offset in the SFDP area to start reading data from
+ * @len: number of bytes to read
+ * @buf: buffer where the SFDP data are copied into
+ *
+ * Whatever the actual numbers of bytes for address and dummy cycles are
+ * for (Fast) Read commands, the Read SFDP (5Ah) instruction is always
+ * followed by a 3-byte address and 8 dummy clock cycles.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_read_sfdp(struct spi_nor *nor, u32 addr,
+ size_t len, void *buf)
+{
+ u8 addr_width, read_opcode, read_dummy;
+ int ret;
+
+ read_opcode = nor->read_opcode;
+ addr_width = nor->addr_width;
+ read_dummy = nor->read_dummy;
+
+ nor->read_opcode = SPINOR_OP_RDSFDP;
+ nor->addr_width = 3;
+ nor->read_dummy = 8;
+
+ while (len) {
+ ret = nor->read(nor, addr, len, (u8 *)buf);
+ if (!ret || ret > len) {
+ ret = -EIO;
+ goto read_err;
+ }
+ if (ret < 0)
+ goto read_err;
+
+ buf += ret;
+ addr += ret;
+ len -= ret;
+ }
+ ret = 0;
+
+read_err:
+ nor->read_opcode = read_opcode;
+ nor->addr_width = addr_width;
+ nor->read_dummy = read_dummy;
+
+ return ret;
+}
+
+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;
+};
+
+#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
+
+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;
+};
+
+/* 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];
+};
+
+/* Fast Read settings. */
+
+static inline void
+spi_nor_set_read_settings_from_bfpt(struct spi_nor_read_command *read,
+ u16 half,
+ enum spi_nor_protocol proto)
+{
+ read->num_mode_clocks = (half >> 5) & 0x07;
+ read->num_wait_states = (half >> 0) & 0x1f;
+ read->opcode = (half >> 8) & 0xff;
+ read->proto = proto;
+}
+
+struct sfdp_bfpt_read {
+ /* The Fast Read x-y-z hardware capability in params->hwcaps.mask. */
+ u32 hwcaps;
+
+ /*
+ * The <supported_bit> bit in <supported_dword> BFPT DWORD tells us
+ * whether the Fast Read x-y-z command is supported.
+ */
+ u32 supported_dword;
+ u32 supported_bit;
+
+ /*
+ * The half-word at offset <setting_shift> in <setting_dword> BFPT DWORD
+ * encodes the op code, the number of mode clocks and the number of wait
+ * states to be used by Fast Read x-y-z command.
+ */
+ u32 settings_dword;
+ u32 settings_shift;
+
+ /* The SPI protocol for this Fast Read x-y-z command. */
+ enum spi_nor_protocol proto;
+};
+
+static const struct sfdp_bfpt_read sfdp_bfpt_reads[] = {
+ /* Fast Read 1-1-2 */
+ {
+ SNOR_HWCAPS_READ_1_1_2,
+ BFPT_DWORD(1), BIT(16), /* Supported bit */
+ BFPT_DWORD(4), 0, /* Settings */
+ SNOR_PROTO_1_1_2,
+ },
+
+ /* Fast Read 1-2-2 */
+ {
+ SNOR_HWCAPS_READ_1_2_2,
+ BFPT_DWORD(1), BIT(20), /* Supported bit */
+ BFPT_DWORD(4), 16, /* Settings */
+ SNOR_PROTO_1_2_2,
+ },
+
+ /* Fast Read 2-2-2 */
+ {
+ SNOR_HWCAPS_READ_2_2_2,
+ BFPT_DWORD(5), BIT(0), /* Supported bit */
+ BFPT_DWORD(6), 16, /* Settings */
+ SNOR_PROTO_2_2_2,
+ },
+
+ /* Fast Read 1-1-4 */
+ {
+ SNOR_HWCAPS_READ_1_1_4,
+ BFPT_DWORD(1), BIT(22), /* Supported bit */
+ BFPT_DWORD(3), 16, /* Settings */
+ SNOR_PROTO_1_1_4,
+ },
+
+ /* Fast Read 1-4-4 */
+ {
+ SNOR_HWCAPS_READ_1_4_4,
+ BFPT_DWORD(1), BIT(21), /* Supported bit */
+ BFPT_DWORD(3), 0, /* Settings */
+ SNOR_PROTO_1_4_4,
+ },
+
+ /* Fast Read 4-4-4 */
+ {
+ SNOR_HWCAPS_READ_4_4_4,
+ BFPT_DWORD(5), BIT(4), /* Supported bit */
+ BFPT_DWORD(7), 16, /* Settings */
+ SNOR_PROTO_4_4_4,
+ },
+};
+
+struct sfdp_bfpt_erase {
+ /*
+ * The half-word at offset <shift> in DWORD <dwoard> encodes the
+ * op code and erase sector size to be used by Sector Erase commands.
+ */
+ u32 dword;
+ u32 shift;
+};
+
+static const struct sfdp_bfpt_erase sfdp_bfpt_erases[] = {
+ /* Erase Type 1 in DWORD8 bits[15:0] */
+ {BFPT_DWORD(8), 0},
+
+ /* Erase Type 2 in DWORD8 bits[31:16] */
+ {BFPT_DWORD(8), 16},
+
+ /* Erase Type 3 in DWORD9 bits[15:0] */
+ {BFPT_DWORD(9), 0},
+
+ /* Erase Type 4 in DWORD9 bits[31:16] */
+ {BFPT_DWORD(9), 16},
+};
+
+static int spi_nor_hwcaps_read2cmd(u32 hwcaps);
+
+/**
+ * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
+ * @nor: pointer to a 'struct spi_nor'
+ * @bfpt_header: pointer to the 'struct sfdp_parameter_header' describing
+ * the Basic Flash Parameter Table length and version
+ * @params: pointer to the 'struct spi_nor_flash_parameter' to be
+ * filled
+ *
+ * The Basic Flash Parameter Table is the main and only mandatory table as
+ * defined by the SFDP (JESD216) specification.
+ * It provides us with the total size (memory density) of the data array and
+ * the number of address bytes for Fast Read, Page Program and Sector Erase
+ * commands.
+ * For Fast READ commands, it also gives the number of mode clock cycles and
+ * wait states (regrouped in the number of dummy clock cycles) for each
+ * supported instruction op code.
+ * For Page Program, the page size is now available since JESD216 rev A, however
+ * the supported instruction op codes are still not provided.
+ * For Sector Erase commands, this table stores the supported instruction op
+ * codes and the associated sector sizes.
+ * Finally, the Quad Enable Requirements (QER) are also available since JESD216
+ * rev A. The QER bits encode the manufacturer dependent procedure to be
+ * executed to set the Quad Enable (QE) bit in some internal register of the
+ * Quad SPI memory. Indeed the QE bit, when it exists, must be set before
+ * sending any Quad SPI command to the memory. Actually, setting the QE bit
+ * tells the memory to reassign its WP# and HOLD#/RESET# pins to functions IO2
+ * and IO3 hence enabling 4 (Quad) I/O lines.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_parse_bfpt(struct spi_nor *nor,
+ const struct sfdp_parameter_header *bfpt_header,
+ struct spi_nor_flash_parameter *params)
+{
+ struct mtd_info *mtd = &nor->mtd;
+ struct sfdp_bfpt bfpt;
+ size_t len;
+ int i, cmd, err;
+ u32 addr;
+ u16 half;
+
+ /* 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_nor_read_sfdp(nor, 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:
+ nor->addr_width = 3;
+ break;
+
+ case BFPT_DWORD1_ADDRESS_BYTES_4_ONLY:
+ nor->addr_width = 4;
+ break;
+
+ default:
+ break;
+ }
+
+ /* Flash Memory Density (in bits). */
+ params->size = bfpt.dwords[BFPT_DWORD(2)];
+ if (params->size & BIT(31)) {
+ params->size &= ~BIT(31);
+ params->size = 1ULL << params->size;
+ } else {
+ params->size++;
+ }
+ params->size >>= 3; /* Convert to bytes. */
+
+ /* Fast Read settings. */
+ for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_reads); i++) {
+ const struct sfdp_bfpt_read *rd = &sfdp_bfpt_reads[i];
+ struct spi_nor_read_command *read;
+
+ if (!(bfpt.dwords[rd->supported_dword] & rd->supported_bit)) {
+ params->hwcaps.mask &= ~rd->hwcaps;
+ continue;
+ }
+
+ params->hwcaps.mask |= rd->hwcaps;
+ cmd = spi_nor_hwcaps_read2cmd(rd->hwcaps);
+ read = &params->reads[cmd];
+ half = bfpt.dwords[rd->settings_dword] >> rd->settings_shift;
+ spi_nor_set_read_settings_from_bfpt(read, half, rd->proto);
+ }
+
+ /* Sector Erase settings. */
+ for (i = 0; i < ARRAY_SIZE(sfdp_bfpt_erases); i++) {
+ const struct sfdp_bfpt_erase *er = &sfdp_bfpt_erases[i];
+ u32 erasesize;
+ u8 opcode;
+
+ half = bfpt.dwords[er->dword] >> er->shift;
+ erasesize = half & 0xff;
+
+ /* erasesize == 0 means this Erase Type is not supported. */
+ if (!erasesize)
+ continue;
+
+ erasesize = 1U << erasesize;
+ opcode = (half >> 8) & 0xff;
+#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
+ if (erasesize == SZ_4K) {
+ nor->erase_opcode = opcode;
+ mtd->erasesize = erasesize;
+ break;
+ }
+#endif
+ if (!mtd->erasesize || mtd->erasesize < erasesize) {
+ nor->erase_opcode = opcode;
+ mtd->erasesize = erasesize;
+ }
+ }
+
+ /* 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. */
+ params->page_size = bfpt.dwords[BFPT_DWORD(11)];
+ params->page_size &= BFPT_DWORD11_PAGE_SIZE_MASK;
+ params->page_size >>= BFPT_DWORD11_PAGE_SIZE_SHIFT;
+ params->page_size = 1U << params->page_size;
+
+ /* Quad Enable Requirements. */
+ switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
+ case BFPT_DWORD15_QER_NONE:
+ params->quad_enable = NULL;
+ break;
+
+ case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
+ case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
+ params->quad_enable = spansion_no_read_cr_quad_enable;
+ break;
+
+ case BFPT_DWORD15_QER_SR1_BIT6:
+ params->quad_enable = macronix_quad_enable;
+ break;
+
+ case BFPT_DWORD15_QER_SR2_BIT7:
+ params->quad_enable = sr2_bit7_quad_enable;
+ break;
+
+ case BFPT_DWORD15_QER_SR2_BIT1:
+ params->quad_enable = spansion_read_cr_quad_enable;
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
+ * @nor: pointer to a 'struct spi_nor'
+ * @params: pointer to the 'struct spi_nor_flash_parameter' to be
+ * filled
+ *
+ * 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 such
+ * as Fast Read, Page Program or Sector Erase commands.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_parse_sfdp(struct spi_nor *nor,
+ struct spi_nor_flash_parameter *params)
+{
+ const struct sfdp_parameter_header *param_header, *bfpt_header;
+ struct sfdp_parameter_header *param_headers = NULL;
+ struct sfdp_header header;
+ struct device *dev = nor->dev;
+ size_t psize;
+ int i, err;
+
+ /* Get the SFDP header. */
+ err = spi_nor_read_sfdp(nor, 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 ||
+ header.minor < SFDP_JESD216_MINOR)
+ 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. Also we use kmalloc() instead of devm_kmalloc()
+ * because we don't need to keep these parameter headers: the allocated
+ * memory is always released with kfree() before exiting this function.
+ */
+ if (header.nph) {
+ psize = header.nph * sizeof(*param_headers);
+
+ param_headers = kmalloc(psize, GFP_KERNEL);
+ if (!param_headers)
+ return -ENOMEM;
+
+ err = spi_nor_read_sfdp(nor, 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_nor_parse_bfpt(nor, bfpt_header, params);
+ if (err)
+ goto exit;
+
+ /* Parse other parameter headers. */
+ for (i = 0; i < header.nph; i++) {
+ param_header = &param_headers[i];
+
+ switch (SFDP_PARAM_HEADER_ID(param_header)) {
+ case SFDP_SECTOR_MAP_ID:
+ dev_info(dev, "non-uniform erase sector maps are not supported yet.\n");
+ break;
+
+ default:
+ break;
+ }
+
+ if (err)
+ goto exit;
+ }
+
+exit:
+ kfree(param_headers);
+ return err;
+}
+
static int spi_nor_init_params(struct spi_nor *nor,
const struct flash_info *info,
struct spi_nor_flash_parameter *params)
@@ -1644,11 +2370,28 @@ static int spi_nor_init_params(struct spi_nor *nor,
break;

default:
+ /* Kept only for backward compatibility purpose. */
params->quad_enable = spansion_quad_enable;
break;
}
}

+ /* Override the parameters with data read from SFDP tables. */
+ nor->addr_width = 0;
+ nor->mtd.erasesize = 0;
+ if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
+ !(info->flags & SPI_NOR_SKIP_SFDP)) {
+ struct spi_nor_flash_parameter sfdp_params;
+
+ memcpy(&sfdp_params, params, sizeof(sfdp_params));
+ if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
+ nor->addr_width = 0;
+ nor->mtd.erasesize = 0;
+ } else {
+ memcpy(params, &sfdp_params, sizeof(*params));
+ }
+ }
+
return 0;
}

@@ -1760,6 +2503,10 @@ static int spi_nor_select_erase(struct spi_nor *nor,
{
struct mtd_info *mtd = &nor->mtd;

+ /* Do nothing if already configured from SFDP. */
+ if (mtd->erasesize)
+ return 0;
+
#ifdef CONFIG_MTD_SPI_NOR_USE_4K_SECTORS
/* prefer "small sector" erase if possible */
if (info->flags & SECT_4K) {
@@ -1992,9 +2739,11 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (ret)
return ret;

- if (info->addr_width)
+ if (nor->addr_width) {
+ /* already configured from SFDP */
+ } else if (info->addr_width) {
nor->addr_width = info->addr_width;
- else if (mtd->size > 0x1000000) {
+ } else if (mtd->size > 0x1000000) {
/* enable 4-byte addressing if the device exceeds 16MiB */
nor->addr_width = 4;
if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 55faa2f07cca..0df3638ff0b8 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -41,6 +41,8 @@
#define SPINOR_OP_WREN 0x06 /* Write enable */
#define SPINOR_OP_RDSR 0x05 /* Read status register */
#define SPINOR_OP_WRSR 0x01 /* Write status register 1 byte */
+#define SPINOR_OP_RDSR2 0x3f /* Read status register 2 */
+#define SPINOR_OP_WRSR2 0x3e /* Write status register 2 */
#define SPINOR_OP_READ 0x03 /* Read data bytes (low frequency) */
#define SPINOR_OP_READ_FAST 0x0b /* Read data bytes (high frequency) */
#define SPINOR_OP_READ_1_1_2 0x3b /* Read data bytes (Dual Output SPI) */
@@ -56,6 +58,7 @@
#define SPINOR_OP_CHIP_ERASE 0xc7 /* Erase whole flash chip */
#define SPINOR_OP_SE 0xd8 /* Sector erase (usually 64KiB) */
#define SPINOR_OP_RDID 0x9f /* Read JEDEC ID */
+#define SPINOR_OP_RDSFDP 0x5a /* Read SFDP */
#define SPINOR_OP_RDCR 0x35 /* Read configuration register */
#define SPINOR_OP_RDFSR 0x70 /* Read flag status register */

@@ -128,6 +131,9 @@
/* Configuration Register bits. */
#define CR_QUAD_EN_SPAN BIT(1) /* Spansion Quad I/O */

+/* Status Register 2 bits. */
+#define SR2_QUAD_EN_BIT7 BIT(7)
+
/* Supported SPI protocols */
#define SNOR_PROTO_INST_MASK GENMASK(23, 16)
#define SNOR_PROTO_INST_SHIFT 16
--
2.7.4

2017-06-26 13:10:25

by Cyrille Pitchen

[permalink] [raw]
Subject: [PATCH v3 1/2] mtd: st_spi_fsm: remove SPINOR_OP_RDSR2 and use SPINOR_OP_RDCR instead

The 35h instruction op code has two aliases/macro definitions:
- SPINOR_OP_RDCR from include/linux/mtd/spi-nor.h
- SPINOR_OP_RDSR2 from drivers/mtd/devices/serial_flash_cmds.h

Actually, some manufacturers name the associated internal register Status
Register 2 whereas other manufacturers name it Configuration Register
hence the two different macros for the very same instruction op code.

Since the spi-nor.h file is the reference file for all SPI NOR instruction
op codes, this patch removes the definition of the SPINOR_OP_RDSR2 macro.

Also the SPINOR_OP_RDSR2 macro will be associated to another instruction
op code in a further patch so we need to avoid a conflict defining this
macro twice. Indeed the JESD216 rev B specification, defining the SFDP
tables, also refers to the 3Eh and 3Fh instruction op codes to write/read
the Status Register 2 on some SPI NOR flash memories, the 35h op code
still being used to read the Configuration Register/Status Register 2 on
other memories.

Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/mtd/devices/serial_flash_cmds.h | 1 -
drivers/mtd/devices/st_spi_fsm.c | 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h
index 8b81e15105dd..eba125c9f23f 100644
--- a/drivers/mtd/devices/serial_flash_cmds.h
+++ b/drivers/mtd/devices/serial_flash_cmds.h
@@ -13,7 +13,6 @@
#define _MTD_SERIAL_FLASH_CMDS_H

/* Generic Flash Commands/OPCODEs */
-#define SPINOR_OP_RDSR2 0x35
#define SPINOR_OP_WRVCR 0x81
#define SPINOR_OP_RDVCR 0x85

diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
index 804313a33f2b..21afd94cd904 100644
--- a/drivers/mtd/devices/st_spi_fsm.c
+++ b/drivers/mtd/devices/st_spi_fsm.c
@@ -1445,7 +1445,7 @@ static int stfsm_s25fl_config(struct stfsm *fsm)
}

/* Check status of 'QE' bit, update if required. */
- stfsm_read_status(fsm, SPINOR_OP_RDSR2, &cr1, 1);
+ stfsm_read_status(fsm, SPINOR_OP_RDCR, &cr1, 1);
data_pads = ((fsm->stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
if (data_pads == 4) {
if (!(cr1 & STFSM_S25FL_CONFIG_QE)) {
@@ -1490,7 +1490,7 @@ static int stfsm_w25q_config(struct stfsm *fsm)
return ret;

/* Check status of 'QE' bit, update if required. */
- stfsm_read_status(fsm, SPINOR_OP_RDSR2, &sr2, 1);
+ stfsm_read_status(fsm, SPINOR_OP_RDCR, &sr2, 1);
data_pads = ((fsm->stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
if (data_pads == 4) {
if (!(sr2 & W25Q_STATUS_QE)) {
--
2.7.4

2017-06-27 10:20:24

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

On 06/26/2017 03:10 PM, Cyrille Pitchen wrote:
> This patch adds support to the JESD216 rev B standard and parses the SFDP
> tables to dynamically initialize the 'struct spi_nor_flash_parameter'.
>
> Signed-off-by: Cyrille Pitchen <[email protected]>

Changelog is missing. And for such a huge patch, the description is
really tiny.

> ---
> drivers/mtd/spi-nor/spi-nor.c | 775 +++++++++++++++++++++++++++++++++++++++++-
> include/linux/mtd/spi-nor.h | 6 +
> 2 files changed, 768 insertions(+), 13 deletions(-)

Otherwise,
Reviewed-by: Marek Vasut <[email protected]>

--
Best regards,
Marek Vasut

2017-06-27 10:20:53

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] mtd: st_spi_fsm: remove SPINOR_OP_RDSR2 and use SPINOR_OP_RDCR instead

On 06/26/2017 03:09 PM, Cyrille Pitchen wrote:
> The 35h instruction op code has two aliases/macro definitions:
> - SPINOR_OP_RDCR from include/linux/mtd/spi-nor.h
> - SPINOR_OP_RDSR2 from drivers/mtd/devices/serial_flash_cmds.h
>
> Actually, some manufacturers name the associated internal register Status
> Register 2 whereas other manufacturers name it Configuration Register
> hence the two different macros for the very same instruction op code.
>
> Since the spi-nor.h file is the reference file for all SPI NOR instruction
> op codes, this patch removes the definition of the SPINOR_OP_RDSR2 macro.
>
> Also the SPINOR_OP_RDSR2 macro will be associated to another instruction
> op code in a further patch so we need to avoid a conflict defining this
> macro twice. Indeed the JESD216 rev B specification, defining the SFDP
> tables, also refers to the 3Eh and 3Fh instruction op codes to write/read
> the Status Register 2 on some SPI NOR flash memories, the 35h op code
> still being used to read the Configuration Register/Status Register 2 on
> other memories.
>
> Signed-off-by: Cyrille Pitchen <[email protected]>

Acked-by: Marek Vasut <[email protected]>

> ---
> drivers/mtd/devices/serial_flash_cmds.h | 1 -
> drivers/mtd/devices/st_spi_fsm.c | 4 ++--
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h
> index 8b81e15105dd..eba125c9f23f 100644
> --- a/drivers/mtd/devices/serial_flash_cmds.h
> +++ b/drivers/mtd/devices/serial_flash_cmds.h
> @@ -13,7 +13,6 @@
> #define _MTD_SERIAL_FLASH_CMDS_H
>
> /* Generic Flash Commands/OPCODEs */
> -#define SPINOR_OP_RDSR2 0x35
> #define SPINOR_OP_WRVCR 0x81
> #define SPINOR_OP_RDVCR 0x85
>
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index 804313a33f2b..21afd94cd904 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -1445,7 +1445,7 @@ static int stfsm_s25fl_config(struct stfsm *fsm)
> }
>
> /* Check status of 'QE' bit, update if required. */
> - stfsm_read_status(fsm, SPINOR_OP_RDSR2, &cr1, 1);
> + stfsm_read_status(fsm, SPINOR_OP_RDCR, &cr1, 1);
> data_pads = ((fsm->stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
> if (data_pads == 4) {
> if (!(cr1 & STFSM_S25FL_CONFIG_QE)) {
> @@ -1490,7 +1490,7 @@ static int stfsm_w25q_config(struct stfsm *fsm)
> return ret;
>
> /* Check status of 'QE' bit, update if required. */
> - stfsm_read_status(fsm, SPINOR_OP_RDSR2, &sr2, 1);
> + stfsm_read_status(fsm, SPINOR_OP_RDCR, &sr2, 1);
> data_pads = ((fsm->stfsm_seq_read.seq_cfg >> 16) & 0x3) + 1;
> if (data_pads == 4) {
> if (!(sr2 & W25Q_STATUS_QE)) {
>


--
Best regards,
Marek Vasut

2017-06-27 20:10:00

by Cyrille Pitchen

[permalink] [raw]
Subject: Re: [PATCH v3 0/2] mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

Le 26/06/2017 à 15:09, Cyrille Pitchen a écrit :
> Hi all,
>
> a new version of the SFDP patch based on next-20170626
>
> tested on sama5d2 xplained with the following QSPI memories:
> Macronix
> - mx25l25673g
>
> Spansion/Cypress
> - s25fl164
> - s25fl127
> - s25fl512
>
> Winbond
> - w25q256
> - w25m512
>
> SST
> - sst26vf064
>
> Micron
> - m25q128
> - n25q128a
> - m25ql512
> - m25ql01g
>
> For my tests, I used mtd_debug to erase, write then read back some areas
> inside data array of the SPI NOR flash memory.
> To verify the integrity of the data, I used sha1sum to compare the
> original file with the one read from the SPI flash memory.
>
> For memories with a non-uniform erase map (sst26vf064, s25fl512, ...), I
> chose an offset in the data array so the sector size is the one set into
> nor->mtd.erasesize.
>
> CONFIG_MTD_SPI_NOR_USE_4K_SECTORS was defined in my .config file.
> Older tests were also run with this macro undefined and with other memory
> parts.
> Here I'm only talking about tests performed within the last few days.
>
> For sst26vf064, the Global Unlock Block Protection command was already
> sent to the memory by the bootloader so once in Linux, the memory was in a
> rw mode, otherwise Sector Erase and Page Program would fail.
>
> Flash unlock (block protection) and non-uniform erase map are out of the
> scope of this patch and would be addressed later in dedicated patches.
>
> I added/updated few entries in the spi_nor_ids[] array so I could use all
> the memory parts listed above with SFDP support.
>
> My s25fl127 sample is buggy and I don't know whether the issue has been
> fixed by Cypress with the later revisions of this part number (s25fl128s):
> The SFDP data programmed in my sample claim that the memory is compliant
> with JESD216 rev B (version 1.6) however DWORDs 10 to 16 of the Basic
> Flash Parameter Table are all 0xFFFFFFFF, only the first 9 DWORDs are
> programmed correctly as is the memory was only compliant with JESD216
> (version 1.0). Hence when testing the QER bits in DWORD15, the reserved
> value 111b is read. That why I've changed the default case for QER bits
> so it now returns -EINVAL when an unexpected value is read.
> So the SFDP data are reported as invalid and just ignored.
> I didn't use the SPI_NOR_SKIP_SFDP info->flags because s25fl127 and
> s25fl128s share the same JEDEC ID and I want to give a chance to use the
> SFDP tables of the later revisions of this memory part if the above
> issue has been fixed.
>
> Best regards,
>
> Cyrille
>
>
> ChangeLog
>
> v2 > v3:
> - add a small patch to fix a conflict when the SPINOR_OP_RDSR2 macro was
> defined twice: first in drivers/mtd/devices/serial_flash_cmds.h and
> secondly in include/linux/mtd/spi-nor.h. It resulted in a build warning.
> - add the missing () in the kernel-doc comments for functions
> - add a Return: section in the kernel-doc comments for functions
> - improve spi_nor_read_sfdp() to take into account the case where all SFDP
> data can't be read in a single nor->read() call.
>
> v1 -> v2:
> - add kernel-doc to the main functions introduced by this patch.
> - rename spansion_new_quad_enable() into spansion_read_cr_quad_enable().
> - add spansion_no_read_cr_quad_enable(): the explanation is given in
> the kernel-doc.
> - take Marek's comments into account (add new lines, remove parenthesis,
> keep new error messages on the same line as dev_err(), already exisiting
> error messages are left unchanged, use u32 instead of int for >>
> operand, use sizeof(u32) instead of sizeof(uint32_t)).
> - propagate return code of spi_nor_read_sfdp().
> - handle the default case of the QER bits differently: now returns -EINVAL
>
>
> Cyrille Pitchen (2):
> mtd: st_spi_fsm: remove SPINOR_OP_RDSR2 and use SPINOR_OP_RDCR instead
> mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables
>
> drivers/mtd/devices/serial_flash_cmds.h | 1 -
> drivers/mtd/devices/st_spi_fsm.c | 4 +-
> drivers/mtd/spi-nor/spi-nor.c | 775 +++++++++++++++++++++++++++++++-
> include/linux/mtd/spi-nor.h | 6 +
> 4 files changed, 770 insertions(+), 16 deletions(-)
>

series applied to the spi-nor/next branch of l2-mtd

2017-09-06 16:51:53

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

Hi Cyrille, Boris,

On Mon, Jun 26, 2017 at 3:10 PM, Cyrille Pitchen
<[email protected]> wrote:
> This patch adds support to the JESD216 rev B standard and parses the SFDP
> tables to dynamically initialize the 'struct spi_nor_flash_parameter'.
>
> Signed-off-by: Cyrille Pitchen <[email protected]>

This patch, which is now commit f384b352cbf0310f ("mtd: spi-nor: parse
Serial Flash
Discoverable Parameters (SFDP) tables") in -next, and which was included in the
"MTD updates for 4.14" pull request, broke QSPI FLASH on R-Car Gen2.

E.g. on r8a7794/alt, the following warning is printed with
CONFIG_DMA_API_DEBUG=y:

WARNING: CPU: 1 PID: 1 at lib/dma-debug.c:1188 check_for_stack+0xcc/0x114
rcar-dmac e6720000.dma-controller: DMA-API: device driver maps
memory from stack [addr=ef43dad8]
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted
4.13.0-rcar2-initrd-00404-g3d49bfdcf5ecd040 #76
Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
[<c020e664>] (unwind_backtrace) from [<c020a13c>] (show_stack+0x10/0x14)
[<c020a13c>] (show_stack) from [<c06a5c50>] (dump_stack+0x7c/0x9c)
[<c06a5c50>] (dump_stack) from [<c021ef58>] (__warn+0xcc/0xfc)
[<c021ef58>] (__warn) from [<c021efbc>] (warn_slowpath_fmt+0x34/0x44)
[<c021efbc>] (warn_slowpath_fmt) from [<c0403228>]
(check_for_stack+0xcc/0x114)
[<c0403228>] (check_for_stack) from [<c0403a9c>]
(debug_dma_map_sg+0xe4/0x168)
[<c0403a9c>] (debug_dma_map_sg) from [<c04cd7b8>] (spi_map_buf+0x2f4/0x32c)
[<c04cd7b8>] (spi_map_buf) from [<c04ce560>]
(__spi_pump_messages+0x430/0x4d8)
[<c04ce560>] (__spi_pump_messages) from [<c04ce77c>]
(__spi_sync+0x168/0x194)
[<c04ce77c>] (__spi_sync) from [<c04ce7cc>] (spi_sync+0x24/0x3c)
[<c04ce7cc>] (spi_sync) from [<c04c97fc>] (m25p80_read+0x2e4/0x300)
[<c04c97fc>] (m25p80_read) from [<c04c99d8>] (spi_nor_read_sfdp+0x58/0xa8)
[<c04c99d8>] (spi_nor_read_sfdp) from [<c04c9fc8>]
(spi_nor_init_params+0x2e8/0x5f4)
[<c04c9fc8>] (spi_nor_init_params) from [<c04cb510>]
(spi_nor_scan+0xc4/0x830)
[<c04cb510>] (spi_nor_scan) from [<c04c9928>] (m25p_probe+0x110/0x168)
[<c04c9928>] (m25p_probe) from [<c04cce0c>] (spi_drv_probe+0x84/0xa0)
[<c04cce0c>] (spi_drv_probe) from [<c0470f80>]
(driver_probe_device+0x150/0x2d0)
[<c0470f80>] (driver_probe_device) from [<c046f7fc>]
(bus_for_each_drv+0x84/0x94)
[<c046f7fc>] (bus_for_each_drv) from [<c0470d98>]
(__device_attach+0x88/0xfc)
[<c0470d98>] (__device_attach) from [<c0470450>]
(bus_probe_device+0x28/0x80)
[<c0470450>] (bus_probe_device) from [<c046ebac>] (device_add+0x3f8/0x50c)
[<c046ebac>] (device_add) from [<c04cef54>] (spi_add_device+0xd0/0x14c)
[<c04cef54>] (spi_add_device) from [<c04cf308>]
(of_register_spi_device+0x200/0x2f4)
[<c04cf308>] (of_register_spi_device) from [<c04cf878>]
(spi_register_controller+0x47c/0x5e4)
[<c04cf878>] (spi_register_controller) from [<c04cfa10>]
(devm_spi_register_controller+0x30/0x6c)
[<c04cfa10>] (devm_spi_register_controller) from [<c04d190c>]
(rspi_probe+0x258/0x404)
[<c04d190c>] (rspi_probe) from [<c0472780>] (platform_drv_probe+0x50/0xa0)
[<c0472780>] (platform_drv_probe) from [<c0470f80>]
(driver_probe_device+0x150/0x2d0)
[<c0470f80>] (driver_probe_device) from [<c0471180>]
(__driver_attach+0x80/0xa4)
[<c0471180>] (__driver_attach) from [<c046f740>]
(bus_for_each_dev+0x6c/0x90)
[<c046f740>] (bus_for_each_dev) from [<c047064c>]
(bus_add_driver+0xc8/0x1e4)
[<c047064c>] (bus_add_driver) from [<c0471a14>] (driver_register+0x9c/0xe0)
[<c0471a14>] (driver_register) from [<c0201718>]
(do_one_initcall+0xa8/0x14c)
[<c0201718>] (do_one_initcall) from [<c0c00d84>]
(kernel_init_freeable+0x114/0x:
[<c0c00d84>] (kernel_init_freeable) from [<c06b6244>]
(kernel_init+0x8/0x10c)
[<c06b6244>] (kernel_init) from [<c0206b68>] (ret_from_fork+0x14/0x2c)

This leads to the SPI FLASH size being misdetected as zero:

-m25p80 spi0.0: s25fl512s (65536 Kbytes)
+m25p80 spi0.0: s25fl512s (0 Kbytes)
3 ofpart partitions found on MTD device spi0.0
Creating 3 MTD partitions on "spi0.0":
0x000000000000-0x000000040000 : "loader"
+mtd: partition "loader" is out of reach -- disabled
0x000000040000-0x000000080000 : "system"
+mtd: partition "system" is out of reach -- disabled
0x000000080000-0x000004000000 : "user"
+mtd: partition "user" is out of reach -- disabled

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2017-09-06 20:52:58

by Cyrille Pitchen

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables

Hi Geert,

thanks for your report: I'm writing a bug fix right now!

Best regards,

Cyrille

Le 06/09/2017 à 18:51, Geert Uytterhoeven a écrit :
> Hi Cyrille, Boris,
>
> On Mon, Jun 26, 2017 at 3:10 PM, Cyrille Pitchen
> <[email protected]> wrote:
>> This patch adds support to the JESD216 rev B standard and parses the SFDP
>> tables to dynamically initialize the 'struct spi_nor_flash_parameter'.
>>
>> Signed-off-by: Cyrille Pitchen <[email protected]>
>
> This patch, which is now commit f384b352cbf0310f ("mtd: spi-nor: parse
> Serial Flash
> Discoverable Parameters (SFDP) tables") in -next, and which was included in the
> "MTD updates for 4.14" pull request, broke QSPI FLASH on R-Car Gen2.
>
> E.g. on r8a7794/alt, the following warning is printed with
> CONFIG_DMA_API_DEBUG=y:
>
> WARNING: CPU: 1 PID: 1 at lib/dma-debug.c:1188 check_for_stack+0xcc/0x114
> rcar-dmac e6720000.dma-controller: DMA-API: device driver maps
> memory from stack [addr=ef43dad8]
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 4.13.0-rcar2-initrd-00404-g3d49bfdcf5ecd040 #76
> Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [<c020e664>] (unwind_backtrace) from [<c020a13c>] (show_stack+0x10/0x14)
> [<c020a13c>] (show_stack) from [<c06a5c50>] (dump_stack+0x7c/0x9c)
> [<c06a5c50>] (dump_stack) from [<c021ef58>] (__warn+0xcc/0xfc)
> [<c021ef58>] (__warn) from [<c021efbc>] (warn_slowpath_fmt+0x34/0x44)
> [<c021efbc>] (warn_slowpath_fmt) from [<c0403228>]
> (check_for_stack+0xcc/0x114)
> [<c0403228>] (check_for_stack) from [<c0403a9c>]
> (debug_dma_map_sg+0xe4/0x168)
> [<c0403a9c>] (debug_dma_map_sg) from [<c04cd7b8>] (spi_map_buf+0x2f4/0x32c)
> [<c04cd7b8>] (spi_map_buf) from [<c04ce560>]
> (__spi_pump_messages+0x430/0x4d8)
> [<c04ce560>] (__spi_pump_messages) from [<c04ce77c>]
> (__spi_sync+0x168/0x194)
> [<c04ce77c>] (__spi_sync) from [<c04ce7cc>] (spi_sync+0x24/0x3c)
> [<c04ce7cc>] (spi_sync) from [<c04c97fc>] (m25p80_read+0x2e4/0x300)
> [<c04c97fc>] (m25p80_read) from [<c04c99d8>] (spi_nor_read_sfdp+0x58/0xa8)
> [<c04c99d8>] (spi_nor_read_sfdp) from [<c04c9fc8>]
> (spi_nor_init_params+0x2e8/0x5f4)
> [<c04c9fc8>] (spi_nor_init_params) from [<c04cb510>]
> (spi_nor_scan+0xc4/0x830)
> [<c04cb510>] (spi_nor_scan) from [<c04c9928>] (m25p_probe+0x110/0x168)
> [<c04c9928>] (m25p_probe) from [<c04cce0c>] (spi_drv_probe+0x84/0xa0)
> [<c04cce0c>] (spi_drv_probe) from [<c0470f80>]
> (driver_probe_device+0x150/0x2d0)
> [<c0470f80>] (driver_probe_device) from [<c046f7fc>]
> (bus_for_each_drv+0x84/0x94)
> [<c046f7fc>] (bus_for_each_drv) from [<c0470d98>]
> (__device_attach+0x88/0xfc)
> [<c0470d98>] (__device_attach) from [<c0470450>]
> (bus_probe_device+0x28/0x80)
> [<c0470450>] (bus_probe_device) from [<c046ebac>] (device_add+0x3f8/0x50c)
> [<c046ebac>] (device_add) from [<c04cef54>] (spi_add_device+0xd0/0x14c)
> [<c04cef54>] (spi_add_device) from [<c04cf308>]
> (of_register_spi_device+0x200/0x2f4)
> [<c04cf308>] (of_register_spi_device) from [<c04cf878>]
> (spi_register_controller+0x47c/0x5e4)
> [<c04cf878>] (spi_register_controller) from [<c04cfa10>]
> (devm_spi_register_controller+0x30/0x6c)
> [<c04cfa10>] (devm_spi_register_controller) from [<c04d190c>]
> (rspi_probe+0x258/0x404)
> [<c04d190c>] (rspi_probe) from [<c0472780>] (platform_drv_probe+0x50/0xa0)
> [<c0472780>] (platform_drv_probe) from [<c0470f80>]
> (driver_probe_device+0x150/0x2d0)
> [<c0470f80>] (driver_probe_device) from [<c0471180>]
> (__driver_attach+0x80/0xa4)
> [<c0471180>] (__driver_attach) from [<c046f740>]
> (bus_for_each_dev+0x6c/0x90)
> [<c046f740>] (bus_for_each_dev) from [<c047064c>]
> (bus_add_driver+0xc8/0x1e4)
> [<c047064c>] (bus_add_driver) from [<c0471a14>] (driver_register+0x9c/0xe0)
> [<c0471a14>] (driver_register) from [<c0201718>]
> (do_one_initcall+0xa8/0x14c)
> [<c0201718>] (do_one_initcall) from [<c0c00d84>]
> (kernel_init_freeable+0x114/0x:
> [<c0c00d84>] (kernel_init_freeable) from [<c06b6244>]
> (kernel_init+0x8/0x10c)
> [<c06b6244>] (kernel_init) from [<c0206b68>] (ret_from_fork+0x14/0x2c)
>
> This leads to the SPI FLASH size being misdetected as zero:
>
> -m25p80 spi0.0: s25fl512s (65536 Kbytes)
> +m25p80 spi0.0: s25fl512s (0 Kbytes)
> 3 ofpart partitions found on MTD device spi0.0
> Creating 3 MTD partitions on "spi0.0":
> 0x000000000000-0x000000040000 : "loader"
> +mtd: partition "loader" is out of reach -- disabled
> 0x000000040000-0x000000080000 : "system"
> +mtd: partition "system" is out of reach -- disabled
> 0x000000080000-0x000004000000 : "user"
> +mtd: partition "user" is out of reach -- disabled
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>