2021-07-13 13:07:13

by Apurva Nandan

Subject: [PATCH 00/13] mtd: spinand: Add Octal DTR SPI (8D-8D-8D) mode support

Hi,
This series proposes patches for adding the following functionality
in SPI NAND core:

- Octal DTR SPI (8D-8D-8D) mode support

- Winbond W35N01JW SPI NAND chip support

- Power-on-Reset instruction support

This series has been tested on TI J721e EVM with the Winbond W35N01JW
flash with following test utilities:

- nandtest
Test log: https://textbin.net/raw/fhypoz63f9

- mtd_stresstest
Test log: https://textbin.net/raw/0xqjmqntj7

- UBIFS LTP stress test (NAND_XL_STRESS_DD_RW_UBIFS).
Test log: https://textbin.net/raw/pyokws7wku

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Apurva Nandan (13):
spi: spi-mem: Add DTR templates for cmd, address, dummy and data phase
mtd: spinand: Add enum spinand_proto to indicate current SPI IO mode
mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto
mtd: spinand: Fix odd byte addr and data phase in read/write reg op
and write VCR op for Octal DTR mode
mtd: spinand: Add adjust_op() in manufacturer_ops to modify the ops
for manufacturer specific changes
mtd: spinand: Add macros for Octal DTR page read and write operations
mtd: spinand: Allow enabling Octal DTR mode in the core
mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is
missing in manufacturer_op
mtd: spinand: Add support for write volatile configuration register op
mtd: spinand: Add octal_dtr_enable() for Winbond manufacturer_ops
mtd: spinand: Add support for Power-on-Reset (PoR) instruction
mtd: spinand: Perform Power-on-Reset when runtime_pm suspend is issued
mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash

drivers/mtd/nand/spi/core.c | 196 +++++++++++++++++++++++++++++++--
drivers/mtd/nand/spi/winbond.c | 186 +++++++++++++++++++++++++++++--
include/linux/mtd/spinand.h | 67 +++++++++++
include/linux/spi/spi-mem.h | 87 ++++++++++-----
4 files changed, 494 insertions(+), 42 deletions(-)

--
2.17.1

2021-07-13 13:07:19

by Apurva Nandan

Subject: [PATCH 02/13] mtd: spinand: Add enum spinand_proto to indicate current SPI IO mode

Unlike Dual and Quad SPI modes flashes, Octal DTR SPI NAND flashes
require all instructions to be made in 8D-8D-8D protocol when the
flash is in Octal DTR mode. Hence, storing the current SPI IO mode
becomes necessary for correctly generating non-array access operations.

Store the current SPI IO mode in the spinand struct using a reg_proto
enum. This would act as a flag, denoting that the core should use
the given SPI protocol for non-page access operations.

Also provide basic macros for extracting buswidth and dtr mode
information from the spinand_proto enum.

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 2 ++
include/linux/mtd/spinand.h | 30 ++++++++++++++++++++++++++++++
2 files changed, 32 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 446ba8d43fbc..a4f25649e293 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1153,6 +1153,7 @@ static void spinand_mtd_resume(struct mtd_info *mtd)
struct spinand_device *spinand = mtd_to_spinand(mtd);
int ret;

+ spinand->reg_proto = SPINAND_SINGLE_STR;
ret = spinand_reset_op(spinand);
if (ret)
return;
@@ -1179,6 +1180,7 @@ static int spinand_init(struct spinand_device *spinand)
if (!spinand->scratchbuf)
return -ENOMEM;

+ spinand->reg_proto = SPINAND_SINGLE_STR;
ret = spinand_detect(spinand);
if (ret)
goto err_free_bufs;
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6988956b8492..f6093cd98d7b 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -140,6 +140,31 @@
SPI_MEM_OP_NO_DUMMY, \
SPI_MEM_OP_DATA_OUT(len, buf, 4))

+#define SPINAND_PROTO_BUSWIDTH_MASK GENMASK(6, 0)
+#define SPINAND_PROTO_DTR_BIT BIT(7)
+
+#define SPINAND_PROTO_STR(__buswidth) \
+ ((u8)(((__buswidth) - 1) & SPINAND_PROTO_BUSWIDTH_MASK))
+#define SPINAND_PROTO_DTR(__buswidth) \
+ (SPINAND_PROTO_DTR_BIT | SPINAND_PROTO_STR(__buswidth))
+
+#define SPINAND_PROTO_BUSWIDTH(__proto) \
+ ((u8)(((__proto) & SPINAND_PROTO_BUSWIDTH_MASK) + 1))
+#define SPINAND_PROTO_IS_DTR(__proto) (!!((__proto) & SPINAND_PROTO_DTR_BIT))
+
+/**
+ * enum spinand_proto - List allowable SPI protocol variants for read reg,
+ * write reg, blk erase, write enable/disable, page read
+ * and program exec operations.
+ */
+enum spinand_proto {
+ SPINAND_SINGLE_STR = SPINAND_PROTO_STR(1),
+ SPINAND_DUAL_STR = SPINAND_PROTO_STR(2),
+ SPINAND_QUAD_STR = SPINAND_PROTO_STR(4),
+ SPINAND_OCTAL_STR = SPINAND_PROTO_STR(8),
+ SPINAND_OCTAL_DTR = SPINAND_PROTO_DTR(8),
+};
+
/**
* Standard SPI NAND flash commands
*/
@@ -407,6 +432,9 @@ struct spinand_dirmap {
* this die. Only required if your chip exposes several dies
* @cur_target: currently selected target/die
* @eccinfo: on-die ECC information
+ * @reg_proto: select a variant of SPI IO protocol (single, quad, octal or
+ * octal DTR) for read_reg/write_reg/erase operations. Update on
+ * successful transition into a different SPI IO protocol.
* @cfg_cache: config register cache. One entry per die
* @databuf: bounce buffer for data
* @oobbuf: bounce buffer for OOB data
@@ -438,6 +466,8 @@ struct spinand_device {

struct spinand_ecc_info eccinfo;

+ enum spinand_proto reg_proto;
+
u8 *cfg_cache;
u8 *databuf;
u8 *oobbuf;
--
2.17.1

2021-07-13 13:07:19

by Apurva Nandan

Subject: [PATCH 03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

Currently, the op macros in spinand.h don't give the option to setup
any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
Having a function that setups the op based on reg_proto would be
better than trying to write all the setup logic in op macros.

Create a spimem_setup_op() that would setup cmd, addr, dummy and data
phase of the spi_mem op, for the given spinand->reg_proto. And hence,
call the spimem_setup_op() before executing any spi_mem op.

Note: In this commit, spimem_setup_op() isn't called in the
read_reg_op(), write_reg_op() and wait() functions, as they need
modifications in address value and data nbytes when in Octal DTR mode.
This will be fixed in a later commit.

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 51 +++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index a4f25649e293..2e59faecc8f5 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -20,6 +20,51 @@
#include <linux/spi/spi.h>
#include <linux/spi/spi-mem.h>

+/**
+ * spinand_setup_op() - Helper function to setup the spi_mem op based on the
+ * spinand->reg_proto
+ * @spinand: the spinand device
+ * @op: the spi_mem op to setup
+ *
+ * Set up buswidth and dtr fields for cmd, addr, dummy and data phase. Also
+ * adjust cmd opcode and dummy nbytes. This function doesn't make any changes
+ * to addr val or data buf.
+ */
+static void spinand_setup_op(const struct spinand_device *spinand,
+ struct spi_mem_op *op)
+{
+ u8 op_buswidth = SPINAND_PROTO_BUSWIDTH(spinand->reg_proto);
+ u8 op_is_dtr = SPINAND_PROTO_IS_DTR(spinand->reg_proto);
+
+ if (spinand->reg_proto == SPINAND_SINGLE_STR)
+ return;
+
+ op->cmd.buswidth = op_buswidth;
+ op->cmd.dtr = op_is_dtr;
+ if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
+ op->cmd.opcode = (op->cmd.opcode << 8) | op->cmd.opcode;
+ op->cmd.nbytes = 2;
+ }
+
+ if (op->addr.nbytes) {
+ op->addr.buswidth = op_buswidth;
+ op->addr.dtr = op_is_dtr;
+ }
+
+ if (op->dummy.nbytes) {
+ op->dummy.buswidth = op_buswidth;
+ if (op_is_dtr) {
+ op->dummy.nbytes *= 2;
+ op->dummy.dtr = true;
+ }
+ }
+
+ if (op->data.nbytes) {
+ op->data.buswidth = op_buswidth;
+ op->data.dtr = op_is_dtr;
+ }
+}
+
static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
{
struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg,
@@ -341,6 +386,7 @@ static int spinand_write_enable_op(struct spinand_device *spinand)
{
struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);

+ spinand_setup_op(spinand, &op);
return spi_mem_exec_op(spinand->spimem, &op);
}

@@ -351,6 +397,7 @@ static int spinand_load_page_op(struct spinand_device *spinand,
unsigned int row = nanddev_pos_to_row(nand, &req->pos);
struct spi_mem_op op = SPINAND_PAGE_READ_OP(row);

+ spinand_setup_op(spinand, &op);
return spi_mem_exec_op(spinand->spimem, &op);
}

@@ -475,6 +522,7 @@ static int spinand_program_op(struct spinand_device *spinand,
unsigned int row = nanddev_pos_to_row(nand, &req->pos);
struct spi_mem_op op = SPINAND_PROG_EXEC_OP(row);

+ spinand_setup_op(spinand, &op);
return spi_mem_exec_op(spinand->spimem, &op);
}

@@ -485,6 +533,7 @@ static int spinand_erase_op(struct spinand_device *spinand,
unsigned int row = nanddev_pos_to_row(nand, pos);
struct spi_mem_op op = SPINAND_BLK_ERASE_OP(row);

+ spinand_setup_op(spinand, &op);
return spi_mem_exec_op(spinand->spimem, &op);
}

@@ -531,6 +580,7 @@ static int spinand_read_id_op(struct spinand_device *spinand, u8 naddr,
naddr, ndummy, spinand->scratchbuf, SPINAND_MAX_ID_LEN);
int ret;

+ spinand_setup_op(spinand, &op);
ret = spi_mem_exec_op(spinand->spimem, &op);
if (!ret)
memcpy(buf, spinand->scratchbuf, SPINAND_MAX_ID_LEN);
@@ -543,6 +593,7 @@ static int spinand_reset_op(struct spinand_device *spinand)
struct spi_mem_op op = SPINAND_RESET_OP;
int ret;

+ spinand_setup_op(spinand, &op);
ret = spi_mem_exec_op(spinand->spimem, &op);
if (ret)
return ret;
--
2.17.1

2021-07-13 13:07:25

by Apurva Nandan

Subject: [PATCH 04/13] mtd: spinand: Fix odd byte addr and data phase in read/write reg op and write VCR op for Octal DTR mode

In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock
cycle, and half-cycle instruction phases aren't supported yet. So,
every DTR spi_mem_op needs to have even nbytes in all phases for
non-erratic behaviour from the SPI controller.

The odd length cmd and dummy phases get handled by spimem_setup_op()
but the odd length address and data phases need to be handled according
to the use case. For example in Octal DTR mode, read register operation
has one byte long address and data phase. So it needs to extend it
by adding a suitable extra byte in addr and reading 2 bytes of data,
discarding the second byte.

Handle address and data phases for Octal DTR mode in read/write
register and write volatile configuration register operations
by adding a suitable extra byte in the address and data phase.

Create spimem_setup_reg_op() helper function to ease setting up
read/write register operations in other functions, e.g. wait().

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 2e59faecc8f5..a5334ad34f96 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -65,12 +65,27 @@ static void spinand_setup_op(const struct spinand_device *spinand,
}
}

+static void spinand_setup_reg_op(const struct spinand_device *spinand,
+ struct spi_mem_op *op)
+{
+ if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
+ /*
+ * Assigning same first and second byte will result in constant
+ * bits on ths SPI bus between positive and negative clock edges
+ */
+ op->addr.val = (op->addr.val << 8) | op->addr.val;
+ op->addr.nbytes = 2;
+ op->data.nbytes = 2;
+ }
+ spinand_setup_op(spinand, op);
+}
+
static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
{
- struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg,
- spinand->scratchbuf);
+ struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg, spinand->scratchbuf);
int ret;

+ spinand_setup_reg_op(spinand, &op);
ret = spi_mem_exec_op(spinand->spimem, &op);
if (ret)
return ret;
@@ -81,10 +96,10 @@ static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)

static int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8 val)
{
- struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg,
- spinand->scratchbuf);
+ struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg, spinand->scratchbuf);

- *spinand->scratchbuf = val;
+ spinand_setup_reg_op(spinand, &op);
+ memset(spinand->scratchbuf, val, op.data.nbytes);
return spi_mem_exec_op(spinand->spimem, &op);
}

@@ -547,6 +562,7 @@ static int spinand_wait(struct spinand_device *spinand,
u8 status;
int ret;

+ spinand_setup_reg_op(spinand, &op);
ret = spi_mem_poll_status(spinand->spimem, &op, STATUS_BUSY, 0,
initial_delay_us,
poll_delay_us,
--
2.17.1

2021-07-13 13:07:35

by Apurva Nandan

Subject: [PATCH 07/13] mtd: spinand: Allow enabling Octal DTR mode in the core

Enable Octal DTR SPI mode, i.e. 8D-8D-8D mode, if the SPI NAND flash
device supports it. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes
(1S-1D-8D), etc. aren't supported yet.

The method to switch to Octal DTR SPI mode may vary across
manufacturers. For example, for Winbond, it is enabled by writing
values to the volatile configuration register. So, let the
manufacturer's code have their own implementation for switching to
Octal DTR SPI mode. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes
(1S-1D-8D), etc. aren't supported yet.

Check for the SPI NAND device's support for Octal DTR mode using
spinand flags, and if the op_templates allow 8D-8D-8D, call
octal_dtr_enable() manufacturer op. If the SPI controller doesn't
supports these modes, the selected op_templates would prevent switching
to the Octal DTR mode. And finally update the spinand reg_proto
if success.

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 46 +++++++++++++++++++++++++++++++++++++
include/linux/mtd/spinand.h | 3 +++
2 files changed, 49 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 1e619b6d777f..19d8affac058 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -256,6 +256,48 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
enable ? CFG_QUAD_ENABLE : 0);
}

+static bool spinand_op_is_octal_dtr(const struct spi_mem_op *op)
+{
+ return op->cmd.buswidth == 8 && op->cmd.dtr &&
+ op->addr.buswidth == 8 && op->addr.dtr &&
+ op->data.buswidth == 8 && op->data.dtr;
+}
+
+static int spinand_init_octal_dtr_enable(struct spinand_device *spinand)
+{
+ struct device *dev = &spinand->spimem->spi->dev;
+ int ret;
+
+ if (!(spinand->flags & SPINAND_HAS_OCTAL_DTR_BIT))
+ return 0;
+
+ if (!(spinand_op_is_octal_dtr(spinand->op_templates.read_cache) &&
+ spinand_op_is_octal_dtr(spinand->op_templates.write_cache) &&
+ spinand_op_is_octal_dtr(spinand->op_templates.update_cache)))
+ return 0;
+
+ if (!spinand->manufacturer->ops->octal_dtr_enable) {
+ dev_err(dev,
+ "Missing ->octal_dtr_enable(), unable to switch mode\n");
+ return -EINVAL;
+ }
+
+ ret = spinand->manufacturer->ops->octal_dtr_enable(spinand);
+ if (ret) {
+ dev_err(dev,
+ "Failed to enable Octal DTR SPI mode (err = %d)\n",
+ ret);
+ return ret;
+ }
+
+ spinand->reg_proto = SPINAND_OCTAL_DTR;
+
+ dev_dbg(dev,
+ "%s SPI NAND switched to Octal DTR SPI (8D-8D-8D) mode\n",
+ spinand->manufacturer->name);
+ return 0;
+}
+
static int spinand_ecc_enable(struct spinand_device *spinand,
bool enable)
{
@@ -1189,6 +1231,10 @@ static int spinand_init_flash(struct spinand_device *spinand)
if (ret)
return ret;

+ ret = spinand_init_octal_dtr_enable(spinand);
+ if (ret)
+ return ret;
+
ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
if (ret)
return ret;
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 35816b8cfe81..daa2ac5c3110 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -271,6 +271,7 @@ struct spinand_devid {
* @init: initialize a SPI NAND device
* @adjust_op: modify the ops for any variation in their cmd, address, dummy or
* data phase by the manufacturer
+ * @octal_dtr_enable: switch the SPI NAND flash into Octal DTR SPI mode
* @cleanup: cleanup a SPI NAND device
*
* Each SPI NAND manufacturer driver should implement this interface so that
@@ -280,6 +281,7 @@ struct spinand_manufacturer_ops {
int (*init)(struct spinand_device *spinand);
void (*adjust_op)(struct spi_mem_op *op,
const enum spinand_proto reg_proto);
+ int (*octal_dtr_enable)(struct spinand_device *spinand);
void (*cleanup)(struct spinand_device *spinand);
};

@@ -348,6 +350,7 @@ struct spinand_ecc_info {

#define SPINAND_HAS_QE_BIT BIT(0)
#define SPINAND_HAS_CR_FEAT_BIT BIT(1)
+#define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)

/**
* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
--
2.17.1

2021-07-13 13:07:42

by Apurva Nandan

Subject: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

The SPI NAND core doesn't know how to switch the flash to Octal DTR
mode (i.e. which operations to perform). If the manufacturer hasn't
implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
wouldn't be able to switch to 8D-8D-8D mode and will also not be able
to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
cache op_templates.

So, avoid choosing a Octal DTR SPI op_template for read_cache,
write_cache and update_cache operations, if the manufacturer_op
octal_dtr_enable() is missing.

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 19d8affac058..8711e887b795 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
if (id[0] != manufacturer->id)
continue;

+ spinand->manufacturer = manufacturer;
+
ret = spinand_match_and_init(spinand,
manufacturer->chips,
manufacturer->nchips,
@@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
if (ret < 0)
continue;

- spinand->manufacturer = manufacturer;
return 0;
}
return -ENOTSUPP;
@@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
unsigned int nbytes;
int ret;

+ if (spinand_op_is_octal_dtr(&op) &&
+ !spinand->manufacturer->ops->octal_dtr_enable)
+ continue;
+
nbytes = nanddev_per_page_oobsize(nand) +
nanddev_page_size(nand);

--
2.17.1

2021-07-13 13:07:56

by Apurva Nandan

Subject: [PATCH 10/13] mtd: spinand: Add octal_dtr_enable() for Winbond manufacturer_ops

Add implementation of octal_dtr_enable() manufacturer_ops for Winbond.
To switch to Ocatl DTR mode, setting programmable dummy cycles and
SPI IO mode using the volatile configuration register is required. To
function at max 120MHz SPI clock in Octal DTR mode, 12 programmable
dummy clock cycle setting is required. (Default number of dummy cycle
are 8 clocks)

Set the programmable dummy cycle to 12 clocks, and SPI IO mode to
Octal DTR with Data Strobe in the VCR. Also, perform a READ ID
operation in Octal DTR SPI mode to ensure the switch was successful.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index a7052a9ca171..58cda07c15a0 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -16,6 +16,14 @@

#define WINBOND_CFG_BUF_READ BIT(3)

+/* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
+#define WINBOND_IO_MODE_VCR_OCTAL_DTR 0xE7
+#define WINBOND_IO_MODE_VCR_ADDR 0x00
+
+/* Use 12 dummy clk cycles for using Octal DTR SPI at max 120MHZ */
+#define WINBOND_DUMMY_CLK_COUNT 12
+#define WINBOND_DUMMY_CLK_VCR_ADDR 0x01
+
static SPINAND_OP_VARIANTS(read_cache_variants,
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
@@ -142,8 +150,42 @@ static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
return 0;
}

+static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
+{
+ int ret;
+ struct spi_mem_op op;
+
+ ret = winbond_write_vcr_op(spinand, WINBOND_DUMMY_CLK_VCR_ADDR,
+ WINBOND_DUMMY_CLK_COUNT);
+ if (ret)
+ return ret;
+
+ ret = winbond_write_vcr_op(spinand, WINBOND_IO_MODE_VCR_ADDR,
+ WINBOND_IO_MODE_VCR_OCTAL_DTR);
+ if (ret)
+ return ret;
+
+ /* Read flash ID to make sure the switch was successful. */
+ op = (struct spi_mem_op)
+ SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9f9f, 8),
+ SPI_MEM_OP_NO_ADDR,
+ SPI_MEM_OP_DUMMY_DTR(16, 8),
+ SPI_MEM_OP_DATA_IN_DTR(SPINAND_MAX_ID_LEN,
+ spinand->scratchbuf, 8));
+
+ ret = spi_mem_exec_op(spinand->spimem, &op);
+ if (ret)
+ return ret;
+
+ if (memcmp(spinand->scratchbuf, spinand->id.data, SPINAND_MAX_ID_LEN))
+ return -EINVAL;
+
+ return 0;
+}
+
static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
.init = winbond_spinand_init,
+ .octal_dtr_enable = winbond_spinand_octal_dtr_enable,
};

