LinuxLists.cc - [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

[permalink] [raw]

Subject: [PATCH v2 2/5] mtd: spi-nor: sfdp: Add support for xSPI profile 1.0 table

xSPI(eXpanded Serial Peripheral Interface) is for Non Volatile Memory
Devices supports Octal DTR mode.

Extract information like:
Read Fast command, the number of dummy cycles and address bytes
for Read Status Register command.

Read/Write volatile Register command for Configuration(CFG) Register 2.

The dummy cycless used for various frequencies.

Signed-off-by: Mason Yang <[email protected]>
---
drivers/mtd/spi-nor/core.h | 14 ++++++
drivers/mtd/spi-nor/sfdp.c | 106 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 6f2f6b2..2ea11fa 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -208,6 +208,12 @@ struct spi_nor_locking_ops {
* e.g. different opcodes, specific address calculation,
* page size, etc.
* @locking_ops: SPI NOR locking methods.
+ * @dtr_read_cmd: xSPI Octal DTR Read Fast command.
+ * @rdsr_addr_nbytes: xSPI Octal address bytes for read status register.
+ * @rdsr_dummy_cycles: xSPI Octal dummy cycles for read status register.
+ * @rd_reg_cmd: xSPI Octal read volatile register command.
+ * @wr_reg_cmd: xSPI Octal write volatile register command.
+ * @dummy_cycles: xSPI Octal dummy cycles used for various frequencies.
*/
struct spi_nor_flash_parameter {
u64 size;
@@ -225,6 +231,14 @@ struct spi_nor_flash_parameter {
int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);

const struct spi_nor_locking_ops *locking_ops;
+
+ /* xSPI profile 1.0 parameter for Octal 8S-8S-8S/8D-8D-8D */
+ u8 dtr_read_cmd;
+ u8 rdsr_addr_nbytes;
+ u8 rdsr_dummy_cycles;
+ u8 rd_reg_cmd;
+ u8 wr_reg_cmd;
+ u8 dummy_cycles;
};

/**
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index f6038d3..26814a1 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -7,6 +7,7 @@
#include <linux/slab.h>
#include <linux/sort.h>
#include <linux/mtd/spi-nor.h>
+#include <linux/bitfield.h>

#include "core.h"

@@ -19,6 +20,7 @@
#define SFDP_BFPT_ID 0xff00 /* Basic Flash Parameter Table */
#define SFDP_SECTOR_MAP_ID 0xff81 /* Sector Map Table */
#define SFDP_4BAIT_ID 0xff84 /* 4-byte Address Instruction Table */
+#define SFDP_XSPI_PF1_ID 0xff05 /* xSPI Profile 1.0 table. */

#define SFDP_SIGNATURE 0x50444653U
#define SFDP_JESD216_MAJOR 1
@@ -26,6 +28,28 @@
#define SFDP_JESD216A_MINOR 5
#define SFDP_JESD216B_MINOR 6

+/* xSPI Profile 1.0 table (from JESD216D.01). */
+#define XSPI_PF1_DWORD1_RD_CMD GENMASK(15, 8)
+#define XSPI_PF1_DWORD1_RDSR_ADDR_BYTES BIT(29)
+#define XSPI_PF1_DWORD1_RDSR_DUMMY_CYCLES BIT(28)
+
+#define XSPI_PF1_DWORD2_RD_REG_CMD GENMASK(31, 24)
+#define XSPI_PF1_DWORD2_WR_REG_CMD GENMASK(15, 8)
+
+#define XSPI_DWORD(x) ((x) - 1)
+#define XSPI_DWORD_MAX 5
+
+struct sfdp_xspi {
+ u32 dwords[XSPI_DWORD_MAX];
+};
+
+struct xspi_dummy_cycles {
+ u16 speed_hz; /* Speed MHz */
+ u8 dwords; /* Dwords index */
+ u32 mask; /* Mask */
+ u8 shift; /* Bit shift */
+};
+
struct sfdp_header {
u32 signature; /* Ox50444653U <=> "SFDP" */
u8 minor;
@@ -1081,6 +1105,83 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
}

/**
+ * spi_nor_parse_xspi_pf1() - parse the xSPI Profile 1.0 table
+ * @nor: pointer to a 'struct spi_nor'
+ * @param_header: pointer to the 'struct sfdp_parameter_header' describing
+ * the 4-Byte Address Instruction Table length and version.
+ * @params: pointer to the 'struct spi_nor_flash_parameter' to be.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_parse_xspi_pf1(struct spi_nor *nor,
+ const struct sfdp_parameter_header *header,
+ struct spi_nor_flash_parameter *params)
+{
+ struct sfdp_xspi pfile1;
+ u32 i, addr;
+ size_t len;
+ int ret;
+ static const struct xspi_dummy_cycles dummy[] = {
+ /* {MHz, Dwords index, Mask, Bit shift} */
+ { 200, 4, GENMASK(11, 7), 7},
+ { 166, 5, GENMASK(31, 27), 27},
+ { 133, 5, GENMASK(21, 17), 17},
+ { 100, 5, GENMASK(11, 7), 7},
+ };
+
+ if (header->major != SFDP_JESD216_MAJOR ||
+ header->length < XSPI_DWORD_MAX)
+ return -EINVAL;
+
+ len = min_t(size_t, sizeof(pfile1),
+ header->length * sizeof(u32));
+
+ memset(&pfile1, 0, sizeof(pfile1));
+
+ addr = SFDP_PARAM_HEADER_PTP(header);
+ ret = spi_nor_read_sfdp(nor, addr, len, &pfile1);
+ if (ret)
+ goto out;
+
+ /* Fix endianness of the xSPI 1.0 DWORDs. */
+ le32_to_cpu_array(pfile1.dwords, XSPI_DWORD_MAX);
+
+ /* Get 8D-8D-8D fast read opcode and dummy cycles. */
+ params->dtr_read_cmd = FIELD_GET(XSPI_PF1_DWORD1_RD_CMD,
+ pfile1.dwords[XSPI_DWORD(1)]);
+
+ if (pfile1.dwords[XSPI_DWORD(1)] & XSPI_PF1_DWORD1_RDSR_ADDR_BYTES)
+ params->rdsr_addr_nbytes = 4;
+ else
+ params->rdsr_addr_nbytes = 0;
+
+ if (pfile1.dwords[XSPI_DWORD(1)] & XSPI_PF1_DWORD1_RDSR_DUMMY_CYCLES)
+ params->rdsr_dummy_cycles = 8;
+ else
+ params->rdsr_dummy_cycles = 4;
+
+ params->rd_reg_cmd = FIELD_GET(XSPI_PF1_DWORD2_RD_REG_CMD,
+ pfile1.dwords[XSPI_DWORD(2)]);
+ params->wr_reg_cmd = FIELD_GET(XSPI_PF1_DWORD2_WR_REG_CMD,
+ pfile1.dwords[XSPI_DWORD(2)]);
+
+ for (i = 0; i < ARRAY_SIZE(dummy); i++) {
+ if (params->octal_max_speed == dummy[i].speed_hz) {
+ params->dummy_cycles =
+ (dummy[i].mask &
+ pfile1.dwords[XSPI_DWORD(dummy[i].dwords)]) >>
+ dummy[i].shift;
+ break;
+ }
+ }
+ if (i == ARRAY_SIZE(dummy))
+ params->dummy_cycles = 20;
+
+out:
+ return ret;
+}
+
+/**
* 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
@@ -1171,7 +1272,6 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
/* Parse optional parameter tables. */
for (i = 0; i < header.nph; i++) {
param_header = &param_headers[i];
-
switch (SFDP_PARAM_HEADER_ID(param_header)) {
case SFDP_SECTOR_MAP_ID:
err = spi_nor_parse_smpt(nor, param_header, params);
@@ -1181,6 +1281,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
err = spi_nor_parse_4bait(nor, param_header, params);
break;

+ case SFDP_XSPI_PF1_ID:
+ err = spi_nor_parse_xspi_pf1(nor, param_header, params);
+ break;
+
default:
break;
}
--
1.9.1

2020-04-21 07:10:26

[permalink] [raw]

Subject: [PATCH v2 3/5] mtd: spi-nor: Parse BFPT DWORD-18,19 and 20 for Octal 8D-8D-8D mode

Based on JESD216C BFPT(Basic Flash Parameter Table) to add:
18th DWORD:
Octal DTR(8D-8D-8D) command and command extension (00b: same, 01b: inverse)
- Get extension command type

19th DWORD:
Octal mode enable sequences by two instructions or write CFG Reg2
- Get enable Octal mode sequences
- implemented Read/write CFG Reg 2 function
- setup the call-back function for octal mode enable

20th DWORD:
Maximum operation speed of device in Octal mode
- Get the Octal maximum operation speed

Also defined the relevant macros and enum to add such modes and make sure
op->xxx.dtr fields, command nbytes and extension command are properly
filled and unmask DTR and X-X-X modes in spi_nor_spimem_adjust_hwcaps()
so that DTR and X-X-X support detection is done through
spi_mem_supports_op().

Signed-off-by: Mason Yang <[email protected]>
---
drivers/mtd/spi-nor/core.c | 220 ++++++++++++++++++++++++++++++++++++++++++--
drivers/mtd/spi-nor/core.h | 17 ++++
drivers/mtd/spi-nor/sfdp.c | 116 +++++++++++++++++++++++
drivers/mtd/spi-nor/sfdp.h | 16 +++-
include/linux/mtd/spi-nor.h | 51 +++++++++-
5 files changed, 404 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index cc68ea8..b67c65d 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -40,6 +40,47 @@

#define SPI_NOR_MAX_ADDR_WIDTH 4

+#define SET_SPIMEM_OP_FULL_OCTAL_READ_BUSWIDTH() \
+ { \
+ op.cmd.buswidth = 8; \
+ op.addr.buswidth = 8; \
+ op.dummy.buswidth = 8; \
+ op.data.buswidth = 8; \
+ op.cmd.nbytes = 2; \
+ }
+
+#define SET_SPIMEM_OP_FULL_OCTAL_WRITE_BUSWIDTH() \
+ { \
+ op.cmd.buswidth = 8; \
+ op.addr.buswidth = 8; \
+ op.data.buswidth = 8; \
+ op.cmd.nbytes = 2; \
+ }
+
+#define SET_SPIMEM_OP_DTR_READ() \
+ { \
+ op.dummy.nbytes *= 2; \
+ op.cmd.dtr = true; \
+ op.addr.dtr = true; \
+ op.dummy.dtr = true; \
+ op.data.dtr = true; \
+ }
+
+#define SET_SPIMEM_OP_DTR_WRITE() \
+ { \
+ op.cmd.dtr = true; \
+ op.addr.dtr = true; \
+ op.data.dtr = true; \
+ }
+
+#define SET_OCTAL_EXTION_OPCODE() \
+ { \
+ if (nor->ext_cmd_mode == EXT_CMD_IS_INVERSE) \
+ op.cmd.ext_opcode = ~op.cmd.opcode; \
+ else \
+ op.cmd.ext_opcode = op.cmd.opcode; \
+ }
+
/**
* spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data
* transfer
@@ -113,6 +154,16 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
/* convert the dummy cycles to the number of bytes */
op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;

+ if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
+ op.cmd.nbytes = 2;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->read_proto))
+ SET_SPIMEM_OP_DTR_READ();
+
+ memcpy(&nor->dirmap.rdesc->info.op_tmpl, &op, sizeof(op));
+ }
+
usebouncebuf = spi_nor_spimem_bounce(nor, &op);

if (nor->dirmap.rdesc) {
@@ -176,6 +227,16 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
op.addr.nbytes = 0;

+ if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
+ op.cmd.nbytes = 2;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
+ SET_SPIMEM_OP_DTR_WRITE();
+
+ memcpy(&nor->dirmap.wdesc->info.op_tmpl, &op, sizeof(op));
+ }
+
if (spi_nor_spimem_bounce(nor, &op))
memcpy(nor->bouncebuf, buf, op.data.nbytes);

@@ -227,6 +288,15 @@ int spi_nor_write_enable(struct spi_nor *nor)
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_NO_DATA);

+ if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
+ op.cmd.buswidth = 8;
+ op.cmd.nbytes = 2;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
+ op.cmd.dtr = true;
+ }
+
ret = spi_mem_exec_op(nor->spimem, &op);
} else {
ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN,
@@ -256,6 +326,15 @@ int spi_nor_write_disable(struct spi_nor *nor)
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_NO_DATA);

+ if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
+ op.cmd.buswidth = 8;
+ op.cmd.nbytes = 2;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
+ op.cmd.dtr = true;
+ }
+
ret = spi_mem_exec_op(nor->spimem, &op);
} else {
ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRDI,
@@ -287,6 +366,16 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_DATA_IN(1, sr, 1));

+ if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
+ SET_SPIMEM_OP_FULL_OCTAL_READ_BUSWIDTH();
+ op.addr.nbytes = nor->params->rdsr_addr_nbytes;
+ op.dummy.nbytes = nor->params->rdsr_dummy_cycles;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->read_proto))
+ SET_SPIMEM_OP_DTR_READ();
+ }
+
ret = spi_mem_exec_op(nor->spimem, &op);
} else {
ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
@@ -300,6 +389,92 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
}

/**
+ * spi_nor_read_cr2() - Read the Configuration Register 2.
+ * @nor: pointer to 'struct spi_nor'.
+ * @addr: offset address to read.
+ * @cr2: pointer to a DMA-able buffer where the value of the
+ * Configuration Register 2 will be written.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_read_cr2(struct spi_nor *nor, u32 addr, u8 *cr2)
+{
+ int ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(nor->params->rd_reg_cmd, 1),
+ SPI_MEM_OP_ADDR(4, addr, 1),
+ SPI_MEM_OP_DUMMY(4, 1),
+ SPI_MEM_OP_DATA_IN(1, cr2, 1));
+
+ if (spi_nor_protocol_is_8_8_8(nor->read_proto)) {
+ SET_SPIMEM_OP_FULL_OCTAL_READ_BUSWIDTH();
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->read_proto))
+ SET_SPIMEM_OP_DTR_READ();
+ }
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = nor->controller_ops->read_reg(nor,
+ nor->params->rd_reg_cmd,
+ cr2, 1);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d reading CR2\n", ret);
+
+ return ret;
+}
+
+/**
+ * spi_nor_write_cr2() - Write the Configuration Register 2.
+ * @nor: pointer to 'struct spi_nor'.
+ * @addr: offset address to write.
+ * @cr2: pointer to a DMA-able buffer where the value of the
+ * Configuratin Register 2 will be read.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_cr2(struct spi_nor *nor, u32 addr, u8 *cr2)
+{
+ int ret;
+
+ ret = spi_nor_write_enable(nor);
+ if (ret)
+ return ret;
+
+ if (nor->spimem) {
+ struct spi_mem_op op =
+ SPI_MEM_OP(SPI_MEM_OP_CMD(nor->params->wr_reg_cmd, 1),
+ SPI_MEM_OP_ADDR(4, addr, 1),
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_OUT(1, cr2, 1));
+
+ if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
+ SET_SPIMEM_OP_FULL_OCTAL_WRITE_BUSWIDTH();
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
+ SET_SPIMEM_OP_DTR_WRITE();
+ }
+
+ ret = spi_mem_exec_op(nor->spimem, &op);
+ } else {
+ ret = nor->controller_ops->write_reg(nor,
+ nor->params->wr_reg_cmd,
+ cr2, 1);
+ }
+
+ if (ret)
+ dev_dbg(nor->dev, "error %d write CFG Reg 2\n", ret);
+
+ return ret;
+}
+
+/**
* spi_nor_read_fsr() - Read the Flag Status Register.
* @nor: pointer to 'struct spi_nor'
* @fsr: pointer to a DMA-able buffer where the value of the
@@ -1002,6 +1177,15 @@ static int spi_nor_erase_chip(struct spi_nor *nor)
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_NO_DATA);

+ if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
+ op.cmd.buswidth = 8;
+ op.cmd.nbytes = 2;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto))
+ op.cmd.dtr = true;
+ }
+
ret = spi_mem_exec_op(nor->spimem, &op);
} else {
ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CHIP_ERASE,
@@ -1144,6 +1328,18 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
SPI_MEM_OP_NO_DUMMY,
SPI_MEM_OP_NO_DATA);

+ if (spi_nor_protocol_is_8_8_8(nor->write_proto)) {
+ op.cmd.buswidth = 8;
+ op.addr.buswidth = 8;
+ op.cmd.nbytes = 2;
+ SET_OCTAL_EXTION_OPCODE();
+
+ if (spi_nor_protocol_is_8D_8D_8D(nor->write_proto)) {
+ op.cmd.dtr = true;
+ op.addr.dtr = true;
+ }
+ }
+
return spi_mem_exec_op(nor->spimem, &op);
} else if (nor->controller_ops->erase) {
return nor->controller_ops->erase(nor, addr);
@@ -2204,7 +2400,7 @@ static int spi_nor_check(struct spi_nor *nor)
return 0;
}

-static void
+void
spi_nor_set_read_settings(struct spi_nor_read_command *read,
u8 num_mode_clocks,
u8 num_wait_states,
@@ -2253,6 +2449,7 @@ int spi_nor_hwcaps_read2cmd(u32 hwcaps)
{ SNOR_HWCAPS_READ_1_8_8, SNOR_CMD_READ_1_8_8 },
{ SNOR_HWCAPS_READ_8_8_8, SNOR_CMD_READ_8_8_8 },
{ SNOR_HWCAPS_READ_1_8_8_DTR, SNOR_CMD_READ_1_8_8_DTR },
+ { SNOR_HWCAPS_READ_8_8_8_DTR, SNOR_CMD_READ_8_8_8_DTR },
};

return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
@@ -2269,6 +2466,7 @@ static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
{ SNOR_HWCAPS_PP_1_1_8, SNOR_CMD_PP_1_1_8 },
{ SNOR_HWCAPS_PP_1_8_8, SNOR_CMD_PP_1_8_8 },
{ SNOR_HWCAPS_PP_8_8_8, SNOR_CMD_PP_8_8_8 },
+ { SNOR_HWCAPS_PP_8_8_8_DTR, SNOR_CMD_PP_8_8_8_DTR },
};

return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
@@ -2368,12 +2566,6 @@ static int spi_nor_spimem_check_pp(struct spi_nor *nor,
struct spi_nor_flash_parameter *params = nor->params;
unsigned int cap;

- /* DTR modes are not supported yet, mask them all. */
- *hwcaps &= ~SNOR_HWCAPS_DTR;
-
- /* X-X-X modes are not supported yet, mask them all. */
- *hwcaps &= ~SNOR_HWCAPS_X_X_X;
-
for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
int rdidx, ppidx;

@@ -2614,7 +2806,6 @@ static int spi_nor_default_setup(struct spi_nor *nor,
* controller and the SPI flash memory.
*/
shared_mask = hwcaps->mask & params->hwcaps.mask;
-
if (nor->spimem) {
/*
* When called from spi_nor_probe(), all caps are set and we
@@ -2831,12 +3022,21 @@ static void spi_nor_post_sfdp_fixups(struct spi_nor *nor)
*/
static void spi_nor_late_init_params(struct spi_nor *nor)
{
+ struct spi_nor_flash_parameter *params = nor->params;
+ int ret = 0;
+
/*
* NOR protection support. When locking_ops are not provided, we pick
* the default ones.
*/
if (nor->flags & SNOR_F_HAS_LOCK && !nor->params->locking_ops)
nor->params->locking_ops = &spi_nor_sr_locking_ops;
+
+ if (nor->params->xspi_enable)
+ ret = nor->params->xspi_enable(nor,
+ (params->dtr_read_cmd) ? 1 : 0);
+ if (ret)
+ dev_err(nor->dev, " enable xSPI mode failed %d\n", ret);
}

/**
@@ -2886,8 +3086,8 @@ static int spi_nor_init_params(struct spi_nor *nor)

spi_nor_manufacturer_init_params(nor);

- if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
- !(nor->info->flags & SPI_NOR_SKIP_SFDP))
+ if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+ SPI_NOR_OCTAL_RD_WR)) && !(nor->info->flags & SPI_NOR_SKIP_SFDP))
spi_nor_sfdp_init_params(nor);

spi_nor_post_sfdp_fixups(nor);
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 2ea11fa..ef32b86 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -62,6 +62,7 @@ enum spi_nor_read_command_index {
SNOR_CMD_READ_1_8_8,
SNOR_CMD_READ_8_8_8,
SNOR_CMD_READ_1_8_8_DTR,
+ SNOR_CMD_READ_8_8_8_DTR,

SNOR_CMD_READ_MAX
};
@@ -78,6 +79,7 @@ enum spi_nor_pp_command_index {
SNOR_CMD_PP_1_1_8,
SNOR_CMD_PP_1_8_8,
SNOR_CMD_PP_8_8_8,
+ SNOR_CMD_PP_8_8_8_DTR,

SNOR_CMD_PP_MAX
};
@@ -208,6 +210,8 @@ struct spi_nor_locking_ops {
* e.g. different opcodes, specific address calculation,
* page size, etc.
* @locking_ops: SPI NOR locking methods.
+ * @xspi_enable: enables xSPI Octal mode for 8S-8S-8S/8D-8D-8D.
+ * @octal_max_speed: xSPI Octal maximum operation speed (MHz).
* @dtr_read_cmd: xSPI Octal DTR Read Fast command.
* @rdsr_addr_nbytes: xSPI Octal address bytes for read status register.
* @rdsr_dummy_cycles: xSPI Octal dummy cycles for read status register.
@@ -232,6 +236,11 @@ struct spi_nor_flash_parameter {

const struct spi_nor_locking_ops *locking_ops;

+ /* BFPT DWORD19 Octal mode enable sequences */
+ int (*xspi_enable)(struct spi_nor *nor, bool dtr);
+ /* BFPT DWORD20 Maximum operation speed of device in Octal mode */
+ u16 octal_max_speed;
+
/* xSPI profile 1.0 parameter for Octal 8S-8S-8S/8D-8D-8D */
u8 dtr_read_cmd;
u8 rdsr_addr_nbytes;
@@ -325,6 +334,7 @@ struct flash_info {
* BP3 is bit 6 of status register.
* Must be used with SPI_NOR_4BIT_BP.
*/
+#define SPI_NOR_OCTAL_RD_WR BIT(19) /* Flash supports Octal Read & Write */

/* Part specific fixup hooks. */
const struct spi_nor_fixups *fixups;
@@ -413,6 +423,8 @@ struct spi_nor_manufacturer {
extern const struct spi_nor_manufacturer spi_nor_xilinx;
extern const struct spi_nor_manufacturer spi_nor_xmc;

+int spi_nor_read_cr2(struct spi_nor *nor, u32 addr, u8 *cr2);
+int spi_nor_write_cr2(struct spi_nor *nor, u32 addr, u8 *cr2);
int spi_nor_write_enable(struct spi_nor *nor);
int spi_nor_write_disable(struct spi_nor *nor);
int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
@@ -432,6 +444,11 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,

int spi_nor_hwcaps_read2cmd(u32 hwcaps);
u8 spi_nor_convert_3to4_read(u8 opcode);
+void spi_nor_set_read_settings(struct spi_nor_read_command *read,
+ u8 num_mode_clocks,
+ u8 num_wait_states,
+ u8 opcode,
+ enum spi_nor_protocol proto);
void spi_nor_set_pp_settings(struct spi_nor_pp_command *pp, u8 opcode,
enum spi_nor_protocol proto);

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 26814a1..85a8509 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -50,6 +50,12 @@ struct xspi_dummy_cycles {
u8 shift; /* Bit shift */
};

+/* Basic Flash Parameter Table 20th DWORD, Max operation speed of device */
+struct octal_max_speed {
+ u8 idx; /* Bits value */
+ u16 hz; /* MHz */
+};
+
struct sfdp_header {
u32 signature; /* Ox50444653U <=> "SFDP" */
u8 minor;
@@ -423,6 +429,64 @@ static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
}

/**
+ * spi_nor_cfg_reg2_octal_enable() - enable octal mode by CFG Reg 2
+ * @nor: pointer to a 'struct spi_nor'
+ * @dtr: true for DTR mode
+ *
+ * The Basic Flash Parameter Table 19th DWORD defined the Octal mode
+ * enable sequences by writing CFG Reg 2, 1: 8S-8S-8S, 2: 8D-8D-8D.
+ *
+ * Set Octal mode read/PP command, protocol and read dummy cycles.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_cfg_reg2_octal_enable(struct spi_nor *nor, bool dtr)
+{
+ struct spi_nor_flash_parameter *p = nor->params;
+ int ret;
+ u8 cr2 = 0;
+
+ if (!(nor->spimem->spi->mode & (SPI_RX_OCTAL | SPI_TX_OCTAL)))
+ return -ENOTSUPP;
+
+ if (dtr) {
+ cr2 |= CR2_OCTAL_MODE_DTR;
+ ret = spi_nor_write_cr2(nor, CR2_OCTAL_MODE_ADDR, &cr2);
+ if (ret)
+ return ret;
+
+ /* Octal 8D-8D-8D mode */
+ p->hwcaps.mask |= SNOR_HWCAPS_OPI_FULL_DTR;
+ spi_nor_set_read_settings(&p->reads[SNOR_CMD_READ_8_8_8_DTR],
+ 0, p->dummy_cycles,
+ p->dtr_read_cmd,
+ SNOR_PROTO_8_8_8_DTR);
+
+ spi_nor_set_pp_settings(&p->page_programs
+ [SNOR_CMD_PP_8_8_8_DTR],
+ SPINOR_OP_PP_8D_8D_8D,
+ SNOR_PROTO_8_8_8_DTR);
+ } else {
+ cr2 |= CR2_OCTAL_MODE_STR;
+ ret = spi_nor_write_cr2(nor, CR2_OCTAL_MODE_ADDR, &cr2);
+ if (ret)
+ return ret;
+
+ /* Octal 8S-8S-8S mode */
+ p->hwcaps.mask |= SNOR_HWCAPS_OPI_FULL_STR;
+ spi_nor_set_read_settings(&p->reads[SNOR_CMD_READ_8_8_8],
+ 0, p->dummy_cycles,
+ SPINOR_OP_READ_8_8_8,
+ SNOR_PROTO_8_8_8);
+
+ spi_nor_set_pp_settings(&p->page_programs[SNOR_CMD_PP_8_8_8],
+ SPINOR_OP_PP_8_8_8,
+ SNOR_PROTO_8_8_8);
+ }
+ return 0;
+}
+
+/**
* 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
@@ -464,6 +528,22 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
u32 addr;
u16 half;
u8 erase_mask;
+ static const struct octal_max_speed max_hz[] = {
+ /* Bits value, MHz */
+ { 0x0c, 400 },
+ { 0x0b, 333 },
+ { 0x0a, 266 },
+ { 0x09, 250 },
+ { 0x08, 200 },
+ { 0x07, 166 },
+ { 0x06, 133 },
+ { 0x05, 100 },
+ { 0x04, 80 },
+ { 0x03, 66 },
+ { 0x02, 50 },
+ { 0x01, 33 },
+ };
+ u8 idx;

/* JESD216 Basic Flash Parameter Table length is at least 9 DWORDs. */
if (bfpt_header->length < BFPT_DWORD_MAX_JESD216)
@@ -628,6 +708,42 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
return -EINVAL;
}

+ /* 8D-8D-8D command extension. */
+ switch (bfpt.dwords[BFPT_DWORD(18)] & BFPT_DWORD18_CMD_EXT_MASK) {
+ case BFPT_DWORD18_CMD_EXT_REP:
+ nor->ext_cmd_mode = EXT_CMD_IS_REPEAT;
+ break;
+ case BFPT_DWORD18_CMD_EXT_INV:
+ nor->ext_cmd_mode = EXT_CMD_IS_INVERSE;
+ break;
+ default:
+ break;
+ }
+
+ /* Octal mode enable sequences. */
+ switch (bfpt.dwords[BFPT_DWORD(19)] & BFPT_DWORD19_OCTAL_SEQ_MASK) {
+ case BFPT_DWORD19_TWO_INST:
+ break;
+ case BFPT_DWORD19_CFG_REG2:
+ params->xspi_enable = spi_nor_cfg_reg2_octal_enable;
+ break;
+ default:
+ break;
+ }
+
+ /* Octal mode max speed */
+ idx = max(FIELD_GET(BFPT_DWORD20_OCTAL_DTR_MAX_SPEED,
+ bfpt.dwords[BFPT_DWORD(20)]),
+ FIELD_GET(BFPT_DWORD20_OCTAL_STR_MAX_SPEED,
+ bfpt.dwords[BFPT_DWORD(20)]));
+
+ for (i = 0; i < ARRAY_SIZE(max_hz); i++) {
+ if (max_hz[i].idx == idx) {
+ params->octal_max_speed = max_hz[i].hz;
+ break;
+ }
+ }
+
return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params);
}

diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index e0a8ded..7a55b4d 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -10,11 +10,11 @@
/* Basic Flash Parameter Table */