const struct spinand_manufacturer winbond_spinand_manufacturer = {
--
2.17.1

2021-07-13 13:08:04

by Apurva Nandan

Subject: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
functionality in their SPI NAND flash chips. PoR instruction consists
of a 66h command followed by 99h command, and is different from the FFh
reset. The reset command FFh just clears the status only registers,
while the PoR command erases all the configurations written to the
flash and is equivalent to a power-down -> power-up cycle.

Add support for the Power-on-Reset command for any flash that provides
this feature.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 43 +++++++++++++++++++++++++++++++++++++
include/linux/mtd/spinand.h | 17 +++++++++++++++
2 files changed, 60 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index f577e72da2c4..608f4eb85b0a 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -9,6 +9,7 @@

#define pr_fmt(fmt) "spi-nand: " fmt

+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/jiffies.h>
#include <linux/kernel.h>
@@ -665,6 +666,48 @@ static int spinand_reset_op(struct spinand_device *spinand)
NULL);
}

+static int spinand_power_on_rst_op(struct spinand_device *spinand)
+{
+ struct spi_mem_op op;
+ int ret;
+
+ if (!(spinand->flags & SPINAND_HAS_POR_CMD_BIT))
+ return -EOPNOTSUPP;
+
+ /*
+ * If flash is in a busy state, wait for it to finish the operation.
+ * As the operation is unknown, use reset poll delays here.
+ */
+ ret = spinand_wait(spinand,
+ SPINAND_RESET_INITIAL_DELAY_US,
+ SPINAND_RESET_POLL_DELAY_US,
+ NULL);
+ if (ret)
+ return ret;
+
+ op = (struct spi_mem_op)SPINAND_EN_POWER_ON_RST_OP;
+
+ spinand_setup_op(spinand, &op);
+ ret = spi_mem_exec_op(spinand->spimem, &op);
+ if (ret)
+ return ret;
+
+ op = (struct spi_mem_op)SPINAND_POWER_ON_RST_OP;
+
+ spinand_setup_op(spinand, &op);
+ ret = spi_mem_exec_op(spinand->spimem, &op);
+ if (ret)
+ return ret;
+
+ /* PoR can take max 500 us to complete, so sleep for 1000 to 1200 us*/
+ usleep_range(SPINAND_POR_MIN_DELAY_US, SPINAND_POR_MAX_DELAY_US);
+
+ dev_dbg(&spinand->spimem->spi->dev,
+ "%s SPI NAND reset to Power-On-Reset state.\n",
+ spinand->manufacturer->name);
+ return 0;
+}
+
static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
{
return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 21a4e5adcd59..7a97bd2b42cc 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -26,6 +26,18 @@
SPI_MEM_OP_NO_DUMMY, \
SPI_MEM_OP_NO_DATA)

+#define SPINAND_EN_POWER_ON_RST_OP \
+ SPI_MEM_OP(SPI_MEM_OP_CMD(0x66, 1), \
+ SPI_MEM_OP_NO_ADDR, \
+ SPI_MEM_OP_NO_DUMMY, \
+ SPI_MEM_OP_NO_DATA)
+
+#define SPINAND_POWER_ON_RST_OP \
+ SPI_MEM_OP(SPI_MEM_OP_CMD(0x99, 1), \
+ SPI_MEM_OP_NO_ADDR, \
+ SPI_MEM_OP_NO_DUMMY, \
+ SPI_MEM_OP_NO_DATA)
+
#define SPINAND_WR_EN_DIS_OP(enable) \
SPI_MEM_OP(SPI_MEM_OP_CMD((enable) ? 0x06 : 0x04, 1), \
SPI_MEM_OP_NO_ADDR, \
@@ -218,6 +230,8 @@ struct spinand_device;
* reading/programming/erasing when the RESET occurs. Since we always
* issue a RESET when the device is IDLE, 5us is selected for both initial
* and poll delay.
+ * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us
+ * to 1200 us safely.
*/
#define SPINAND_READ_INITIAL_DELAY_US 6
#define SPINAND_READ_POLL_DELAY_US 5
@@ -227,6 +241,8 @@ struct spinand_device;
#define SPINAND_WRITE_POLL_DELAY_US 15
#define SPINAND_ERASE_INITIAL_DELAY_US 250
#define SPINAND_ERASE_POLL_DELAY_US 50
+#define SPINAND_POR_MIN_DELAY_US 1000
+#define SPINAND_POR_MAX_DELAY_US 1200

#define SPINAND_WAITRDY_TIMEOUT_MS 400

@@ -351,6 +367,7 @@ struct spinand_ecc_info {
#define SPINAND_HAS_QE_BIT BIT(0)
#define SPINAND_HAS_CR_FEAT_BIT BIT(1)
#define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
+#define SPINAND_HAS_POR_CMD_BIT BIT(3)

/**
* struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
--
2.17.1

2021-07-13 13:08:20

by Apurva Nandan

Subject: [PATCH 12/13] mtd: spinand: Perform Power-on-Reset when runtime_pm suspend is issued

A soft reset using FFh command doesn't erase the flash's configuration
and doesn't reset the SPI IO mode also. This can result in the flash
being in a different SPI IO mode, e.g. Octal DTR, when resuming from
sleep. This would render the flash in an unusable state.

Perform a Power-on-Reset (PoR), if available in the flash, when
suspending the device by runtime_pm. This would set the flash to clean
state for reinitialization during resume and would also ensure that it
is in standard SPI IO mode (1S-1S-1S) before the resume begins.

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 608f4eb85b0a..6fb3aa6af540 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1329,6 +1329,21 @@ static void spinand_mtd_resume(struct mtd_info *mtd)
spinand_ecc_enable(spinand, false);
}

+static int spinand_mtd_suspend(struct mtd_info *mtd)
+{
+ struct spinand_device *spinand = mtd_to_spinand(mtd);
+ int ret;
+
+ if (!(spinand->flags & SPINAND_HAS_POR_CMD_BIT))
+ return 0;
+
+ ret = spinand_power_on_rst_op(spinand);
+ if (ret)
+ dev_err(&spinand->spimem->spi->dev, "suspend() failed\n");
+
+ return ret;
+}
+
static int spinand_init(struct spinand_device *spinand)
{
struct device *dev = &spinand->spimem->spi->dev;
@@ -1401,6 +1416,7 @@ static int spinand_init(struct spinand_device *spinand)
mtd->_erase = spinand_mtd_erase;
mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
mtd->_resume = spinand_mtd_resume;
+ mtd->_suspend = spinand_mtd_suspend;

if (nand->ecc.engine) {
ret = mtd_ooblayout_count_freebytes(mtd);
--
2.17.1

2021-07-13 13:08:36

by Apurva Nandan

Subject: [PATCH 06/13] mtd: spinand: Add macros for Octal DTR page read and write operations

Define new PAGE_READ_FROM_CACHE and PROG_LOAD op templates for Octal
DTR SPI mode. These templates would used in op_variants and
op_templates for defining Octal DTR read from cache and write to
cache operations.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by: Apurva Nandan <[email protected]>
---
include/linux/mtd/spinand.h | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index ebb19b2cec84..35816b8cfe81 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -122,6 +122,12 @@
SPI_MEM_OP_DUMMY(ndummy, 4), \
SPI_MEM_OP_DATA_IN(len, buf, 4))

+#define SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(addr, ndummy, buf, len) \
+ SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9d9d, 8), \
+ SPI_MEM_OP_ADDR_DTR(2, addr, 8), \
+ SPI_MEM_OP_DUMMY_DTR(ndummy, 8), \
+ SPI_MEM_OP_DATA_IN_DTR(len, buf, 8))
+
#define SPINAND_PROG_EXEC_OP(addr) \
SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1), \
SPI_MEM_OP_ADDR(3, addr, 1), \
@@ -140,6 +146,12 @@
SPI_MEM_OP_NO_DUMMY, \
SPI_MEM_OP_DATA_OUT(len, buf, 4))

+#define SPINAND_PROG_LOAD_OCTALIO_DTR(reset, addr, buf, len) \
+ SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, reset ? 0x0202 : 0x8484, 8), \
+ SPI_MEM_OP_ADDR_DTR(2, addr, 8), \
+ SPI_MEM_OP_NO_DUMMY, \
+ SPI_MEM_OP_DATA_OUT_DTR(len, buf, 8))
+
#define SPINAND_PROTO_BUSWIDTH_MASK GENMASK(6, 0)
#define SPINAND_PROTO_DTR_BIT BIT(7)

--
2.17.1

2021-07-13 13:09:18

by Apurva Nandan

Subject: [PATCH 09/13] mtd: spinand: Add support for write volatile configuration register op

Volatile configuration register are a different set of configuration
registers, i.e. they differ from the status registers. A different
SPI instruction is required to write to these registers. Any changes
to the Volatile Configuration Register get transferred directly to
the Internal Configuration Register and instantly reflect on the
device operation.

In Winbond W35N01JW, these volatile configuration register must be
configured in order to switch to Octal DTR SPI mode.

Add support for writing to volatile configuration registers using a
new WRITE_VCR_OP template.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 2 +-
drivers/mtd/nand/spi/winbond.c | 28 ++++++++++++++++++++++++++++
include/linux/mtd/spinand.h | 1 +
3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 8711e887b795..f577e72da2c4 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -442,7 +442,7 @@ static void spinand_ondie_ecc_save_status(struct nand_device *nand, u8 status)
engine_conf->status = status;
}

-static int spinand_write_enable_op(struct spinand_device *spinand)
+int spinand_write_enable_op(struct spinand_device *spinand)
{
struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index 76684428354e..a7052a9ca171 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -7,6 +7,7 @@
* Boris Brezillon <[email protected]>
*/

+#include <linux/delay.h>
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/mtd/spinand.h>
@@ -114,6 +115,33 @@ static int winbond_spinand_init(struct spinand_device *spinand)
return 0;
}

+static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
+{
+ int ret;
+ struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x81, 1),
+ SPI_MEM_OP_ADDR(3, reg, 1),
+ SPI_MEM_OP_NO_DUMMY,
+ SPI_MEM_OP_DATA_OUT(1, spinand->scratchbuf, 1));
+
+ *spinand->scratchbuf = val;
+
+ ret = spinand_write_enable_op(spinand);
+ if (ret)
+ return ret;
+
+ ret = spi_mem_exec_op(spinand->spimem, &op);
+ if (ret)
+ return ret;
+
+ /*
+ * Write VCR operation doesn't set the busy bit in SR, so can't perform
+ * a status poll. Minimum time of 50ns is needed to complete the write.
+ * So, give thrice the minimum required delay.
+ */
+ ndelay(150);
+ return 0;
+}
+
static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
.init = winbond_spinand_init,
};
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index daa2ac5c3110..21a4e5adcd59 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -560,5 +560,6 @@ int spinand_match_and_init(struct spinand_device *spinand,

int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
int spinand_select_target(struct spinand_device *spinand, unsigned int target);
+int spinand_write_enable_op(struct spinand_device *spinand);

#endif /* __LINUX_MTD_SPINAND_H */
--
2.17.1

2021-07-13 13:09:27

by Apurva Nandan

Subject: [PATCH 13/13] mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash

Winbond W35N01JW is SPI NAND flash supporting Octal DTR SPI protocol.
Add op_vairants for W35N01JW, which include the Octal DTR read/write
page ops as well. Add W35N01JW's oob layout functions for the
mtd_ooblayout_ops. Add all op adjustments required for Octal DTR SPI
mode using the adjust_op(). Finally, add an entry for W35N01JW in
spinand_info table.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/winbond.c | 116 ++++++++++++++++++++++++++++++---
1 file changed, 107 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
index 58cda07c15a0..5c2b9e61b624 100644
--- a/drivers/mtd/nand/spi/winbond.c
+++ b/drivers/mtd/nand/spi/winbond.c
@@ -16,6 +16,13 @@

#define WINBOND_CFG_BUF_READ BIT(3)

+#define WINBOND_BLK_ERASE_OPCODE 0xD8
+#define WINBOND_PAGE_READ_OPCODE 0x13
+#define WINBOND_PROG_EXEC_OPCODE 0x10
+#define WINBOND_READ_REG_OPCODE_1 0x05
+#define WINBOND_READ_REG_OPCODE_2 0x0F
+#define WINBOND_READ_VCR_OPCODE 0x85
+
/* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
#define WINBOND_IO_MODE_VCR_OCTAL_DTR 0xE7
#define WINBOND_IO_MODE_VCR_ADDR 0x00
@@ -24,7 +31,7 @@
#define WINBOND_DUMMY_CLK_COUNT 12
#define WINBOND_DUMMY_CLK_VCR_ADDR 0x01

-static SPINAND_OP_VARIANTS(read_cache_variants,
+static SPINAND_OP_VARIANTS(read_cache_variants_w25xxgv,
SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
@@ -32,14 +39,27 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));

-static SPINAND_OP_VARIANTS(write_cache_variants,
+static SPINAND_OP_VARIANTS(write_cache_variants_w25xxgv,
SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
SPINAND_PROG_LOAD(true, 0, NULL, 0));

-static SPINAND_OP_VARIANTS(update_cache_variants,
+static SPINAND_OP_VARIANTS(update_cache_variants_w25xxgv,
SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
SPINAND_PROG_LOAD(false, 0, NULL, 0));

+static SPINAND_OP_VARIANTS(read_cache_variants_w35n01jw,
+ SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
+ SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
+ SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
+
+static SPINAND_OP_VARIANTS(write_cache_variants_w35n01jw,
+ SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
+ SPINAND_PROG_LOAD(true, 0, NULL, 0));
+
+static SPINAND_OP_VARIANTS(update_cache_variants_w35n01jw,
+ SPINAND_PROG_LOAD_OCTALIO_DTR(false, 0, NULL, 0),
+ SPINAND_PROG_LOAD(false, 0, NULL, 0));
+
static int w25m02gv_ooblayout_ecc(struct mtd_info *mtd, int section,
struct mtd_oob_region *region)
{
@@ -64,11 +84,40 @@ static int w25m02gv_ooblayout_free(struct mtd_info *mtd, int section,
return 0;
}

+static int w35n01jw_ooblayout_ecc(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
+{
+ if (section > 7)
+ return -ERANGE;
+
+ region->offset = (16 * section) + 12;
+ region->length = 4;
+
+ return 0;
+}
+
+static int w35n01jw_ooblayout_free(struct mtd_info *mtd, int section,
+ struct mtd_oob_region *region)
+{
+ if (section > 7)
+ return -ERANGE;
+
+ region->offset = (16 * section) + 2;
+ region->length = 10;
+
+ return 0;
+}
+
static const struct mtd_ooblayout_ops w25m02gv_ooblayout = {
.ecc = w25m02gv_ooblayout_ecc,
.free = w25m02gv_ooblayout_free,
};

+static const struct mtd_ooblayout_ops w35n01jw_ooblayout = {
+ .ecc = w35n01jw_ooblayout_ecc,
+ .free = w35n01jw_ooblayout_free,
+};
+
static int w25m02gv_select_target(struct spinand_device *spinand,
unsigned int target)
{
@@ -88,9 +137,9 @@ static const struct spinand_info winbond_spinand_table[] = {
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xab),
NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 2),
NAND_ECCREQ(1, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w25xxgv,
+ &write_cache_variants_w25xxgv,
+ &update_cache_variants_w25xxgv),
0,
SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
SPINAND_SELECT_TARGET(w25m02gv_select_target)),
@@ -98,11 +147,22 @@ static const struct spinand_info winbond_spinand_table[] = {
SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xaa),
NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
NAND_ECCREQ(1, 512),
- SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
- &write_cache_variants,
- &update_cache_variants),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w25xxgv,
+ &write_cache_variants_w25xxgv,
+ &update_cache_variants_w25xxgv),
0,
SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
+ SPINAND_INFO("W35N01JW",
+ SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc),
+ NAND_MEMORG(1, 4096, 128, 64, 512, 20, 1, 1, 1),
+ NAND_ECCREQ(1, 512),
+ SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w35n01jw,
+ &write_cache_variants_w35n01jw,
+ &update_cache_variants_w35n01jw),
+ SPINAND_HAS_OCTAL_DTR_BIT | SPINAND_HAS_POR_CMD_BIT |
+ SPINAND_HAS_CR_FEAT_BIT,
+ SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL)),
+
};

static int winbond_spinand_init(struct spinand_device *spinand)
@@ -183,9 +243,47 @@ static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
return 0;
}

+static void winbond_spinand_adjust_op(struct spi_mem_op *op,
+ const enum spinand_proto reg_proto)
+{
+ /*
+ * To support both 1 byte opcode and 2 byte opcodes, extract the MSB
+ * byte from the opcode as the LSB byte in 2 byte opcode is treated as
+ * don't care.
+ */
+ u8 opcode = op->cmd.opcode >> (8 * (op->cmd.nbytes - 1));
+
+ if (reg_proto == SPINAND_OCTAL_DTR) {
+ switch (opcode) {
+ case WINBOND_READ_REG_OPCODE_1:
+ case WINBOND_READ_REG_OPCODE_2:
+ op->dummy.nbytes = 14;
+ op->dummy.buswidth = 8;
+ op->dummy.dtr = true;
+ return;
+
+ case WINBOND_READ_VCR_OPCODE:
+ op->dummy.nbytes = 16;
+ op->dummy.buswidth = 8;
+ op->dummy.dtr = true;
+ return;
+
+ case WINBOND_BLK_ERASE_OPCODE:
+ case WINBOND_PAGE_READ_OPCODE:
+ case WINBOND_PROG_EXEC_OPCODE:
+ op->addr.nbytes = 2;
+ return;
+
+ default:
+ return;
+ }
+ }
+}
+
static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
.init = winbond_spinand_init,
.octal_dtr_enable = winbond_spinand_octal_dtr_enable,
+ .adjust_op = winbond_spinand_adjust_op,
};