/*
- * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * JESD216 rev D defines a Basic Flash Parameter Table of 20 DWORDs.
* They are indexed from 1 but C arrays are indexed from 0.
*/
#define BFPT_DWORD(i) ((i) - 1)
-#define BFPT_DWORD_MAX 16
+#define BFPT_DWORD_MAX 20

struct sfdp_bfpt {
u32 dwords[BFPT_DWORD_MAX];
@@ -83,6 +83,18 @@ struct sfdp_bfpt {
#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD (0x4UL << 20)
#define BFPT_DWORD15_QER_SR2_BIT1 (0x5UL << 20) /* Spansion */

+#define BFPT_DWORD18_CMD_EXT_MASK GENMASK(30, 29)
+#define BFPT_DWORD18_CMD_EXT_REP (0x0UL << 29) /* Repeat */
+#define BFPT_DWORD18_CMD_EXT_INV (0x1UL << 29) /* Invert */
+
+#define BFPT_DWORD19_OCTAL_SEQ_MASK GENMASK(6, 5)
+#define BFPT_DWORD19_TWO_INST (0x1UL << 5) /* Two Inst. */
+#define BFPT_DWORD19_CFG_REG2 (0x2UL << 5) /* CFG Reg2 */
+
+#define BFPT_DWORD20_OCTAL_MAX_SPEED_MASK GENMASK(31, 16)
+#define BFPT_DWORD20_OCTAL_DTR_MAX_SPEED GENMASK(31, 28)
+#define BFPT_DWORD20_OCTAL_STR_MAX_SPEED GENMASK(19, 16)
+
struct sfdp_parameter_header {
u8 id_lsb;
u8 minor;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 1e2af0e..778b52e 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -68,6 +68,9 @@
#define SPINOR_OP_BE_4K_4B 0x21 /* Erase 4KiB block */
#define SPINOR_OP_BE_32K_4B 0x5c /* Erase 32KiB block */
#define SPINOR_OP_SE_4B 0xdc /* Sector erase (usually 64KiB) */
+#define SPINOR_OP_READ_8_8_8 SPINOR_OP_READ_1_4_4_4B
+#define SPINOR_OP_PP_8_8_8 SPINOR_OP_PP_4B
+#define SPINOR_OP_PP_8D_8D_8D SPINOR_OP_PP_4B

/* Double Transfer Rate opcodes - defined in JEDEC JESD216B. */
#define SPINOR_OP_READ_1_1_1_DTR 0x0d
@@ -91,7 +94,6 @@
#define XSR_PAGESIZE BIT(0) /* Page size in Po2 or Linear */
#define XSR_RDY BIT(7) /* Ready */

-
/* Used for Macronix and Winbond flashes. */
#define SPINOR_OP_EN4B 0xb7 /* Enter 4-byte mode */
#define SPINOR_OP_EX4B 0xe9 /* Exit 4-byte mode */
@@ -137,6 +139,13 @@
#define SR2_QUAD_EN_BIT1 BIT(1)
#define SR2_QUAD_EN_BIT7 BIT(7)

+/* Configuration Register 2, JEDEC216-D */
+#define CR2_OCTAL_MODE_ADDR 0x0
+#define CR2_OCTAL_MODE_MASK GENMASK(1, 0)
+#define CR2_SPI_MODE 0
+#define CR2_OCTAL_MODE_STR 1
+#define CR2_OCTAL_MODE_DTR 2
+
/* Supported SPI protocols */
#define SNOR_PROTO_INST_MASK GENMASK(23, 16)
#define SNOR_PROTO_INST_SHIFT 16
@@ -157,15 +166,21 @@
SNOR_PROTO_DATA_MASK)

#define SNOR_PROTO_IS_DTR BIT(24) /* Double Transfer Rate */
+#define SNOR_PROTO_IS_FULL_DTR BIT(25) /* Full Double Transfer Rate */

#define SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits) \
(SNOR_PROTO_INST(_inst_nbits) | \
SNOR_PROTO_ADDR(_addr_nbits) | \
SNOR_PROTO_DATA(_data_nbits))
+
#define SNOR_PROTO_DTR(_inst_nbits, _addr_nbits, _data_nbits) \
(SNOR_PROTO_IS_DTR | \
SNOR_PROTO_STR(_inst_nbits, _addr_nbits, _data_nbits))

+#define SNOR_PROTO_8D_8D_8D(_inst_nbits, _addr_nbits, _data_nbits) \
+ (SNOR_PROTO_IS_FULL_DTR | \
+ SNOR_PROTO_DTR(_inst_nbits, _addr_nbits, _data_nbits))
+
enum spi_nor_protocol {
SNOR_PROTO_1_1_1 = SNOR_PROTO_STR(1, 1, 1),
SNOR_PROTO_1_1_2 = SNOR_PROTO_STR(1, 1, 2),
@@ -182,6 +197,7 @@ enum spi_nor_protocol {
SNOR_PROTO_1_2_2_DTR = SNOR_PROTO_DTR(1, 2, 2),
SNOR_PROTO_1_4_4_DTR = SNOR_PROTO_DTR(1, 4, 4),
SNOR_PROTO_1_8_8_DTR = SNOR_PROTO_DTR(1, 8, 8),
+ SNOR_PROTO_8_8_8_DTR = SNOR_PROTO_8D_8D_8D(8, 8, 8),
};

static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
@@ -189,6 +205,16 @@ static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
return !!(proto & SNOR_PROTO_IS_DTR);
}

+static inline bool spi_nor_protocol_is_8_8_8(enum spi_nor_protocol proto)
+{
+ return !!(proto & SNOR_PROTO_8_8_8);
+}
+
+static inline bool spi_nor_protocol_is_8D_8D_8D(enum spi_nor_protocol proto)
+{
+ return !!(proto & SNOR_PROTO_IS_FULL_DTR);
+}
+
static inline u8 spi_nor_get_protocol_inst_nbits(enum spi_nor_protocol proto)
{
return ((unsigned long)(proto & SNOR_PROTO_INST_MASK)) >>
@@ -228,7 +254,7 @@ struct spi_nor_hwcaps {
* then Quad SPI protocols before Dual SPI protocols, Fast Read and lastly
* (Slow) Read.
*/
-#define SNOR_HWCAPS_READ_MASK GENMASK(14, 0)
+#define SNOR_HWCAPS_READ_MASK GENMASK(15, 0)
#define SNOR_HWCAPS_READ BIT(0)
#define SNOR_HWCAPS_READ_FAST BIT(1)
#define SNOR_HWCAPS_READ_1_1_1_DTR BIT(2)
@@ -250,6 +276,7 @@ struct spi_nor_hwcaps {
#define SNOR_HWCAPS_READ_1_8_8 BIT(12)
#define SNOR_HWCAPS_READ_8_8_8 BIT(13)
#define SNOR_HWCAPS_READ_1_8_8_DTR BIT(14)
+#define SNOR_HWCAPS_READ_8_8_8_DTR BIT(15)

/*
* Page Program capabilities.
@@ -260,7 +287,7 @@ struct spi_nor_hwcaps {
* JEDEC/SFDP standard to define them. Also at this moment no SPI flash memory
* implements such commands.
*/
-#define SNOR_HWCAPS_PP_MASK GENMASK(22, 16)
+#define SNOR_HWCAPS_PP_MASK GENMASK(23, 16)
#define SNOR_HWCAPS_PP BIT(16)

#define SNOR_HWCAPS_PP_QUAD GENMASK(19, 17)
@@ -272,6 +299,13 @@ struct spi_nor_hwcaps {
#define SNOR_HWCAPS_PP_1_1_8 BIT(20)
#define SNOR_HWCAPS_PP_1_8_8 BIT(21)
#define SNOR_HWCAPS_PP_8_8_8 BIT(22)
+#define SNOR_HWCAPS_PP_8_8_8_DTR BIT(23)
+
+#define SNOR_HWCAPS_OPI_FULL_STR (SNOR_HWCAPS_READ_8_8_8 | \
+ SNOR_HWCAPS_PP_8_8_8)
+
+#define SNOR_HWCAPS_OPI_FULL_DTR (SNOR_HWCAPS_READ_8_8_8_DTR | \
+ SNOR_HWCAPS_PP_8_8_8_DTR)

#define SNOR_HWCAPS_X_X_X (SNOR_HWCAPS_READ_2_2_2 | \
SNOR_HWCAPS_READ_4_4_4 | \
@@ -282,7 +316,9 @@ struct spi_nor_hwcaps {
#define SNOR_HWCAPS_DTR (SNOR_HWCAPS_READ_1_1_1_DTR | \
SNOR_HWCAPS_READ_1_2_2_DTR | \
SNOR_HWCAPS_READ_1_4_4_DTR | \
- SNOR_HWCAPS_READ_1_8_8_DTR)
+ SNOR_HWCAPS_READ_1_8_8_DTR | \
+ SNOR_HWCAPS_READ_8_8_8_DTR | \
+ SNOR_HWCAPS_PP_8_8_8_DTR)

#define SNOR_HWCAPS_ALL (SNOR_HWCAPS_READ_MASK | \
SNOR_HWCAPS_PP_MASK)
@@ -326,6 +362,11 @@ struct spi_nor_controller_ops {
struct spi_nor_manufacturer;
struct spi_nor_flash_parameter;

+enum extension_cmd_mode {
+ EXT_CMD_IS_REPEAT,
+ EXT_CMD_IS_INVERSE,
+};
+
/**
* struct spi_nor - Structure for defining a the SPI NOR layer
* @mtd: point to a mtd_info structure
@@ -335,6 +376,7 @@ struct spi_nor_controller_ops {
* @bouncebuf: bounce buffer used when the buffer passed by the MTD
* layer is not DMA-able
* @bouncebuf_size: size of the bounce buffer
+ * @ext_cmd_mode: xspi extension command mode, 0: repeat, 1:inverse
* @info: spi-nor part JDEC MFR id and other info
* @manufacturer: spi-nor manufacturer
* @page_size: the page size of the SPI NOR
@@ -363,6 +405,7 @@ struct spi_nor {
struct spi_mem *spimem;
u8 *bouncebuf;
size_t bouncebuf_size;
+ enum extension_cmd_mode ext_cmd_mode;
const struct flash_info *info;
const struct spi_nor_manufacturer *manufacturer;
u32 page_size;
--
1.9.1

2020-04-21 07:10:28

[permalink] [raw]

Subject: [PATCH v2 1/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

According to JEDEC216C SPI NORs are using 2 bytes opcodes
when operated in OPI (Octal Peripheral Interface).

Add extension command, command bytes number and dtr fields to
the spi_mem_op struct and make sure all DTR operations are
rejected for now.

Signed-off-by: Boris Brezillon <[email protected]>
Signed-off-by: Mason Yang <[email protected]>
---
drivers/spi/spi-mem.c | 8 +++++++-
include/linux/spi/spi-mem.h | 13 +++++++++++++
2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index adaa0c4..de682c5 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -154,6 +154,12 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
op->data.dir == SPI_MEM_DATA_OUT))
return false;

+ if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
+ return false;
+
+ if (op->cmd.nbytes != 1)
+ return false;
+
return true;
}
EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
@@ -168,7 +174,7 @@ static bool spi_mem_buswidth_is_valid(u8 buswidth)

static int spi_mem_check_op(const struct spi_mem_op *op)
{
- if (!op->cmd.buswidth)
+ if (!op->cmd.buswidth || op->cmd.nbytes < 1 || op->cmd.nbytes > 2)
return -EINVAL;

if ((op->addr.nbytes && !op->addr.buswidth) ||
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index af9ff2f..bf54079 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -17,6 +17,7 @@
{ \
.buswidth = __buswidth, \
.opcode = __opcode, \
+ .nbytes = 1, \
}

#define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth) \
@@ -69,11 +70,15 @@ enum spi_mem_data_dir {

/**
* struct spi_mem_op - describes a SPI memory operation
+ * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid)
* @cmd.buswidth: number of IO lines used to transmit the command
+ * @cmd.dtr: set true to transfer opcode in double transfer rate mode
* @cmd.opcode: operation opcode
+ * @cmd.ext_opcode: extension operation opcode
* @addr.nbytes: number of address bytes to send. Can be zero if the operation
* does not need to send an address
* @addr.buswidth: number of IO lines used to transmit the address cycles
+ * @addr.dtr: set true to transfer address bytes in double transfer rate mode
* @addr.val: address value. This value is always sent MSB first on the bus.
* Note that only @addr.nbytes are taken into account in this
* address value, so users should make sure the value fits in the
@@ -81,34 +86,42 @@ enum spi_mem_data_dir {
* @dummy.nbytes: number of dummy bytes to send after an opcode or address. Can
* be zero if the operation does not require dummy bytes
* @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
+ * @dummy.dtr: set true to transfer dummy bytes in double transfer rate mode
* @data.buswidth: number of IO lanes used to send/receive the data
* @data.dir: direction of the transfer
* @data.nbytes: number of data bytes to send/receive. Can be zero if the
* operation does not involve transferring data
+ * @data.dtr: set true to transfer data bytes in double transfer rate mode
* @data.buf.in: input buffer (must be DMA-able)
* @data.buf.out: output buffer (must be DMA-able)
*/
struct spi_mem_op {
struct {
+ u8 nbytes;
u8 buswidth;
+ bool dtr;
u8 opcode;
+ u8 ext_opcode;
} cmd;

struct {
u8 nbytes;
u8 buswidth;
+ bool dtr;
u64 val;
} addr;

struct {
u8 nbytes;
u8 buswidth;
+ bool dtr;
} dummy;

struct {
u8 buswidth;
enum spi_mem_data_dir dir;
unsigned int nbytes;
+ bool dtr;
union {
void *in;
const void *out;
--
1.9.1

2020-04-21 07:10:42

[permalink] [raw]

Subject: [PATCH v2 4/5] mtd: spi-nor: macronix: Add Octal 8D-8D-8D supports for Macronix mx25uw51245g

Macronix mx25uw51245g is a SPI NOR that supports 1-1-1/8-8-8 mode and
JEDEC216D spec. included BFPT DWORD-18,19, 20 and xSPI profile 1.0 table.

Correct the dummy cycles for various frequency after xSPI 1.0 table parsed.

Enable mx25uw51245g to Octal 8D-8D-8D mode by writing CFG Reg2 in the
late initialization of default flash parameters spi_nor_late_init_params();

Signed-off-by: Mason Yang <[email protected]>
---
drivers/mtd/spi-nor/macronix.c | 41 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index ab0f963..46bcfe5 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,43 @@

#include "core.h"

+#define MXIC_CR2_DUMMY_SET_ADDR 0x300
+
+/* Fixup the dummy cycles after SFDP xSPI 1.0 parsed */
+static void mx25uw51245g_post_sfdp_fixups(struct spi_nor *nor)
+{
+ struct spi_nor_flash_parameter *params = nor->params;
+ int ret;
+ u8 rdc, wdc;
+
+ ret = spi_nor_read_cr2(nor, MXIC_CR2_DUMMY_SET_ADDR, &rdc);
+ if (ret)
+ return;
+
+ /* Refer to dummy cycle and frequency table(MHz) */
+ switch (params->dummy_cycles) {
+ case 10: /* 10 dummy cycles for 104 MHz */
+ wdc = 5;
+ break;
+ case 12: /* 12 dummy cycles for 133 MHz */
+ wdc = 4;
+ break;
+ case 16: /* 16 dummy cycles for 166 MHz */
+ wdc = 2;
+ break;
+ case 20: /* 20 dummy cycles for 200 MHz */
+ default:
+ wdc = 0;
+ }
+
+ if (rdc != wdc)
+ spi_nor_write_cr2(nor, MXIC_CR2_DUMMY_SET_ADDR, &wdc);
+}
+
+static struct spi_nor_fixups mx25uw51245g_fixups = {
+ .post_sfdp = mx25uw51245g_post_sfdp_fixups,
+};
+
static int
mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
const struct sfdp_parameter_header *bfpt_header,
@@ -78,6 +115,10 @@
SPI_NOR_QUAD_READ) },
{ "mx66l1g55g", INFO(0xc2261b, 0, 64 * 1024, 2048,
SPI_NOR_QUAD_READ) },
+ { "mx25uw51245g", INFO(0xc2813a, 0, 64 * 1024, 1024,
+ SECT_4K | SPI_NOR_4B_OPCODES |
+ SPI_NOR_OCTAL_RD_WR)
+ .fixups = &mx25uw51245g_fixups },
};

static void macronix_default_init(struct spi_nor *nor)
--
1.9.1

2020-04-21 07:12:44

[permalink] [raw]

Subject: [PATCH v2 5/5] spi: mxic: Patch for Octal 8D-8D-8D mode support

Driver patch for Octal 8S-8S-8S and 8D-8D-8D mode support.

Signed-off-by: Mason Yang <[email protected]>
---
drivers/spi/spi-mxic.c | 101 ++++++++++++++++++++++++++++++++++---------------
1 file changed, 70 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 69491f3..8054f2c 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -280,10 +280,58 @@ static void mxic_spi_hw_init(struct mxic_spi *mxic)
mxic->regs + HC_CFG);
}

+static u32 mxic_spi_mem_prep_op_cfg(const struct spi_mem_op *op)
+{
+ u32 cfg = OP_CMD_BYTES(op->cmd.nbytes) |
+ OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
+ (op->cmd.dtr ? OP_CMD_DDR : 0);
+
+ if (op->addr.nbytes)
+ cfg |= OP_ADDR_BYTES(op->addr.nbytes) |
+ OP_ADDR_BUSW(fls(op->addr.buswidth) - 1) |
+ (op->addr.dtr ? OP_ADDR_DDR : 0);
+
+ if (op->dummy.nbytes)
+ cfg |= OP_DUMMY_CYC(op->dummy.nbytes);
+
+ if (op->data.nbytes) {
+ cfg |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
+ (op->data.dtr ? OP_DATA_DDR : 0);
+ if (op->data.dir == SPI_MEM_DATA_IN) {
+ cfg |= OP_READ;
+ if (op->data.dtr == OP_DATA_DDR)
+ cfg |= OP_DQS_EN;
+ }
+ }
+
+ return cfg;
+}
+
+static void mxic_spi_set_hc_cfg(struct spi_device *spi, u32 flags)
+{
+ struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
+ int nio = 1;
+
+ if (spi->mode & (SPI_RX_OCTAL | SPI_TX_OCTAL))
+ nio = 8;
+ else if (spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
+ nio = 4;
+ else if (spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
+ nio = 2;
+
+ writel(flags | HC_CFG_NIO(nio) |
+ HC_CFG_TYPE(spi->chip_select, HC_CFG_TYPE_SPI_NOR) |
+ HC_CFG_SLV_ACT(spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1),
+ mxic->regs + HC_CFG);
+}
+
static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
void *rxbuf, unsigned int len)
{
unsigned int pos = 0;
+ bool dtr_enabled;
+
+ dtr_enabled = (readl(mxic->regs + SS_CTRL(0)) & OP_DATA_DDR);

while (pos < len) {
unsigned int nbytes = len - pos;
@@ -302,6 +350,9 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
if (ret)
return ret;

+ if (dtr_enabled && len & 0x1)
+ nbytes++;
+
writel(data, mxic->regs + TXD(nbytes % 4));

if (rxbuf) {
@@ -319,6 +370,8 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,

data = readl(mxic->regs + RXD);
data >>= (8 * (4 - nbytes));
+ if (dtr_enabled && len & 0x1)
+ nbytes++;
memcpy(rxbuf + pos, &data, nbytes);
WARN_ON(readl(mxic->regs + INT_STS) & INT_RX_NOT_EMPTY);
} else {
@@ -335,8 +388,8 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
- if (op->data.buswidth > 4 || op->addr.buswidth > 4 ||
- op->dummy.buswidth > 4 || op->cmd.buswidth > 4)
+ if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
+ op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
return false;

if (op->data.nbytes && op->dummy.nbytes &&
@@ -346,6 +399,9 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
if (op->addr.nbytes > 7)
return false;

+ if (op->cmd.buswidth == 8 && op->cmd.nbytes == 2)
+ return true;
+
return spi_mem_default_supports_op(mem, op);
}

@@ -353,47 +409,29 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
const struct spi_mem_op *op)
{
struct mxic_spi *mxic = spi_master_get_devdata(mem->spi->master);
- int nio = 1, i, ret;
- u32 ss_ctrl;
+ int i, ret;
u8 addr[8];
+ u8 cmd[2];

ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
if (ret)
return ret;

- if (mem->spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
- nio = 4;
- else if (mem->spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
- nio = 2;
+ mxic_spi_set_hc_cfg(mem->spi, HC_CFG_MAN_CS_EN);

- writel(HC_CFG_NIO(nio) |
- HC_CFG_TYPE(mem->spi->chip_select, HC_CFG_TYPE_SPI_NOR) |
- HC_CFG_SLV_ACT(mem->spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1) |
- HC_CFG_MAN_CS_EN,
- mxic->regs + HC_CFG);
writel(HC_EN_BIT, mxic->regs + HC_EN);

- ss_ctrl = OP_CMD_BYTES(1) | OP_CMD_BUSW(fls(op->cmd.buswidth) - 1);
-
- if (op->addr.nbytes)
- ss_ctrl |= OP_ADDR_BYTES(op->addr.nbytes) |
- OP_ADDR_BUSW(fls(op->addr.buswidth) - 1);
-
- if (op->dummy.nbytes)
- ss_ctrl |= OP_DUMMY_CYC(op->dummy.nbytes);
-
- if (op->data.nbytes) {
- ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1);
- if (op->data.dir == SPI_MEM_DATA_IN)
- ss_ctrl |= OP_READ;
- }
-
- writel(ss_ctrl, mxic->regs + SS_CTRL(mem->spi->chip_select));
+ writel(mxic_spi_mem_prep_op_cfg(op),
+ mxic->regs + SS_CTRL(mem->spi->chip_select));

writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
mxic->regs + HC_CFG);

- ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
+ cmd[0] = op->cmd.opcode;
+ if (op->cmd.nbytes == 2)
+ cmd[1] = op->cmd.ext_opcode;
+
+ ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
if (ret)
goto out;

@@ -566,7 +604,8 @@ static int mxic_spi_probe(struct platform_device *pdev)
master->bits_per_word_mask = SPI_BPW_MASK(8);
master->mode_bits = SPI_CPOL | SPI_CPHA |
SPI_RX_DUAL | SPI_TX_DUAL |
- SPI_RX_QUAD | SPI_TX_QUAD;
+ SPI_RX_QUAD | SPI_TX_QUAD |
+ SPI_RX_OCTAL | SPI_TX_OCTAL;

mxic_spi_hw_init(mxic);

--
1.9.1

2020-04-21 07:25:17

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

+Pratyush who's working on a similar patchet [1].

Hello Mason,

On Tue, 21 Apr 2020 14:39:42 +0800
Mason Yang <[email protected]> wrote:

> Hello,
>
> This is repost of patchset from Boris Brezillon's
> [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].

I only quickly went through the patches you sent and saying it's a
repost of the RFC is a bit of a lie. You completely ignored the state
tracking I was trying to do to avoid leaving the flash in 8D mode when
suspending/resetting the board, and I think that part is crucial. If I
remember correctly, we already had this discussion so I must say I'm a
bit disappointed.

Can you sync with Pratyush? I think his series [1] is better in that it
tries to restore the flash in single-SPI mode before suspend (it's
missing the shutdown case, but that can be easily added I think). Of
course that'd be even better to have proper state tracking at the SPI
NOR level.

Regards,

Boris

[1]https://lkml.org/lkml/2020/3/13/659

2020-04-21 09:37:28

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On 21/04/20 12:53 pm, Boris Brezillon wrote:
> +Pratyush who's working on a similar patchet [1].
>
> Hello Mason,
>
> On Tue, 21 Apr 2020 14:39:42 +0800
> Mason Yang <[email protected]> wrote:
>
>> Hello,
>>
>> This is repost of patchset from Boris Brezillon's
>> [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
>
> I only quickly went through the patches you sent and saying it's a
> repost of the RFC is a bit of a lie. You completely ignored the state
> tracking I was trying to do to avoid leaving the flash in 8D mode when
> suspending/resetting the board, and I think that part is crucial. If I
> remember correctly, we already had this discussion so I must say I'm a
> bit disappointed.
>
> Can you sync with Pratyush? I think his series [1] is better in that it
> tries to restore the flash in single-SPI mode before suspend (it's
> missing the shutdown case, but that can be easily added I think). Of
> course that'd be even better to have proper state tracking at the SPI
> NOR level.
>

[1] does soft reset on shutdown which should put it to reset default
state of 1S-1S-1S mode (if thats the POR default)

But, there is still one open question now that we are considering
supporting stateful modes:

What to do with flashes that power up in 8D mode either due to factory
defaults or if 8D mode NV bit is set? Do we say SPI NOR framework won't
support such flashes?
Auto discovery of such flashes is quite difficult as different flashes
use different protocols for RDID cmd in 8D mode (address phase may or
may not be present, dummy cycles vary etc) is almost impossible w/o any
hint passed to the driver?

> Regards,
>
> Boris
>
> [1]https://lkml.org/lkml/2020/3/13/659
>

2020-04-21 12:19:48

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On Tue, 21 Apr 2020 15:05:08 +0530
Vignesh Raghavendra <[email protected]> wrote:

> On 21/04/20 12:53 pm, Boris Brezillon wrote:
> > +Pratyush who's working on a similar patchet [1].
> >
> > Hello Mason,
> >
> > On Tue, 21 Apr 2020 14:39:42 +0800
> > Mason Yang <[email protected]> wrote:
> >
> >> Hello,
> >>
> >> This is repost of patchset from Boris Brezillon's
> >> [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> >
> > I only quickly went through the patches you sent and saying it's a
> > repost of the RFC is a bit of a lie. You completely ignored the state
> > tracking I was trying to do to avoid leaving the flash in 8D mode when
> > suspending/resetting the board, and I think that part is crucial. If I
> > remember correctly, we already had this discussion so I must say I'm a
> > bit disappointed.
> >
> > Can you sync with Pratyush? I think his series [1] is better in that it
> > tries to restore the flash in single-SPI mode before suspend (it's
> > missing the shutdown case, but that can be easily added I think). Of
> > course that'd be even better to have proper state tracking at the SPI
> > NOR level.
> >
>
> [1] does soft reset on shutdown which should put it to reset default
> state of 1S-1S-1S mode (if thats the POR default)

Oh ok, looks like I didn't read the patch series carefully enough.

>
> But, there is still one open question now that we are considering
> supporting stateful modes:
>
> What to do with flashes that power up in 8D mode either due to factory
> defaults or if 8D mode NV bit is set? Do we say SPI NOR framework won't
> support such flashes?
> Auto discovery of such flashes is quite difficult as different flashes
> use different protocols for RDID cmd in 8D mode (address phase may or
> may not be present, dummy cycles vary etc) is almost impossible w/o any
> hint passed to the driver?

I don't know yet. Looks like we'll have to pass the part-id and default
mode for those flashes (part-name being a part-specific compatible, and
boot-up mode being an extra property). But maybe we can ignore that for
now and focus on flashes booting in single SPI mode first :P.

2020-04-24 15:47:18

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 5/5] spi: mxic: Patch for Octal 8D-8D-8D mode support

Hi Mason,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on spi/for-next]
[also build test WARNING on linus/master v5.7-rc2 next-20200424]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url: https://github.com/0day-ci/linux/commits/Mason-Yang/mtd-spi-nor-Add-support-for-Octal-8D-8D-8D-mode/20200423-041224
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
config: x86_64-randconfig-a003-20200424 (attached as .config)
compiler: gcc-6 (Debian 6.3.0-18+deb9u1) 6.3.0 20170516
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/spi/spi-mxic.c: In function 'mxic_spi_mem_prep_op_cfg':
>> drivers/spi/spi-mxic.c:302:21: warning: comparison of constant '256ul' with boolean expression is always false [-Wbool-compare]
if (op->data.dtr == OP_DATA_DDR)
^~

vim +/256ul +302 drivers/spi/spi-mxic.c

282
283 static u32 mxic_spi_mem_prep_op_cfg(const struct spi_mem_op *op)
284 {
285 u32 cfg = OP_CMD_BYTES(op->cmd.nbytes) |
286 OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
287 (op->cmd.dtr ? OP_CMD_DDR : 0);
288
289 if (op->addr.nbytes)
290 cfg |= OP_ADDR_BYTES(op->addr.nbytes) |
291 OP_ADDR_BUSW(fls(op->addr.buswidth) - 1) |
292 (op->addr.dtr ? OP_ADDR_DDR : 0);
293
294 if (op->dummy.nbytes)
295 cfg |= OP_DUMMY_CYC(op->dummy.nbytes);
296
297 if (op->data.nbytes) {
298 cfg |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
299 (op->data.dtr ? OP_DATA_DDR : 0);
300 if (op->data.dir == SPI_MEM_DATA_IN) {
301 cfg |= OP_READ;
> 302 if (op->data.dtr == OP_DATA_DDR)
303 cfg |= OP_DQS_EN;
304 }
305 }
306
307 return cfg;
308 }
309

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (2.32 kB)
.config.gz (31.52 kB)
Download all attachments

2020-04-27 18:00:06

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On 21/04/20 09:23AM, Boris Brezillon wrote:
> +Pratyush who's working on a similar patchet [1].
>
> Hello Mason,
>
> On Tue, 21 Apr 2020 14:39:42 +0800
> Mason Yang <[email protected]> wrote:
>
> > Hello,
> >
> > This is repost of patchset from Boris Brezillon's
> > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
>
> I only quickly went through the patches you sent and saying it's a
> repost of the RFC is a bit of a lie. You completely ignored the state
> tracking I was trying to do to avoid leaving the flash in 8D mode when
> suspending/resetting the board, and I think that part is crucial. If I
> remember correctly, we already had this discussion so I must say I'm a
> bit disappointed.
>
> Can you sync with Pratyush? I think his series [1] is better in that it
> tries to restore the flash in single-SPI mode before suspend (it's
> missing the shutdown case, but that can be easily added I think). Of
> course that'd be even better to have proper state tracking at the SPI
> NOR level.

Hi Mason,

I posted a re-roll of my series here [0]. Could you please base your
changes on top of it? Let me know if the series is missing something you
need.

[0] https://lore.kernel.org/linux-mtd/[email protected]/

--
Regards,
Pratyush Yadav

2020-04-28 06:20:28

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Pratyush,

> > On Tue, 21 Apr 2020 14:39:42 +0800
> > Mason Yang <[email protected]> wrote:
> >
> > > Hello,
> > >
> > > This is repost of patchset from Boris Brezillon's
> > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> >
> > I only quickly went through the patches you sent and saying it's a
> > repost of the RFC is a bit of a lie. You completely ignored the state
> > tracking I was trying to do to avoid leaving the flash in 8D mode when
> > suspending/resetting the board, and I think that part is crucial. If I
> > remember correctly, we already had this discussion so I must say I'm a
> > bit disappointed.
> >
> > Can you sync with Pratyush? I think his series [1] is better in that
it
> > tries to restore the flash in single-SPI mode before suspend (it's
> > missing the shutdown case, but that can be easily added I think). Of
> > course that'd be even better to have proper state tracking at the SPI
> > NOR level.
>
> Hi Mason,
>
> I posted a re-roll of my series here [0]. Could you please base your
> changes on top of it? Let me know if the series is missing something you

> need.
>
> [0]
https://lore.kernel.org/linux-mtd/[email protected]/

Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI profile
1.0,
and it comply with BFPT DWORD-19, octal mode enable sequences by write CFG
Reg2
with instruction 0x72. Therefore, I can't apply your patches.

I quickly went through your patches but can't reply them in each your
patches.

i.e,.
1) [v4,03/16] spi: spi-mem: allow specifying a command's extension

- u8 opcode;
+ u16 opcode;

big/little Endian issue, right?
why not just u8 ext_opcode;
No any impact for exist code and actually only xSPI device use extension
command.

2) [v4,08/16] mtd: spi-nor: parse xSPI Profile 1.0 table

need extract more data from xSPI profile 1.0 table and no other specific
setting.

3) [v4,11/16] mtd: spi-nor: enable octal DTR mode when possible

+static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+ int ret;
+
+ if (!nor->params->octal_dtr_enable)
+ return 0;
+
+ if (!(spi_nor_get_protocol_width(nor->read_proto) == 8 ||
+ spi_nor_get_protocol_width(nor->write_proto) == 8))
+ return 0;
+
+ ret = nor->params->octal_dtr_enable(nor, enable);
+ if (ret)
+ return ret;
+
+ if (enable)
+ nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
+ else
+ nor->reg_proto = SNOR_PROTO_1_1_1;
+
+ return 0;
+}
+

it seems you enable device in Octal mode after SPI-NOR Framework is
already
in Octal protocol.
Driver should set device by SPI 1-1-1 mode to enter Octal mode and then
setup
Read/PP command and protocol by spi_nor_set_read/pp_setting() for Octal
mode,
right ?

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-04-28 06:37:10

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On Tue, 28 Apr 2020 14:14:31 +0800
[email protected] wrote:

> Hi Pratyush,
>
> > > On Tue, 21 Apr 2020 14:39:42 +0800
> > > Mason Yang <[email protected]> wrote:
> > >
> > > > Hello,
> > > >
> > > > This is repost of patchset from Boris Brezillon's
> > > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> > >
> > > I only quickly went through the patches you sent and saying it's a
> > > repost of the RFC is a bit of a lie. You completely ignored the state
> > > tracking I was trying to do to avoid leaving the flash in 8D mode when
> > > suspending/resetting the board, and I think that part is crucial. If I
> > > remember correctly, we already had this discussion so I must say I'm a
> > > bit disappointed.
> > >
> > > Can you sync with Pratyush? I think his series [1] is better in that
> it
> > > tries to restore the flash in single-SPI mode before suspend (it's
> > > missing the shutdown case, but that can be easily added I think). Of
> > > course that'd be even better to have proper state tracking at the SPI
> > > NOR level.
> >
> > Hi Mason,
> >
> > I posted a re-roll of my series here [0]. Could you please base your
> > changes on top of it? Let me know if the series is missing something you
>
> > need.
> >
> > [0]
> https://lore.kernel.org/linux-mtd/[email protected]/
>
>
> Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI profile
> 1.0,
> and it comply with BFPT DWORD-19, octal mode enable sequences by write CFG
> Reg2
> with instruction 0x72. Therefore, I can't apply your patches.
>
> I quickly went through your patches but can't reply them in each your
> patches.

Why?!! Aren't you registered to the MTD mailing list? Sorry but having
reviews outside of the original thread is far from ideal. Please find a
way to reply to the original patchset.

>
> i.e,.
> 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
>
> - u8 opcode;
> + u16 opcode;
>
> big/little Endian issue, right?

There's no endianness issue since it's SPI controller responsibility to
interpret this field.

> why not just u8 ext_opcode;

Because I see the ext_ext_code comimg, and it's also more consistent
with the addr field if we use an u16 and a number of cmd cycles.

> No any impact for exist code and actually only xSPI device use extension
> command.

And extending the opcode field to an u16 has no impact either.

2020-04-28 08:38:03

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On 28/04/20 08:34AM, Boris Brezillon wrote:
> On Tue, 28 Apr 2020 14:14:31 +0800
> [email protected] wrote:
> >
> > I quickly went through your patches but can't reply them in each
> > your patches.
>
> Why?!! Aren't you registered to the MTD mailing list? Sorry but having
> reviews outside of the original thread is far from ideal. Please find a
> way to reply to the original patchset.

Yes, inline replies to the original patchset would be nice.

FWIW, I'm not subscribed to the list either. I use the NNTP server at
nntp.lore.kernel.org, and the newsgroup org.infradead.lists.linux-mtd to
read and reply to the list. Most popular email clients should have NNTP
support. I use neomutt, but AFAIK, Thunderbird and gnus also have NNTP
support.

--
Regards,
Pratyush Yadav

2020-04-28 08:56:39

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Mason,

On 28/04/20 02:14PM, [email protected] wrote:
>
> Hi Pratyush,
>
> > > On Tue, 21 Apr 2020 14:39:42 +0800
> > > Mason Yang <[email protected]> wrote:
> > >
> > > > Hello,
> > > >
> > > > This is repost of patchset from Boris Brezillon's
> > > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> > >
> > > I only quickly went through the patches you sent and saying it's a
> > > repost of the RFC is a bit of a lie. You completely ignored the state
> > > tracking I was trying to do to avoid leaving the flash in 8D mode when
> > > suspending/resetting the board, and I think that part is crucial. If I
> > > remember correctly, we already had this discussion so I must say I'm a
> > > bit disappointed.
> > >
> > > Can you sync with Pratyush? I think his series [1] is better in that
> it
> > > tries to restore the flash in single-SPI mode before suspend (it's
> > > missing the shutdown case, but that can be easily added I think). Of
> > > course that'd be even better to have proper state tracking at the SPI
> > > NOR level.
> >
> > Hi Mason,
> >
> > I posted a re-roll of my series here [0]. Could you please base your
> > changes on top of it? Let me know if the series is missing something you
>
> > need.
> >
> > [0]
> https://lore.kernel.org/linux-mtd/[email protected]/
>
>
> Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI profile
> 1.0,
> and it comply with BFPT DWORD-19, octal mode enable sequences by write CFG
> Reg2
> with instruction 0x72. Therefore, I can't apply your patches.

I didn't mean apply my patches directly. I meant more along the lines of
edit your patches to work on top of my series. It should be as easy as
adding your flash's fixup hooks and its octal DTR enable hook, but if my
series is missing something you need (like complete Profile 1.0 parsing,
which I left out because I wanted to be conservative and didn't see any
immediate use-case for us), let me know, and we can work together to
address it.

> I quickly went through your patches but can't reply them in each your
> patches.
>
> i.e,.
> 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
>
> - u8 opcode;
> + u16 opcode;
>
> big/little Endian issue, right?
> why not just u8 ext_opcode;
> No any impact for exist code and actually only xSPI device use extension
> command.

Boris already explained the reasoning behind it.

> 2) [v4,08/16] mtd: spi-nor: parse xSPI Profile 1.0 table
>
> need extract more data from xSPI profile 1.0 table and no other specific
> setting.

Not sure what you mean by "no other specific setting".

> 3) [v4,11/16] mtd: spi-nor: enable octal DTR mode when possible
>
> +static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
> +{
> + int ret;
> +
> + if (!nor->params->octal_dtr_enable)
> + return 0;
> +
> + if (!(spi_nor_get_protocol_width(nor->read_proto) == 8 ||
> + spi_nor_get_protocol_width(nor->write_proto) == 8))
> + return 0;
> +
> + ret = nor->params->octal_dtr_enable(nor, enable);
> + if (ret)
> + return ret;
> +
> + if (enable)
> + nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
> + else
> + nor->reg_proto = SNOR_PROTO_1_1_1;
> +
> + return 0;
> +}
> +
>
> it seems you enable device in Octal mode after SPI-NOR Framework is
> already
> in Octal protocol.
> Driver should set device by SPI 1-1-1 mode to enter Octal mode and then
> setup
> Read/PP command and protocol by spi_nor_set_read/pp_setting() for Octal
> mode,
> right ?

No. We need to setup Read and PP settings first. The overall flow is
that we first run spi_nor_setup(). This will perform a "negotiation"
between the controller and the flash to find out a common protocol they
both support, and then change to that protocol in spi_nor_init(). Even
if the flash supports octal DTR protocol, we can't use if if the
controller doesn't. That is why we want to first select the protocol in
the framework, and only then change the flash to that protocol.