const struct spinand_manufacturer winbond_spinand_manufacturer = {
--
2.17.1

2021-07-13 13:09:46

by Apurva Nandan

Subject: [PATCH 05/13] mtd: spinand: Add adjust_op() in manufacturer_ops to modify the ops for manufacturer specific changes

Manufacturers might use a variation of standard SPI NAND flash
instructions, e.g. Winbond W35N01JW changes the dummy cycle length for
read register commands when in Octal DTR mode.

Add new function in manufacturer_ops: adjust_op(), which can be called
to correct the spi_mem op for any alteration in the instruction made by
the manufacturers. And hence, this function can also be used for
incorporating variations of SPI instructions in Octal DTR mode.

Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf

Signed-off-by: Apurva Nandan <[email protected]>
---
drivers/mtd/nand/spi/core.c | 3 +++
include/linux/mtd/spinand.h | 4 ++++
2 files changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index a5334ad34f96..1e619b6d777f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -63,6 +63,9 @@ static void spinand_setup_op(const struct spinand_device *spinand,
op->data.buswidth = op_buswidth;
op->data.dtr = op_is_dtr;
}
+
+ if (spinand->manufacturer->ops->adjust_op)
+ spinand->manufacturer->ops->adjust_op(op, spinand->reg_proto);
}

static void spinand_setup_reg_op(const struct spinand_device *spinand,
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index f6093cd98d7b..ebb19b2cec84 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -257,6 +257,8 @@ struct spinand_devid {
/**
* struct manufacurer_ops - SPI NAND manufacturer specific operations
* @init: initialize a SPI NAND device
+ * @adjust_op: modify the ops for any variation in their cmd, address, dummy or
+ * data phase by the manufacturer
* @cleanup: cleanup a SPI NAND device
*
* Each SPI NAND manufacturer driver should implement this interface so that
@@ -264,6 +266,8 @@ struct spinand_devid {
*/
struct spinand_manufacturer_ops {
int (*init)(struct spinand_device *spinand);
+ void (*adjust_op)(struct spi_mem_op *op,
+ const enum spinand_proto reg_proto);
void (*cleanup)(struct spinand_device *spinand);
};

--
2.17.1

2021-07-20 16:57:11

by Apurva Nandan

Subject: Re: [PATCH 00/13] mtd: spinand: Add Octal DTR SPI (8D-8D-8D) mode support

On 13-Jul-21 6:35 PM, Apurva Nandan wrote:
> Hi,
> This series proposes patches for adding the following functionality
> in SPI NAND core:
>
> - Octal DTR SPI (8D-8D-8D) mode support
>
> - Winbond W35N01JW SPI NAND chip support
>
> - Power-on-Reset instruction support
>
> This series has been tested on TI J721e EVM with the Winbond W35N01JW
> flash with following test utilities:
>
> - nandtest
> Test log: https://textbin.net/raw/fhypoz63f9
>
> - mtd_stresstest
> Test log: https://textbin.net/raw/0xqjmqntj7
>
> - UBIFS LTP stress test (NAND_XL_STRESS_DD_RW_UBIFS).
> Test log: https://textbin.net/raw/pyokws7wku
>
> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>
> Apurva Nandan (13):
> spi: spi-mem: Add DTR templates for cmd, address, dummy and data phase
> mtd: spinand: Add enum spinand_proto to indicate current SPI IO mode
> mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto
> mtd: spinand: Fix odd byte addr and data phase in read/write reg op
> and write VCR op for Octal DTR mode
> mtd: spinand: Add adjust_op() in manufacturer_ops to modify the ops
> for manufacturer specific changes
> mtd: spinand: Add macros for Octal DTR page read and write operations
> mtd: spinand: Allow enabling Octal DTR mode in the core
> mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is
> missing in manufacturer_op
> mtd: spinand: Add support for write volatile configuration register op
> mtd: spinand: Add octal_dtr_enable() for Winbond manufacturer_ops
> mtd: spinand: Add support for Power-on-Reset (PoR) instruction
> mtd: spinand: Perform Power-on-Reset when runtime_pm suspend is issued
> mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash
>
> drivers/mtd/nand/spi/core.c | 196 +++++++++++++++++++++++++++++++--
> drivers/mtd/nand/spi/winbond.c | 186 +++++++++++++++++++++++++++++--
> include/linux/mtd/spinand.h | 67 +++++++++++
> include/linux/spi/spi-mem.h | 87 ++++++++++-----
> 4 files changed, 494 insertions(+), 42 deletions(-)
>

Hi,
Can anyone please provide some comments/suggestions/reviews?

Thanks,
Apurva Nandan

2021-08-07 00:02:08

by Miquel Raynal

Subject: Re: [PATCH 03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:28
+0000:

> Currently, the op macros in spinand.h don't give the option to setup
> any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
> Having a function that setups the op based on reg_proto would be
> better than trying to write all the setup logic in op macros.
>
> Create a spimem_setup_op() that would setup cmd, addr, dummy and data
> phase of the spi_mem op, for the given spinand->reg_proto. And hence,
> call the spimem_setup_op() before executing any spi_mem op.
>
> Note: In this commit, spimem_setup_op() isn't called in the
> read_reg_op(), write_reg_op() and wait() functions, as they need
> modifications in address value and data nbytes when in Octal DTR mode.
> This will be fixed in a later commit.

Thanks for this series!

So far I am fine with your changes, but I don't like the setup_op()
naming much. I don't yet have a better idea, could you propose
something more meaningful?

> Signed-off-by: Apurva Nandan <[email protected]>

Thanks,
Miquèl

2021-08-07 00:04:14

by Miquel Raynal

Subject: Re: [PATCH 06/13] mtd: spinand: Add macros for Octal DTR page read and write operations

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:31
+0000:

> Define new PAGE_READ_FROM_CACHE and PROG_LOAD op templates for Octal
> DTR SPI mode. These templates would used in op_variants and

will be

> op_templates for defining Octal DTR read from cache and write to
> cache operations.
>
> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> include/linux/mtd/spinand.h | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index ebb19b2cec84..35816b8cfe81 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -122,6 +122,12 @@
> SPI_MEM_OP_DUMMY(ndummy, 4), \
> SPI_MEM_OP_DATA_IN(len, buf, 4))
>
> +#define SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(addr, ndummy, buf, len) \
> + SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9d9d, 8), \
> + SPI_MEM_OP_ADDR_DTR(2, addr, 8), \
> + SPI_MEM_OP_DUMMY_DTR(ndummy, 8), \
> + SPI_MEM_OP_DATA_IN_DTR(len, buf, 8))
> +
> #define SPINAND_PROG_EXEC_OP(addr) \
> SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1), \
> SPI_MEM_OP_ADDR(3, addr, 1), \
> @@ -140,6 +146,12 @@
> SPI_MEM_OP_NO_DUMMY, \
> SPI_MEM_OP_DATA_OUT(len, buf, 4))
>
> +#define SPINAND_PROG_LOAD_OCTALIO_DTR(reset, addr, buf, len) \
> + SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, reset ? 0x0202 : 0x8484, 8), \
> + SPI_MEM_OP_ADDR_DTR(2, addr, 8), \
> + SPI_MEM_OP_NO_DUMMY, \
> + SPI_MEM_OP_DATA_OUT_DTR(len, buf, 8))
> +
> #define SPINAND_PROTO_BUSWIDTH_MASK GENMASK(6, 0)
> #define SPINAND_PROTO_DTR_BIT BIT(7)
>

Thanks,
Miquèl

2021-08-07 00:04:17

by Miquel Raynal

Subject: Re: [PATCH 04/13] mtd: spinand: Fix odd byte addr and data phase in read/write reg op and write VCR op for Octal DTR mode

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:29
+0000:

> In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock
> cycle, and half-cycle instruction phases aren't supported yet. So,
> every DTR spi_mem_op needs to have even nbytes in all phases for
> non-erratic behaviour from the SPI controller.
>
> The odd length cmd and dummy phases get handled by spimem_setup_op()
> but the odd length address and data phases need to be handled according
> to the use case. For example in Octal DTR mode, read register operation
> has one byte long address and data phase. So it needs to extend it
> by adding a suitable extra byte in addr and reading 2 bytes of data,
> discarding the second byte.
>
> Handle address and data phases for Octal DTR mode in read/write
> register and write volatile configuration register operations
> by adding a suitable extra byte in the address and data phase.
>
> Create spimem_setup_reg_op() helper function to ease setting up
> read/write register operations in other functions, e.g. wait().
>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 2e59faecc8f5..a5334ad34f96 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -65,12 +65,27 @@ static void spinand_setup_op(const struct spinand_device *spinand,
> }
> }
>
> +static void spinand_setup_reg_op(const struct spinand_device *spinand,
> + struct spi_mem_op *op)

Same remark about the naming. In fact I believe we could have this
logic in _setup_op() (or whatever its name) and add a specific
parameter for it?

> +{
> + if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
> + /*
> + * Assigning same first and second byte will result in constant
> + * bits on ths SPI bus between positive and negative clock edges

the

> + */
> + op->addr.val = (op->addr.val << 8) | op->addr.val;

I am not sure to understand what you do here?

> + op->addr.nbytes = 2;
> + op->data.nbytes = 2;
> + }

Space

> + spinand_setup_op(spinand, op);
> +}
> +
> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
> {
> - struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg,
> - spinand->scratchbuf);
> + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg, spinand->scratchbuf);

You can do this, but in a different commit.

> int ret;
>
> + spinand_setup_reg_op(spinand, &op);
> ret = spi_mem_exec_op(spinand->spimem, &op);
> if (ret)
> return ret;
> @@ -81,10 +96,10 @@ static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>
> static int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8 val)
> {
> - struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg,
> - spinand->scratchbuf);
> + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg, spinand->scratchbuf);

Same here.