In case the controller doesn't support 8D-8D-8D protocol, we would want
to use 1S-1S-1S protocol so the flash is at least usable. Changing to 8D
mode before finding this out would make the flash unusable.

--
Regards,
Pratyush Yadav

2020-04-29 06:02:31

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Boris,

> > > > On Tue, 21 Apr 2020 14:39:42 +0800
> > > > Mason Yang <[email protected]> wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > This is repost of patchset from Boris Brezillon's
> > > > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> > > >
> > > > I only quickly went through the patches you sent and saying it's a
> > > > repost of the RFC is a bit of a lie. You completely ignored the
state
> > > > tracking I was trying to do to avoid leaving the flash in 8D mode
when
> > > > suspending/resetting the board, and I think that part is crucial.
If I
> > > > remember correctly, we already had this discussion so I must say
I'm a
> > > > bit disappointed.
> > > >
> > > > Can you sync with Pratyush? I think his series [1] is better in
that
> > it
> > > > tries to restore the flash in single-SPI mode before suspend (it's
> > > > missing the shutdown case, but that can be easily added I think).
Of
> > > > course that'd be even better to have proper state tracking at the
SPI
> > > > NOR level.
> > >
> > > Hi Mason,
> > >
> > > I posted a re-roll of my series here [0]. Could you please base your

> > > changes on top of it? Let me know if the series is missing something
you
> >
> > > need.
> > >
> > > [0]
> >
https://lore.kernel.org/linux-mtd/[email protected]/
> >
> >
> > Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
profile
> > 1.0,
> > and it comply with BFPT DWORD-19, octal mode enable sequences by write
CFG
> > Reg2
> > with instruction 0x72. Therefore, I can't apply your patches.
> >
> > I quickly went through your patches but can't reply them in each your
> > patches.
>
> Why?!! Aren't you registered to the MTD mailing list? Sorry but having
> reviews outside of the original thread is far from ideal. Please find a
> way to reply to the original patchset.
>
> >
> > i.e,.
> > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> >
> > - u8 opcode;
> > + u16 opcode;
> >
> > big/little Endian issue, right?
>
> There's no endianness issue since it's SPI controller responsibility to
> interpret this field.
>
> > why not just u8 ext_opcode;
>
> Because I see the ext_ext_code comimg, and it's also more consistent
> with the addr field if we use an u16 and a number of cmd cycles.
>
> > No any impact for exist code and actually only xSPI device use
extension
> > command.
>
> And extending the opcode field to an u16 has no impact either.
>

okay, got your point.

thanks for your time & comments.
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-04-29 07:34:27

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Pratyush,

> > > > On Tue, 21 Apr 2020 14:39:42 +0800
> > > > Mason Yang <[email protected]> wrote:
> > > >
> > > > > Hello,
> > > > >
> > > > > This is repost of patchset from Boris Brezillon's
> > > > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> > > >
> > > > I only quickly went through the patches you sent and saying it's a
> > > > repost of the RFC is a bit of a lie. You completely ignored the
state
> > > > tracking I was trying to do to avoid leaving the flash in 8D mode
when
> > > > suspending/resetting the board, and I think that part is crucial.
If I
> > > > remember correctly, we already had this discussion so I must say
I'm a
> > > > bit disappointed.
> > > >
> > > > Can you sync with Pratyush? I think his series [1] is better in
that
> > it
> > > > tries to restore the flash in single-SPI mode before suspend (it's
> > > > missing the shutdown case, but that can be easily added I think).
Of
> > > > course that'd be even better to have proper state tracking at the
SPI
> > > > NOR level.
> > >
> > > Hi Mason,
> > >
> > > I posted a re-roll of my series here [0]. Could you please base your

> > > changes on top of it? Let me know if the series is missing something
you
> >
> > > need.
> > >
> > > [0]
> >
https://lore.kernel.org/linux-mtd/[email protected]/
> >
> >
> > Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
profile
> > 1.0,
> > and it comply with BFPT DWORD-19, octal mode enable sequences by write
CFG
> > Reg2
> > with instruction 0x72. Therefore, I can't apply your patches.
>
> I didn't mean apply my patches directly. I meant more along the lines of

> edit your patches to work on top of my series. It should be as easy as
> adding your flash's fixup hooks and its octal DTR enable hook, but if my

> series is missing something you need (like complete Profile 1.0 parsing,

> which I left out because I wanted to be conservative and didn't see any
> immediate use-case for us), let me know, and we can work together to
> address it.

yes,sure!
let's work together to upstream the Octal 8D-8D-8D driver to mainline.

The main concern is where and how to enable xSPI octal mode?

Vignesh don't agree to enable it in fixup hooks and that's why I patched
it to spi_nor_late_init_params() and confirmed the device support xSPI
Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.

I can't apply your patches to enable xSPI Octal mode for mx25uw51245g
because your patches set up Octal protocol first and then using Octal
protocol to write Configuration Register 2(CFG Reg2). I think driver
should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
write CFG Reg 2 is success and then setup Octa protocol in the last.

As JESD216F description on BFPT DOWRD 19th, only two way to enable
xSPI Octal mode;
one is by two instruction: issue instruction 06h(WREN) and then E8h.
the other is issue instruction 06h, then issue instruction 72h (Write
CFG Reg2), address 0h and data 02h (8D-8D-8D).

Let our patches comply with this. you may refer to my patches
[v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D
mode

/* Octal mode enable sequences. */
switch (bfpt.dwords[BFPT_DWORD(19)] &
BFPT_DWORD19_OCTAL_SEQ_MASK) {
case BFPT_DWORD19_TWO_INST:
+ ----> to patch here.
break;
case BFPT_DWORD19_CFG_REG2:
params->xspi_enable =
spi_nor_cfg_reg2_octal_enable;
break;
default:
break;
}

>
> > I quickly went through your patches but can't reply them in each your
> > patches.
> >
> > i.e,.
> > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> >
> > - u8 opcode;
> > + u16 opcode;
> >
> > big/little Endian issue, right?
> > why not just u8 ext_opcode;
> > No any impact for exist code and actually only xSPI device use
extension
> > command.
>
> Boris already explained the reasoning behind it.

yup, I got his point and please make sure CPU data access.

i.e,.
Fix endianness of the BFPT DWORDs and xSPI in sfdp.c

and your patch,
+ ext = spi_nor_get_cmd_ext(nor, op);
+ op->cmd.opcode = (op->cmd.opcode << 8) |
ext;
+ op->cmd.nbytes = 2;

I think maybe using u8 opcode[2] could avoid endianness.

Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
please check his comments on
https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/

Let's open this discussion and maybe Vighesh and Tudor could have some
comments on it.
thanks a lot.

>
> > 2) [v4,08/16] mtd: spi-nor: parse xSPI Profile 1.0 table
> >
> > need extract more data from xSPI profile 1.0 table and no other
specific
> > setting.
>
> Not sure what you mean by "no other specific setting".

I mean this function just do xSPI profile 1.0 table parse, no read/pp
setting
i.e,.

+ /*
+ * Update the fast read settings. We set the default
dummy cycles to 20
+ * here. Flashes can change this value if they need to
when enabling
+ * octal mode.
+ */
+ params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+ spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
+ 0, 20,
opcode,
+ SNOR_PROTO_8_8_8_DTR);
+
+ /*
+ * Since the flash supports xSPI DTR reads, it should
also support DTR
+ * Page Program opcodes.
+ */
+ params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;

>
> > 3) [v4,11/16] mtd: spi-nor: enable octal DTR mode when possible
> >
> > +static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
> > +{
> > + int ret;
> > +
> > + if (!nor->params->octal_dtr_enable)
> > + return 0;
> > +
> > + if (!(spi_nor_get_protocol_width(nor->read_proto) ==
8 ||
> > + spi_nor_get_protocol_width(nor->write_proto) ==
8))
> > + return 0;
> > +
> > + ret = nor->params->octal_dtr_enable(nor, enable);
> > + if (ret)
> > + return ret;
> > +
> > + if (enable)
> > + nor->reg_proto =
SNOR_PROTO_8_8_8_DTR;
> > + else
> > + nor->reg_proto = SNOR_PROTO_1_1_1;
> > +
> > + return 0;
> > +}
> > +
> >
> > it seems you enable device in Octal mode after SPI-NOR Framework is
> > already
> > in Octal protocol.
> > Driver should set device by SPI 1-1-1 mode to enter Octal mode and
then
> > setup
> > Read/PP command and protocol by spi_nor_set_read/pp_setting() for
Octal
> > mode,
> > right ?
>
> No. We need to setup Read and PP settings first. The overall flow is
> that we first run spi_nor_setup(). This will perform a "negotiation"
> between the controller and the flash to find out a common protocol they
> both support, and then change to that protocol in spi_nor_init(). Even
> if the flash supports octal DTR protocol, we can't use if if the
> controller doesn't. That is why we want to first select the protocol in
> the framework, and only then change the flash to that protocol.
>
> In case the controller doesn't support 8D-8D-8D protocol, we would want
> to use 1S-1S-1S protocol so the flash is at least usable. Changing to 8D

> mode before finding this out would make the flash unusable.
>

as my concern above.

> --
> Regards,
> Pratyush Yadav

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-04-29 08:40:14

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On Wed, 29 Apr 2020 15:31:35 +0800
[email protected] wrote:

> Hi Pratyush,
>
>
> > > > > On Tue, 21 Apr 2020 14:39:42 +0800
> > > > > Mason Yang <[email protected]> wrote:
> > > > >
> > > > > > Hello,
> > > > > >
> > > > > > This is repost of patchset from Boris Brezillon's
> > > > > > [RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support [1].
> > > > >
> > > > > I only quickly went through the patches you sent and saying it's a
> > > > > repost of the RFC is a bit of a lie. You completely ignored the
> state
> > > > > tracking I was trying to do to avoid leaving the flash in 8D mode
> when
> > > > > suspending/resetting the board, and I think that part is crucial.
> If I
> > > > > remember correctly, we already had this discussion so I must say
> I'm a
> > > > > bit disappointed.
> > > > >
> > > > > Can you sync with Pratyush? I think his series [1] is better in
> that
> > > it
> > > > > tries to restore the flash in single-SPI mode before suspend (it's
> > > > > missing the shutdown case, but that can be easily added I think).
> Of
> > > > > course that'd be even better to have proper state tracking at the
> SPI
> > > > > NOR level.
> > > >
> > > > Hi Mason,
> > > >
> > > > I posted a re-roll of my series here [0]. Could you please base your
>
> > > > changes on top of it? Let me know if the series is missing something
> you
> > >
> > > > need.
> > > >
> > > > [0]
> > >
> https://lore.kernel.org/linux-mtd/[email protected]/
> > >
> > >
> > > Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
> profile
> > > 1.0,
> > > and it comply with BFPT DWORD-19, octal mode enable sequences by write
> CFG
> > > Reg2
> > > with instruction 0x72. Therefore, I can't apply your patches.
> >
> > I didn't mean apply my patches directly. I meant more along the lines of
>
> > edit your patches to work on top of my series. It should be as easy as
> > adding your flash's fixup hooks and its octal DTR enable hook, but if my
>
> > series is missing something you need (like complete Profile 1.0 parsing,
>
> > which I left out because I wanted to be conservative and didn't see any
> > immediate use-case for us), let me know, and we can work together to
> > address it.
>
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
>
> The main concern is where and how to enable xSPI octal mode?
>
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
>
> I can't apply your patches to enable xSPI Octal mode for mx25uw51245g
> because your patches set up Octal protocol first and then using Octal
> protocol to write Configuration Register 2(CFG Reg2). I think driver
> should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
> write CFG Reg 2 is success and then setup Octa protocol in the last.
>
> As JESD216F description on BFPT DOWRD 19th, only two way to enable
> xSPI Octal mode;
> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
>
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D
> mode
>
> /* Octal mode enable sequences. */
> switch (bfpt.dwords[BFPT_DWORD(19)] &
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
> case BFPT_DWORD19_TWO_INST:
> + ----> to patch here.
> break;
> case BFPT_DWORD19_CFG_REG2:
> params->xspi_enable =
> spi_nor_cfg_reg2_octal_enable;
> break;
> default:
> break;
> }
>
>
> >
> > > I quickly went through your patches but can't reply them in each your
> > > patches.
> > >
> > > i.e,.
> > > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > >
> > > - u8 opcode;
> > > + u16 opcode;
> > >
> > > big/little Endian issue, right?
> > > why not just u8 ext_opcode;
> > > No any impact for exist code and actually only xSPI device use
> extension
> > > command.
> >
> > Boris already explained the reasoning behind it.
>
> yup, I got his point and please make sure CPU data access.
>
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
>
> and your patch,
> + ext = spi_nor_get_cmd_ext(nor, op);
> + op->cmd.opcode = (op->cmd.opcode << 8) |
> ext;
> + op->cmd.nbytes = 2;
>
> I think maybe using u8 opcode[2] could avoid endianness.
>

Again, if there's an endianness issue it's in your SPI driver, not
here, and I suspect you'd have the same issue with the address cycles.
SPI mem protocols has been using big endian for everything, and I think
that should be applied to dual-byte opcodes too.

> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/
>
>
>
> Let's open this discussion and maybe Vighesh and Tudor could have some
> comments on it.

Changing for opcode[2] would mean patching all spi-mem drivers. That's
doable, but given we already have the address field encoded in a u64, I
don't see a good reason to not apply that rule to the opcode.

2020-04-29 18:23:22

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Mason,

On 29/04/20 03:31PM, [email protected] wrote:
> Hi Pratyush,
> > > > Hi Mason,
> > > >
> > > > I posted a re-roll of my series here [0]. Could you please base your
>
> > > > changes on top of it? Let me know if the series is missing something
> you
> > >
> > > > need.
> > > >
> > > > [0]
> > >
> https://lore.kernel.org/linux-mtd/[email protected]/
> > >
> > >
> > > Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
> profile
> > > 1.0,
> > > and it comply with BFPT DWORD-19, octal mode enable sequences by write
> CFG
> > > Reg2
> > > with instruction 0x72. Therefore, I can't apply your patches.
> >
> > I didn't mean apply my patches directly. I meant more along the lines of
>
> > edit your patches to work on top of my series. It should be as easy as
> > adding your flash's fixup hooks and its octal DTR enable hook, but if my
>
> > series is missing something you need (like complete Profile 1.0 parsing,
>
> > which I left out because I wanted to be conservative and didn't see any
> > immediate use-case for us), let me know, and we can work together to
> > address it.
>
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
>
> The main concern is where and how to enable xSPI octal mode?
>
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.

My series does it in a octal_dtr_enable() hook, which is called from
spi_nor_init(). Just like how it is done for quad_enable(). So, the
expectation is that you populate the octal DTR hook for your flash in a
flash-specific hook (like the default_init() fixup hook). Example of
this can be seen in patches 15 and 16 of my series in
spi_nor_cypress_octal_enable() and spi_nor_micron_octal_dtr_enable().

An alternative for this would be a generic way to enable these flashes,
like from BFPT DWORD 19. That doesn't work for either of the flashes I
had, so I didn't implement it because I wouldn't be able to test it. If
it works for you, please implement it. I don't mind doing it myself, but
then you would need to help me test it.

> I can't apply your patches to enable xSPI Octal mode for mx25uw51245g
> because your patches set up Octal protocol first and then using Octal
> protocol to write Configuration Register 2(CFG Reg2). I think driver
> should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
> write CFG Reg 2 is success and then setup Octa protocol in the last.

Register writes should work in 1S mode, because nor->reg_proto is only
set _after_ 8D mode is enabled (see spi_nor_octal_dtr_enable()). In
fact, both patch 15 and 16 in my series use register writes in 1S mode.

> As JESD216F description on BFPT DOWRD 19th, only two way to enable
> xSPI Octal mode;
> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
>
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D
> mode

The Cypress Semper S28 flash family says that it does not have an Octal
Enable bit (i.e, the Octal Enable Requirements field is 0), even though
it does have an Octal Enable bit. That bit resides in CFG Reg 5. And the
Micron MT35ABA family, doesn't have DWORD19 in their BFPT at all. So,
the two flashes I need to support don't have this. At the very least, we
need to provide a flash-specific way to enable Octal DTR mode, along
with an xSPI compliant way.

So I suggest that we have two separate type of 8D enable functions. One
type which is generic and works on any xSPI complint device, like the
spi_nor_cfg_reg2_octal_enable() in your patch. The other would be
flash-specific ones, which flashes can set in their fixup hooks.

> /* Octal mode enable sequences. */
> switch (bfpt.dwords[BFPT_DWORD(19)] &
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
> case BFPT_DWORD19_TWO_INST:
> + ----> to patch here.
> break;
> case BFPT_DWORD19_CFG_REG2:
> params->xspi_enable =
> spi_nor_cfg_reg2_octal_enable;
> break;
> default:
> break;
> }
>
>
> >
> > > I quickly went through your patches but can't reply them in each your
> > > patches.
> > >
> > > i.e,.
> > > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > >
> > > - u8 opcode;
> > > + u16 opcode;
> > >
> > > big/little Endian issue, right?
> > > why not just u8 ext_opcode;
> > > No any impact for exist code and actually only xSPI device use
> extension
> > > command.
> >
> > Boris already explained the reasoning behind it.
>
> yup, I got his point and please make sure CPU data access.
>
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
>
> and your patch,
> + ext = spi_nor_get_cmd_ext(nor, op);
> + op->cmd.opcode = (op->cmd.opcode << 8) |
> ext;
> + op->cmd.nbytes = 2;
>
> I think maybe using u8 opcode[2] could avoid endianness.

Again, thanks Boris for answering this. FWIW, I don't see anything wrong
with his suggestion.

To clarify a bit more, the idea is that we transmit the opcode MSB
first, just we do for the address. Assume we want to issue the command
0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat
is as a 1-byte value, so the MSB is the same as the LSB. We directly
send 0x5 on the bus.

If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension
is invert of command). So we send the MSB (0x05) first, and LSB (0xFA)
next.

In all this, I don't see where endianness comes into the picture. While
the _location_ of the MSB in memory may change because of the
endianness, the MSB of the _number_ will always be 0x05. So, regardless
of the endianness, the operation (opcode >> 8) should always give 0x05
and (opcode & F) should always give 0xFA. Endianness is just how you
represent these values in memory.

> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/

In fact, that's how I did it in the first version of my series as well,
but refactored it on Boris's suggestion.