>
> - *spinand->scratchbuf = val;
> + spinand_setup_reg_op(spinand, &op);
> + memset(spinand->scratchbuf, val, op.data.nbytes);
> return spi_mem_exec_op(spinand->spimem, &op);
> }
>
> @@ -547,6 +562,7 @@ static int spinand_wait(struct spinand_device *spinand,
> u8 status;
> int ret;
>
> + spinand_setup_reg_op(spinand, &op);
> ret = spi_mem_poll_status(spinand->spimem, &op, STATUS_BUSY, 0,
> initial_delay_us,
> poll_delay_us,

Thanks,
Miquèl

2021-08-07 00:04:47

by Miquel Raynal

Subject: Re: [PATCH 07/13] mtd: spinand: Allow enabling Octal DTR mode in the core

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:32
+0000:

> Enable Octal DTR SPI mode, i.e. 8D-8D-8D mode, if the SPI NAND flash
> device supports it. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes
> (1S-1D-8D), etc. aren't supported yet.
>
> The method to switch to Octal DTR SPI mode may vary across
> manufacturers. For example, for Winbond, it is enabled by writing
> values to the volatile configuration register. So, let the
> manufacturer's code have their own implementation for switching to
> Octal DTR SPI mode. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes
> (1S-1D-8D), etc. aren't supported yet.

You can drop the final sentence which is a repetition of the previous
paragraph.

> Check for the SPI NAND device's support for Octal DTR mode using
> spinand flags, and if the op_templates allow 8D-8D-8D, call
allows

> octal_dtr_enable() manufacturer op. If the SPI controller doesn't
> supports these modes, the selected op_templates would prevent switching

will

> to the Octal DTR mode. And finally update the spinand reg_proto
> if success.

on

>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/core.c | 46 +++++++++++++++++++++++++++++++++++++
> include/linux/mtd/spinand.h | 3 +++
> 2 files changed, 49 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 1e619b6d777f..19d8affac058 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -256,6 +256,48 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
> enable ? CFG_QUAD_ENABLE : 0);
> }
>
> +static bool spinand_op_is_octal_dtr(const struct spi_mem_op *op)
> +{
> + return op->cmd.buswidth == 8 && op->cmd.dtr &&
> + op->addr.buswidth == 8 && op->addr.dtr &&
> + op->data.buswidth == 8 && op->data.dtr;
> +}
> +
> +static int spinand_init_octal_dtr_enable(struct spinand_device *spinand)
> +{
> + struct device *dev = &spinand->spimem->spi->dev;
> + int ret;
> +
> + if (!(spinand->flags & SPINAND_HAS_OCTAL_DTR_BIT))
> + return 0;
> +
> + if (!(spinand_op_is_octal_dtr(spinand->op_templates.read_cache) &&
> + spinand_op_is_octal_dtr(spinand->op_templates.write_cache) &&
> + spinand_op_is_octal_dtr(spinand->op_templates.update_cache)))
> + return 0;
> +
> + if (!spinand->manufacturer->ops->octal_dtr_enable) {
> + dev_err(dev,
> + "Missing ->octal_dtr_enable(), unable to switch mode\n");

I don't think we want an error here. Perhaps a debug or info call, but
no more.

> + return -EINVAL;
> + }
> +
> + ret = spinand->manufacturer->ops->octal_dtr_enable(spinand);
> + if (ret) {
> + dev_err(dev,
> + "Failed to enable Octal DTR SPI mode (err = %d)\n",
> + ret);
> + return ret;
> + }
> +
> + spinand->reg_proto = SPINAND_OCTAL_DTR;
> +
> + dev_dbg(dev,
> + "%s SPI NAND switched to Octal DTR SPI (8D-8D-8D) mode\n",
> + spinand->manufacturer->name);
> + return 0;
> +}
> +
> static int spinand_ecc_enable(struct spinand_device *spinand,
> bool enable)
> {
> @@ -1189,6 +1231,10 @@ static int spinand_init_flash(struct spinand_device *spinand)
> if (ret)
> return ret;
>
> + ret = spinand_init_octal_dtr_enable(spinand);
> + if (ret)
> + return ret;
> +
> ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> if (ret)
> return ret;
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 35816b8cfe81..daa2ac5c3110 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -271,6 +271,7 @@ struct spinand_devid {
> * @init: initialize a SPI NAND device
> * @adjust_op: modify the ops for any variation in their cmd, address, dummy or
> * data phase by the manufacturer
> + * @octal_dtr_enable: switch the SPI NAND flash into Octal DTR SPI mode
> * @cleanup: cleanup a SPI NAND device
> *
> * Each SPI NAND manufacturer driver should implement this interface so that
> @@ -280,6 +281,7 @@ struct spinand_manufacturer_ops {
> int (*init)(struct spinand_device *spinand);
> void (*adjust_op)(struct spi_mem_op *op,
> const enum spinand_proto reg_proto);
> + int (*octal_dtr_enable)(struct spinand_device *spinand);
> void (*cleanup)(struct spinand_device *spinand);
> };
>
> @@ -348,6 +350,7 @@ struct spinand_ecc_info {
>
> #define SPINAND_HAS_QE_BIT BIT(0)
> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
> +#define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
>
> /**
> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure




Thanks,
Miquèl

2021-08-07 00:05:49

by Miquel Raynal

Subject: Re: [PATCH 09/13] mtd: spinand: Add support for write volatile configuration register op

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:34
+0000:

> Volatile configuration register are a different set of configuration
> registers, i.e. they differ from the status registers. A different
> SPI instruction is required to write to these registers. Any changes
> to the Volatile Configuration Register get transferred directly to
> the Internal Configuration Register and instantly reflect on the
> device operation.
>
> In Winbond W35N01JW, these volatile configuration register must be
> configured in order to switch to Octal DTR SPI mode.
>
> Add support for writing to volatile configuration registers using a
> new WRITE_VCR_OP template.
>
> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/core.c | 2 +-
> drivers/mtd/nand/spi/winbond.c | 28 ++++++++++++++++++++++++++++
> include/linux/mtd/spinand.h | 1 +
> 3 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 8711e887b795..f577e72da2c4 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -442,7 +442,7 @@ static void spinand_ondie_ecc_save_status(struct nand_device *nand, u8 status)
> engine_conf->status = status;
> }
>
> -static int spinand_write_enable_op(struct spinand_device *spinand)
> +int spinand_write_enable_op(struct spinand_device *spinand)
> {
> struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);
>
> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
> index 76684428354e..a7052a9ca171 100644
> --- a/drivers/mtd/nand/spi/winbond.c
> +++ b/drivers/mtd/nand/spi/winbond.c
> @@ -7,6 +7,7 @@
> * Boris Brezillon <[email protected]>
> */
>
> +#include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <linux/mtd/spinand.h>
> @@ -114,6 +115,33 @@ static int winbond_spinand_init(struct spinand_device *spinand)
> return 0;
> }
>
> +static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)

Maybe a comment to tell people what vcr is?

> +{
> + int ret;
> + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x81, 1),
> + SPI_MEM_OP_ADDR(3, reg, 1),
> + SPI_MEM_OP_NO_DUMMY,
> + SPI_MEM_OP_DATA_OUT(1, spinand->scratchbuf, 1));
> +
> + *spinand->scratchbuf = val;
> +
> + ret = spinand_write_enable_op(spinand);
> + if (ret)
> + return ret;
> +
> + ret = spi_mem_exec_op(spinand->spimem, &op);
> + if (ret)
> + return ret;
> +
> + /*
> + * Write VCR operation doesn't set the busy bit in SR, so can't perform
> + * a status poll. Minimum time of 50ns is needed to complete the write.
> + * So, give thrice the minimum required delay.

Isn't there an official maximum time?

> + */
> + ndelay(150);
> + return 0;
> +}
> +
> static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
> .init = winbond_spinand_init,
> };
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index daa2ac5c3110..21a4e5adcd59 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -560,5 +560,6 @@ int spinand_match_and_init(struct spinand_device *spinand,
>
> int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
> int spinand_select_target(struct spinand_device *spinand, unsigned int target);
> +int spinand_write_enable_op(struct spinand_device *spinand);
>
> #endif /* __LINUX_MTD_SPINAND_H */




Thanks,
Miquèl

2021-08-07 00:05:58

by Miquel Raynal

Subject: Re: [PATCH 10/13] mtd: spinand: Add octal_dtr_enable() for Winbond manufacturer_ops

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:35
+0000:

> Add implementation of octal_dtr_enable() manufacturer_ops for Winbond.
> To switch to Ocatl DTR mode, setting programmable dummy cycles and
> SPI IO mode using the volatile configuration register is required. To
> function at max 120MHz SPI clock in Octal DTR mode, 12 programmable
> dummy clock cycle setting is required. (Default number of dummy cycle
> are 8 clocks)
>
> Set the programmable dummy cycle to 12 clocks, and SPI IO mode to
> Octal DTR with Data Strobe in the VCR. Also, perform a READ ID
> operation in Octal DTR SPI mode to ensure the switch was successful.

Commit title should contain "winbond:" (same for the previous patch and
possibly next ones as well).

> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
> index a7052a9ca171..58cda07c15a0 100644
> --- a/drivers/mtd/nand/spi/winbond.c
> +++ b/drivers/mtd/nand/spi/winbond.c
> @@ -16,6 +16,14 @@
>
> #define WINBOND_CFG_BUF_READ BIT(3)
>
> +/* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
> +#define WINBOND_IO_MODE_VCR_OCTAL_DTR 0xE7
> +#define WINBOND_IO_MODE_VCR_ADDR 0x00
> +
> +/* Use 12 dummy clk cycles for using Octal DTR SPI at max 120MHZ */
> +#define WINBOND_DUMMY_CLK_COUNT 12
> +#define WINBOND_DUMMY_CLK_VCR_ADDR 0x01
> +
> static SPINAND_OP_VARIANTS(read_cache_variants,
> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> @@ -142,8 +150,42 @@ static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
> return 0;
> }
>
> +static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
> +{
> + int ret;
> + struct spi_mem_op op;
> +
> + ret = winbond_write_vcr_op(spinand, WINBOND_DUMMY_CLK_VCR_ADDR,
> + WINBOND_DUMMY_CLK_COUNT);
> + if (ret)
> + return ret;
> +
> + ret = winbond_write_vcr_op(spinand, WINBOND_IO_MODE_VCR_ADDR,
> + WINBOND_IO_MODE_VCR_OCTAL_DTR);
> + if (ret)
> + return ret;
> +
> + /* Read flash ID to make sure the switch was successful. */
> + op = (struct spi_mem_op)
> + SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9f9f, 8),
> + SPI_MEM_OP_NO_ADDR,
> + SPI_MEM_OP_DUMMY_DTR(16, 8),
> + SPI_MEM_OP_DATA_IN_DTR(SPINAND_MAX_ID_LEN,
> + spinand->scratchbuf, 8));
> +
> + ret = spi_mem_exec_op(spinand->spimem, &op);
> + if (ret)
> + return ret;
> +
> + if (memcmp(spinand->scratchbuf, spinand->id.data, SPINAND_MAX_ID_LEN))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
> .init = winbond_spinand_init,
> + .octal_dtr_enable = winbond_spinand_octal_dtr_enable,
> };
>
> const struct spinand_manufacturer winbond_spinand_manufacturer = {




Thanks,
Miquèl

2021-08-07 00:06:17

by Miquel Raynal

Subject: Re: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33
+0000:

> The SPI NAND core doesn't know how to switch the flash to Octal DTR
> mode (i.e. which operations to perform). If the manufacturer hasn't
> implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
> wouldn't be able to switch to 8D-8D-8D mode and will also not be able
> to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
> cache op_templates.
>
> So, avoid choosing a Octal DTR SPI op_template for read_cache,
> write_cache and update_cache operations, if the manufacturer_op
> octal_dtr_enable() is missing.

After looking at your previous commit I don't see why this patch would
be needed. octal_dtr_enable() only updates the mode when it succeeds so
I don't think this patch is really needed.

>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 19d8affac058..8711e887b795 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
> if (id[0] != manufacturer->id)
> continue;
>
> + spinand->manufacturer = manufacturer;
> +
> ret = spinand_match_and_init(spinand,
> manufacturer->chips,
> manufacturer->nchips,
> @@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
> if (ret < 0)
> continue;
>
> - spinand->manufacturer = manufacturer;
> return 0;
> }
> return -ENOTSUPP;
> @@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
> unsigned int nbytes;
> int ret;
>
> + if (spinand_op_is_octal_dtr(&op) &&
> + !spinand->manufacturer->ops->octal_dtr_enable)
> + continue;
> +
> nbytes = nanddev_per_page_oobsize(nand) +
> nanddev_page_size(nand);
>

Thanks,
Miquèl

2021-08-07 00:06:53

by Miquel Raynal

Subject: Re: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36
+0000:

> Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
> functionality in their SPI NAND flash chips. PoR instruction consists
> of a 66h command followed by 99h command, and is different from the FFh
> reset. The reset command FFh just clears the status only registers,
> while the PoR command erases all the configurations written to the
> flash and is equivalent to a power-down -> power-up cycle.
>
> Add support for the Power-on-Reset command for any flash that provides
> this feature.
>
> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---

[...]
\
> @@ -218,6 +230,8 @@ struct spinand_device;
> * reading/programming/erasing when the RESET occurs. Since we always
> * issue a RESET when the device is IDLE, 5us is selected for both initial
> * and poll delay.
> + * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us

s/max upto/up to/

> + * to 1200 us safely.

I don't really get why, if the maximum is 500, then let's wait for
500us.

> */
> #define SPINAND_READ_INITIAL_DELAY_US 6
> #define SPINAND_READ_POLL_DELAY_US 5
> @@ -227,6 +241,8 @@ struct spinand_device;
> #define SPINAND_WRITE_POLL_DELAY_US 15
> #define SPINAND_ERASE_INITIAL_DELAY_US 250
> #define SPINAND_ERASE_POLL_DELAY_US 50
> +#define SPINAND_POR_MIN_DELAY_US 1000
> +#define SPINAND_POR_MAX_DELAY_US 1200
>
> #define SPINAND_WAITRDY_TIMEOUT_MS 400
>
> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
> #define SPINAND_HAS_QE_BIT BIT(0)
> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
> #define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
> +#define SPINAND_HAS_POR_CMD_BIT BIT(3)
>
> /**
> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure




Thanks,
Miquèl

2021-08-07 00:07:17

by Miquel Raynal

Subject: Re: [PATCH 12/13] mtd: spinand: Perform Power-on-Reset when runtime_pm suspend is issued

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:37
+0000:

> A soft reset using FFh command doesn't erase the flash's configuration
> and doesn't reset the SPI IO mode also. This can result in the flash
> being in a different SPI IO mode, e.g. Octal DTR, when resuming from
> sleep. This would render the flash in an unusable state.

could put the falsh in?

> Perform a Power-on-Reset (PoR), if available in the flash, when
> suspending the device by runtime_pm. This would set the flash to clean

I think runtime_pm is something else.

> state for reinitialization during resume and would also ensure that it
> is in standard SPI IO mode (1S-1S-1S) before the resume begins.

Please add a comment about this to explain why we don't do this reset
at resume time.

>
> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/core.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 608f4eb85b0a..6fb3aa6af540 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1329,6 +1329,21 @@ static void spinand_mtd_resume(struct mtd_info *mtd)
> spinand_ecc_enable(spinand, false);
> }
>
> +static int spinand_mtd_suspend(struct mtd_info *mtd)
> +{
> + struct spinand_device *spinand = mtd_to_spinand(mtd);
> + int ret;
> +
> + if (!(spinand->flags & SPINAND_HAS_POR_CMD_BIT))
> + return 0;
> +
> + ret = spinand_power_on_rst_op(spinand);
> + if (ret)
> + dev_err(&spinand->spimem->spi->dev, "suspend() failed\n");
> +
> + return ret;
> +}
> +
> static int spinand_init(struct spinand_device *spinand)
> {
> struct device *dev = &spinand->spimem->spi->dev;
> @@ -1401,6 +1416,7 @@ static int spinand_init(struct spinand_device *spinand)
> mtd->_erase = spinand_mtd_erase;
> mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
> mtd->_resume = spinand_mtd_resume;
> + mtd->_suspend = spinand_mtd_suspend;
>
> if (nand->ecc.engine) {
> ret = mtd_ooblayout_count_freebytes(mtd);


Thanks,
Miquèl

2021-08-07 00:07:54

by Miquel Raynal

Subject: Re: [PATCH 13/13] mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:38
+0000:

> Winbond W35N01JW is SPI NAND flash supporting Octal DTR SPI protocol.

a

> Add op_vairants for W35N01JW, which include the Octal DTR read/write

variants

> page ops as well. Add W35N01JW's oob layout functions for the

OOB

> mtd_ooblayout_ops. Add all op adjustments required for Octal DTR SPI
> mode using the adjust_op(). Finally, add an entry for W35N01JW in
> spinand_info table.
>
> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>

Maybe we can split this into two parts:
1/ support the chip
2/ add 8-D support

> Signed-off-by: Apurva Nandan <[email protected]>
> ---
> drivers/mtd/nand/spi/winbond.c | 116 ++++++++++++++++++++++++++++++---
> 1 file changed, 107 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
> index 58cda07c15a0..5c2b9e61b624 100644
> --- a/drivers/mtd/nand/spi/winbond.c
> +++ b/drivers/mtd/nand/spi/winbond.c
> @@ -16,6 +16,13 @@
>
> #define WINBOND_CFG_BUF_READ BIT(3)
>
> +#define WINBOND_BLK_ERASE_OPCODE 0xD8
> +#define WINBOND_PAGE_READ_OPCODE 0x13
> +#define WINBOND_PROG_EXEC_OPCODE 0x10
> +#define WINBOND_READ_REG_OPCODE_1 0x05
> +#define WINBOND_READ_REG_OPCODE_2 0x0F
> +#define WINBOND_READ_VCR_OPCODE 0x85
> +
> /* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
> #define WINBOND_IO_MODE_VCR_OCTAL_DTR 0xE7
> #define WINBOND_IO_MODE_VCR_ADDR 0x00
> @@ -24,7 +31,7 @@
> #define WINBOND_DUMMY_CLK_COUNT 12
> #define WINBOND_DUMMY_CLK_VCR_ADDR 0x01
>
> -static SPINAND_OP_VARIANTS(read_cache_variants,
> +static SPINAND_OP_VARIANTS(read_cache_variants_w25xxgv,
> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
> @@ -32,14 +39,27 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
> SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
>
> -static SPINAND_OP_VARIANTS(write_cache_variants,
> +static SPINAND_OP_VARIANTS(write_cache_variants_w25xxgv,
> SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
> SPINAND_PROG_LOAD(true, 0, NULL, 0));
>
> -static SPINAND_OP_VARIANTS(update_cache_variants,
> +static SPINAND_OP_VARIANTS(update_cache_variants_w25xxgv,
> SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
> SPINAND_PROG_LOAD(false, 0, NULL, 0));
>
> +static SPINAND_OP_VARIANTS(read_cache_variants_w35n01jw,
> + SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
> + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
> + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(write_cache_variants_w35n01jw,
> + SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
> + SPINAND_PROG_LOAD(true, 0, NULL, 0));
> +
> +static SPINAND_OP_VARIANTS(update_cache_variants_w35n01jw,
> + SPINAND_PROG_LOAD_OCTALIO_DTR(false, 0, NULL, 0),
> + SPINAND_PROG_LOAD(false, 0, NULL, 0));
> +
> static int w25m02gv_ooblayout_ecc(struct mtd_info *mtd, int section,
> struct mtd_oob_region *region)
> {
> @@ -64,11 +84,40 @@ static int w25m02gv_ooblayout_free(struct mtd_info *mtd, int section,
> return 0;
> }
>
> +static int w35n01jw_ooblayout_ecc(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> +{
> + if (section > 7)
> + return -ERANGE;
> +
> + region->offset = (16 * section) + 12;
> + region->length = 4;
> +
> + return 0;
> +}
> +
> +static int w35n01jw_ooblayout_free(struct mtd_info *mtd, int section,
> + struct mtd_oob_region *region)
> +{
> + if (section > 7)
> + return -ERANGE;
> +
> + region->offset = (16 * section) + 2;
> + region->length = 10;
> +
> + return 0;
> +}
> +
> static const struct mtd_ooblayout_ops w25m02gv_ooblayout = {
> .ecc = w25m02gv_ooblayout_ecc,
> .free = w25m02gv_ooblayout_free,
> };
>
> +static const struct mtd_ooblayout_ops w35n01jw_ooblayout = {
> + .ecc = w35n01jw_ooblayout_ecc,
> + .free = w35n01jw_ooblayout_free,
> +};
> +
> static int w25m02gv_select_target(struct spinand_device *spinand,
> unsigned int target)
> {
> @@ -88,9 +137,9 @@ static const struct spinand_info winbond_spinand_table[] = {
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xab),
> NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 2),
> NAND_ECCREQ(1, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w25xxgv,
> + &write_cache_variants_w25xxgv,
> + &update_cache_variants_w25xxgv),
> 0,
> SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
> SPINAND_SELECT_TARGET(w25m02gv_select_target)),
> @@ -98,11 +147,22 @@ static const struct spinand_info winbond_spinand_table[] = {
> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xaa),
> NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
> NAND_ECCREQ(1, 512),
> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
> - &write_cache_variants,
> - &update_cache_variants),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w25xxgv,
> + &write_cache_variants_w25xxgv,
> + &update_cache_variants_w25xxgv),
> 0,
> SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
> + SPINAND_INFO("W35N01JW",
> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc),
> + NAND_MEMORG(1, 4096, 128, 64, 512, 20, 1, 1, 1),
> + NAND_ECCREQ(1, 512),
> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w35n01jw,
> + &write_cache_variants_w35n01jw,
> + &update_cache_variants_w35n01jw),
> + SPINAND_HAS_OCTAL_DTR_BIT | SPINAND_HAS_POR_CMD_BIT |
> + SPINAND_HAS_CR_FEAT_BIT,
> + SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL)),
> +
> };
>
> static int winbond_spinand_init(struct spinand_device *spinand)
> @@ -183,9 +243,47 @@ static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
> return 0;
> }
>
> +static void winbond_spinand_adjust_op(struct spi_mem_op *op,
> + const enum spinand_proto reg_proto)
> +{
> + /*
> + * To support both 1 byte opcode and 2 byte opcodes, extract the MSB
> + * byte from the opcode as the LSB byte in 2 byte opcode is treated as
> + * don't care.
> + */
> + u8 opcode = op->cmd.opcode >> (8 * (op->cmd.nbytes - 1));
> +
> + if (reg_proto == SPINAND_OCTAL_DTR) {
> + switch (opcode) {
> + case WINBOND_READ_REG_OPCODE_1:
> + case WINBOND_READ_REG_OPCODE_2:
> + op->dummy.nbytes = 14;
> + op->dummy.buswidth = 8;
> + op->dummy.dtr = true;
> + return;
> +
> + case WINBOND_READ_VCR_OPCODE:
> + op->dummy.nbytes = 16;
> + op->dummy.buswidth = 8;
> + op->dummy.dtr = true;
> + return;
> +
> + case WINBOND_BLK_ERASE_OPCODE:
> + case WINBOND_PAGE_READ_OPCODE:
> + case WINBOND_PROG_EXEC_OPCODE:
> + op->addr.nbytes = 2;
> + return;
> +
> + default:
> + return;
> + }
> + }
> +}
> +
> static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
> .init = winbond_spinand_init,
> .octal_dtr_enable = winbond_spinand_octal_dtr_enable,
> + .adjust_op = winbond_spinand_adjust_op,
> };
>
> const struct spinand_manufacturer winbond_spinand_manufacturer = {

Thanks,
Miquèl

2021-08-20 09:54:44

by Apurva Nandan

Subject: Re: [PATCH 03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

Hi Miquèl,

On 07/08/21 12:00 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:28
> +0000:
>
>> Currently, the op macros in spinand.h don't give the option to setup
>> any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
>> Having a function that setups the op based on reg_proto would be
>> better than trying to write all the setup logic in op macros.
>>
>> Create a spimem_setup_op() that would setup cmd, addr, dummy and data
>> phase of the spi_mem op, for the given spinand->reg_proto. And hence,
>> call the spimem_setup_op() before executing any spi_mem op.
>>
>> Note: In this commit, spimem_setup_op() isn't called in the
>> read_reg_op(), write_reg_op() and wait() functions, as they need
>> modifications in address value and data nbytes when in Octal DTR mode.
>> This will be fixed in a later commit.
>
> Thanks for this series!
>
> So far I am fine with your changes, but I don't like the setup_op()
> naming much. I don't yet have a better idea, could you propose
> something more meaningful?
>

I made this similar to the spi_nor_spimem_setup_op(), which essentially
does the same task as this in the spi-nor core.

Other names that I can think of are:

- config_op(), adjust_op(), amend_op(), patch_op()

or

- handle_op_variations(), apply_op_variations()

What do you suggest?

>> Signed-off-by: Apurva Nandan <[email protected]>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>
Thanks,
Apurva Nandan

2021-08-20 10:29:07

by Apurva Nandan

Subject: Re: [PATCH 04/13] mtd: spinand: Fix odd byte addr and data phase in read/write reg op and write VCR op for Octal DTR mode

Hi Miquèl,

On 07/08/21 12:13 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:29
> +0000:
>
>> In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock
>> cycle, and half-cycle instruction phases aren't supported yet. So,
>> every DTR spi_mem_op needs to have even nbytes in all phases for
>> non-erratic behaviour from the SPI controller.
>>
>> The odd length cmd and dummy phases get handled by spimem_setup_op()
>> but the odd length address and data phases need to be handled according
>> to the use case. For example in Octal DTR mode, read register operation
>> has one byte long address and data phase. So it needs to extend it
>> by adding a suitable extra byte in addr and reading 2 bytes of data,
>> discarding the second byte.
>>
>> Handle address and data phases for Octal DTR mode in read/write
>> register and write volatile configuration register operations
>> by adding a suitable extra byte in the address and data phase.
>>
>> Create spimem_setup_reg_op() helper function to ease setting up
>> read/write register operations in other functions, e.g. wait().
>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
>> 1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 2e59faecc8f5..a5334ad34f96 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -65,12 +65,27 @@ static void spinand_setup_op(const struct spinand_device *spinand,
>> }
>> }
>>
>> +static void spinand_setup_reg_op(const struct spinand_device *spinand,
>> + struct spi_mem_op *op)
>
> Same remark about the naming. In fact I believe we could have this
> logic in _setup_op() (or whatever its name) and add a specific
> parameter for it?
>

Okay, I will add a parameter in argument and include this logic in
_setup_op().

>> +{
>> + if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
>> + /*
>> + * Assigning same first and second byte will result in constant
>> + * bits on ths SPI bus between positive and negative clock edges
>
> the
>

Ok.

>> + */
>> + op->addr.val = (op->addr.val << 8) | op->addr.val;
>
> I am not sure to understand what you do here?
>

In Octal DTR mode, 2 bytes of data are sent in a clock cycle. So, we
need to append one extra byte when sending a single byte. This extra
byte would be sent on the negative edge.

It will make sense to keep both the bytes same, as when it will be set
on the SPI pins, the bits on the SPI IO ports will remain constant
between the positive and negative edges (as 1 complete byte is set in
one clock edge in MSB order). There are no restrictions by the
manufacturers on this, the relevant address byte needs to be on positive
edge and second byte on negative edge is don't care.

>> + op->addr.nbytes = 2;
>> + op->data.nbytes = 2;
>> + }
>
> Space
>

Ok.

>> + spinand_setup_op(spinand, op);
>> +}
>> +
>> static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>> {
>> - struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg,
>> - spinand->scratchbuf);
>> + struct spi_mem_op op = SPINAND_GET_FEATURE_OP(reg, spinand->scratchbuf);
>
> You can do this, but in a different commit.
>

Got it.

>> int ret;
>>
>> + spinand_setup_reg_op(spinand, &op);
>> ret = spi_mem_exec_op(spinand->spimem, &op);
>> if (ret)
>> return ret;
>> @@ -81,10 +96,10 @@ static int spinand_read_reg_op(struct spinand_device *spinand, u8 reg, u8 *val)
>>
>> static int spinand_write_reg_op(struct spinand_device *spinand, u8 reg, u8 val)
>> {
>> - struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg,
>> - spinand->scratchbuf);
>> + struct spi_mem_op op = SPINAND_SET_FEATURE_OP(reg, spinand->scratchbuf);
>
> Same here.
>

Yes!

>>
>> - *spinand->scratchbuf = val;
>> + spinand_setup_reg_op(spinand, &op);
>> + memset(spinand->scratchbuf, val, op.data.nbytes);
>> return spi_mem_exec_op(spinand->spimem, &op);
>> }
>>
>> @@ -547,6 +562,7 @@ static int spinand_wait(struct spinand_device *spinand,
>> u8 status;
>> int ret;
>>
>> + spinand_setup_reg_op(spinand, &op);
>> ret = spi_mem_poll_status(spinand->spimem, &op, STATUS_BUSY, 0,
>> initial_delay_us,
>> poll_delay_us,
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 10:38:14

by Apurva Nandan

Subject: Re: [PATCH 06/13] mtd: spinand: Add macros for Octal DTR page read and write operations

Hi Miquèl,

On 07/08/21 12:24 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:31
> +0000:
>
>> Define new PAGE_READ_FROM_CACHE and PROG_LOAD op templates for Octal
>> DTR SPI mode. These templates would used in op_variants and
>
> will be
>

Yeah, ok!

>> op_templates for defining Octal DTR read from cache and write to
>> cache operations.
>>
>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> include/linux/mtd/spinand.h | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index ebb19b2cec84..35816b8cfe81 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -122,6 +122,12 @@
>> SPI_MEM_OP_DUMMY(ndummy, 4), \
>> SPI_MEM_OP_DATA_IN(len, buf, 4))
>>
>> +#define SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(addr, ndummy, buf, len) \
>> + SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9d9d, 8), \
>> + SPI_MEM_OP_ADDR_DTR(2, addr, 8), \
>> + SPI_MEM_OP_DUMMY_DTR(ndummy, 8), \
>> + SPI_MEM_OP_DATA_IN_DTR(len, buf, 8))
>> +
>> #define SPINAND_PROG_EXEC_OP(addr) \
>> SPI_MEM_OP(SPI_MEM_OP_CMD(0x10, 1), \
>> SPI_MEM_OP_ADDR(3, addr, 1), \
>> @@ -140,6 +146,12 @@
>> SPI_MEM_OP_NO_DUMMY, \
>> SPI_MEM_OP_DATA_OUT(len, buf, 4))
>>
>> +#define SPINAND_PROG_LOAD_OCTALIO_DTR(reset, addr, buf, len) \
>> + SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, reset ? 0x0202 : 0x8484, 8), \
>> + SPI_MEM_OP_ADDR_DTR(2, addr, 8), \
>> + SPI_MEM_OP_NO_DUMMY, \
>> + SPI_MEM_OP_DATA_OUT_DTR(len, buf, 8))
>> +
>> #define SPINAND_PROTO_BUSWIDTH_MASK GENMASK(6, 0)
>> #define SPINAND_PROTO_DTR_BIT BIT(7)
>>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 10:45:11

by Apurva Nandan

Subject: Re: [PATCH 07/13] mtd: spinand: Allow enabling Octal DTR mode in the core

Hi Miquèl,

On 07/08/21 12:28 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:32
> +0000:
>
>> Enable Octal DTR SPI mode, i.e. 8D-8D-8D mode, if the SPI NAND flash
>> device supports it. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes
>> (1S-1D-8D), etc. aren't supported yet.
>>
>> The method to switch to Octal DTR SPI mode may vary across
>> manufacturers. For example, for Winbond, it is enabled by writing
>> values to the volatile configuration register. So, let the
>> manufacturer's code have their own implementation for switching to
>> Octal DTR SPI mode. Mixed OSPI (1S-1S-8S & 1S-8S-8S), mixed DTR modes
>> (1S-1D-8D), etc. aren't supported yet.
>
> You can drop the final sentence which is a repetition of the previous
> paragraph.
>

Yes right!

>> Check for the SPI NAND device's support for Octal DTR mode using
>> spinand flags, and if the op_templates allow 8D-8D-8D, call
> allows
>
>> octal_dtr_enable() manufacturer op. If the SPI controller doesn't
>> supports these modes, the selected op_templates would prevent switching
>
> will
>
>> to the Octal DTR mode. And finally update the spinand reg_proto
>> if success.
>
> on
>

Okay, will correct all!