> Let's open this discussion and maybe Vighesh and Tudor could have some
> comments on it.
> thanks a lot.
>
> >
> > > 2) [v4,08/16] mtd: spi-nor: parse xSPI Profile 1.0 table
> > >
> > > need extract more data from xSPI profile 1.0 table and no other
> specific
> > > setting.
> >
> > Not sure what you mean by "no other specific setting".
>
> I mean this function just do xSPI profile 1.0 table parse, no read/pp
> setting
> i.e,.
>
> + /*
> + * Update the fast read settings. We set the default
> dummy cycles to 20
> + * here. Flashes can change this value if they need to
> when enabling
> + * octal mode.
> + */
> + params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> + 0, 20,
> opcode,
> + SNOR_PROTO_8_8_8_DTR);
> +
> + /*
> + * Since the flash supports xSPI DTR reads, it should
> also support DTR
> + * Page Program opcodes.
> + */
> + params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;

Ok. Fair enough.

>
> >
> > > 3) [v4,11/16] mtd: spi-nor: enable octal DTR mode when possible
> > >
> > > +static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
> > > +{
> > > + int ret;
> > > +
> > > + if (!nor->params->octal_dtr_enable)
> > > + return 0;
> > > +
> > > + if (!(spi_nor_get_protocol_width(nor->read_proto) ==
> 8 ||
> > > + spi_nor_get_protocol_width(nor->write_proto) ==
> 8))
> > > + return 0;
> > > +
> > > + ret = nor->params->octal_dtr_enable(nor, enable);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (enable)
> > > + nor->reg_proto =
> SNOR_PROTO_8_8_8_DTR;
> > > + else
> > > + nor->reg_proto = SNOR_PROTO_1_1_1;
> > > +
> > > + return 0;
> > > +}
> > > +
> > >
> > > it seems you enable device in Octal mode after SPI-NOR Framework is
> > > already
> > > in Octal protocol.
> > > Driver should set device by SPI 1-1-1 mode to enter Octal mode and
> then
> > > setup
> > > Read/PP command and protocol by spi_nor_set_read/pp_setting() for
> Octal
> > > mode,
> > > right ?
> >
> > No. We need to setup Read and PP settings first. The overall flow is
> > that we first run spi_nor_setup(). This will perform a "negotiation"
> > between the controller and the flash to find out a common protocol they
> > both support, and then change to that protocol in spi_nor_init(). Even
> > if the flash supports octal DTR protocol, we can't use if if the
> > controller doesn't. That is why we want to first select the protocol in
> > the framework, and only then change the flash to that protocol.
> >
> > In case the controller doesn't support 8D-8D-8D protocol, we would want
> > to use 1S-1S-1S protocol so the flash is at least usable. Changing to 8D
>
> > mode before finding this out would make the flash unusable.
> >
>
> as my concern above.

I hope my answer above clears this up.

--
Regards,
Pratyush Yadav

2020-04-30 08:24:27

by Vignesh Raghavendra

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Mason,

On 29/04/20 1:01 pm, [email protected] wrote:
>
> Hi Pratyush,
>
>
>>>>> On Tue, 21 Apr 2020 14:39:42 +0800
>>>>> Mason Yang <[email protected]> wrote:
[...]
>>>
> https://lore.kernel.org/linux-mtd/[email protected]/
>>>
>>>
>>> Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
> profile
>>> 1.0,
>>> and it comply with BFPT DWORD-19, octal mode enable sequences by write
> CFG
>>> Reg2
>>> with instruction 0x72. Therefore, I can't apply your patches.
>>
>> I didn't mean apply my patches directly. I meant more along the lines of
>
>> edit your patches to work on top of my series. It should be as easy as
>> adding your flash's fixup hooks and its octal DTR enable hook, but if my
>
>> series is missing something you need (like complete Profile 1.0 parsing,
>
>> which I left out because I wanted to be conservative and didn't see any
>> immediate use-case for us), let me know, and we can work together to
>> address it.
>
> yes,sure!
> let's work together to upstream the Octal 8D-8D-8D driver to mainline.
>
> The main concern is where and how to enable xSPI octal mode?
>
> Vignesh don't agree to enable it in fixup hooks and that's why I patched
> it to spi_nor_late_init_params() and confirmed the device support xSPI
> Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
>

My suggestion was to use SFDP wherever possible.. E.g: it is possible to
get opcode extension type from BFPT...

But using BFPT DWORD-19 is not correct for switching to 8D-8D-8D mode:

Per JESD216D.01 Bits 22:20 of 19th DWORD of BFPT:

Octal Enable Requirements:

This field describes whether the device contains a Octal Enable bit used
to enable 1-1-8 and 1-
8-8 octal read or octal program operations.

So, this cannot be used for enabling 8D-8D-8D mode... Flashes that only
support 1S-1S-1S and 8D-8D-8D will set this field to 0.

There is a separate table to enable 8D mode called
"Command Sequences to Change to Octal DDR (8D-8D-8D) mode". But if flash
does not have the table or has bad data, fixup hook is the only way...

If mx25* supports above table, please build on top of Pratyush's series
to add support for parsing this table. Otherwise, macronix would have to
use a fixup hook too...

> I can't apply your patches to enable xSPI Octal mode for mx25uw51245g
> because your patches set up Octal protocol first and then using Octal
> protocol to write Configuration Register 2(CFG Reg2). I think driver
> should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
> write CFG Reg 2 is success and then setup Octa protocol in the last.
>
> As JESD216F description on BFPT DOWRD 19th, only two way to enable
> xSPI Octal mode;

Where is JESD216F? Latest I can find is JESD216D.01

> one is by two instruction: issue instruction 06h(WREN) and then E8h.
> the other is issue instruction 06h, then issue instruction 72h (Write
> CFG Reg2), address 0h and data 02h (8D-8D-8D).
>
> Let our patches comply with this. you may refer to my patches
> [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal 8D-8D-8D
> mode

As I pointed out earlier using above DWORDS seems wrong for 8D-8D-8D,
they can be used for 1-1-8 and 1-
8-8

>
> /* Octal mode enable sequences. */
> switch (bfpt.dwords[BFPT_DWORD(19)] &
> BFPT_DWORD19_OCTAL_SEQ_MASK) {
> case BFPT_DWORD19_TWO_INST:
> + ----> to patch here.
> break;
> case BFPT_DWORD19_CFG_REG2:
> params->xspi_enable =
> spi_nor_cfg_reg2_octal_enable;
> break;
> default:
> break;
> }
>
>
>>
>>> I quickly went through your patches but can't reply them in each your
>>> patches.
>>>
>>> i.e,.
>>> 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
>>>
>>> - u8 opcode;
>>> + u16 opcode;
>>>
>>> big/little Endian issue, right?

Is the big/little Endian issue a quirk of the flash or controller? If
its controller specific then it needs to handled in controller driver.

If this is a flash quirk, please point to the waveforms in the flash
datasheet...

>>> why not just u8 ext_opcode;
>>> No any impact for exist code and actually only xSPI device use
> extension
>>> command.
>>
>> Boris already explained the reasoning behind it.
>
> yup, I got his point and please make sure CPU data access.
>
> i.e,.
> Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
>
> and your patch,
> + ext = spi_nor_get_cmd_ext(nor, op);
> + op->cmd.opcode = (op->cmd.opcode << 8) |
> ext;
> + op->cmd.nbytes = 2;
>
> I think maybe using u8 opcode[2] could avoid endianness.
>
> Moreover, Vignesh think it's fine to use u8 ext_opcode in my v1 patches.
> please check his comments on
> https://patchwork.ozlabs.org/project/linux-mtd/patch/[email protected]/
>
>
>
> Let's open this discussion and maybe Vighesh and Tudor could have some
> comments on it.
> thanks a lot.
>

Sorry , but others clearly see having single variable to store cmd +
extension is beneficial here. So, I take back my suggestion.

Regards
Vignesh

2020-05-05 09:34:55

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Pratyush,

> > > > >
> > > > > I posted a re-roll of my series here [0]. Could you please base
your
> >
> > > > > changes on top of it? Let me know if the series is missing
something
> > you
> > > >
> > > > > need.
> > > > >
> > > > > [0]
> > > >
> >
https://lore.kernel.org/linux-mtd/[email protected]/
> > > >
> > > >
> > > > Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
> > profile
> > > > 1.0,
> > > > and it comply with BFPT DWORD-19, octal mode enable sequences by
write
> > CFG
> > > > Reg2
> > > > with instruction 0x72. Therefore, I can't apply your patches.
> > >
> > > I didn't mean apply my patches directly. I meant more along the
lines of
> >
> > > edit your patches to work on top of my series. It should be as easy
as
> > > adding your flash's fixup hooks and its octal DTR enable hook, but
if my
> >
> > > series is missing something you need (like complete Profile 1.0
parsing,
> >
> > > which I left out because I wanted to be conservative and didn't see
any
> > > immediate use-case for us), let me know, and we can work together to

> > > address it.
> >
> > yes,sure!
> > let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> >
> > The main concern is where and how to enable xSPI octal mode?
> >
> > Vignesh don't agree to enable it in fixup hooks and that's why I
patched
> > it to spi_nor_late_init_params() and confirmed the device support xSPI

> > Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
>
> My series does it in a octal_dtr_enable() hook, which is called from
> spi_nor_init(). Just like how it is done for quad_enable(). So, the
> expectation is that you populate the octal DTR hook for your flash in a
> flash-specific hook (like the default_init() fixup hook). Example of
> this can be seen in patches 15 and 16 of my series in
> spi_nor_cypress_octal_enable() and spi_nor_micron_octal_dtr_enable().
>
> An alternative for this would be a generic way to enable these flashes,
> like from BFPT DWORD 19. That doesn't work for either of the flashes I
> had, so I didn't implement it because I wouldn't be able to test it. If
> it works for you, please implement it. I don't mind doing it myself, but

> then you would need to help me test it.
>
> > I can't apply your patches to enable xSPI Octal mode for mx25uw51245g
> > because your patches set up Octal protocol first and then using Octal
> > protocol to write Configuration Register 2(CFG Reg2). I think driver
> > should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
> > write CFG Reg 2 is success and then setup Octa protocol in the last.
>
> Register writes should work in 1S mode, because nor->reg_proto is only
> set _after_ 8D mode is enabled (see spi_nor_octal_dtr_enable()). In
> fact, both patch 15 and 16 in my series use register writes in 1S mode.

but I didn't see driver roll back "nor->read/write_proto = 1"
if xxx->octal_dtr_enable() return failed!

>
> > As JESD216F description on BFPT DOWRD 19th, only two way to enable
> > xSPI Octal mode;
> > one is by two instruction: issue instruction 06h(WREN) and then E8h.
> > the other is issue instruction 06h, then issue instruction 72h (Write
> > CFG Reg2), address 0h and data 02h (8D-8D-8D).
> >
> > Let our patches comply with this. you may refer to my patches
> > [v2,3/5] mtd: spi-nor: Parse BFPT DWORD-18, 19 and 20 for Octal
8D-8D-8D
> > mode
>
> The Cypress Semper S28 flash family says that it does not have an Octal
> Enable bit (i.e, the Octal Enable Requirements field is 0), even though
> it does have an Octal Enable bit. That bit resides in CFG Reg 5. And the

> Micron MT35ABA family, doesn't have DWORD19 in their BFPT at all. So,
> the two flashes I need to support don't have this. At the very least, we

> need to provide a flash-specific way to enable Octal DTR mode, along
> with an xSPI compliant way.
>
> So I suggest that we have two separate type of 8D enable functions. One
> type which is generic and works on any xSPI complint device, like the
> spi_nor_cfg_reg2_octal_enable() in your patch. The other would be
> flash-specific ones, which flashes can set in their fixup hooks.

okay, sure.

>
> > /* Octal mode enable sequences. */
> > switch (bfpt.dwords[BFPT_DWORD(19)] &
> > BFPT_DWORD19_OCTAL_SEQ_MASK) {
> > case BFPT_DWORD19_TWO_INST:
> > + ----> to patch here.
> > break;
> > case BFPT_DWORD19_CFG_REG2:
> > params->xspi_enable =
> > spi_nor_cfg_reg2_octal_enable;
> > break;
> > default:
> > break;
> > }
> >
> >
> > >
> > > > I quickly went through your patches but can't reply them in each
your
> > > > patches.
> > > >
> > > > i.e,.
> > > > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > > >
> > > > - u8 opcode;
> > > > + u16 opcode;
> > > >
> > > > big/little Endian issue, right?
> > > > why not just u8 ext_opcode;
> > > > No any impact for exist code and actually only xSPI device use
> > extension
> > > > command.
> > >
> > > Boris already explained the reasoning behind it.
> >
> > yup, I got his point and please make sure CPU data access.
> >
> > i.e,.
> > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> >
> > and your patch,
> > + ext = spi_nor_get_cmd_ext(nor, op);
> > + op->cmd.opcode = (op->cmd.opcode <<
8) |
> > ext;
> > + op->cmd.nbytes = 2;
> >
> > I think maybe using u8 opcode[2] could avoid endianness.
>
> Again, thanks Boris for answering this. FWIW, I don't see anything wrong

> with his suggestion.
>
> To clarify a bit more, the idea is that we transmit the opcode MSB
> first, just we do for the address. Assume we want to issue the command
> 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat

> is as a 1-byte value, so the MSB is the same as the LSB. We directly
> send 0x5 on the bus.

There are many SPI controllers driver use "op->cmd.opcode" directly,
so is spi-mxic.c.

i.e,.
ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);

>
> If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension
> is invert of command). So we send the MSB (0x05) first, and LSB (0xFA)
> next.

My platform is Xilinx Zynq platform which CPU is ARMv7 processor.

In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
in 8D-8D-8D mode, I need to patch

i.e.,
op->cmd.opcode = op->cmd.opcode | (ext << 8);

rather than your patch.

>
> In all this, I don't see where endianness comes into the picture. While
> the _location_ of the MSB in memory may change because of the
> endianness, the MSB of the _number_ will always be 0x05. So, regardless
> of the endianness, the operation (opcode >> 8) should always give 0x05
> and (opcode & F) should always give 0xFA. Endianness is just how you
> represent these values in memory.
>

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-05-05 09:47:26

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On Tue, 5 May 2020 17:31:45 +0800
[email protected] wrote:

> > > > > I quickly went through your patches but can't reply them in each
> your
> > > > > patches.
> > > > >
> > > > > i.e,.
> > > > > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > > > >
> > > > > - u8 opcode;
> > > > > + u16 opcode;
> > > > >
> > > > > big/little Endian issue, right?
> > > > > why not just u8 ext_opcode;
> > > > > No any impact for exist code and actually only xSPI device use
> > > extension
> > > > > command.
> > > >
> > > > Boris already explained the reasoning behind it.
> > >
> > > yup, I got his point and please make sure CPU data access.
> > >
> > > i.e,.
> > > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> > >
> > > and your patch,
> > > + ext = spi_nor_get_cmd_ext(nor, op);
> > > + op->cmd.opcode = (op->cmd.opcode <<
> 8) |
> > > ext;
> > > + op->cmd.nbytes = 2;
> > >
> > > I think maybe using u8 opcode[2] could avoid endianness.
> >
> > Again, thanks Boris for answering this. FWIW, I don't see anything wrong
>
> > with his suggestion.
> >
> > To clarify a bit more, the idea is that we transmit the opcode MSB
> > first, just we do for the address. Assume we want to issue the command
> > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat
>
> > is as a 1-byte value, so the MSB is the same as the LSB. We directly
> > send 0x5 on the bus.
>
> There are many SPI controllers driver use "op->cmd.opcode" directly,
> so is spi-mxic.c.
>
> i.e,.
> ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);

Just because you do it doesn't mean it's right. And most controllers use
the opcode value, they don't dereference the pointer as you do here.

>
> >
> > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension
> > is invert of command). So we send the MSB (0x05) first, and LSB (0xFA)
> > next.
>
> My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
>
> In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> in 8D-8D-8D mode, I need to patch
>
> i.e.,
> op->cmd.opcode = op->cmd.opcode | (ext << 8);
>
> rather than your patch.

Seriously, how hard is it to extract each byte from the u16 if your
controller needs to pass things in a different order? I mean, that's
already how it's done for the address cycle, so why is it a problem
here? This sounds like bikeshedding to me. If the order is properly
documented in the kernel doc, I see no problem having it grouped in one
u16, with the first cmd cycle placed in the MSB and the second one in
the LSB.

2020-05-05 10:03:38

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On Tue, 5 May 2020 11:44:43 +0200
Boris Brezillon <[email protected]> wrote:

> On Tue, 5 May 2020 17:31:45 +0800
> [email protected] wrote:
>
>
> > > > > > I quickly went through your patches but can't reply them in each
> > your
> > > > > > patches.
> > > > > >
> > > > > > i.e,.
> > > > > > 1) [v4,03/16] spi: spi-mem: allow specifying a command's extension
> > > > > >
> > > > > > - u8 opcode;
> > > > > > + u16 opcode;
> > > > > >
> > > > > > big/little Endian issue, right?
> > > > > > why not just u8 ext_opcode;
> > > > > > No any impact for exist code and actually only xSPI device use
> > > > extension
> > > > > > command.
> > > > >
> > > > > Boris already explained the reasoning behind it.
> > > >
> > > > yup, I got his point and please make sure CPU data access.
> > > >
> > > > i.e,.
> > > > Fix endianness of the BFPT DWORDs and xSPI in sfdp.c
> > > >
> > > > and your patch,
> > > > + ext = spi_nor_get_cmd_ext(nor, op);
> > > > + op->cmd.opcode = (op->cmd.opcode <<
> > 8) |
> > > > ext;
> > > > + op->cmd.nbytes = 2;
> > > >
> > > > I think maybe using u8 opcode[2] could avoid endianness.
> > >
> > > Again, thanks Boris for answering this. FWIW, I don't see anything wrong
> >
> > > with his suggestion.
> > >
> > > To clarify a bit more, the idea is that we transmit the opcode MSB
> > > first, just we do for the address. Assume we want to issue the command
> > > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1. Treat
> >
> > > is as a 1-byte value, so the MSB is the same as the LSB. We directly
> > > send 0x5 on the bus.
> >
> > There are many SPI controllers driver use "op->cmd.opcode" directly,
> > so is spi-mxic.c.
> >
> > i.e,.
> > ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, op->cmd.nbytes);
>
> Just because you do it doesn't mean it's right. And most controllers use
> the opcode value, they don't dereference the pointer as you do here.
>
> >
> > >
> > > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming extension
> > > is invert of command). So we send the MSB (0x05) first, and LSB (0xFA)
> > > next.
> >
> > My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
> >
> > In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> > in 8D-8D-8D mode, I need to patch
> >
> > i.e.,
> > op->cmd.opcode = op->cmd.opcode | (ext << 8);
> >
> > rather than your patch.
>
> Seriously, how hard is it to extract each byte from the u16 if your
> controller needs to pass things in a different order? I mean, that's
> already how it's done for the address cycle, so why is it a problem
> here? This sounds like bikeshedding to me. If the order is properly
> documented in the kernel doc, I see no problem having it grouped in one
> u16, with the first cmd cycle placed in the MSB and the second one in
> the LSB.

So, I gave it a try, and we're talking about something as simple as the
below diff. And yes, the mxic controller needs to be patched before
extending the cmd.opcode field, but we're talking about one driver here
(all other drivers should be fine). Oh, and if you look a few lines below
the changed lines, you'll notice that we do exactly the same for the
address.

--->8---
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 69491f3a515d..c3f4136a7c1d 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
int nio = 1, i, ret;
u32 ss_ctrl;
u8 addr[8];
+ u8 cmd[2];

ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
if (ret)
@@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
mxic->regs + HC_CFG);

- ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
+ for (i = 0; i < op->cmd.nbytes; i++)
+ cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i - 1));
+
+ ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
if (ret)
goto out;

2020-05-06 09:42:42

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

On 05/05/20 05:31PM, [email protected] wrote:
> Hi Pratyush,
> > > I can't apply your patches to enable xSPI Octal mode for
> > > mx25uw51245g because your patches set up Octal protocol first and
> > > then using Octal protocol to write Configuration Register 2(CFG
> > > Reg2). I think driver
> > > should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make sure
> > > write CFG Reg 2 is success and then setup Octa protocol in the last.
> >
> > Register writes should work in 1S mode, because nor->reg_proto is only
> > set _after_ 8D mode is enabled (see spi_nor_octal_dtr_enable()). In
> > fact, both patch 15 and 16 in my series use register writes in 1S mode.
>
> but I didn't see driver roll back "nor->read/write_proto = 1"
> if xxx->octal_dtr_enable() return failed!

I copied what spi_nor_quad_enable() did, and made failure fatal. So if
xxx->octal_dtr_enable() fails, the probe would fail and the flash would
be unusable. You can try your hand at a fallback system where you try
all possible protocols available, but I think that should be a different
patchset.

--
Regards,
Pratyush Yadav

2020-05-11 03:27:38

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Vignesh,

> >>>
> >>> Our mx25uw51245g supports BFPT DWORD-18,19 and 20 data and xSPI
> > profile
> >>> 1.0,
> >>> and it comply with BFPT DWORD-19, octal mode enable sequences by
write
> > CFG
> >>> Reg2
> >>> with instruction 0x72. Therefore, I can't apply your patches.
> >>
> >> I didn't mean apply my patches directly. I meant more along the lines
of
> >
> >> edit your patches to work on top of my series. It should be as easy
as
> >> adding your flash's fixup hooks and its octal DTR enable hook, but if
my
> >
> >> series is missing something you need (like complete Profile 1.0
parsing,
> >
> >> which I left out because I wanted to be conservative and didn't see
any
> >> immediate use-case for us), let me know, and we can work together to
> >> address it.
> >
> > yes,sure!
> > let's work together to upstream the Octal 8D-8D-8D driver to mainline.
> >
> > The main concern is where and how to enable xSPI octal mode?
> >
> > Vignesh don't agree to enable it in fixup hooks and that's why I
patched
> > it to spi_nor_late_init_params() and confirmed the device support xSPI

> > Octal mode after BFPT DWORD-19 and xSPI pf 1.0 have been parsed.
> >
>
> My suggestion was to use SFDP wherever possible.. E.g: it is possible to
> get opcode extension type from BFPT...
>
> But using BFPT DWORD-19 is not correct for switching to 8D-8D-8D mode:
>
> Per JESD216D.01 Bits 22:20 of 19th DWORD of BFPT:
>
> Octal Enable Requirements:
>
> This field describes whether the device contains a Octal Enable bit used
> to enable 1-1-8 and 1-
> 8-8 octal read or octal program operations.
>
> So, this cannot be used for enabling 8D-8D-8D mode... Flashes that only
> support 1S-1S-1S and 8D-8D-8D will set this field to 0.

yes, you are right, the bits 22~20 your mentioned are for 1-1-8 and 1-8-8
mode enable requirements and they are zero if Flash only supports
1S-1S-1S,
8S-8S-8S and 8D-8D-8D, just like mx25xx series.

There are bits 8~4 for 8S-8S-8S and 8D-8D-8D mode enable sequences and
I have patched these in this patches.

By bits 8~4 in 19 th DWORD of BFPT, driver will know enable 8S-8S-8S or
8D-8D-8D by either issue two instruction (06h and E8h) or
by Write CFG Reg 2.

mx25xx series supports enable Octal 8S-8S-8S/8D-8D-8D mode by Write CFG
Reg 2.

>
> There is a separate table to enable 8D mode called
> "Command Sequences to Change to Octal DDR (8D-8D-8D) mode". But if flash
> does not have the table or has bad data, fixup hook is the only way...
>
> If mx25* supports above table, please build on top of Pratyush's series
> to add support for parsing this table. Otherwise, macronix would have to
> use a fixup hook too...

mx25xx series also supports "Command Sequences to Change to Octal DDR
(8D-8D-8D) mode" for sure. I will patch them in next version.

For mx25* series, a fixup hook will only setup specific dummy cycles to
device for various frequency after xSPI 1.0 table has been parsed.

thanks for your time & comments.
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-05-11 06:58:34

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Boris,

> > > >
> > > > To clarify a bit more, the idea is that we transmit the opcode MSB

> > > > first, just we do for the address. Assume we want to issue the
command
> > > > 0x05. In 1S mode, we set cmd.opcode to 0x05. Here cmd.nbytes == 1.
Treat
> > >
> > > > is as a 1-byte value, so the MSB is the same as the LSB. We
directly
> > > > send 0x5 on the bus.
> > >
> > > There are many SPI controllers driver use "op->cmd.opcode" directly,
> > > so is spi-mxic.c.
> > >
> > > i.e,.
> > > ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL,
op->cmd.nbytes);
> >
> > Just because you do it doesn't mean it's right. And most controllers
use
> > the opcode value, they don't dereference the pointer as you do here.
> >
> > >
> > > >
> > > > If cmd.nbytes == 2, then the opcode would be 0x05FA (assuming
extension
> > > > is invert of command). So we send the MSB (0x05) first, and LSB
(0xFA)
> > > > next.
> > >
> > > My platform is Xilinx Zynq platform which CPU is ARMv7 processor.
> > >
> > > In 1-1-1 mode, it's OK to send 1 byte command by u16 opcode but
> > > in 8D-8D-8D mode, I need to patch
> > >
> > > i.e.,
> > > op->cmd.opcode = op->cmd.opcode | (ext << 8);
> > >
> > > rather than your patch.
> >
> > Seriously, how hard is it to extract each byte from the u16 if your
> > controller needs to pass things in a different order? I mean, that's
> > already how it's done for the address cycle, so why is it a problem
> > here? This sounds like bikeshedding to me. If the order is properly
> > documented in the kernel doc, I see no problem having it grouped in
one
> > u16, with the first cmd cycle placed in the MSB and the second one in
> > the LSB.
>
> So, I gave it a try, and we're talking about something as simple as the
> below diff. And yes, the mxic controller needs to be patched before
> extending the cmd.opcode field, but we're talking about one driver here
> (all other drivers should be fine). Oh, and if you look a few lines
below
> the changed lines, you'll notice that we do exactly the same for the
> address.

yup,
thanks again for your time & comments.

>
> --->8---
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 69491f3a515d..c3f4136a7c1d 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
> int nio = 1, i, ret;
> u32 ss_ctrl;
> u8 addr[8];
> + u8 cmd[2];
>
> ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
> if (ret)
> @@ -393,7 +394,10 @@ static int mxic_spi_mem_exec_op(struct spi_mem
*mem,
> writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
> mxic->regs + HC_CFG);
>
> - ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> + for (i = 0; i < op->cmd.nbytes; i++)
> + cmd[i] = op->cmd.opcode >> (8 * (op->cmd.nbytes - i -
1));
> +
> + ret = mxic_spi_data_xfer(mxic, cmd, NULL, op->cmd.nbytes);
> if (ret)
> goto out;
>

best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-05-15 04:41:56

[permalink] [raw]

Subject: Re: [PATCH v2 0/5] mtd: spi-nor: Add support for Octal 8D-8D-8D mode

Hi Pratyush,

> > > > I can't apply your patches to enable xSPI Octal mode for
> > > > mx25uw51245g because your patches set up Octal protocol first and
> > > > then using Octal protocol to write Configuration Register 2(CFG
> > > > Reg2). I think driver
> > > > should write CFG Reg2 in SPI 1-1-1 mode (power on state) and make
sure
> > > > write CFG Reg 2 is success and then setup Octa protocol in the
last.
> > >
> > > Register writes should work in 1S mode, because nor->reg_proto is
only
> > > set _after_ 8D mode is enabled (see spi_nor_octal_dtr_enable()). In
> > > fact, both patch 15 and 16 in my series use register writes in 1S
mode.
> >
> > but I didn't see driver roll back "nor->read/write_proto = 1"
> > if xxx->octal_dtr_enable() return failed!
>
> I copied what spi_nor_quad_enable() did, and made failure fatal. So if
> xxx->octal_dtr_enable() fails, the probe would fail and the flash would
> be unusable. You can try your hand at a fallback system where you try

IMHO, it's not a good for system booting from SPI-NOR,
driver should still keep system alive in SPI 1-1-1 mode in case of
enable Octal/Quad failed.

Therefore, my patches is to setup nor->read/write_proto = 8 in case
driver enable Octal mode is success. And to enable Octal mode in
spi_nor_late_init_params()rather than as spi_nor_quad_enable()did.

> all possible protocols available, but I think that should be a different

> patchset.
>
> --
> Regards,
> Pratyush Yadav

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information
and/or personal data, which is protected by applicable laws. Please be
reminded that duplication, disclosure, distribution, or use of this e-mail
(and/or its attachments) or any part thereof is prohibited. If you receive
this e-mail in error, please notify us immediately and delete this mail as
well as its attachment(s) from your system. In addition, please be
informed that collection, processing, and/or use of personal data is
prohibited unless expressly permitted by personal data protection laws.
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================

2020-05-15 06:57:39