>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/core.c | 46 +++++++++++++++++++++++++++++++++++++
>> include/linux/mtd/spinand.h | 3 +++
>> 2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 1e619b6d777f..19d8affac058 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -256,6 +256,48 @@ static int spinand_init_quad_enable(struct spinand_device *spinand)
>> enable ? CFG_QUAD_ENABLE : 0);
>> }
>>
>> +static bool spinand_op_is_octal_dtr(const struct spi_mem_op *op)
>> +{
>> + return op->cmd.buswidth == 8 && op->cmd.dtr &&
>> + op->addr.buswidth == 8 && op->addr.dtr &&
>> + op->data.buswidth == 8 && op->data.dtr;
>> +}
>> +
>> +static int spinand_init_octal_dtr_enable(struct spinand_device *spinand)
>> +{
>> + struct device *dev = &spinand->spimem->spi->dev;
>> + int ret;
>> +
>> + if (!(spinand->flags & SPINAND_HAS_OCTAL_DTR_BIT))
>> + return 0;
>> +
>> + if (!(spinand_op_is_octal_dtr(spinand->op_templates.read_cache) &&
>> + spinand_op_is_octal_dtr(spinand->op_templates.write_cache) &&
>> + spinand_op_is_octal_dtr(spinand->op_templates.update_cache)))
>> + return 0;
>> +
>> + if (!spinand->manufacturer->ops->octal_dtr_enable) {
>> + dev_err(dev,
>> + "Missing ->octal_dtr_enable(), unable to switch mode\n");
>
> I don't think we want an error here. Perhaps a debug or info call, but
> no more.
>

Agree!

>> + return -EINVAL;
>> + }
>> +
>> + ret = spinand->manufacturer->ops->octal_dtr_enable(spinand);
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to enable Octal DTR SPI mode (err = %d)\n",
>> + ret);
>> + return ret;
>> + }
>> +
>> + spinand->reg_proto = SPINAND_OCTAL_DTR;
>> +
>> + dev_dbg(dev,
>> + "%s SPI NAND switched to Octal DTR SPI (8D-8D-8D) mode\n",
>> + spinand->manufacturer->name);
>> + return 0;
>> +}
>> +
>> static int spinand_ecc_enable(struct spinand_device *spinand,
>> bool enable)
>> {
>> @@ -1189,6 +1231,10 @@ static int spinand_init_flash(struct spinand_device *spinand)
>> if (ret)
>> return ret;
>>
>> + ret = spinand_init_octal_dtr_enable(spinand);
>> + if (ret)
>> + return ret;
>> +
>> ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>> if (ret)
>> return ret;
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index 35816b8cfe81..daa2ac5c3110 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -271,6 +271,7 @@ struct spinand_devid {
>> * @init: initialize a SPI NAND device
>> * @adjust_op: modify the ops for any variation in their cmd, address, dummy or
>> * data phase by the manufacturer
>> + * @octal_dtr_enable: switch the SPI NAND flash into Octal DTR SPI mode
>> * @cleanup: cleanup a SPI NAND device
>> *
>> * Each SPI NAND manufacturer driver should implement this interface so that
>> @@ -280,6 +281,7 @@ struct spinand_manufacturer_ops {
>> int (*init)(struct spinand_device *spinand);
>> void (*adjust_op)(struct spi_mem_op *op,
>> const enum spinand_proto reg_proto);
>> + int (*octal_dtr_enable)(struct spinand_device *spinand);
>> void (*cleanup)(struct spinand_device *spinand);
>> };
>>
>> @@ -348,6 +350,7 @@ struct spinand_ecc_info {
>>
>> #define SPINAND_HAS_QE_BIT BIT(0)
>> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
>> +#define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
>>
>> /**
>> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
>
>
>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 11:28:16

by Apurva Nandan

Subject: Re: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

On 07/08/21 12:31 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33
> +0000:
>
>> The SPI NAND core doesn't know how to switch the flash to Octal DTR
>> mode (i.e. which operations to perform). If the manufacturer hasn't
>> implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
>> wouldn't be able to switch to 8D-8D-8D mode and will also not be able
>> to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
>> cache op_templates.
>>
>> So, avoid choosing a Octal DTR SPI op_template for read_cache,
>> write_cache and update_cache operations, if the manufacturer_op
>> octal_dtr_enable() is missing.
>
> After looking at your previous commit I don't see why this patch would
> be needed. octal_dtr_enable() only updates the mode when it succeeds so
> I don't think this patch is really needed.
>

I added it to prevent any errors happening dues to a missing
implementation of octal_dtr_enable() from manufacturer driver side.
So, if the manufacturers skips the octal_dtr_enable() implementation, we
want the spinand core to run in 1s-1s-1s mode.

Read/write/update op variant selection happens in select_op_variant(),
much before octal_dtr_enable(). So just check if there is a definition
of octal_dtr_enable in manufacturer ops and then only use 8D op variants.

Removing this wouldn't break anything in the current implementation.
Do you think we should drop this?

>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/core.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 19d8affac058..8711e887b795 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
>> if (id[0] != manufacturer->id)
>> continue;
>>
>> + spinand->manufacturer = manufacturer;
>> +
>> ret = spinand_match_and_init(spinand,
>> manufacturer->chips,
>> manufacturer->nchips,
>> @@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
>> if (ret < 0)
>> continue;
>>
>> - spinand->manufacturer = manufacturer;
>> return 0;
>> }
>> return -ENOTSUPP;
>> @@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
>> unsigned int nbytes;
>> int ret;
>>
>> + if (spinand_op_is_octal_dtr(&op) &&
>> + !spinand->manufacturer->ops->octal_dtr_enable)
>> + continue;
>> +
>> nbytes = nanddev_per_page_oobsize(nand) +
>> nanddev_page_size(nand);
>>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 11:34:05

by Apurva Nandan

Subject: Re: [PATCH 09/13] mtd: spinand: Add support for write volatile configuration register op

Hi Miquèl,

On 07/08/21 12:35 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:34
> +0000:
>
>> Volatile configuration register are a different set of configuration
>> registers, i.e. they differ from the status registers. A different
>> SPI instruction is required to write to these registers. Any changes
>> to the Volatile Configuration Register get transferred directly to
>> the Internal Configuration Register and instantly reflect on the
>> device operation.
>>
>> In Winbond W35N01JW, these volatile configuration register must be
>> configured in order to switch to Octal DTR SPI mode.
>>
>> Add support for writing to volatile configuration registers using a
>> new WRITE_VCR_OP template.
>>
>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/core.c | 2 +-
>> drivers/mtd/nand/spi/winbond.c | 28 ++++++++++++++++++++++++++++
>> include/linux/mtd/spinand.h | 1 +
>> 3 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 8711e887b795..f577e72da2c4 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -442,7 +442,7 @@ static void spinand_ondie_ecc_save_status(struct nand_device *nand, u8 status)
>> engine_conf->status = status;
>> }
>>
>> -static int spinand_write_enable_op(struct spinand_device *spinand)
>> +int spinand_write_enable_op(struct spinand_device *spinand)
>> {
>> struct spi_mem_op op = SPINAND_WR_EN_DIS_OP(true);
>>
>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>> index 76684428354e..a7052a9ca171 100644
>> --- a/drivers/mtd/nand/spi/winbond.c
>> +++ b/drivers/mtd/nand/spi/winbond.c
>> @@ -7,6 +7,7 @@
>> * Boris Brezillon <[email protected]>
>> */
>>
>> +#include <linux/delay.h>
>> #include <linux/device.h>
>> #include <linux/kernel.h>
>> #include <linux/mtd/spinand.h>
>> @@ -114,6 +115,33 @@ static int winbond_spinand_init(struct spinand_device *spinand)
>> return 0;
>> }
>>
>> +static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
>
> Maybe a comment to tell people what vcr is?
>

Okay sure!

>> +{
>> + int ret;
>> + struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(0x81, 1),
>> + SPI_MEM_OP_ADDR(3, reg, 1),
>> + SPI_MEM_OP_NO_DUMMY,
>> + SPI_MEM_OP_DATA_OUT(1, spinand->scratchbuf, 1));
>> +
>> + *spinand->scratchbuf = val;
>> +
>> + ret = spinand_write_enable_op(spinand);
>> + if (ret)
>> + return ret;
>> +
>> + ret = spi_mem_exec_op(spinand->spimem, &op);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Write VCR operation doesn't set the busy bit in SR, so can't perform
>> + * a status poll. Minimum time of 50ns is needed to complete the write.
>> + * So, give thrice the minimum required delay.
>
> Isn't there an official maximum time?
>

No, there is only an official minimum time. No maximum time..

>> + */
>> + ndelay(150);
>> + return 0;
>> +}
>> +
>> static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
>> .init = winbond_spinand_init,
>> };
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index daa2ac5c3110..21a4e5adcd59 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -560,5 +560,6 @@ int spinand_match_and_init(struct spinand_device *spinand,
>>
>> int spinand_upd_cfg(struct spinand_device *spinand, u8 mask, u8 val);
>> int spinand_select_target(struct spinand_device *spinand, unsigned int target);
>> +int spinand_write_enable_op(struct spinand_device *spinand);
>>
>> #endif /* __LINUX_MTD_SPINAND_H */
>
>
>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 11:35:18

by Apurva Nandan

Subject: Re: [PATCH 10/13] mtd: spinand: Add octal_dtr_enable() for Winbond manufacturer_ops

Hi Miquèl,

On 07/08/21 12:36 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:35
> +0000:
>
>> Add implementation of octal_dtr_enable() manufacturer_ops for Winbond.
>> To switch to Ocatl DTR mode, setting programmable dummy cycles and
>> SPI IO mode using the volatile configuration register is required. To
>> function at max 120MHz SPI clock in Octal DTR mode, 12 programmable
>> dummy clock cycle setting is required. (Default number of dummy cycle
>> are 8 clocks)
>>
>> Set the programmable dummy cycle to 12 clocks, and SPI IO mode to
>> Octal DTR with Data Strobe in the VCR. Also, perform a READ ID
>> operation in Octal DTR SPI mode to ensure the switch was successful.
>
> Commit title should contain "winbond:" (same for the previous patch and
> possibly next ones as well).
>

Okay, got it!

>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/winbond.c | 42 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 42 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>> index a7052a9ca171..58cda07c15a0 100644
>> --- a/drivers/mtd/nand/spi/winbond.c
>> +++ b/drivers/mtd/nand/spi/winbond.c
>> @@ -16,6 +16,14 @@
>>
>> #define WINBOND_CFG_BUF_READ BIT(3)
>>
>> +/* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
>> +#define WINBOND_IO_MODE_VCR_OCTAL_DTR 0xE7
>> +#define WINBOND_IO_MODE_VCR_ADDR 0x00
>> +
>> +/* Use 12 dummy clk cycles for using Octal DTR SPI at max 120MHZ */
>> +#define WINBOND_DUMMY_CLK_COUNT 12
>> +#define WINBOND_DUMMY_CLK_VCR_ADDR 0x01
>> +
>> static SPINAND_OP_VARIANTS(read_cache_variants,
>> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
>> @@ -142,8 +150,42 @@ static int winbond_write_vcr_op(struct spinand_device *spinand, u8 reg, u8 val)
>> return 0;
>> }
>>
>> +static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
>> +{
>> + int ret;
>> + struct spi_mem_op op;
>> +
>> + ret = winbond_write_vcr_op(spinand, WINBOND_DUMMY_CLK_VCR_ADDR,
>> + WINBOND_DUMMY_CLK_COUNT);
>> + if (ret)
>> + return ret;
>> +
>> + ret = winbond_write_vcr_op(spinand, WINBOND_IO_MODE_VCR_ADDR,
>> + WINBOND_IO_MODE_VCR_OCTAL_DTR);
>> + if (ret)
>> + return ret;
>> +
>> + /* Read flash ID to make sure the switch was successful. */
>> + op = (struct spi_mem_op)
>> + SPI_MEM_OP(SPI_MEM_OP_CMD_DTR(2, 0x9f9f, 8),
>> + SPI_MEM_OP_NO_ADDR,
>> + SPI_MEM_OP_DUMMY_DTR(16, 8),
>> + SPI_MEM_OP_DATA_IN_DTR(SPINAND_MAX_ID_LEN,
>> + spinand->scratchbuf, 8));
>> +
>> + ret = spi_mem_exec_op(spinand->spimem, &op);
>> + if (ret)
>> + return ret;
>> +
>> + if (memcmp(spinand->scratchbuf, spinand->id.data, SPINAND_MAX_ID_LEN))
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
>> .init = winbond_spinand_init,
>> + .octal_dtr_enable = winbond_spinand_octal_dtr_enable,
>> };
>>
>> const struct spinand_manufacturer winbond_spinand_manufacturer = {
>
>
>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 11:45:08

by Apurva Nandan

Subject: Re: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Hi Miquèl,

On 07/08/21 12:38 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36
> +0000:
>
>> Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
>> functionality in their SPI NAND flash chips. PoR instruction consists
>> of a 66h command followed by 99h command, and is different from the FFh
>> reset. The reset command FFh just clears the status only registers,
>> while the PoR command erases all the configurations written to the
>> flash and is equivalent to a power-down -> power-up cycle.
>>
>> Add support for the Power-on-Reset command for any flash that provides
>> this feature.
>>
>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>
> [...]
> \
>> @@ -218,6 +230,8 @@ struct spinand_device;
>> * reading/programming/erasing when the RESET occurs. Since we always
>> * issue a RESET when the device is IDLE, 5us is selected for both initial
>> * and poll delay.
>> + * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us
>
> s/max upto/up to/
>

Okay!

>> + * to 1200 us safely.
>
> I don't really get why, if the maximum is 500, then let's wait for
> 500us.
>

Generally we keep some margin from the maximum time, no?

>> */
>> #define SPINAND_READ_INITIAL_DELAY_US 6
>> #define SPINAND_READ_POLL_DELAY_US 5
>> @@ -227,6 +241,8 @@ struct spinand_device;
>> #define SPINAND_WRITE_POLL_DELAY_US 15
>> #define SPINAND_ERASE_INITIAL_DELAY_US 250
>> #define SPINAND_ERASE_POLL_DELAY_US 50
>> +#define SPINAND_POR_MIN_DELAY_US 1000
>> +#define SPINAND_POR_MAX_DELAY_US 1200
>>
>> #define SPINAND_WAITRDY_TIMEOUT_MS 400
>>
>> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
>> #define SPINAND_HAS_QE_BIT BIT(0)
>> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
>> #define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
>> +#define SPINAND_HAS_POR_CMD_BIT BIT(3)
>>
>> /**
>> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
>
>
>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 11:49:46

by Apurva Nandan

Subject: Re: [PATCH 12/13] mtd: spinand: Perform Power-on-Reset when runtime_pm suspend is issued

On 07/08/21 12:42 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:37
> +0000:
>
>> A soft reset using FFh command doesn't erase the flash's configuration
>> and doesn't reset the SPI IO mode also. This can result in the flash
>> being in a different SPI IO mode, e.g. Octal DTR, when resuming from
>> sleep. This would render the flash in an unusable state.
>
> could put the falsh in?
>

Okay, will make it clearer.
Basically, we don't want the flash to be in an ambiguous state. It might
or might not have undergone a power off during the suspend state. So,
the spinand core wouldn't know if the flash is still in Octal DTR mode
or not. If it is still in Octal DTR mode, then none of the SPI
instruction during mtd_resume() would work. So this is an ambiguous
situation for driver. To avoid this, perform a PoR reset before suspending.

>> Perform a Power-on-Reset (PoR), if available in the flash, when
>> suspending the device by runtime_pm. This would set the flash to clean
>
> I think runtime_pm is something else.
>

Yeah, will make it clearer.

>> state for reinitialization during resume and would also ensure that it
>> is in standard SPI IO mode (1S-1S-1S) before the resume begins.
>
> Please add a comment about this to explain why we don't do this reset
> at resume time.
>

Yes sure!

>>
>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/core.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 608f4eb85b0a..6fb3aa6af540 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1329,6 +1329,21 @@ static void spinand_mtd_resume(struct mtd_info *mtd)
>> spinand_ecc_enable(spinand, false);
>> }
>>
>> +static int spinand_mtd_suspend(struct mtd_info *mtd)
>> +{
>> + struct spinand_device *spinand = mtd_to_spinand(mtd);
>> + int ret;
>> +
>> + if (!(spinand->flags & SPINAND_HAS_POR_CMD_BIT))
>> + return 0;
>> +
>> + ret = spinand_power_on_rst_op(spinand);
>> + if (ret)
>> + dev_err(&spinand->spimem->spi->dev, "suspend() failed\n");
>> +
>> + return ret;
>> +}
>> +
>> static int spinand_init(struct spinand_device *spinand)
>> {
>> struct device *dev = &spinand->spimem->spi->dev;
>> @@ -1401,6 +1416,7 @@ static int spinand_init(struct spinand_device *spinand)
>> mtd->_erase = spinand_mtd_erase;
>> mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
>> mtd->_resume = spinand_mtd_resume;
>> + mtd->_suspend = spinand_mtd_suspend;
>>
>> if (nand->ecc.engine) {
>> ret = mtd_ooblayout_count_freebytes(mtd);
>
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks,
Apurva Nandan

2021-08-20 11:53:24

by Apurva Nandan

Subject: Re: [PATCH 13/13] mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash

Hi Miquèl,

On 07/08/21 12:44 am, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:38
> +0000:
>
>> Winbond W35N01JW is SPI NAND flash supporting Octal DTR SPI protocol.
>
> a
>
>> Add op_vairants for W35N01JW, which include the Octal DTR read/write
>
> variants
>
>> page ops as well. Add W35N01JW's oob layout functions for the
>
> OOB
>

Okay, will correct these.

>> mtd_ooblayout_ops. Add all op adjustments required for Octal DTR SPI
>> mode using the adjust_op(). Finally, add an entry for W35N01JW in
>> spinand_info table.
>>
>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>
>
> Maybe we can split this into two parts:
> 1/ support the chip
> 2/ add 8-D support
>

I can split the patch into:
1/ Add implementation of manufacturer_ops: adjust_op() to handle
variations of ops in 8D-8D-8D mode
2/ Add support/entry for Winbond W35N01JW SPI NAND flash chip

As 8-D support has already been added in a previous patch.

>> Signed-off-by: Apurva Nandan <[email protected]>
>> ---
>> drivers/mtd/nand/spi/winbond.c | 116 ++++++++++++++++++++++++++++++---
>> 1 file changed, 107 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/spi/winbond.c b/drivers/mtd/nand/spi/winbond.c
>> index 58cda07c15a0..5c2b9e61b624 100644
>> --- a/drivers/mtd/nand/spi/winbond.c
>> +++ b/drivers/mtd/nand/spi/winbond.c
>> @@ -16,6 +16,13 @@
>>
>> #define WINBOND_CFG_BUF_READ BIT(3)
>>
>> +#define WINBOND_BLK_ERASE_OPCODE 0xD8
>> +#define WINBOND_PAGE_READ_OPCODE 0x13
>> +#define WINBOND_PROG_EXEC_OPCODE 0x10
>> +#define WINBOND_READ_REG_OPCODE_1 0x05
>> +#define WINBOND_READ_REG_OPCODE_2 0x0F
>> +#define WINBOND_READ_VCR_OPCODE 0x85
>> +
>> /* Octal DTR SPI mode (8D-8D-8D) with Data Strobe output*/
>> #define WINBOND_IO_MODE_VCR_OCTAL_DTR 0xE7
>> #define WINBOND_IO_MODE_VCR_ADDR 0x00
>> @@ -24,7 +31,7 @@
>> #define WINBOND_DUMMY_CLK_COUNT 12
>> #define WINBOND_DUMMY_CLK_VCR_ADDR 0x01
>>
>> -static SPINAND_OP_VARIANTS(read_cache_variants,
>> +static SPINAND_OP_VARIANTS(read_cache_variants_w25xxgv,
>> SPINAND_PAGE_READ_FROM_CACHE_QUADIO_OP(0, 2, NULL, 0),
>> SPINAND_PAGE_READ_FROM_CACHE_X4_OP(0, 1, NULL, 0),
>> SPINAND_PAGE_READ_FROM_CACHE_DUALIO_OP(0, 1, NULL, 0),
>> @@ -32,14 +39,27 @@ static SPINAND_OP_VARIANTS(read_cache_variants,
>> SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
>> SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
>>
>> -static SPINAND_OP_VARIANTS(write_cache_variants,
>> +static SPINAND_OP_VARIANTS(write_cache_variants_w25xxgv,
>> SPINAND_PROG_LOAD_X4(true, 0, NULL, 0),
>> SPINAND_PROG_LOAD(true, 0, NULL, 0));
>>
>> -static SPINAND_OP_VARIANTS(update_cache_variants,
>> +static SPINAND_OP_VARIANTS(update_cache_variants_w25xxgv,
>> SPINAND_PROG_LOAD_X4(false, 0, NULL, 0),
>> SPINAND_PROG_LOAD(false, 0, NULL, 0));
>>
>> +static SPINAND_OP_VARIANTS(read_cache_variants_w35n01jw,
>> + SPINAND_PAGE_READ_FROM_CACHE_OCTALIO_DTR_OP(0, 24, NULL, 0),
>> + SPINAND_PAGE_READ_FROM_CACHE_OP(true, 0, 1, NULL, 0),
>> + SPINAND_PAGE_READ_FROM_CACHE_OP(false, 0, 1, NULL, 0));
>> +
>> +static SPINAND_OP_VARIANTS(write_cache_variants_w35n01jw,
>> + SPINAND_PROG_LOAD_OCTALIO_DTR(true, 0, NULL, 0),
>> + SPINAND_PROG_LOAD(true, 0, NULL, 0));
>> +
>> +static SPINAND_OP_VARIANTS(update_cache_variants_w35n01jw,
>> + SPINAND_PROG_LOAD_OCTALIO_DTR(false, 0, NULL, 0),
>> + SPINAND_PROG_LOAD(false, 0, NULL, 0));
>> +
>> static int w25m02gv_ooblayout_ecc(struct mtd_info *mtd, int section,
>> struct mtd_oob_region *region)
>> {
>> @@ -64,11 +84,40 @@ static int w25m02gv_ooblayout_free(struct mtd_info *mtd, int section,
>> return 0;
>> }
>>
>> +static int w35n01jw_ooblayout_ecc(struct mtd_info *mtd, int section,
>> + struct mtd_oob_region *region)
>> +{
>> + if (section > 7)
>> + return -ERANGE;
>> +
>> + region->offset = (16 * section) + 12;
>> + region->length = 4;
>> +
>> + return 0;
>> +}
>> +
>> +static int w35n01jw_ooblayout_free(struct mtd_info *mtd, int section,
>> + struct mtd_oob_region *region)
>> +{
>> + if (section > 7)
>> + return -ERANGE;
>> +
>> + region->offset = (16 * section) + 2;
>> + region->length = 10;
>> +
>> + return 0;
>> +}
>> +
>> static const struct mtd_ooblayout_ops w25m02gv_ooblayout = {
>> .ecc = w25m02gv_ooblayout_ecc,
>> .free = w25m02gv_ooblayout_free,
>> };
>>
>> +static const struct mtd_ooblayout_ops w35n01jw_ooblayout = {
>> + .ecc = w35n01jw_ooblayout_ecc,
>> + .free = w35n01jw_ooblayout_free,
>> +};
>> +
>> static int w25m02gv_select_target(struct spinand_device *spinand,
>> unsigned int target)
>> {
>> @@ -88,9 +137,9 @@ static const struct spinand_info winbond_spinand_table[] = {
>> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xab),
>> NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 2),
>> NAND_ECCREQ(1, 512),
>> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> - &write_cache_variants,
>> - &update_cache_variants),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w25xxgv,
>> + &write_cache_variants_w25xxgv,
>> + &update_cache_variants_w25xxgv),
>> 0,
>> SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL),
>> SPINAND_SELECT_TARGET(w25m02gv_select_target)),
>> @@ -98,11 +147,22 @@ static const struct spinand_info winbond_spinand_table[] = {
>> SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xaa),
>> NAND_MEMORG(1, 2048, 64, 64, 1024, 20, 1, 1, 1),
>> NAND_ECCREQ(1, 512),
>> - SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>> - &write_cache_variants,
>> - &update_cache_variants),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w25xxgv,
>> + &write_cache_variants_w25xxgv,
>> + &update_cache_variants_w25xxgv),
>> 0,
>> SPINAND_ECCINFO(&w25m02gv_ooblayout, NULL)),
>> + SPINAND_INFO("W35N01JW",
>> + SPINAND_ID(SPINAND_READID_METHOD_OPCODE_DUMMY, 0xdc),
>> + NAND_MEMORG(1, 4096, 128, 64, 512, 20, 1, 1, 1),
>> + NAND_ECCREQ(1, 512),
>> + SPINAND_INFO_OP_VARIANTS(&read_cache_variants_w35n01jw,
>> + &write_cache_variants_w35n01jw,
>> + &update_cache_variants_w35n01jw),
>> + SPINAND_HAS_OCTAL_DTR_BIT | SPINAND_HAS_POR_CMD_BIT |
>> + SPINAND_HAS_CR_FEAT_BIT,
>> + SPINAND_ECCINFO(&w35n01jw_ooblayout, NULL)),
>> +
>> };
>>
>> static int winbond_spinand_init(struct spinand_device *spinand)
>> @@ -183,9 +243,47 @@ static int winbond_spinand_octal_dtr_enable(struct spinand_device *spinand)
>> return 0;
>> }
>>
>> +static void winbond_spinand_adjust_op(struct spi_mem_op *op,
>> + const enum spinand_proto reg_proto)
>> +{
>> + /*
>> + * To support both 1 byte opcode and 2 byte opcodes, extract the MSB
>> + * byte from the opcode as the LSB byte in 2 byte opcode is treated as
>> + * don't care.
>> + */
>> + u8 opcode = op->cmd.opcode >> (8 * (op->cmd.nbytes - 1));
>> +
>> + if (reg_proto == SPINAND_OCTAL_DTR) {
>> + switch (opcode) {
>> + case WINBOND_READ_REG_OPCODE_1:
>> + case WINBOND_READ_REG_OPCODE_2:
>> + op->dummy.nbytes = 14;
>> + op->dummy.buswidth = 8;
>> + op->dummy.dtr = true;
>> + return;
>> +
>> + case WINBOND_READ_VCR_OPCODE:
>> + op->dummy.nbytes = 16;
>> + op->dummy.buswidth = 8;
>> + op->dummy.dtr = true;
>> + return;
>> +
>> + case WINBOND_BLK_ERASE_OPCODE:
>> + case WINBOND_PAGE_READ_OPCODE:
>> + case WINBOND_PROG_EXEC_OPCODE:
>> + op->addr.nbytes = 2;
>> + return;
>> +
>> + default:
>> + return;
>> + }
>> + }
>> +}
>> +
>> static const struct spinand_manufacturer_ops winbond_spinand_manuf_ops = {
>> .init = winbond_spinand_init,
>> .octal_dtr_enable = winbond_spinand_octal_dtr_enable,
>> + .adjust_op = winbond_spinand_adjust_op,
>> };
>>
>> const struct spinand_manufacturer winbond_spinand_manufacturer = {
>
> Thanks,
> Miquèl
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Thanks a lot for the reviewing!

Regards,
Apurva Nandan

2021-08-20 12:07:10

by Miquel Raynal

Subject: Re: [PATCH 13/13] mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:21:33
+0530:

> Hi Miquèl,
>
> On 07/08/21 12:44 am, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:38
> > +0000:
> >
> >> Winbond W35N01JW is SPI NAND flash supporting Octal DTR SPI protocol.
> >
> > a
> >
> >> Add op_vairants for W35N01JW, which include the Octal DTR read/write
> >
> > variants
> >
> >> page ops as well. Add W35N01JW's oob layout functions for the
> >
> > OOB
> >
>
> Okay, will correct these.
>
> >> mtd_ooblayout_ops. Add all op adjustments required for Octal DTR SPI
> >> mode using the adjust_op(). Finally, add an entry for W35N01JW in
> >> spinand_info table.
> >>
> >> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
> >>
> >
> > Maybe we can split this into two parts:
> > 1/ support the chip
> > 2/ add 8-D support
> >
>
> I can split the patch into:
> 1/ Add implementation of manufacturer_ops: adjust_op() to handle variations of ops in 8D-8D-8D mode
> 2/ Add support/entry for Winbond W35N01JW SPI NAND flash chip
>
> As 8-D support has already been added in a previous patch.

I also don't want the renaming to happen in the patch adding more
logic.

Thanks,
Miquèl

2021-08-20 12:08:32

by Miquel Raynal

Subject: Re: [PATCH 04/13] mtd: spinand: Fix odd byte addr and data phase in read/write reg op and write VCR op for Octal DTR mode

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 15:57:36
+0530:

> Hi Miquèl,
>
> On 07/08/21 12:13 am, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:29
> > +0000:
> >
> >> In Octal DTR SPI mode, 2 bytes of data gets transmitted over one clock
> >> cycle, and half-cycle instruction phases aren't supported yet. So,
> >> every DTR spi_mem_op needs to have even nbytes in all phases for
> >> non-erratic behaviour from the SPI controller.
> >>
> >> The odd length cmd and dummy phases get handled by spimem_setup_op()
> >> but the odd length address and data phases need to be handled according
> >> to the use case. For example in Octal DTR mode, read register operation
> >> has one byte long address and data phase. So it needs to extend it
> >> by adding a suitable extra byte in addr and reading 2 bytes of data,
> >> discarding the second byte.
> >>
> >> Handle address and data phases for Octal DTR mode in read/write
> >> register and write volatile configuration register operations
> >> by adding a suitable extra byte in the address and data phase.
> >>
> >> Create spimem_setup_reg_op() helper function to ease setting up
> >> read/write register operations in other functions, e.g. wait().
> >>
> >> Signed-off-by: Apurva Nandan <[email protected]>
> >> ---
> >> drivers/mtd/nand/spi/core.c | 26 +++++++++++++++++++++-----
> >> 1 file changed, 21 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> >> index 2e59faecc8f5..a5334ad34f96 100644
> >> --- a/drivers/mtd/nand/spi/core.c
> >> +++ b/drivers/mtd/nand/spi/core.c
> >> @@ -65,12 +65,27 @@ static void spinand_setup_op(const struct spinand_device *spinand,
> >> }
> >> }
> >> >> +static void spinand_setup_reg_op(const struct spinand_device *spinand,
> >> + struct spi_mem_op *op)
> >
> > Same remark about the naming. In fact I believe we could have this
> > logic in _setup_op() (or whatever its name) and add a specific
> > parameter for it?
> >
>
> Okay, I will add a parameter in argument and include this logic in _setup_op().
>
> >> +{
> >> + if (spinand->reg_proto == SPINAND_OCTAL_DTR) {
> >> + /*
> >> + * Assigning same first and second byte will result in constant
> >> + * bits on ths SPI bus between positive and negative clock edges
> >
> > the
> >
>
> Ok.
>
> >> + */
> >> + op->addr.val = (op->addr.val << 8) | op->addr.val;
> >
> > I am not sure to understand what you do here?
> >
>
> In Octal DTR mode, 2 bytes of data are sent in a clock cycle. So, we need to append one extra byte when sending a single byte. This extra byte would be sent on the negative edge.
>
> It will make sense to keep both the bytes same, as when it will be set on the SPI pins, the bits on the SPI IO ports will remain constant between the positive and negative edges (as 1 complete byte is set in one clock edge in MSB order). There are no restrictions by the manufacturers on this, the relevant address byte needs to be on positive edge and second byte on negative edge is don't care.

I was bothered by the shift but actually my head was mixing with the
raw NAND core where these addresses are in an array but here it is a
u64 which is then fine.

(I will continue answering probably next week)

Thanks,
Miquèl

2021-08-20 12:09:39

by Miquel Raynal

Subject: Re: [PATCH 03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

Hi Apurva,

Boris, you might have a good idea for the naming discussed below?

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 15:22:54
+0530:

> Hi Miquèl,
>
> On 07/08/21 12:00 am, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:28
> > +0000:
> >
> >> Currently, the op macros in spinand.h don't give the option to setup
> >> any non-array access instructions for Dual/Quad/Octal DTR SPI bus.
> >> Having a function that setups the op based on reg_proto would be
> >> better than trying to write all the setup logic in op macros.
> >>
> >> Create a spimem_setup_op() that would setup cmd, addr, dummy and data
> >> phase of the spi_mem op, for the given spinand->reg_proto. And hence,
> >> call the spimem_setup_op() before executing any spi_mem op.
> >>
> >> Note: In this commit, spimem_setup_op() isn't called in the
> >> read_reg_op(), write_reg_op() and wait() functions, as they need
> >> modifications in address value and data nbytes when in Octal DTR mode.
> >> This will be fixed in a later commit.
> >
> > Thanks for this series!
> >
> > So far I am fine with your changes, but I don't like the setup_op()
> > naming much. I don't yet have a better idea, could you propose
> > something more meaningful?
> >
>
> I made this similar to the spi_nor_spimem_setup_op(), which essentially does the same task as this in the spi-nor core.
>
> Other names that I can think of are:
>
> - config_op(), adjust_op(), amend_op(), patch_op()
>
> or
>
> - handle_op_variations(), apply_op_variations()
>
> What do you suggest?
>
> >> Signed-off-by: Apurva Nandan <[email protected]>
> >
> > Thanks,
> > Miquèl
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
> Thanks,
> Apurva Nandan

Thanks,
Miquèl

2021-08-20 12:21:22

by Miquel Raynal

Subject: Re: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07
+0530:

> Hi Miquèl,
>
> On 07/08/21 12:38 am, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36
> > +0000:
> >
> >> Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
> >> functionality in their SPI NAND flash chips. PoR instruction consists
> >> of a 66h command followed by 99h command, and is different from the FFh
> >> reset. The reset command FFh just clears the status only registers,
> >> while the PoR command erases all the configurations written to the
> >> flash and is equivalent to a power-down -> power-up cycle.
> >>
> >> Add support for the Power-on-Reset command for any flash that provides
> >> this feature.
> >>
> >> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
> >>
> >> Signed-off-by: Apurva Nandan <[email protected]>
> >> ---
> >
> > [...]
> > \
> >> @@ -218,6 +230,8 @@ struct spinand_device;
> >> * reading/programming/erasing when the RESET occurs. Since we always
> >> * issue a RESET when the device is IDLE, 5us is selected for both initial
> >> * and poll delay.
> >> + * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us
> >
> > s/max upto/up to/
> >
>
> Okay!
>
> >> + * to 1200 us safely.
> >
> > I don't really get why, if the maximum is 500, then let's wait for
> > 500us.
> >
>
> Generally we keep some margin from the maximum time, no?

Well, yes and no.

If you know that an operation will last Xms and have nothing else to
do, then you can take some margin if you are in a probe (called once)
but definitely not if you are in a fast path.

Otherwise the best is to have some kind of signaling but I'm not sure
you'll have one for the reset op...

>
> >> */
> >> #define SPINAND_READ_INITIAL_DELAY_US 6
> >> #define SPINAND_READ_POLL_DELAY_US 5
> >> @@ -227,6 +241,8 @@ struct spinand_device;
> >> #define SPINAND_WRITE_POLL_DELAY_US 15
> >> #define SPINAND_ERASE_INITIAL_DELAY_US 250
> >> #define SPINAND_ERASE_POLL_DELAY_US 50
> >> +#define SPINAND_POR_MIN_DELAY_US 1000
> >> +#define SPINAND_POR_MAX_DELAY_US 1200
> >> >> #define SPINAND_WAITRDY_TIMEOUT_MS 400
> >> >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
> >> #define SPINAND_HAS_QE_BIT BIT(0)
> >> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
> >> #define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
> >> +#define SPINAND_HAS_POR_CMD_BIT BIT(3)
> >> >> /**
> >> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
> >
> >
> >
> >
> > Thanks,
> > Miquèl
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
>
> Thanks,
> Apurva Nandan

Thanks,
Miquèl

2021-08-20 12:21:23

by Miquel Raynal

Subject: Re: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50
+0530:

> On 07/08/21 12:31 am, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33
> > +0000:
> >
> >> The SPI NAND core doesn't know how to switch the flash to Octal DTR
> >> mode (i.e. which operations to perform). If the manufacturer hasn't
> >> implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
> >> wouldn't be able to switch to 8D-8D-8D mode and will also not be able
> >> to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
> >> cache op_templates.
> >>
> >> So, avoid choosing a Octal DTR SPI op_template for read_cache,
> >> write_cache and update_cache operations, if the manufacturer_op
> >> octal_dtr_enable() is missing.
> >
> > After looking at your previous commit I don't see why this patch would
> > be needed. octal_dtr_enable() only updates the mode when it succeeds so
> > I don't think this patch is really needed.
> >
>
> I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side.
> So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.

I still don't get the point: you fail the probe if the octal bit is
enabled but the manufacturer did not implement octal_dtr_enable(), so
how could we have issues? Maybe I am overlooking something though, but
this seemed completely redundant to my eyes so far.

>
> Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.
>
> Removing this wouldn't break anything in the current implementation.
> Do you think we should drop this?
>
> >>
> >> Signed-off-by: Apurva Nandan <[email protected]>
> >> ---
> >> drivers/mtd/nand/spi/core.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> >> index 19d8affac058..8711e887b795 100644
> >> --- a/drivers/mtd/nand/spi/core.c
> >> +++ b/drivers/mtd/nand/spi/core.c
> >> @@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
> >> if (id[0] != manufacturer->id)
> >> continue;
> >> >> + spinand->manufacturer = manufacturer;
> >> +
> >> ret = spinand_match_and_init(spinand,
> >> manufacturer->chips,
> >> manufacturer->nchips,
> >> @@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
> >> if (ret < 0)
> >> continue;
> >> >> - spinand->manufacturer = manufacturer;
> >> return 0;
> >> }
> >> return -ENOTSUPP;
> >> @@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
> >> unsigned int nbytes;
> >> int ret;
> >> >> + if (spinand_op_is_octal_dtr(&op) &&
> >> + !spinand->manufacturer->ops->octal_dtr_enable)
> >> + continue;
> >> +
> >> nbytes = nanddev_per_page_oobsize(nand) +
> >> nanddev_page_size(nand);
> >> > > Thanks,
> > Miquèl
> >
> > ______________________________________________________
> > Linux MTD discussion mailing list
> > http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >
>
> Thanks,
> Apurva Nandan




Thanks,
Miquèl

2021-08-20 13:17:34

by Apurva Nandan

Subject: Re: [PATCH 13/13] mtd: spinand: Add support for Winbond W35N01JW SPI NAND flash

On 20/08/21 5:32 pm, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:21:33
> +0530:
>
>> Hi Miquèl,
>>
>> On 07/08/21 12:44 am, Miquel Raynal wrote:
>>> Hi Apurva,
>>>
>>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:38
>>> +0000:
>>>
>>>> Winbond W35N01JW is SPI NAND flash supporting Octal DTR SPI protocol.
>>>
>>> a
>>>
>>>> Add op_vairants for W35N01JW, which include the Octal DTR read/write
>>>
>>> variants
>>>
>>>> page ops as well. Add W35N01JW's oob layout functions for the
>>>
>>> OOB
>>>
>>
>> Okay, will correct these.
>>
>>>> mtd_ooblayout_ops. Add all op adjustments required for Octal DTR SPI
>>>> mode using the adjust_op(). Finally, add an entry for W35N01JW in
>>>> spinand_info table.
>>>>
>>>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>>>
>>>
>>> Maybe we can split this into two parts:
>>> 1/ support the chip
>>> 2/ add 8-D support
>>>
>>
>> I can split the patch into:
>> 1/ Add implementation of manufacturer_ops: adjust_op() to handle variations of ops in 8D-8D-8D mode
>> 2/ Add support/entry for Winbond W35N01JW SPI NAND flash chip
>>
>> As 8-D support has already been added in a previous patch.
>
> I also don't want the renaming to happen in the patch adding more
> logic.
>

Okay, got it. Will amend this.

> Thanks,
> Miquèl
>

Thanks,
Apurva Nandan

2021-08-20 13:44:10

by Apurva Nandan

Subject: Re: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Hi Miquèl,

On 20/08/21 5:48 pm, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07
> +0530:
>
>> Hi Miquèl,
>>
>> On 07/08/21 12:38 am, Miquel Raynal wrote:
>>> Hi Apurva,
>>>
>>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36
>>> +0000:
>>>
>>>> Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
>>>> functionality in their SPI NAND flash chips. PoR instruction consists
>>>> of a 66h command followed by 99h command, and is different from the FFh
>>>> reset. The reset command FFh just clears the status only registers,
>>>> while the PoR command erases all the configurations written to the
>>>> flash and is equivalent to a power-down -> power-up cycle.
>>>>
>>>> Add support for the Power-on-Reset command for any flash that provides
>>>> this feature.
>>>>
>>>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>>>
>>>> Signed-off-by: Apurva Nandan <[email protected]>
>>>> ---
>>>
>>> [...]
>>> \
>>>> @@ -218,6 +230,8 @@ struct spinand_device;
>>>> * reading/programming/erasing when the RESET occurs. Since we always
>>>> * issue a RESET when the device is IDLE, 5us is selected for both initial
>>>> * and poll delay.
>>>> + * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us
>>>
>>> s/max upto/up to/
>>>
>>
>> Okay!
>>
>>>> + * to 1200 us safely.
>>>
>>> I don't really get why, if the maximum is 500, then let's wait for
>>> 500us.
>>>
>>
>> Generally we keep some margin from the maximum time, no?
>
> Well, yes and no.
>
> If you know that an operation will last Xms and have nothing else to
> do, then you can take some margin if you are in a probe (called once)
> but definitely not if you are in a fast path.
>

I think as PoR reset would be called at every mtd_suspend() call, so we
can reduce the delay. And we would be expecting some time gap before the
next mtd_resume() call.

> Otherwise the best is to have some kind of signaling but I'm not sure
> you'll have one for the reset op...
>

According to public datasheet, it doesn't set the busy bit during reset.

So do you suggest in the favor of removing the delay margin?

>>
>>>> */
>>>> #define SPINAND_READ_INITIAL_DELAY_US 6
>>>> #define SPINAND_READ_POLL_DELAY_US 5
>>>> @@ -227,6 +241,8 @@ struct spinand_device;
>>>> #define SPINAND_WRITE_POLL_DELAY_US 15
>>>> #define SPINAND_ERASE_INITIAL_DELAY_US 250
>>>> #define SPINAND_ERASE_POLL_DELAY_US 50
>>>> +#define SPINAND_POR_MIN_DELAY_US 1000
>>>> +#define SPINAND_POR_MAX_DELAY_US 1200
>>>> >> #define SPINAND_WAITRDY_TIMEOUT_MS 400
>>>> >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
>>>> #define SPINAND_HAS_QE_BIT BIT(0)
>>>> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
>>>> #define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
>>>> +#define SPINAND_HAS_POR_CMD_BIT BIT(3)
>>>> >> /**
>>>> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
>>>
>>>
>>>
>>>
>>> Thanks,
>>> Miquèl
>>>
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>>
>>
>> Thanks,
>> Apurva Nandan
>
> Thanks,
> Miquèl
>

Thanks,
Apurva Nandan

2021-08-20 13:56:26

by Apurva Nandan

Subject: Re: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

Hi Miquèl,

On 20/08/21 5:44 pm, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50
> +0530:
>
>> On 07/08/21 12:31 am, Miquel Raynal wrote:
>>> Hi Apurva,
>>>
>>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33
>>> +0000:
>>>
>>>> The SPI NAND core doesn't know how to switch the flash to Octal DTR
>>>> mode (i.e. which operations to perform). If the manufacturer hasn't
>>>> implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
>>>> wouldn't be able to switch to 8D-8D-8D mode and will also not be able
>>>> to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
>>>> cache op_templates.
>>>>
>>>> So, avoid choosing a Octal DTR SPI op_template for read_cache,
>>>> write_cache and update_cache operations, if the manufacturer_op
>>>> octal_dtr_enable() is missing.
>>>
>>> After looking at your previous commit I don't see why this patch would
>>> be needed. octal_dtr_enable() only updates the mode when it succeeds so
>>> I don't think this patch is really needed.
>>>
>>
>> I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side.
>> So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.
>
> I still don't get the point: you fail the probe if the octal bit is
> enabled but the manufacturer did not implement octal_dtr_enable(), so
> how could we have issues? Maybe I am overlooking something though, but
> this seemed completely redundant to my eyes so far.
>

Okay, I feel this may be redundant. This is for the case when the
manufacturer has added Octal DTR read/write/update cache variants but
hasn't implemented the octal_dtr_enable() method.

Without this patch, the probe would fail, if the manufacturer did not
implement octal_dtr_enable(). But after using this patch, spinand can
still use the chip in 1s-1s-1s mode in that case and just skip the Octal
DTR op variants during the selection. And also the probe would succeed.

>>
>> Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.
>>
>> Removing this wouldn't break anything in the current implementation.
>> Do you think we should drop this?
>>
>>>>
>>>> Signed-off-by: Apurva Nandan <[email protected]>
>>>> ---
>>>> drivers/mtd/nand/spi/core.c | 7 ++++++-
>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>>>> index 19d8affac058..8711e887b795 100644
>>>> --- a/drivers/mtd/nand/spi/core.c
>>>> +++ b/drivers/mtd/nand/spi/core.c
>>>> @@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
>>>> if (id[0] != manufacturer->id)
>>>> continue;
>>>> >> + spinand->manufacturer = manufacturer;
>>>> +
>>>> ret = spinand_match_and_init(spinand,
>>>> manufacturer->chips,
>>>> manufacturer->nchips,
>>>> @@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
>>>> if (ret < 0)
>>>> continue;
>>>> >> - spinand->manufacturer = manufacturer;
>>>> return 0;
>>>> }
>>>> return -ENOTSUPP;
>>>> @@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
>>>> unsigned int nbytes;
>>>> int ret;
>>>> >> + if (spinand_op_is_octal_dtr(&op) &&
>>>> + !spinand->manufacturer->ops->octal_dtr_enable)
>>>> + continue;
>>>> +
>>>> nbytes = nanddev_per_page_oobsize(nand) +
>>>> nanddev_page_size(nand);
>>>> > > Thanks,
>>> Miquèl
>>>
>>> ______________________________________________________
>>> Linux MTD discussion mailing list
>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>>
>>
>> Thanks,
>> Apurva Nandan
>
>
>
>
> Thanks,
> Miquèl
>

Thanks,
Apurva Nandan

2021-08-20 14:20:11

by Miquel Raynal

Subject: Re: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 19:11:58
+0530:

> Hi Miquèl,
>
> On 20/08/21 5:48 pm, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07
> > +0530:
> >
> >> Hi Miquèl,
> >>
> >> On 07/08/21 12:38 am, Miquel Raynal wrote:
> >>> Hi Apurva,
> >>>
> >>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36
> >>> +0000:
> >>> >>>> Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
> >>>> functionality in their SPI NAND flash chips. PoR instruction consists
> >>>> of a 66h command followed by 99h command, and is different from the FFh
> >>>> reset. The reset command FFh just clears the status only registers,
> >>>> while the PoR command erases all the configurations written to the
> >>>> flash and is equivalent to a power-down -> power-up cycle.
> >>>>
> >>>> Add support for the Power-on-Reset command for any flash that provides
> >>>> this feature.
> >>>>
> >>>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
> >>>>
> >>>> Signed-off-by: Apurva Nandan <[email protected]>
> >>>> ---
> >>>
> >>> [...]
> >>> \
> >>>> @@ -218,6 +230,8 @@ struct spinand_device;
> >>>> * reading/programming/erasing when the RESET occurs. Since we always
> >>>> * issue a RESET when the device is IDLE, 5us is selected for both initial
> >>>> * and poll delay.
> >>>> + * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us
> >>>
> >>> s/max upto/up to/
> >>> >>
> >> Okay!
> >>
> >>>> + * to 1200 us safely.
> >>>
> >>> I don't really get why, if the maximum is 500, then let's wait for
> >>> 500us.
> >>> >>
> >> Generally we keep some margin from the maximum time, no?
> >
> > Well, yes and no.
> >
> > If you know that an operation will last Xms and have nothing else to
> > do, then you can take some margin if you are in a probe (called once)
> > but definitely not if you are in a fast path.
> >
>
> I think as PoR reset would be called at every mtd_suspend() call, so we can reduce the delay. And we would be expecting some time gap before the next mtd_resume() call.
>
> > Otherwise the best is to have some kind of signaling but I'm not sure
> > you'll have one for the reset op...
> >
>
> According to public datasheet, it doesn't set the busy bit during reset.
>
> So do you suggest in the favor of removing the delay margin?

Well, it's microseconds, maybe you can reduce it a little bit but that
will be ok.

>
> >>
> >>>> */
> >>>> #define SPINAND_READ_INITIAL_DELAY_US 6
> >>>> #define SPINAND_READ_POLL_DELAY_US 5
> >>>> @@ -227,6 +241,8 @@ struct spinand_device;
> >>>> #define SPINAND_WRITE_POLL_DELAY_US 15
> >>>> #define SPINAND_ERASE_INITIAL_DELAY_US 250
> >>>> #define SPINAND_ERASE_POLL_DELAY_US 50
> >>>> +#define SPINAND_POR_MIN_DELAY_US 1000
> >>>> +#define SPINAND_POR_MAX_DELAY_US 1200
> >>>> >> #define SPINAND_WAITRDY_TIMEOUT_MS 400
> >>>> >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
> >>>> #define SPINAND_HAS_QE_BIT BIT(0)
> >>>> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
> >>>> #define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
> >>>> +#define SPINAND_HAS_POR_CMD_BIT BIT(3)
> >>>> >> /**
> >>>> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
> >>>
> >>>
> >>>
> >>>
> >>> Thanks,
> >>> Miquèl
> >>>
> >>> ______________________________________________________
> >>> Linux MTD discussion mailing list
> >>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >>> >>
> >> Thanks,
> >> Apurva Nandan
> >
> > Thanks,
> > Miquèl
> >
>
> Thanks,
> Apurva Nandan

Thanks,
Miquèl

2021-08-20 14:40:21

by Miquel Raynal

Subject: Re: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

Hi Apurva,

Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 19:24:34
+0530:

> Hi Miquèl,
>
> On 20/08/21 5:44 pm, Miquel Raynal wrote:
> > Hi Apurva,
> >
> > Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50
> > +0530:
> >
> >> On 07/08/21 12:31 am, Miquel Raynal wrote:
> >>> Hi Apurva,
> >>>
> >>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33
> >>> +0000:
> >>> >>>> The SPI NAND core doesn't know how to switch the flash to Octal DTR
> >>>> mode (i.e. which operations to perform). If the manufacturer hasn't
> >>>> implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
> >>>> wouldn't be able to switch to 8D-8D-8D mode and will also not be able
> >>>> to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
> >>>> cache op_templates.
> >>>>
> >>>> So, avoid choosing a Octal DTR SPI op_template for read_cache,
> >>>> write_cache and update_cache operations, if the manufacturer_op
> >>>> octal_dtr_enable() is missing.
> >>>
> >>> After looking at your previous commit I don't see why this patch would
> >>> be needed. octal_dtr_enable() only updates the mode when it succeeds so
> >>> I don't think this patch is really needed.
> >>> >>
> >> I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side.
> >> So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.
> >
> > I still don't get the point: you fail the probe if the octal bit is
> > enabled but the manufacturer did not implement octal_dtr_enable(), so
> > how could we have issues? Maybe I am overlooking something though, but
> > this seemed completely redundant to my eyes so far.
> >
>
> Okay, I feel this may be redundant. This is for the case when the manufacturer has added Octal DTR read/write/update cache variants but hasn't implemented the octal_dtr_enable() method.
>
> Without this patch, the probe would fail, if the manufacturer did not implement octal_dtr_enable(). But after using this patch, spinand can still use the chip in 1s-1s-1s mode in that case and just skip the Octal DTR op variants during the selection. And also the probe would succeed.

Unless I am overlooking something with this series applied
(with or without this patch) the possibilities are:
- no octal bit -> continue as before
- octal bit and vendor callback -> uses octal mode
- octal bit and no vendor callback -> will return an error from
spinand_init_octal_dtr_enable() which will fail the probe (patch 7)

Anyway we have a choice:
- Either we consider the tables describing chips as pure descriptions
and we can support these chips in mode 1-1-1 (will require changes in
your series as this is not what you support as far as I understand
the code)
- Or we consider these tables as "what is currently supported" and in
this case we just fail if one adds the octal bit without any callback
implementation.

I think the latter is better for now. We can update this choice later
if needed anyway.

>
> >>
> >> Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.
> >>
> >> Removing this wouldn't break anything in the current implementation.
> >> Do you think we should drop this?
> >>
> >>>>
> >>>> Signed-off-by: Apurva Nandan <[email protected]>
> >>>> ---
> >>>> drivers/mtd/nand/spi/core.c | 7 ++++++-
> >>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> >>>> index 19d8affac058..8711e887b795 100644
> >>>> --- a/drivers/mtd/nand/spi/core.c
> >>>> +++ b/drivers/mtd/nand/spi/core.c
> >>>> @@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
> >>>> if (id[0] != manufacturer->id)
> >>>> continue;
> >>>> >> + spinand->manufacturer = manufacturer;
> >>>> +
> >>>> ret = spinand_match_and_init(spinand,
> >>>> manufacturer->chips,
> >>>> manufacturer->nchips,
> >>>> @@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
> >>>> if (ret < 0)
> >>>> continue;
> >>>> >> - spinand->manufacturer = manufacturer;
> >>>> return 0;
> >>>> }
> >>>> return -ENOTSUPP;
> >>>> @@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
> >>>> unsigned int nbytes;
> >>>> int ret;
> >>>> >> + if (spinand_op_is_octal_dtr(&op) &&
> >>>> + !spinand->manufacturer->ops->octal_dtr_enable)
> >>>> + continue;
> >>>> +
> >>>> nbytes = nanddev_per_page_oobsize(nand) +
> >>>> nanddev_page_size(nand);
> >>>> > > Thanks,
> >>> Miquèl
> >>>
> >>> ______________________________________________________
> >>> Linux MTD discussion mailing list
> >>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> >>> >>
> >> Thanks,
> >> Apurva Nandan
> >
> >
> >
> >
> > Thanks,
> > Miquèl
> >
>
> Thanks,
> Apurva Nandan

Thanks,
Miquèl

2021-08-20 15:55:55

by Apurva Nandan

Subject: Re: [PATCH 08/13] mtd: spinand: Reject 8D-8D-8D op_templates if octal_dtr_enale() is missing in manufacturer_op

Hi Miquèl,

On 20/08/21 8:08 pm, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 19:24:34
> +0530:
>
>> Hi Miquèl,
>>
>> On 20/08/21 5:44 pm, Miquel Raynal wrote:
>>> Hi Apurva,
>>>
>>> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 16:56:50
>>> +0530:
>>>
>>>> On 07/08/21 12:31 am, Miquel Raynal wrote:
>>>>> Hi Apurva,
>>>>>
>>>>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:33
>>>>> +0000:
>>>>> >>>> The SPI NAND core doesn't know how to switch the flash to Octal DTR
>>>>>> mode (i.e. which operations to perform). If the manufacturer hasn't
>>>>>> implemented the octal_dtr_enable() manufacturer_op, the SPI NAND core
>>>>>> wouldn't be able to switch to 8D-8D-8D mode and will also not be able
>>>>>> to run in 1S-1S-1S mode due to already selected 8D-8D-8D read/write
>>>>>> cache op_templates.
>>>>>>
>>>>>> So, avoid choosing a Octal DTR SPI op_template for read_cache,
>>>>>> write_cache and update_cache operations, if the manufacturer_op
>>>>>> octal_dtr_enable() is missing.
>>>>>
>>>>> After looking at your previous commit I don't see why this patch would
>>>>> be needed. octal_dtr_enable() only updates the mode when it succeeds so
>>>>> I don't think this patch is really needed.
>>>>> >>
>>>> I added it to prevent any errors happening dues to a missing implementation of octal_dtr_enable() from manufacturer driver side.
>>>> So, if the manufacturers skips the octal_dtr_enable() implementation, we want the spinand core to run in 1s-1s-1s mode.
>>>
>>> I still don't get the point: you fail the probe if the octal bit is
>>> enabled but the manufacturer did not implement octal_dtr_enable(), so
>>> how could we have issues? Maybe I am overlooking something though, but
>>> this seemed completely redundant to my eyes so far.
>>>
>>
>> Okay, I feel this may be redundant. This is for the case when the manufacturer has added Octal DTR read/write/update cache variants but hasn't implemented the octal_dtr_enable() method.
>>
>> Without this patch, the probe would fail, if the manufacturer did not implement octal_dtr_enable(). But after using this patch, spinand can still use the chip in 1s-1s-1s mode in that case and just skip the Octal DTR op variants during the selection. And also the probe would succeed.
>
> Unless I am overlooking something with this series applied
> (with or without this patch) the possibilities are:
> - no octal bit -> continue as before
> - octal bit and vendor callback -> uses octal mode
> - octal bit and no vendor callback -> will return an error from
> spinand_init_octal_dtr_enable() which will fail the probe (patch 7)
>
> Anyway we have a choice:
> - Either we consider the tables describing chips as pure descriptions
> and we can support these chips in mode 1-1-1 (will require changes in
> your series as this is not what you support as far as I understand
> the code)
> - Or we consider these tables as "what is currently supported" and in
> this case we just fail if one adds the octal bit without any callback
> implementation.
>
> I think the latter is better for now. We can update this choice later
> if needed anyway.
>

Yes, I fully agree with the latter. I will drop this patch in the v2.
Thanks!

>>
>>>>
>>>> Read/write/update op variant selection happens in select_op_variant(), much before octal_dtr_enable(). So just check if there is a definition of octal_dtr_enable in manufacturer ops and then only use 8D op variants.
>>>>
>>>> Removing this wouldn't break anything in the current implementation.
>>>> Do you think we should drop this?
>>>>
>>>>>>
>>>>>> Signed-off-by: Apurva Nandan <[email protected]>
>>>>>> ---
>>>>>> drivers/mtd/nand/spi/core.c | 7 ++++++-
>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>>>>>> index 19d8affac058..8711e887b795 100644
>>>>>> --- a/drivers/mtd/nand/spi/core.c
>>>>>> +++ b/drivers/mtd/nand/spi/core.c
>>>>>> @@ -1028,6 +1028,8 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
>>>>>> if (id[0] != manufacturer->id)
>>>>>> continue;
>>>>>> >> + spinand->manufacturer = manufacturer;
>>>>>> +
>>>>>> ret = spinand_match_and_init(spinand,
>>>>>> manufacturer->chips,
>>>>>> manufacturer->nchips,
>>>>>> @@ -1035,7 +1037,6 @@ static int spinand_manufacturer_match(struct spinand_device *spinand,
>>>>>> if (ret < 0)
>>>>>> continue;
>>>>>> >> - spinand->manufacturer = manufacturer;
>>>>>> return 0;
>>>>>> }
>>>>>> return -ENOTSUPP;
>>>>>> @@ -1097,6 +1098,10 @@ spinand_select_op_variant(struct spinand_device *spinand,
>>>>>> unsigned int nbytes;
>>>>>> int ret;
>>>>>> >> + if (spinand_op_is_octal_dtr(&op) &&
>>>>>> + !spinand->manufacturer->ops->octal_dtr_enable)
>>>>>> + continue;
>>>>>> +
>>>>>> nbytes = nanddev_per_page_oobsize(nand) +
>>>>>> nanddev_page_size(nand);
>>>>>> > > Thanks,
>>>>> Miquèl
>>>>>
>>>>> ______________________________________________________
>>>>> Linux MTD discussion mailing list
>>>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>>>> >>
>>>> Thanks,
>>>> Apurva Nandan
>>>
>>>
>>>
>>>
>>> Thanks,
>>> Miquèl
>>>
>>
>> Thanks,
>> Apurva Nandan
>
> Thanks,
> Miquèl
>

Thanks,
Apurva Nandan

2021-08-20 15:59:12

by Apurva Nandan

Subject: Re: [PATCH 11/13] mtd: spinand: Add support for Power-on-Reset (PoR) instruction

Hi Miquèl,

On 20/08/21 7:47 pm, Miquel Raynal wrote:
> Hi Apurva,
>
> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 19:11:58
> +0530:
>
>> Hi Miquèl,
>>
>> On 20/08/21 5:48 pm, Miquel Raynal wrote:
>>> Hi Apurva,
>>>
>>> Apurva Nandan <[email protected]> wrote on Fri, 20 Aug 2021 17:09:07
>>> +0530:
>>>
>>>> Hi Miquèl,
>>>>
>>>> On 07/08/21 12:38 am, Miquel Raynal wrote:
>>>>> Hi Apurva,
>>>>>
>>>>> Apurva Nandan <[email protected]> wrote on Tue, 13 Jul 2021 13:05:36
>>>>> +0000:
>>>>> >>>> Manufacturers like Gigadevice and Winbond are adding Power-on-Reset
>>>>>> functionality in their SPI NAND flash chips. PoR instruction consists
>>>>>> of a 66h command followed by 99h command, and is different from the FFh
>>>>>> reset. The reset command FFh just clears the status only registers,
>>>>>> while the PoR command erases all the configurations written to the
>>>>>> flash and is equivalent to a power-down -> power-up cycle.
>>>>>>
>>>>>> Add support for the Power-on-Reset command for any flash that provides
>>>>>> this feature.
>>>>>>
>>>>>> Datasheet: https://www.winbond.com/export/sites/winbond/datasheet/W35N01JW_Datasheet_Brief.pdf
>>>>>>
>>>>>> Signed-off-by: Apurva Nandan <[email protected]>
>>>>>> ---
>>>>>
>>>>> [...]
>>>>> \
>>>>>> @@ -218,6 +230,8 @@ struct spinand_device;
>>>>>> * reading/programming/erasing when the RESET occurs. Since we always
>>>>>> * issue a RESET when the device is IDLE, 5us is selected for both initial
>>>>>> * and poll delay.
>>>>>> + * Power on Reset can take max upto 500 us to complete, so sleep for 1000 us
>>>>>
>>>>> s/max upto/up to/
>>>>> >>
>>>> Okay!
>>>>
>>>>>> + * to 1200 us safely.
>>>>>
>>>>> I don't really get why, if the maximum is 500, then let's wait for
>>>>> 500us.
>>>>> >>
>>>> Generally we keep some margin from the maximum time, no?
>>>
>>> Well, yes and no.
>>>
>>> If you know that an operation will last Xms and have nothing else to
>>> do, then you can take some margin if you are in a probe (called once)
>>> but definitely not if you are in a fast path.
>>>
>>
>> I think as PoR reset would be called at every mtd_suspend() call, so we can reduce the delay. And we would be expecting some time gap before the next mtd_resume() call.
>>
>>> Otherwise the best is to have some kind of signaling but I'm not sure
>>> you'll have one for the reset op...
>>>
>>
>> According to public datasheet, it doesn't set the busy bit during reset.
>>
>> So do you suggest in the favor of removing the delay margin?
>
> Well, it's microseconds, maybe you can reduce it a little bit but that
> will be ok.
>

Yes, I got it. Will improve this in v2. Thanks!

>>
>>>>
>>>>>> */
>>>>>> #define SPINAND_READ_INITIAL_DELAY_US 6
>>>>>> #define SPINAND_READ_POLL_DELAY_US 5
>>>>>> @@ -227,6 +241,8 @@ struct spinand_device;
>>>>>> #define SPINAND_WRITE_POLL_DELAY_US 15
>>>>>> #define SPINAND_ERASE_INITIAL_DELAY_US 250
>>>>>> #define SPINAND_ERASE_POLL_DELAY_US 50
>>>>>> +#define SPINAND_POR_MIN_DELAY_US 1000
>>>>>> +#define SPINAND_POR_MAX_DELAY_US 1200
>>>>>> >> #define SPINAND_WAITRDY_TIMEOUT_MS 400
>>>>>> >> @@ -351,6 +367,7 @@ struct spinand_ecc_info {
>>>>>> #define SPINAND_HAS_QE_BIT BIT(0)
>>>>>> #define SPINAND_HAS_CR_FEAT_BIT BIT(1)
>>>>>> #define SPINAND_HAS_OCTAL_DTR_BIT BIT(2)
>>>>>> +#define SPINAND_HAS_POR_CMD_BIT BIT(3)
>>>>>> >> /**
>>>>>> * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Miquèl
>>>>>
>>>>> ______________________________________________________
>>>>> Linux MTD discussion mailing list
>>>>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>>>> >>
>>>> Thanks,
>>>> Apurva Nandan
>>>
>>> Thanks,
>>> Miquèl
>>>
>>
>> Thanks,
>> Apurva Nandan
>
> Thanks,
> Miquèl
>

Thanks,
Apurva Nandan

2021-08-23 07:12:57

by Boris Brezillon

Subject: Re: [PATCH 03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

On Fri, 20 Aug 2021 14:08:40 +0200
Miquel Raynal <[email protected]> wrote:

> Hi Apurva,
>
> Boris, you might have a good idea for the naming discussed below?

{patch,adjust,tweak}_op() all sound good to me. This being said, I'm
a bit concerned about doing this op tweaking in the hot-path.
Looks like something that should be done at probe or when switching to
8D mode, and kept around. The alternative would be to have per-mode op
tables, which I think would be clearer.

2021-08-23 07:26:12

by Miquel Raynal

Subject: Re: [PATCH 03/13] mtd: spinand: Setup spi_mem_op for the SPI IO protocol using reg_proto

Hi Boris,

Boris Brezillon <[email protected]> wrote on Mon, 23 Aug
2021 09:11:45 +0200:

> On Fri, 20 Aug 2021 14:08:40 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Apurva,
> >
> > Boris, you might have a good idea for the naming discussed below?
>
> {patch,adjust,tweak}_op() all sound good to me. This being said, I'm
> a bit concerned about doing this op tweaking in the hot-path.
> Looks like something that should be done at probe or when switching to
> 8D mode, and kept around. The alternative would be to have per-mode op
> tables, which I think would be clearer.

True! Thanks for the idea!

Cheers,
Miquèl