linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support
@ 2020-06-23 18:30 Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
                   ` (18 more replies)
  0 siblings, 19 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Hi,

This series adds support for octal DTR flashes in the spi-nor framework,
and then adds hooks for the Cypress Semper and Mircom Xcella flashes to
allow running them in octal DTR mode. This series assumes that the flash
is handed to the kernel in Legacy SPI mode.

Tested on TI J721e EVM with 1-bit ECC on the Cypress flash.

Changes in v10:
- Rebase on latest linux-next/master. Drop a couple patches that made it
  in the  previous release.

- Move the code that sets 20 dummy cycles for MT35XU512ABA to its octal
  enable function. This way, if the controller doesn't support 8D mode
  20 dummy cycles won't be used.

Changes in v9:
- Do not use '& 0xff' to get the opcode LSB in spi-mxic and
  spi-zynq-qspi. The cast to u8 will do that anyway.

- Do not use if (opcode) as a check for whether the command phase exists
  in spi-zynq-qspi because the opcode 0 can be valid. Use the new
  cmd.nbytes instead.

Changes in v8:
- Move controller changes in spi-mxic to the commit which introduces
  2-byte opcodes to avoid problems when bisecting.

- Replace usage of sizeof(op->cmd.opcode) with op->cmd.nbytes.

- Extract opcode in spi-zynq-qspi instead of using &op->cmd.opcode.

Changes in v7:
- Reject ops with more than 1 command byte in
  spi_mem_default_supports_op().

- Reject ops with more than 1 command byte in atmel and mtk controllers.

- Reject ops with 0 command bytes in spi_mem_check_op().

- Set cmd.nbytes to 1 when using SPI_MEM_OP_CMD().

- Avoid endianness problems in spi-mxic.

Changes in v6:
- Instead of hard-coding 8D-8D-8D Fast Read dummy cycles to 20, find
  them out from the Profile 1.0 table.

Changes in v5:
- Do not enable stateful X-X-X modes if the reset line is broken.

- Instead of setting SNOR_READ_HWCAPS_8_8_8_DTR from Profile 1.0 table
  parsing, do it in spi_nor_info_init_params() instead based on the
  SPI_NOR_OCTAL_DTR_READ flag instead.

- Set SNOR_HWCAPS_PP_8_8_8_DTR in s28hs post_sfdp hook since this
  capability is no longer set in Profile 1.0 parsing.

- Instead of just checking for spi_nor_get_protocol_width() in
  spi_nor_octal_dtr_enable(), make sure the protocol is
  SNOR_PROTO_8_8_8_DTR since get_protocol_width() only cares about data
  width.

- Drop flag SPI_NOR_SOFT_RESET. Instead, discover soft reset capability
  via BFPT.

- Do not make an invalid Quad Enable BFPT field a fatal error. Silently
  ignore it by assuming no quad enable bit is present.

- Set dummy cycles for Cypress Semper flash to 24 instead of 20. This
  allows for 200MHz operation in 8D mode compared to the 166MHz with 20.

- Rename spi_nor_cypress_octal_enable() to
  spi_nor_cypress_octal_dtr_enable().

- Update spi-mtk-nor.c to reject DTR ops since it doesn't call
  spi_mem_default_supports_op().

Changes in v4:
- Refactor the series to use the new spi-nor framework with the
  manufacturer-specific bits separated from the core.

- Add support for Micron MT35XU512ABA.

- Use cmd.nbytes as the criteria of whether the data phase exists or not
  instead of cmd.buf.in || cmd.buf.out in spi_nor_spimem_setup_op().

- Update Read FSR to use the same dummy cycles and address width as Read
  SR.

- Fix BFPT parsing stopping too early for JESD216 rev B flashes.

- Use 2 byte reads for Read SR and FSR commands in DTR mode.

Changes in v3:
- Drop the DT properties "spi-rx-dtr" and "spi-tx-dtr". Instead, if
  later a need is felt to disable DTR in case someone has a board with
  Octal DTR capable flash but does not support DTR transactions for some
  reason, a property like "spi-no-dtr" can be added.

- Remove mode bits SPI_RX_DTR and SPI_TX_DTR.

- Remove the Cadence Quadspi controller patch to un-block this series. I
  will submit it as a separate patch.

- Rebase on latest 'master' and fix merge conflicts.

- Update read and write dirmap templates to use DTR.

- Rename 'is_dtr' to 'dtr'.

- Make 'dtr' a bitfield.

- Reject DTR ops in spi_mem_default_supports_op().

- Update atmel-quadspi to reject DTR ops. All other controller drivers
  call spi_mem_default_supports_op() so they will automatically reject
  DTR ops.

- Add support for both enabling and disabling DTR modes.

- Perform a Software Reset on flashes that support it when shutting
  down.

- Disable Octal DTR mode on suspend, and re-enable it on resume.

- Drop enum 'spi_mem_cmd_ext' and make command opcode u16 instead.
  Update spi-nor to use the 2-byte command instead of the command
  extension. Since we still need a "extension type", mode that enum to
  spi-nor and name it 'spi_nor_cmd_ext'.

- Default variable address width to 3 to fix SMPT parsing.

- Drop non-volatile change to uniform sector mode and rely on parsing
  SMPT.

Changes in v2:
- Add DT properties "spi-rx-dtr" and "spi-tx-dtr" to allow expressing
  DTR capabilities.

- Set the mode bits SPI_RX_DTR and SPI_TX_DTR when we discover the DT
  properties "spi-rx-dtr" and spi-tx-dtr".

- spi_nor_cypress_octal_enable() was updating nor->params.read[] with
  the intention of setting the correct number of dummy cycles. But this
  function is called _after_ selecting the read so setting
  nor->params.read[] will have no effect. So, update nor->read_dummy
  directly.

- Fix spi_nor_spimem_check_readop() and spi_nor_spimem_check_pp()
  passing nor->read_proto and nor->write_proto to
  spi_nor_spimem_setup_op() instead of read->proto and pp->proto
  respectively.

- Move the call to cqspi_setup_opcode_ext() inside cqspi_enable_dtr().
  This avoids repeating the 'if (f_pdata->is_dtr)
  cqspi_setup_opcode_ext()...` snippet multiple times.

- Call the default 'supports_op()' from cqspi_supports_mem_op(). This
  makes sure the buswidth requirements are also enforced along with the
  DTR requirements.

- Drop the 'is_dtr' argument from spi_check_dtr_req(). We only call it
  when a phase is DTR so it is redundant.

Pratyush Yadav (17):
  spi: spi-mem: allow specifying whether an op is DTR or not
  spi: spi-mem: allow specifying a command's extension
  spi: atmel-quadspi: reject DTR ops
  spi: spi-mtk-nor: reject DTR ops
  mtd: spi-nor: add support for DTR protocol
  mtd: spi-nor: sfdp: get command opcode extension type from BFPT
  mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table
  mtd: spi-nor: core: use dummy cycle and address width info from SFDP
  mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode
  mtd: spi-nor: core: enable octal DTR mode when possible
  mtd: spi-nor: sfdp: do not make invalid quad enable fatal
  mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT
  mtd: spi-nor: core: perform a Soft Reset on shutdown
  mtd: spi-nor: core: disable Octal DTR mode on suspend.
  mtd: spi-nor: core: expose spi_nor_default_setup() in core.h
  mtd: spi-nor: spansion: add support for Cypress Semper flash
  mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode

 drivers/mtd/spi-nor/core.c      | 446 +++++++++++++++++++++++++++-----
 drivers/mtd/spi-nor/core.h      |  22 ++
 drivers/mtd/spi-nor/micron-st.c | 103 +++++++-
 drivers/mtd/spi-nor/sfdp.c      | 131 +++++++++-
 drivers/mtd/spi-nor/sfdp.h      |   8 +
 drivers/mtd/spi-nor/spansion.c  | 166 ++++++++++++
 drivers/spi/atmel-quadspi.c     |   6 +
 drivers/spi/spi-mem.c           |  16 +-
 drivers/spi/spi-mtk-nor.c       |  10 +-
 drivers/spi/spi-mxic.c          |   3 +-
 drivers/spi/spi-zynq-qspi.c     |  11 +-
 include/linux/mtd/spi-nor.h     |  53 +++-
 include/linux/spi/spi-mem.h     |  14 +-
 13 files changed, 889 insertions(+), 100 deletions(-)

--
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-08 16:22   ` Tudor.Ambarus
  2020-07-13  3:55   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension Pratyush Yadav
                   ` (17 subsequent siblings)
  18 siblings, 2 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Each phase is given a separate 'dtr' field so mixed protocols like
4S-4D-4D can be supported.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/spi/spi-mem.c       | 3 +++
 include/linux/spi/spi-mem.h | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 9a86cc27fcc0..93e255287ab9 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -156,6 +156,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 				   op->data.dir == SPI_MEM_DATA_OUT))
 		return false;
 
+	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
+		return false;
+
 	return true;
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index af9ff2f0f1b2..e3dcb956bf61 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -71,9 +71,11 @@ enum spi_mem_data_dir {
  * struct spi_mem_op - describes a SPI memory operation
  * @cmd.buswidth: number of IO lines used to transmit the command
  * @cmd.opcode: operation opcode
+ * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
  * @addr.nbytes: number of address bytes to send. Can be zero if the operation
  *		 does not need to send an address
  * @addr.buswidth: number of IO lines used to transmit the address cycles
+ * @addr.dtr: whether the address should be sent in DTR mode or not
  * @addr.val: address value. This value is always sent MSB first on the bus.
  *	      Note that only @addr.nbytes are taken into account in this
  *	      address value, so users should make sure the value fits in the
@@ -81,7 +83,9 @@ enum spi_mem_data_dir {
  * @dummy.nbytes: number of dummy bytes to send after an opcode or address. Can
  *		  be zero if the operation does not require dummy bytes
  * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
+ * @dummy.dtr: whether the dummy bytes should be sent in DTR mode or not
  * @data.buswidth: number of IO lanes used to send/receive the data
+ * @data.dtr: whether the data should be sent in DTR mode or not
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
@@ -91,22 +95,26 @@ enum spi_mem_data_dir {
 struct spi_mem_op {
 	struct {
 		u8 buswidth;
+		u8 dtr : 1;
 		u8 opcode;
 	} cmd;
 
 	struct {
 		u8 nbytes;
 		u8 buswidth;
+		u8 dtr : 1;
 		u64 val;
 	} addr;
 
 	struct {
 		u8 nbytes;
 		u8 buswidth;
+		u8 dtr : 1;
 	} dummy;
 
 	struct {
 		u8 buswidth;
+		u8 dtr : 1;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
 		union {
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-13  6:15   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops Pratyush Yadav
                   ` (16 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

In xSPI mode, flashes expect 2-byte opcodes. The second byte is called
the "command extension". There can be 3 types of extensions in xSPI:
repeat, invert, and hex. When the extension type is "repeat", the same
opcode is sent twice. When it is "invert", the second byte is the
inverse of the opcode. When it is "hex" an additional opcode byte based
is sent with the command whose value can be anything.

So, make opcode a 16-bit value and add a 'nbytes', similar to how
multiple address widths are handled.

Some places use sizeof(op->cmd.opcode). Replace them with op->cmd.nbytes

The spi-mxic and spi-zynq-qspi drivers directly use op->cmd.opcode as a
buffer. Now that opcode is a 2-byte field, this can result in different
behaviour depending on if the machine is little endian or big endian.
Extract the opcode in a local 1-byte variable and use that as the buffer
instead. Both these drivers would reject multi-byte opcodes in their
supports_op() hook anyway, so we only need to worry about single-byte
opcodes for now.

The above two changes are put in this commit to keep the series
bisectable.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/spi/spi-mem.c       | 13 +++++++------
 drivers/spi/spi-mtk-nor.c   |  4 ++--
 drivers/spi/spi-mxic.c      |  3 ++-
 drivers/spi/spi-zynq-qspi.c | 11 ++++++-----
 include/linux/spi/spi-mem.h |  6 +++++-
 5 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 93e255287ab9..ef53290b7d24 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -159,6 +159,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
 		return false;
 
+	if (op->cmd.nbytes != 1)
+		return false;
+
 	return true;
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
@@ -173,7 +176,7 @@ static bool spi_mem_buswidth_is_valid(u8 buswidth)
 
 static int spi_mem_check_op(const struct spi_mem_op *op)
 {
-	if (!op->cmd.buswidth)
+	if (!op->cmd.buswidth || !op->cmd.nbytes)
 		return -EINVAL;
 
 	if ((op->addr.nbytes && !op->addr.buswidth) ||
@@ -309,8 +312,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 			return ret;
 	}
 
-	tmpbufsize = sizeof(op->cmd.opcode) + op->addr.nbytes +
-		     op->dummy.nbytes;
+	tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
 
 	/*
 	 * Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so
@@ -325,7 +327,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 
 	tmpbuf[0] = op->cmd.opcode;
 	xfers[xferpos].tx_buf = tmpbuf;
-	xfers[xferpos].len = sizeof(op->cmd.opcode);
+	xfers[xferpos].len = op->cmd.nbytes;
 	xfers[xferpos].tx_nbits = op->cmd.buswidth;
 	spi_message_add_tail(&xfers[xferpos], &msg);
 	xferpos++;
@@ -427,8 +429,7 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 		return ctlr->mem_ops->adjust_op_size(mem, op);
 
 	if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
-		len = sizeof(op->cmd.opcode) + op->addr.nbytes +
-		      op->dummy.nbytes;
+		len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
 
 		if (len > spi_max_transfer_size(mem->spi))
 			return -EINVAL;
diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index 7bc302b50396..d5f393871619 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -195,7 +195,7 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
 		}
 	}
 
-	len = MTK_NOR_PRG_MAX_SIZE - sizeof(op->cmd.opcode) - op->addr.nbytes -
+	len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
 	      op->dummy.nbytes;
 	if (op->data.nbytes > len)
 		op->data.nbytes = len;
@@ -219,7 +219,7 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
 			       (op->dummy.buswidth == 0) &&
 			       (op->data.buswidth == 1);
 	}
-	len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
+	len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
 	if ((len > MTK_NOR_PRG_MAX_SIZE) ||
 	    ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
 		return false;
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 69491f3a515d..8c630acb0110 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	int nio = 1, i, ret;
 	u32 ss_ctrl;
 	u8 addr[8];
+	u8 opcode = op->cmd.opcode;
 
 	ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
 	if (ret)
@@ -393,7 +394,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
 	       mxic->regs + HC_CFG);
 
-	ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
+	ret = mxic_spi_data_xfer(mxic, &opcode, NULL, 1);
 	if (ret)
 		goto out;
 
diff --git a/drivers/spi/spi-zynq-qspi.c b/drivers/spi/spi-zynq-qspi.c
index 17641157354d..bbf3d90561f5 100644
--- a/drivers/spi/spi-zynq-qspi.c
+++ b/drivers/spi/spi-zynq-qspi.c
@@ -527,20 +527,21 @@ static int zynq_qspi_exec_mem_op(struct spi_mem *mem,
 	struct zynq_qspi *xqspi = spi_controller_get_devdata(mem->spi->master);
 	int err = 0, i;
 	u8 *tmpbuf;
+	u8 opcode = op->cmd.opcode;
 
 	dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n",
-		op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
+		opcode, op->cmd.buswidth, op->addr.buswidth,
 		op->dummy.buswidth, op->data.buswidth);
 
 	zynq_qspi_chipselect(mem->spi, true);
 	zynq_qspi_config_op(xqspi, mem->spi);
 
-	if (op->cmd.opcode) {
+	if (op->cmd.nbytes) {
 		reinit_completion(&xqspi->data_completion);
-		xqspi->txbuf = (u8 *)&op->cmd.opcode;
+		xqspi->txbuf = &opcode;
 		xqspi->rxbuf = NULL;
-		xqspi->tx_bytes = sizeof(op->cmd.opcode);
-		xqspi->rx_bytes = sizeof(op->cmd.opcode);
+		xqspi->tx_bytes = op->cmd.nbytes;
+		xqspi->rx_bytes = op->cmd.nbytes;
 		zynq_qspi_write_op(xqspi, ZYNQ_QSPI_FIFO_DEPTH, true);
 		zynq_qspi_write(xqspi, ZYNQ_QSPI_IEN_OFFSET,
 				ZYNQ_QSPI_IXR_RXTX_MASK);
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index e3dcb956bf61..159463cc659c 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -17,6 +17,7 @@
 	{							\
 		.buswidth = __buswidth,				\
 		.opcode = __opcode,				\
+		.nbytes = 1,					\
 	}
 
 #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth)		\
@@ -69,6 +70,8 @@ enum spi_mem_data_dir {
 
 /**
  * struct spi_mem_op - describes a SPI memory operation
+ * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
+ *		sent MSB-first.
  * @cmd.buswidth: number of IO lines used to transmit the command
  * @cmd.opcode: operation opcode
  * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
@@ -94,9 +97,10 @@ enum spi_mem_data_dir {
  */
 struct spi_mem_op {
 	struct {
+		u8 nbytes;
 		u8 buswidth;
 		u8 dtr : 1;
-		u8 opcode;
+		u16 opcode;
 	} cmd;
 
 	struct {
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-13  6:19   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 04/17] spi: spi-mtk-nor: " Pratyush Yadav
                   ` (15 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Double Transfer Rate (DTR) ops are added in spi-mem. But this controller
doesn't support DTR transactions. Since we don't use the default
supports_op(), which rejects all DTR ops, do that explicitly in our
supports_op().

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/spi/atmel-quadspi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
index cb44d1e169aa..a898755fb41e 100644
--- a/drivers/spi/atmel-quadspi.c
+++ b/drivers/spi/atmel-quadspi.c
@@ -285,6 +285,12 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
 		op->dummy.nbytes == 0)
 		return false;
 
+	/* DTR ops not supported. */
+	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
+		return false;
+	if (op->cmd.nbytes != 1)
+		return false;
+
 	return true;
 }
 
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 04/17] spi: spi-mtk-nor: reject DTR ops
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (2 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-13  6:24   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol Pratyush Yadav
                   ` (14 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Double Transfer Rate (DTR) ops are added in spi-mem. But this controller
doesn't support DTR transactions. Since we don't use the default
supports_op(), which rejects all DTR ops, do that explicitly in our
supports_op().

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/spi/spi-mtk-nor.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
index d5f393871619..b08d8e9a8ee9 100644
--- a/drivers/spi/spi-mtk-nor.c
+++ b/drivers/spi/spi-mtk-nor.c
@@ -211,6 +211,12 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
 	if (op->cmd.buswidth != 1)
 		return false;
 
+	/* DTR ops not supported. */
+	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
+		return false;
+	if (op->cmd.nbytes != 1)
+		return false;
+
 	if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
 		if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op))
 			return true;
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (3 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 04/17] spi: spi-mtk-nor: " Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-07 17:37   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT Pratyush Yadav
                   ` (13 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Double Transfer Rate (DTR) is SPI protocol in which data is transferred
on each clock edge as opposed to on each clock cycle. Make
framework-level changes to allow supporting flashes in DTR mode.

Right now, mixed DTR modes are not supported. So, for example a mode
like 4S-4D-4D will not work. All phases need to be either DTR or STR.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c  | 305 ++++++++++++++++++++++++++++--------
 drivers/mtd/spi-nor/core.h  |   6 +
 drivers/mtd/spi-nor/sfdp.c  |   9 +-
 include/linux/mtd/spi-nor.h |  51 ++++--
 4 files changed, 295 insertions(+), 76 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0369d98b2d12..22a3832b83a6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -40,6 +40,76 @@
 
 #define SPI_NOR_MAX_ADDR_WIDTH	4
 
+/**
+ * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
+ *			   extension type.
+ * @nor:		pointer to a 'struct spi_nor'
+ * @op:			pointer to the 'struct spi_mem_op' whose properties
+ *			need to be initialized.
+ *
+ * Right now, only "repeat" and "invert" are supported.
+ *
+ * Return: The opcode extension.
+ */
+static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor,
+			      const struct spi_mem_op *op)
+{
+	switch (nor->cmd_ext_type) {
+	case SPI_NOR_EXT_INVERT:
+		return ~op->cmd.opcode;
+
+	case SPI_NOR_EXT_REPEAT:
+		return op->cmd.opcode;
+
+	default:
+		dev_err(nor->dev, "Unknown command extension type\n");
+		return 0;
+	}
+}
+
+/**
+ * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op.
+ * @nor:		pointer to a 'struct spi_nor'
+ * @op:			pointer to the 'struct spi_mem_op' whose properties
+ *			need to be initialized.
+ * @proto:		the protocol from which the properties need to be set.
+ */
+void spi_nor_spimem_setup_op(const struct spi_nor *nor,
+			     struct spi_mem_op *op,
+			     const enum spi_nor_protocol proto)
+{
+	u8 ext;
+
+	op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
+
+	if (op->addr.nbytes)
+		op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
+
+	if (op->dummy.nbytes)
+		op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
+
+	if (op->data.nbytes)
+		op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
+
+	if (spi_nor_protocol_is_dtr(proto)) {
+		/*
+		 * spi-mem supports mixed DTR modes, but right now we can only
+		 * have all phases either DTR or STR. IOW, spi-mem can have
+		 * something like 4S-4D-4D, but spi-nor can't. So, set all 4
+		 * phases to either DTR or STR.
+		 */
+		op->cmd.dtr = op->addr.dtr = op->dummy.dtr
+			       = op->data.dtr = true;
+
+		/* 2 bytes per clock cycle in DTR mode. */
+		op->dummy.nbytes *= 2;
+
+		ext = spi_nor_get_cmd_ext(nor, op);
+		op->cmd.opcode = (op->cmd.opcode << 8) | ext;
+		op->cmd.nbytes = 2;
+	}
+}
+
 /**
  * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data
  *                           transfer
@@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
 	ssize_t nbytes;
 	int error;
 
-	/* get transfer protocols. */
-	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
-	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
-	op.dummy.buswidth = op.addr.buswidth;
-	op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
+	spi_nor_spimem_setup_op(nor, &op, nor->read_proto);
 
 	/* convert the dummy cycles to the number of bytes */
 	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
+	if (spi_nor_protocol_is_dtr(nor->read_proto))
+		op.dummy.nbytes *= 2;
 
 	usebouncebuf = spi_nor_spimem_bounce(nor, &op);
 
@@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
 	ssize_t nbytes;
 	int error;
 
-	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
-	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
-	op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
-
 	if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
 		op.addr.nbytes = 0;
 
+	spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
+
 	if (spi_nor_spimem_bounce(nor, &op))
 		memcpy(nor->bouncebuf, buf, op.data.nbytes);
 
@@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN,
-						     NULL, 0);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_WREN,
+							     NULL, 0);
 	}
 
 	if (ret)
@@ -256,10 +328,16 @@ int spi_nor_write_disable(struct spi_nor *nor)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRDI,
-						     NULL, 0);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_WRDI,
+							     NULL, 0);
 	}
 
 	if (ret)
@@ -318,10 +396,15 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, fsr, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDFSR,
-						    fsr, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDFSR,
+							    fsr, 1);
 	}
 
 	if (ret)
@@ -350,9 +433,15 @@ static int spi_nor_read_cr(struct spi_nor *nor, u8 *cr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, cr, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDCR, cr, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDCR,
+							    cr, 1);
 	}
 
 	if (ret)
@@ -383,12 +472,17 @@ int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 				  SPI_MEM_OP_NO_DUMMY,
 				  SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor,
-						     enable ? SPINOR_OP_EN4B :
-							      SPINOR_OP_EX4B,
-						     NULL, 0);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+						enable ? SPINOR_OP_EN4B :
+							 SPINOR_OP_EX4B,
+						NULL, 0);
 	}
 
 	if (ret)
@@ -419,10 +513,15 @@ static int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_BRWR,
-						     nor->bouncebuf, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor, SPINOR_OP_BRWR,
+							     nor->bouncebuf, 1);
 	}
 
 	if (ret)
@@ -451,10 +550,16 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_OUT(1, nor->bouncebuf, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREAR,
-						     nor->bouncebuf, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_WREAR,
+							     nor->bouncebuf, 1);
 	}
 
 	if (ret)
@@ -482,10 +587,16 @@ int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, sr, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_XRDSR,
-						    sr, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->read_reg(nor,
+							    SPINOR_OP_XRDSR,
+							    sr, 1);
 	}
 
 	if (ret)
@@ -527,10 +638,16 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CLSR,
-						     NULL, 0);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_CLSR,
+							     NULL, 0);
 	}
 
 	if (ret)
@@ -591,10 +708,16 @@ static void spi_nor_clear_fsr(struct spi_nor *nor)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CLFSR,
-						     NULL, 0);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_CLFSR,
+							     NULL, 0);
 	}
 
 	if (ret)
@@ -735,10 +858,16 @@ static int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_OUT(len, sr, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRSR,
-						     sr, len);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_WRSR,
+							     sr, len);
 	}
 
 	if (ret) {
@@ -937,10 +1066,16 @@ static int spi_nor_write_sr2(struct spi_nor *nor, const u8 *sr2)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_OUT(1, sr2, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WRSR2,
-						     sr2, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_WRSR2,
+							     sr2, 1);
 	}
 
 	if (ret) {
@@ -971,10 +1106,16 @@ static int spi_nor_read_sr2(struct spi_nor *nor, u8 *sr2)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, sr2, 1));
 
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR2,
-						    sr2, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->read_reg(nor,
+							    SPINOR_OP_RDSR2,
+							    sr2, 1);
 	}
 
 	if (ret)
@@ -1002,10 +1143,16 @@ static int spi_nor_erase_chip(struct spi_nor *nor)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->write_reg(nor, SPINOR_OP_CHIP_ERASE,
-						     NULL, 0);
+		if (spi_nor_protocol_is_dtr(nor->write_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->write_reg(nor,
+							     SPINOR_OP_CHIP_ERASE,
+							     NULL, 0);
 	}
 
 	if (ret)
@@ -1144,7 +1291,11 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
 
+		spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
+
 		return spi_mem_exec_op(nor->spimem, &op);
+	} else if (spi_nor_protocol_is_dtr(nor->write_proto)) {
+		return -ENOTSUPP;
 	} else if (nor->controller_ops->erase) {
 		return nor->controller_ops->erase(nor, addr);
 	}
@@ -2253,6 +2404,7 @@ int spi_nor_hwcaps_read2cmd(u32 hwcaps)
 		{ SNOR_HWCAPS_READ_1_8_8,	SNOR_CMD_READ_1_8_8 },
 		{ SNOR_HWCAPS_READ_8_8_8,	SNOR_CMD_READ_8_8_8 },
 		{ SNOR_HWCAPS_READ_1_8_8_DTR,	SNOR_CMD_READ_1_8_8_DTR },
+		{ SNOR_HWCAPS_READ_8_8_8_DTR,	SNOR_CMD_READ_8_8_8_DTR },
 	};
 
 	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_read2cmd,
@@ -2269,6 +2421,7 @@ static int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
 		{ SNOR_HWCAPS_PP_1_1_8,		SNOR_CMD_PP_1_1_8 },
 		{ SNOR_HWCAPS_PP_1_8_8,		SNOR_CMD_PP_1_8_8 },
 		{ SNOR_HWCAPS_PP_8_8_8,		SNOR_CMD_PP_8_8_8 },
+		{ SNOR_HWCAPS_PP_8_8_8_DTR,	SNOR_CMD_PP_8_8_8_DTR },
 	};
 
 	return spi_nor_hwcaps2cmd(hwcaps, hwcaps_pp2cmd,
@@ -2322,13 +2475,11 @@ static int spi_nor_spimem_check_readop(struct spi_nor *nor,
 					  SPI_MEM_OP_DUMMY(0, 1),
 					  SPI_MEM_OP_DATA_IN(0, NULL, 1));
 
-	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(read->proto);
-	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(read->proto);
-	op.data.buswidth = spi_nor_get_protocol_data_nbits(read->proto);
-	op.dummy.buswidth = op.addr.buswidth;
 	op.dummy.nbytes = (read->num_mode_clocks + read->num_wait_states) *
 			  op.dummy.buswidth / 8;
 
+	spi_nor_spimem_setup_op(nor, &op, read->proto);
+
 	return spi_nor_spimem_check_op(nor, &op);
 }
 
@@ -2348,9 +2499,7 @@ static int spi_nor_spimem_check_pp(struct spi_nor *nor,
 					  SPI_MEM_OP_NO_DUMMY,
 					  SPI_MEM_OP_DATA_OUT(0, NULL, 1));
 
-	op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(pp->proto);
-	op.addr.buswidth = spi_nor_get_protocol_addr_nbits(pp->proto);
-	op.data.buswidth = spi_nor_get_protocol_data_nbits(pp->proto);
+	spi_nor_spimem_setup_op(nor, &op, pp->proto);
 
 	return spi_nor_spimem_check_op(nor, &op);
 }
@@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
 	struct spi_nor_flash_parameter *params = nor->params;
 	unsigned int cap;
 
-	/* DTR modes are not supported yet, mask them all. */
-	*hwcaps &= ~SNOR_HWCAPS_DTR;
-
 	/* X-X-X modes are not supported yet, mask them all. */
 	*hwcaps &= ~SNOR_HWCAPS_X_X_X;
 
+	/*
+	 * If the reset line is broken, we do not want to enter a stateful
+	 * mode.
+	 */
+	if (nor->flags & SNOR_F_BROKEN_RESET)
+		*hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
+
 	for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
 		int rdidx, ppidx;
 
@@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
 		 * controller directly implements the spi_nor interface.
 		 * Yet another reason to switch to spi-mem.
 		 */
-		ignored_mask = SNOR_HWCAPS_X_X_X;
+		ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
 		if (shared_mask & ignored_mask) {
 			dev_dbg(nor->dev,
 				"SPI n-n-n protocols are not supported.\n");
@@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
 					  SNOR_PROTO_1_1_8);
 	}
 
+	if (info->flags & SPI_NOR_OCTAL_DTR_READ) {
+		params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
+					  0, 20, SPINOR_OP_READ_FAST,
+					  SNOR_PROTO_8_8_8_DTR);
+	}
+
 	/* Page Program settings. */
 	params->hwcaps.mask |= SNOR_HWCAPS_PP;
 	spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
 				SPINOR_OP_PP, SNOR_PROTO_1_1_1);
 
+	/*
+	 * Since xSPI Page Program opcode is backward compatible with
+	 * Legacy SPI, use Legacy SPI opcode there as well.
+	 */
+	spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
+				SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
+
 	/*
 	 * Sector Erase settings. Sort Erase Types in ascending order, with the
 	 * smallest erase size starting at BIT(0).
@@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
 
 	spi_nor_manufacturer_init_params(nor);
 
-	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
+	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
 	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
 		spi_nor_sfdp_init_params(nor);
 
@@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
 		return err;
 	}
 
-	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
+	if (nor->addr_width == 4 &&
+	    !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&
+	    !(nor->flags & SNOR_F_4B_OPCODES)) {
 		/*
 		 * If the RESET# pin isn't hooked up properly, or the system
 		 * otherwise doesn't perform a reset command in the boot
@@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
 {
 	if (nor->addr_width) {
 		/* already configured from SFDP */
+	} else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
+		 /* Always use 4-byte addresses in DTR mode. */
+		nor->addr_width = 4;
 	} else if (nor->info->addr_width) {
 		nor->addr_width = nor->info->addr_width;
 	} else if (nor->mtd.size > 0x1000000) {
@@ -3244,14 +3417,19 @@ static int spi_nor_create_read_dirmap(struct spi_nor *nor)
 	};
 	struct spi_mem_op *op = &info.op_tmpl;
 
-	/* get transfer protocols. */
-	op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
-	op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
-	op->dummy.buswidth = op->addr.buswidth;
-	op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
+	spi_nor_spimem_setup_op(nor, op, nor->read_proto);
 
 	/* convert the dummy cycles to the number of bytes */
 	op->dummy.nbytes = (nor->read_dummy * op->dummy.buswidth) / 8;
+	if (spi_nor_protocol_is_dtr(nor->read_proto))
+		op->dummy.nbytes *= 2;
+
+	/*
+	 * Since spi_nor_spimem_setup_op() only sets buswidth when the number
+	 * of data bytes is non-zero, the data buswidth won't be set here. So,
+	 * do it explicitly.
+	 */
+	op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
 
 	nor->dirmap.rdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem,
 						       &info);
@@ -3270,15 +3448,18 @@ static int spi_nor_create_write_dirmap(struct spi_nor *nor)
 	};
 	struct spi_mem_op *op = &info.op_tmpl;
 
-	/* get transfer protocols. */
-	op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
-	op->addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
-	op->dummy.buswidth = op->addr.buswidth;
-	op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
-
 	if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
 		op->addr.nbytes = 0;
 
+	spi_nor_spimem_setup_op(nor, op, nor->write_proto);
+
+	/*
+	 * Since spi_nor_spimem_setup_op() only sets buswidth when the number
+	 * of data bytes is non-zero, the data buswidth won't be set here. So,
+	 * do it explicitly.
+	 */
+	op->data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
+
 	nor->dirmap.wdesc = devm_spi_mem_dirmap_create(nor->dev, nor->spimem,
 						       &info);
 	return PTR_ERR_OR_ZERO(nor->dirmap.wdesc);
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 6f2f6b27173f..de1e3917889f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -62,6 +62,7 @@ enum spi_nor_read_command_index {
 	SNOR_CMD_READ_1_8_8,
 	SNOR_CMD_READ_8_8_8,
 	SNOR_CMD_READ_1_8_8_DTR,
+	SNOR_CMD_READ_8_8_8_DTR,
 
 	SNOR_CMD_READ_MAX
 };
@@ -78,6 +79,7 @@ enum spi_nor_pp_command_index {
 	SNOR_CMD_PP_1_1_8,
 	SNOR_CMD_PP_1_8_8,
 	SNOR_CMD_PP_8_8_8,
+	SNOR_CMD_PP_8_8_8_DTR,
 
 	SNOR_CMD_PP_MAX
 };
@@ -311,6 +313,7 @@ struct flash_info {
 					 * BP3 is bit 6 of status register.
 					 * Must be used with SPI_NOR_4BIT_BP.
 					 */
+#define SPI_NOR_OCTAL_DTR_READ	BIT(19) /* Flash supports octal DTR Read. */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
@@ -399,6 +402,9 @@ extern const struct spi_nor_manufacturer spi_nor_winbond;
 extern const struct spi_nor_manufacturer spi_nor_xilinx;
 extern const struct spi_nor_manufacturer spi_nor_xmc;
 
+void spi_nor_spimem_setup_op(const struct spi_nor *nor,
+			     struct spi_mem_op *op,
+			     const enum spi_nor_protocol proto);
 int spi_nor_write_enable(struct spi_nor *nor);
 int spi_nor_write_disable(struct spi_nor *nor);
 int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 55c0c508464b..cb6e93a3560a 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -1046,9 +1046,16 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
 	}
 
 	/* 4BAIT is the only SFDP table that indicates page program support. */
-	if (pp_hwcaps & SNOR_HWCAPS_PP)
+	if (pp_hwcaps & SNOR_HWCAPS_PP) {
 		spi_nor_set_pp_settings(&params_pp[SNOR_CMD_PP],
 					SPINOR_OP_PP_4B, SNOR_PROTO_1_1_1);
+		/*
+		 * Since xSPI Page Program opcode is backward compatible with
+		 * Legacy SPI, use Legacy SPI opcode there as well.
+		 */
+		spi_nor_set_pp_settings(&params_pp[SNOR_CMD_PP_8_8_8_DTR],
+					SPINOR_OP_PP_4B, SNOR_PROTO_8_8_8_DTR);
+	}
 	if (pp_hwcaps & SNOR_HWCAPS_PP_1_1_4)
 		spi_nor_set_pp_settings(&params_pp[SNOR_CMD_PP_1_1_4],
 					SPINOR_OP_PP_1_1_4_4B,
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 60bac2c0ec45..cd549042c53d 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -182,6 +182,7 @@ enum spi_nor_protocol {
 	SNOR_PROTO_1_2_2_DTR = SNOR_PROTO_DTR(1, 2, 2),
 	SNOR_PROTO_1_4_4_DTR = SNOR_PROTO_DTR(1, 4, 4),
 	SNOR_PROTO_1_8_8_DTR = SNOR_PROTO_DTR(1, 8, 8),
+	SNOR_PROTO_8_8_8_DTR = SNOR_PROTO_DTR(8, 8, 8),
 };
 
 static inline bool spi_nor_protocol_is_dtr(enum spi_nor_protocol proto)
@@ -228,7 +229,7 @@ struct spi_nor_hwcaps {
  * then Quad SPI protocols before Dual SPI protocols, Fast Read and lastly
  * (Slow) Read.
  */
-#define SNOR_HWCAPS_READ_MASK		GENMASK(14, 0)
+#define SNOR_HWCAPS_READ_MASK		GENMASK(15, 0)
 #define SNOR_HWCAPS_READ		BIT(0)
 #define SNOR_HWCAPS_READ_FAST		BIT(1)
 #define SNOR_HWCAPS_READ_1_1_1_DTR	BIT(2)
@@ -245,11 +246,12 @@ struct spi_nor_hwcaps {
 #define SNOR_HWCAPS_READ_4_4_4		BIT(9)
 #define SNOR_HWCAPS_READ_1_4_4_DTR	BIT(10)
 
-#define SNOR_HWCAPS_READ_OCTAL		GENMASK(14, 11)
+#define SNOR_HWCAPS_READ_OCTAL		GENMASK(15, 11)
 #define SNOR_HWCAPS_READ_1_1_8		BIT(11)
 #define SNOR_HWCAPS_READ_1_8_8		BIT(12)
 #define SNOR_HWCAPS_READ_8_8_8		BIT(13)
 #define SNOR_HWCAPS_READ_1_8_8_DTR	BIT(14)
+#define SNOR_HWCAPS_READ_8_8_8_DTR	BIT(15)
 
 /*
  * Page Program capabilities.
@@ -260,18 +262,19 @@ struct spi_nor_hwcaps {
  * JEDEC/SFDP standard to define them. Also at this moment no SPI flash memory
  * implements such commands.
  */
-#define SNOR_HWCAPS_PP_MASK	GENMASK(22, 16)
-#define SNOR_HWCAPS_PP		BIT(16)
+#define SNOR_HWCAPS_PP_MASK		GENMASK(23, 16)
+#define SNOR_HWCAPS_PP			BIT(16)
 
-#define SNOR_HWCAPS_PP_QUAD	GENMASK(19, 17)
-#define SNOR_HWCAPS_PP_1_1_4	BIT(17)
-#define SNOR_HWCAPS_PP_1_4_4	BIT(18)
-#define SNOR_HWCAPS_PP_4_4_4	BIT(19)
+#define SNOR_HWCAPS_PP_QUAD		GENMASK(19, 17)
+#define SNOR_HWCAPS_PP_1_1_4		BIT(17)
+#define SNOR_HWCAPS_PP_1_4_4		BIT(18)
+#define SNOR_HWCAPS_PP_4_4_4		BIT(19)
 
-#define SNOR_HWCAPS_PP_OCTAL	GENMASK(22, 20)
-#define SNOR_HWCAPS_PP_1_1_8	BIT(20)
-#define SNOR_HWCAPS_PP_1_8_8	BIT(21)
-#define SNOR_HWCAPS_PP_8_8_8	BIT(22)
+#define SNOR_HWCAPS_PP_OCTAL		GENMASK(23, 20)
+#define SNOR_HWCAPS_PP_1_1_8		BIT(20)
+#define SNOR_HWCAPS_PP_1_8_8		BIT(21)
+#define SNOR_HWCAPS_PP_8_8_8		BIT(22)
+#define SNOR_HWCAPS_PP_8_8_8_DTR	BIT(23)
 
 #define SNOR_HWCAPS_X_X_X	(SNOR_HWCAPS_READ_2_2_2 |	\
 				 SNOR_HWCAPS_READ_4_4_4 |	\
@@ -279,10 +282,14 @@ struct spi_nor_hwcaps {
 				 SNOR_HWCAPS_PP_4_4_4 |		\
 				 SNOR_HWCAPS_PP_8_8_8)
 
+#define SNOR_HWCAPS_X_X_X_DTR	(SNOR_HWCAPS_READ_8_8_8_DTR |	\
+				 SNOR_HWCAPS_PP_8_8_8_DTR)
+
 #define SNOR_HWCAPS_DTR		(SNOR_HWCAPS_READ_1_1_1_DTR |	\
 				 SNOR_HWCAPS_READ_1_2_2_DTR |	\
 				 SNOR_HWCAPS_READ_1_4_4_DTR |	\
-				 SNOR_HWCAPS_READ_1_8_8_DTR)
+				 SNOR_HWCAPS_READ_1_8_8_DTR |	\
+				 SNOR_HWCAPS_READ_8_8_8_DTR)
 
 #define SNOR_HWCAPS_ALL		(SNOR_HWCAPS_READ_MASK |	\
 				 SNOR_HWCAPS_PP_MASK)
@@ -318,6 +325,22 @@ struct spi_nor_controller_ops {
 	int (*erase)(struct spi_nor *nor, loff_t offs);
 };
 
+/**
+ * enum spi_nor_cmd_ext - describes the command opcode extension in DTR mode
+ * @SPI_NOR_EXT_NONE: no extension. This is the default, and is used in Legacy
+ *		      SPI mode
+ * @SPI_NOR_EXT_REPEAT: the extension is same as the opcode
+ * @SPI_NOR_EXT_INVERT: the extension is the bitwise inverse of the opcode
+ * @SPI_NOR_EXT_HEX: the extension is any hex value. The command and opcode
+ *		     combine to form a 16-bit opcode.
+ */
+enum spi_nor_cmd_ext {
+	SPI_NOR_EXT_NONE = 0,
+	SPI_NOR_EXT_REPEAT,
+	SPI_NOR_EXT_INVERT,
+	SPI_NOR_EXT_HEX,
+};
+
 /*
  * Forward declarations that are used internally by the core and manufacturer
  * drivers.
@@ -345,6 +368,7 @@ struct spi_nor_flash_parameter;
  * @program_opcode:	the program opcode
  * @sst_write_second:	used by the SST write operation
  * @flags:		flag options for the current SPI NOR (SNOR_F_*)
+ * @cmd_ext_type:	the command opcode extension type for DTR mode.
  * @read_proto:		the SPI protocol for read operations
  * @write_proto:	the SPI protocol for write operations
  * @reg_proto:		the SPI protocol for read_reg/write_reg/erase operations
@@ -376,6 +400,7 @@ struct spi_nor {
 	enum spi_nor_protocol	reg_proto;
 	bool			sst_write_second;
 	u32			flags;
+	enum spi_nor_cmd_ext	cmd_ext_type;
 
 	const struct spi_nor_controller_ops *controller_ops;
 
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (4 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-07 17:53   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table Pratyush Yadav
                   ` (12 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Some devices in DTR mode expect an extra command byte called the
extension. The extension can either be same as the opcode, bitwise
inverse of the opcode, or another additional byte forming a 16-byte
opcode. Get the extension type from the BFPT. For now, only flashes with
"repeat" and "inverse" extensions are supported.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/sfdp.c | 17 +++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h |  6 ++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index cb6e93a3560a..3f709de5ea67 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -605,6 +605,23 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 	if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
 		return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
 						params);
+	/* 8D-8D-8D command extension. */
+	switch (bfpt.dwords[BFPT_DWORD(18)] & BFPT_DWORD18_CMD_EXT_MASK) {
+	case BFPT_DWORD18_CMD_EXT_REP:
+		nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
+		break;
+
+	case BFPT_DWORD18_CMD_EXT_INV:
+		nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
+		break;
+
+	case BFPT_DWORD18_CMD_EXT_RES:
+		return -EINVAL;
+
+	case BFPT_DWORD18_CMD_EXT_16B:
+		dev_err(nor->dev, "16-bit opcodes not supported\n");
+		return -ENOTSUPP;
+	}
 
 	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params);
 }
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index 7f9846b3a1ad..6d7243067252 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -90,6 +90,12 @@ struct sfdp_bfpt {
 #define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
 #define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
 
+#define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
+#define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
+#define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
+#define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
+#define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
+
 struct sfdp_parameter_header {
 	u8		id_lsb;
 	u8		minor;
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (5 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-08 16:01   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP Pratyush Yadav
                   ` (11 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

This table is indication that the flash is xSPI compliant and hence
supports octal DTR mode. Extract information like the fast read opcode,
dummy cycles, the number of dummy cycles needed for a Read Status
Register command, and the number of address bytes needed for a Read
Status Register command.

We don't know what speed the controller is running at. Find the fast
read dummy cycles for the fastest frequency the flash can run at to be
sure we are never short of dummy cycles. If nothing is available,
default to 20. Flashes that use a different value should update it in
their fixup hooks.

Since we want to set read settings, expose spi_nor_set_read_settings()
in core.h.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c |  2 +-
 drivers/mtd/spi-nor/core.h | 10 ++++
 drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 22a3832b83a6..7d24e63fcca8 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2355,7 +2355,7 @@ static int spi_nor_check(struct spi_nor *nor)
 	return 0;
 }
 
-static void
+void
 spi_nor_set_read_settings(struct spi_nor_read_command *read,
 			  u8 num_mode_clocks,
 			  u8 num_wait_states,
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index de1e3917889f..7e6df8322da0 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -192,6 +192,9 @@ struct spi_nor_locking_ops {
  *
  * @size:		the flash memory density in bytes.
  * @page_size:		the page size of the SPI NOR flash memory.
+ * @rdsr_dummy:		dummy cycles needed for Read Status Register command.
+ * @rdsr_addr_nbytes:	dummy address bytes needed for Read Status Register
+ *			command.
  * @hwcaps:		describes the read and page program hardware
  *			capabilities.
  * @reads:		read capabilities ordered by priority: the higher index
@@ -214,6 +217,8 @@ struct spi_nor_locking_ops {
 struct spi_nor_flash_parameter {
 	u64				size;
 	u32				page_size;
+	u8				rdsr_dummy;
+	u8				rdsr_addr_nbytes;
 
 	struct spi_nor_hwcaps		hwcaps;
 	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
@@ -424,6 +429,11 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
 
 int spi_nor_hwcaps_read2cmd(u32 hwcaps);
 u8 spi_nor_convert_3to4_read(u8 opcode);
+void spi_nor_set_read_settings(struct spi_nor_read_command *read,
+			      u8 num_mode_clocks,
+			      u8 num_wait_states,
+			      u8 opcode,
+			      enum spi_nor_protocol proto);
 void spi_nor_set_pp_settings(struct spi_nor_pp_command *pp, u8 opcode,
 			     enum spi_nor_protocol proto);
 
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 3f709de5ea67..d5a24e61813c 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -4,12 +4,15 @@
  * Copyright (C) 2014, Freescale Semiconductor, Inc.
  */
 
+#include <linux/bitfield.h>
 #include <linux/slab.h>
 #include <linux/sort.h>
 #include <linux/mtd/spi-nor.h>
 
 #include "core.h"
 
+#define ROUND_UP_TO(x, y)	(((x) + (y) - 1) / (y) * (y))
+
 #define SFDP_PARAM_HEADER_ID(p)	(((p)->id_msb << 8) | (p)->id_lsb)
 #define SFDP_PARAM_HEADER_PTP(p) \
 	(((p)->parameter_table_pointer[2] << 16) | \
@@ -19,6 +22,7 @@
 #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
 #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
 #define SFDP_4BAIT_ID		0xff84  /* 4-byte Address Instruction Table */
+#define SFDP_PROFILE1_ID	0xff05	/* xSPI Profile 1.0 table. */
 
 #define SFDP_SIGNATURE		0x50444653U
 
@@ -66,6 +70,16 @@ struct sfdp_bfpt_erase {
 	u32			shift;
 };
 
+/* xSPI Profile 1.0 table (from JESD216D.01). */
+#define PROFILE1_DWORD1_RD_FAST_CMD		GENMASK(15, 8)
+#define PROFILE1_DWORD1_RDSR_DUMMY		BIT(28)
+#define PROFILE1_DWORD1_RDSR_ADDR_BYTES		BIT(29)
+#define PROFILE1_DWORD4_DUMMY_200MHZ		GENMASK(11, 7)
+#define PROFILE1_DWORD5_DUMMY_166MHZ		GENMASK(31, 27)
+#define PROFILE1_DWORD5_DUMMY_133MHZ		GENMASK(21, 17)
+#define PROFILE1_DWORD5_DUMMY_100MHZ		GENMASK(11, 7)
+#define PROFILE1_DUMMY_DEFAULT			20
+
 #define SMPT_CMD_ADDRESS_LEN_MASK		GENMASK(23, 22)
 #define SMPT_CMD_ADDRESS_LEN_0			(0x0UL << 22)
 #define SMPT_CMD_ADDRESS_LEN_3			(0x1UL << 22)
@@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
 	return ret;
 }
 
+/**
+ * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
+ * @nor:		pointer to a 'struct spi_nor'
+ * @param_header:	pointer to the 'struct sfdp_parameter_header' describing
+ *			the 4-Byte Address Instruction Table length and version.
+ * @params:		pointer to the 'struct spi_nor_flash_parameter' to be.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_parse_profile1(struct spi_nor *nor,
+				  const struct sfdp_parameter_header *profile1_header,
+				  struct spi_nor_flash_parameter *params)
+{
+	u32 *table, opcode, addr;
+	size_t len;
+	int ret, i;
+	u8 dummy;
+
+	len = profile1_header->length * sizeof(*table);
+	table = kmalloc(len, GFP_KERNEL);
+	if (!table)
+		return -ENOMEM;
+
+	addr = SFDP_PARAM_HEADER_PTP(profile1_header);
+	ret = spi_nor_read_sfdp(nor, addr, len, table);
+	if (ret)
+		goto out;
+
+	/* Fix endianness of the table DWORDs. */
+	for (i = 0; i < profile1_header->length; i++)
+		table[i] = le32_to_cpu(table[i]);
+
+	/* Get 8D-8D-8D fast read opcode and dummy cycles. */
+	opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
+
+	/*
+	 * We don't know what speed the controller is running at. Find the
+	 * dummy cycles for the fastest frequency the flash can run at to be
+	 * sure we are never short of dummy cycles. A value of 0 means the
+	 * frequency is not supported.
+	 *
+	 * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let
+	 * flashes set the correct value if needed in their fixup hooks.
+	 */
+	dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]);
+	if (!dummy)
+		dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]);
+	if (!dummy)
+		dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]);
+	if (!dummy)
+		dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]);
+	if (!dummy)
+		dummy = PROFILE1_DUMMY_DEFAULT;
+
+	/* Round up to an even value to avoid tripping controllers up. */
+	dummy = ROUND_UP_TO(dummy, 2);
+
+	/* Update the fast read settings. */
+	spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
+				  0, dummy, opcode,
+				  SNOR_PROTO_8_8_8_DTR);
+
+	/*
+	 * Set the Read Status Register dummy cycles and dummy address bytes.
+	 */
+	if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY)
+		params->rdsr_dummy = 8;
+	else
+		params->rdsr_dummy = 4;
+
+	if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES)
+		params->rdsr_addr_nbytes = 4;
+	else
+		params->rdsr_addr_nbytes = 0;
+
+out:
+	kfree(table);
+	return ret;
+}
+
 /**
  * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
  * @nor:		pointer to a 'struct spi_nor'
@@ -1207,6 +1301,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
 			err = spi_nor_parse_4bait(nor, param_header, params);
 			break;
 
+		case SFDP_PROFILE1_ID:
+			err = spi_nor_parse_profile1(nor, param_header, params);
+			break;
+
 		default:
 			break;
 		}
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (6 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-08 16:03   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 09/17] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode Pratyush Yadav
                   ` (10 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

The xSPI Profile 1.0 table specifies how many dummy cycles and address
bytes are needed for the Read Status Register command in octal DTR mode.
Use that information to send the correct Read SR command.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 7d24e63fcca8..f2748f1d9957 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -357,6 +357,8 @@ int spi_nor_write_disable(struct spi_nor *nor)
 static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 {
 	int ret;
+	u8 addr_bytes = nor->params->rdsr_addr_nbytes;
+	u8 dummy = nor->params->rdsr_dummy;
 
 	if (nor->spimem) {
 		struct spi_mem_op op =
@@ -365,10 +367,21 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, sr, 1));
 
+		if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
+			op.addr.nbytes = addr_bytes;
+			op.addr.val = 0;
+			op.dummy.nbytes = dummy;
+		}
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
-						    sr, 1);
+		if (spi_nor_protocol_is_dtr(nor->reg_proto))
+			ret = -ENOTSUPP;
+		else
+			ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
+							    sr, 1);
 	}
 
 	if (ret)
@@ -388,6 +401,8 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
 {
 	int ret;
+	u8 addr_bytes = nor->params->rdsr_addr_nbytes;
+	u8 dummy = nor->params->rdsr_dummy;
 
 	if (nor->spimem) {
 		struct spi_mem_op op =
@@ -396,6 +411,12 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, fsr, 1));
 
+		if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
+			op.addr.nbytes = addr_bytes;
+			op.addr.val = 0;
+			op.dummy.nbytes = dummy;
+		}
+
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 09/17] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (7 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 10/17] mtd: spi-nor: core: enable octal DTR mode when possible Pratyush Yadav
                   ` (9 subsequent siblings)
  18 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Some controllers, like the cadence qspi controller, have trouble reading
only 1 byte in DTR mode. So, do 2 byte reads for SR and FSR commands in
DTR mode, and then discard the second byte.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f2748f1d9957..5f7e4a5c33bc 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -350,7 +350,7 @@ int spi_nor_write_disable(struct spi_nor *nor)
  * spi_nor_read_sr() - Read the Status Register.
  * @nor:	pointer to 'struct spi_nor'.
  * @sr:		pointer to a DMA-able buffer where the value of the
- *              Status Register will be written.
+ *              Status Register will be written. Should be at least 2 bytes.
  *
  * Return: 0 on success, -errno otherwise.
  */
@@ -371,6 +371,11 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 			op.addr.nbytes = addr_bytes;
 			op.addr.val = 0;
 			op.dummy.nbytes = dummy;
+			/*
+			 * We don't want to read only one byte in DTR mode. So,
+			 * read 2 and then discard the second byte.
+			 */
+			op.data.nbytes = 2;
 		}
 
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
@@ -394,7 +399,8 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
  * spi_nor_read_fsr() - Read the Flag Status Register.
  * @nor:	pointer to 'struct spi_nor'
  * @fsr:	pointer to a DMA-able buffer where the value of the
- *              Flag Status Register will be written.
+ *              Flag Status Register will be written. Should be at least 2
+ *              bytes.
  *
  * Return: 0 on success, -errno otherwise.
  */
@@ -415,6 +421,11 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
 			op.addr.nbytes = addr_bytes;
 			op.addr.val = 0;
 			op.dummy.nbytes = dummy;
+			/*
+			 * We don't want to read only one byte in DTR mode. So,
+			 * read 2 and then discard the second byte.
+			 */
+			op.data.nbytes = 2;
 		}
 
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 10/17] mtd: spi-nor: core: enable octal DTR mode when possible
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (8 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 09/17] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal Pratyush Yadav
                   ` (8 subsequent siblings)
  18 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Allow flashes to specify a hook to enable octal DTR mode. Use this hook
whenever possible to get optimal transfer speeds.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/core.h |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 5f7e4a5c33bc..4a1f6b343534 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3097,6 +3097,35 @@ static int spi_nor_init_params(struct spi_nor *nor)
 	return 0;
 }
 
+/** spi_nor_octal_dtr_enable() - enable Octal DTR I/O if needed
+ * @nor:                 pointer to a 'struct spi_nor'
+ * @enable:              whether to enable or disable Octal DTR
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	int ret;
+
+	if (!nor->params->octal_dtr_enable)
+		return 0;
+
+	if (!(nor->read_proto == SNOR_PROTO_8_8_8_DTR &&
+	      nor->write_proto == SNOR_PROTO_8_8_8_DTR))
+		return 0;
+
+	ret = nor->params->octal_dtr_enable(nor, enable);
+	if (ret)
+		return ret;
+
+	if (enable)
+		nor->reg_proto = SNOR_PROTO_8_8_8_DTR;
+	else
+		nor->reg_proto = SNOR_PROTO_1_1_1;
+
+	return 0;
+}
+
 /**
  * spi_nor_quad_enable() - enable Quad I/O if needed.
  * @nor:                pointer to a 'struct spi_nor'
@@ -3136,6 +3165,12 @@ static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
 
+	err = spi_nor_octal_dtr_enable(nor, true);
+	if (err) {
+		dev_dbg(nor->dev, "octal mode not supported\n");
+		return err;
+	}
+
 	err = spi_nor_quad_enable(nor);
 	if (err) {
 		dev_dbg(nor->dev, "quad mode not supported\n");
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 7e6df8322da0..6338d32a0d77 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -203,6 +203,7 @@ struct spi_nor_locking_ops {
  *                      higher index in the array, the higher priority.
  * @erase_map:		the erase map parsed from the SFDP Sector Map Parameter
  *                      Table.
+ * @octal_dtr_enable:	enables SPI NOR octal DTR mode.
  * @quad_enable:	enables SPI NOR quad mode.
  * @set_4byte_addr_mode: puts the SPI NOR in 4 byte addressing mode.
  * @convert_addr:	converts an absolute address into something the flash
@@ -226,6 +227,7 @@ struct spi_nor_flash_parameter {
 
 	struct spi_nor_erase_map        erase_map;
 
+	int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
 	int (*quad_enable)(struct spi_nor *nor);
 	int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
 	u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (9 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 10/17] mtd: spi-nor: core: enable octal DTR mode when possible Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-13  9:33   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT Pratyush Yadav
                   ` (7 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

The Micron MT35XU512ABA flash does not support the quad enable bit. But
instead of programming the Quad Enable Require field to 000b ("Device
does not have a QE bit"), it is programmed to 111b ("Reserved").

While this is technically incorrect, it is not reason enough to abort
BFPT parsing. Instead, continue BFPT parsing and let flashes set it in
their fixup hooks.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/sfdp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index d5a24e61813c..7983ff431346 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -612,7 +612,8 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		break;
 
 	default:
-		return -EINVAL;
+		dev_dbg(nor->dev, "BFPT QER reserved value used\n");
+		break;
 	}
 
 	/* Stop here if not JESD216 rev C or later. */
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (10 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-08 16:08   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown Pratyush Yadav
                   ` (6 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

A Soft Reset sequence will return the flash to Power-on-Reset (POR)
state. It consists of two commands: Soft Reset Enable and Soft Reset.
Find out if the sequence is supported from BFPT DWORD 16.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.h | 1 +
 drivers/mtd/spi-nor/sfdp.c | 4 ++++
 drivers/mtd/spi-nor/sfdp.h | 2 ++
 3 files changed, 7 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 6338d32a0d77..79ce952c0539 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -26,6 +26,7 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
 	SNOR_F_HAS_4BIT_BP      = BIT(12),
 	SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
+	SNOR_F_SOFT_RESET	= BIT(14),
 };
 
 struct spi_nor_read_command {
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 7983ff431346..8e0741d8bfd3 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -616,6 +616,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		break;
 	}
 
+	/* Soft Reset support. */
+	if (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_SOFT_RST)
+		nor->flags |= SNOR_F_SOFT_RESET;
+
 	/* Stop here if not JESD216 rev C or later. */
 	if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
 		return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index 6d7243067252..8ae55e98084e 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -90,6 +90,8 @@ struct sfdp_bfpt {
 #define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
 #define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
 
+#define BFPT_DWORD16_SOFT_RST			BIT(12)
+
 #define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
 #define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
 #define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (11 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-08 16:10   ` Tudor.Ambarus
  2020-06-23 18:30 ` [PATCH v10 14/17] mtd: spi-nor: core: disable Octal DTR mode on suspend Pratyush Yadav
                   ` (5 subsequent siblings)
  18 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Perform a Soft Reset on shutdown on flashes that support it so that the
flash can be reset to its initial state and any configurations made by
spi-nor (given that they're only done in volatile registers) will be
reset. This will hand back the flash in pristine state for any further
operations on it.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c  | 42 +++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h |  2 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 4a1f6b343534..27ad9bab06dc 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -40,6 +40,9 @@
 
 #define SPI_NOR_MAX_ADDR_WIDTH	4
 
+#define SPI_NOR_SRST_SLEEP_MIN 200
+#define SPI_NOR_SRST_SLEEP_MAX 400
+
 /**
  * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
  *			   extension type.
@@ -3201,6 +3204,41 @@ static int spi_nor_init(struct spi_nor *nor)
 	return 0;
 }
 
+static void spi_nor_soft_reset(struct spi_nor *nor)
+{
+	struct spi_mem_op op;
+	int ret;
+
+	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 8),
+			SPI_MEM_OP_NO_DUMMY,
+			SPI_MEM_OP_NO_ADDR,
+			SPI_MEM_OP_NO_DATA);
+	spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret) {
+		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+		return;
+	}
+
+	op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 8),
+			SPI_MEM_OP_NO_DUMMY,
+			SPI_MEM_OP_NO_ADDR,
+			SPI_MEM_OP_NO_DATA);
+	spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret) {
+		dev_warn(nor->dev, "Software reset failed: %d\n", ret);
+		return;
+	}
+
+	/*
+	 * Software Reset is not instant, and the delay varies from flash to
+	 * flash. Looking at a few flashes, most range somewhere below 100
+	 * microseconds. So, sleep for a range of 200-400 us.
+	 */
+	usleep_range(SPI_NOR_SRST_SLEEP_MIN, SPI_NOR_SRST_SLEEP_MAX);
+}
+
 /* mtd resume handler */
 static void spi_nor_resume(struct mtd_info *mtd)
 {
@@ -3220,6 +3258,10 @@ void spi_nor_restore(struct spi_nor *nor)
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
 	    nor->flags & SNOR_F_BROKEN_RESET)
 		nor->params->set_4byte_addr_mode(nor, false);
+
+	if (nor->info->flags & SPI_NOR_OCTAL_DTR_READ &&
+	    nor->flags & SNOR_F_SOFT_RESET)
+		spi_nor_soft_reset(nor);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index cd549042c53d..299685d15dc2 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -51,6 +51,8 @@
 #define SPINOR_OP_CLFSR		0x50	/* Clear flag status register */
 #define SPINOR_OP_RDEAR		0xc8	/* Read Extended Address Register */
 #define SPINOR_OP_WREAR		0xc5	/* Write Extended Address Register */
+#define SPINOR_OP_SRSTEN	0x66	/* Software Reset Enable */
+#define SPINOR_OP_SRST		0x99	/* Software Reset */
 
 /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
 #define SPINOR_OP_READ_4B	0x13	/* Read data bytes (low frequency) */
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 14/17] mtd: spi-nor: core: disable Octal DTR mode on suspend.
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (12 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 15/17] mtd: spi-nor: core: expose spi_nor_default_setup() in core.h Pratyush Yadav
                   ` (4 subsequent siblings)
  18 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

On resume, the init procedure will be run that will re-enable it.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 27ad9bab06dc..369d754988f0 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3239,6 +3239,23 @@ static void spi_nor_soft_reset(struct spi_nor *nor)
 	usleep_range(SPI_NOR_SRST_SLEEP_MIN, SPI_NOR_SRST_SLEEP_MAX);
 }
 
+/* mtd suspend handler */
+static int spi_nor_suspend(struct mtd_info *mtd)
+{
+	struct spi_nor *nor = mtd_to_spi_nor(mtd);
+	struct device *dev = nor->dev;
+	int ret;
+
+	/* Disable octal DTR mode if we enabled it. */
+	ret = spi_nor_octal_dtr_enable(nor, false);
+	if (ret) {
+		dev_err(dev, "suspend() failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 /* mtd resume handler */
 static void spi_nor_resume(struct mtd_info *mtd)
 {
@@ -3432,6 +3449,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	mtd->size = nor->params->size;
 	mtd->_erase = spi_nor_erase;
 	mtd->_read = spi_nor_read;
+	mtd->_suspend = spi_nor_suspend;
 	mtd->_resume = spi_nor_resume;
 
 	if (nor->params->locking_ops) {
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 15/17] mtd: spi-nor: core: expose spi_nor_default_setup() in core.h
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (13 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 14/17] mtd: spi-nor: core: disable Octal DTR mode on suspend Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 16/17] mtd: spi-nor: spansion: add support for Cypress Semper flash Pratyush Yadav
                   ` (3 subsequent siblings)
  18 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Flashes might want to add a custom setup hook to configure the flash in
the proper mode for operation. But after that, they would still want to
run the default setup hook because it selects the read, program, and
erase operations. Since there is little point in repeating all that
code, expose the spi_nor_default_setup() in core.h to
manufacturer-specific files.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/core.c | 4 ++--
 drivers/mtd/spi-nor/core.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 369d754988f0..0bc897274278 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2790,8 +2790,8 @@ static int spi_nor_select_erase(struct spi_nor *nor)
 	return 0;
 }
 
-static int spi_nor_default_setup(struct spi_nor *nor,
-				 const struct spi_nor_hwcaps *hwcaps)
+int spi_nor_default_setup(struct spi_nor *nor,
+			  const struct spi_nor_hwcaps *hwcaps)
 {
 	struct spi_nor_flash_parameter *params = nor->params;
 	u32 ignored_mask, shared_mask;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 79ce952c0539..d37a9b1d111f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -452,6 +452,9 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
 			     const struct sfdp_bfpt *bfpt,
 			     struct spi_nor_flash_parameter *params);
 
+int spi_nor_default_setup(struct spi_nor *nor,
+			  const struct spi_nor_hwcaps *hwcaps);
+
 static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
 {
 	return mtd->priv;
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 16/17] mtd: spi-nor: spansion: add support for Cypress Semper flash
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (14 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 15/17] mtd: spi-nor: core: expose spi_nor_default_setup() in core.h Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-06-23 18:30 ` [PATCH v10 17/17] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode Pratyush Yadav
                   ` (2 subsequent siblings)
  18 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

The Cypress Semper flash is an xSPI compliant octal DTR flash. Add
support for using it in octal DTR mode.

The flash by default boots in a hybrid sector mode. But the sector map
table on the part I had was programmed incorrectly and the SMPT values
on the flash don't match the public datasheet. Specifically, in some
places erase type 3 was used instead of 4. In addition, the region sizes
were incorrect in some places. So, for testing I set CFR3N[3] to enable
uniform sector sizes. Since the uniform sector mode bit is a
non-volatile bit, this series does not change it to avoid making any
permanent changes to the flash configuration. The correct data to
implement a fixup is not available right now and will be done in a
follow-up patch if needed.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/spansion.c | 166 +++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index e550cd5c9d3a..393cf4662110 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -8,6 +8,168 @@
 
 #include "core.h"
 
+#define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
+#define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
+#define SPINOR_REG_CYPRESS_CFR2V		0x00800003
+#define SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24	0xb
+#define SPINOR_REG_CYPRESS_CFR3V		0x00800004
+#define SPINOR_REG_CYPRESS_CFR3V_PGSZ		BIT(4) /* Page size. */
+#define SPINOR_REG_CYPRESS_CFR5V		0x00800006
+#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN	0x3
+#define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
+#define SPINOR_OP_CYPRESS_RD_FAST		0xee
+
+/**
+ * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
+ * @nor:		pointer to a 'struct spi_nor'
+ *
+ * This also sets the memory access latency cycles to 24 to allow the flash to
+ * run at up to 200MHz.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_cypress_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf;
+	u8 addr_width;
+	int ret;
+
+	if (enable)
+		addr_width = 3;
+	else
+		addr_width = 4;
+
+	if (enable) {
+		/* Use 24 dummy cycles for memory array reads. */
+		ret = spi_nor_write_enable(nor);
+		if (ret)
+			return ret;
+
+		*buf = SPINOR_REG_CYPRESS_CFR2V_MEMLAT_11_24;
+		op = (struct spi_mem_op)
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
+				   SPI_MEM_OP_ADDR(addr_width,
+						   SPINOR_REG_CYPRESS_CFR2V,
+						   1),
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
+		ret = spi_mem_exec_op(nor->spimem, &op);
+		if (ret) {
+			dev_warn(nor->dev,
+				 "failed to set default memory latency value: %d\n",
+				 ret);
+			return ret;
+		}
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			return ret;
+
+		nor->read_dummy = 24;
+	}
+
+	/* Set/unset the octal and DTR enable bits. */
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (enable)
+		*buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN;
+	else
+		*buf = SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS;
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WR_ANY_REG, 1),
+			   SPI_MEM_OP_ADDR(addr_width,
+					   SPINOR_REG_CYPRESS_CFR5V,
+					   1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_OUT(1, buf, 1));
+
+	if (!enable)
+		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret) {
+		dev_warn(nor->dev, "Failed to enable octal DTR mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void s28hs512t_default_init(struct spi_nor *nor)
+{
+	nor->params->octal_dtr_enable = spi_nor_cypress_octal_dtr_enable;
+}
+
+static void s28hs512t_post_sfdp_fixup(struct spi_nor *nor)
+{
+	/*
+	 * On older versions of the flash the xSPI Profile 1.0 table has the
+	 * 8D-8D-8D Fast Read opcode as 0x00. But it actually should be 0xEE.
+	 */
+	if (nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].opcode == 0)
+		nor->params->reads[SNOR_CMD_READ_8_8_8_DTR].opcode =
+			SPINOR_OP_CYPRESS_RD_FAST;
+
+	nor->params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
+
+	/* This flash is also missing the 4-byte Page Program opcode bit. */
+	spi_nor_set_pp_settings(&nor->params->page_programs[SNOR_CMD_PP],
+				SPINOR_OP_PP_4B, SNOR_PROTO_1_1_1);
+	/*
+	 * Since xSPI Page Program opcode is backward compatible with
+	 * Legacy SPI, use Legacy SPI opcode there as well.
+	 */
+	spi_nor_set_pp_settings(&nor->params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
+				SPINOR_OP_PP_4B, SNOR_PROTO_8_8_8_DTR);
+
+	/*
+	 * The xSPI Profile 1.0 table advertises the number of additional
+	 * address bytes needed for Read Status Register command as 0 but the
+	 * actual value for that is 4.
+	 */
+	nor->params->rdsr_addr_nbytes = 4;
+}
+
+static int s28hs512t_post_bfpt_fixup(struct spi_nor *nor,
+				     const struct sfdp_parameter_header *bfpt_header,
+				     const struct sfdp_bfpt *bfpt,
+				     struct spi_nor_flash_parameter *params)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf;
+	u8 addr_width = 3;
+	int ret;
+
+	/*
+	 * The BFPT table advertises a 512B page size but the page size is
+	 * actually configurable (with the default being 256B). Read from
+	 * CFR3V[4] and set the correct size.
+	 */
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RD_ANY_REG, 1),
+			   SPI_MEM_OP_ADDR(addr_width, SPINOR_REG_CYPRESS_CFR3V, 1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_IN(1, buf, 1));
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret)
+		return ret;
+
+	if (*buf & SPINOR_REG_CYPRESS_CFR3V_PGSZ)
+		params->page_size = 512;
+	else
+		params->page_size = 256;
+
+	return 0;
+}
+
+static struct spi_nor_fixups s28hs512t_fixups = {
+	.default_init = s28hs512t_default_init,
+	.post_sfdp = s28hs512t_post_sfdp_fixup,
+	.post_bfpt = s28hs512t_post_bfpt_fixup,
+};
+
 static int
 s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
 			 const struct sfdp_parameter_header *bfpt_header,
@@ -104,6 +266,10 @@ static const struct flash_info spansion_parts[] = {
 			     SPI_NOR_4B_OPCODES) },
 	{ "cy15x104q",  INFO6(0x042cc2, 0x7f7f7f, 512 * 1024, 1,
 			      SPI_NOR_NO_ERASE) },
+	{ "s28hs512t",   INFO(0x345b1a,      0, 256 * 1024, 256,
+			     SECT_4K | SPI_NOR_OCTAL_DTR_READ)
+		.fixups = &s28hs512t_fixups,
+	},
 };
 
 static void spansion_post_sfdp_fixups(struct spi_nor *nor)
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* [PATCH v10 17/17] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (15 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 16/17] mtd: spi-nor: spansion: add support for Cypress Semper flash Pratyush Yadav
@ 2020-06-23 18:30 ` Pratyush Yadav
  2020-07-13  6:34 ` [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Tudor.Ambarus
  2020-07-14 16:40 ` Mark Brown
  18 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-06-23 18:30 UTC (permalink / raw)
  To: Tudor Ambarus, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Mark Brown, Nicolas Ferre,
	Alexandre Belloni, Ludovic Desroches, Matthias Brugger,
	Michal Simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: Boris Brezillon, Sekhar Nori, Pratyush Yadav

Since this flash doesn't have a Profile 1.0 table, the Octal DTR
capabilities are enabled in the post SFDP fixup, along with the 8D-8D-8D
fast read settings.

Enable Octal DTR mode with 20 dummy cycles to allow running at the
maximum supported frequency of 200Mhz.

The flash supports the soft reset sequence. So, add the flag in the
flash's info.

Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
---
 drivers/mtd/spi-nor/micron-st.c | 103 +++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 3dca5b9af3b6..447b0166de2e 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -8,10 +8,111 @@
 
 #include "core.h"
 
+#define SPINOR_OP_MT_DTR_RD	0xfd	/* Fast Read opcode in DTR mode */
+#define SPINOR_OP_MT_RD_ANY_REG	0x85	/* Read volatile register */
+#define SPINOR_OP_MT_WR_ANY_REG	0x81	/* Write volatile register */
+#define SPINOR_REG_MT_CFR0V	0x00	/* For setting octal DTR mode */
+#define SPINOR_REG_MT_CFR1V	0x01	/* For setting dummy cycles */
+#define SPINOR_MT_DTR_NO_DQS	0xc7	/* Enable Octal DTR without DQS. */
+#define SPINOR_MT_EXSPI		0xff	/* Enable Extended SPI (default) */
+
+static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
+{
+	struct spi_mem_op op;
+	u8 *buf = nor->bouncebuf;
+	u8 addr_width;
+	int ret;
+
+	if (enable)
+		addr_width = 3;
+	else
+		addr_width = 4;
+
+	if (enable) {
+		/* Use 20 dummy cycles for memory array reads. */
+		ret = spi_nor_write_enable(nor);
+		if (ret)
+			return ret;
+
+		*buf = 20;
+		op = (struct spi_mem_op)
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
+				   SPI_MEM_OP_ADDR(addr_width,
+						   SPINOR_REG_MT_CFR1V, 1),
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_OUT(1, buf, 1));
+		ret = spi_mem_exec_op(nor->spimem, &op);
+		if (ret)
+			return ret;
+
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			return ret;
+	}
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	if (enable)
+		*buf = SPINOR_MT_DTR_NO_DQS;
+	else
+		*buf = SPINOR_MT_EXSPI;
+	op = (struct spi_mem_op)
+		SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_MT_WR_ANY_REG, 1),
+			   SPI_MEM_OP_ADDR(addr_width, SPINOR_REG_MT_CFR0V, 1),
+			   SPI_MEM_OP_NO_DUMMY,
+			   SPI_MEM_OP_DATA_OUT(1, buf, 1));
+
+	if (!enable)
+		spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
+
+	ret = spi_mem_exec_op(nor->spimem, &op);
+	if (ret) {
+		dev_err(nor->dev, "Failed to enable octal DTR mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void mt35xu512aba_default_init(struct spi_nor *nor)
+{
+	nor->params->octal_dtr_enable = spi_nor_micron_octal_dtr_enable;
+}
+
+static void mt35xu512aba_post_sfdp_fixup(struct spi_nor *nor)
+{
+	/* Set the Fast Read settings. */
+	nor->params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
+	spi_nor_set_read_settings(&nor->params->reads[SNOR_CMD_READ_8_8_8_DTR],
+				  0, 20, SPINOR_OP_MT_DTR_RD,
+				  SNOR_PROTO_8_8_8_DTR);
+
+	nor->params->hwcaps.mask |= SNOR_HWCAPS_PP_8_8_8_DTR;
+
+	nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
+	nor->params->rdsr_dummy = 8;
+	nor->params->rdsr_addr_nbytes = 0;
+
+	/*
+	 * The BFPT quad enable field is set to a reserved value so the quad
+	 * enable function is ignored by spi_nor_parse_bfpt(). Make sure we
+	 * disable it.
+	 */
+	nor->params->quad_enable = NULL;
+}
+
+static struct spi_nor_fixups mt35xu512aba_fixups = {
+	.default_init = mt35xu512aba_default_init,
+	.post_sfdp = mt35xu512aba_post_sfdp_fixup,
+};
+
 static const struct flash_info micron_parts[] = {
 	{ "mt35xu512aba", INFO(0x2c5b1a, 0, 128 * 1024, 512,
 			       SECT_4K | USE_FSR | SPI_NOR_OCTAL_READ |
-			       SPI_NOR_4B_OPCODES) },
+			       SPI_NOR_4B_OPCODES | SPI_NOR_OCTAL_DTR_READ)
+		.fixups = &mt35xu512aba_fixups},
 	{ "mt35xu02g", INFO(0x2c5b1c, 0, 128 * 1024, 2048,
 			    SECT_4K | USE_FSR | SPI_NOR_OCTAL_READ |
 			    SPI_NOR_4B_OPCODES) },
-- 
2.27.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-06-23 18:30 ` [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol Pratyush Yadav
@ 2020-07-07 17:37   ` Tudor.Ambarus
  2020-07-21 11:29     ` Pratyush Yadav
  0 siblings, 1 reply; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-07 17:37 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

Hi, Pratyush,

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Double Transfer Rate (DTR) is SPI protocol in which data is transferred
> on each clock edge as opposed to on each clock cycle. Make
> framework-level changes to allow supporting flashes in DTR mode.
> 
> Right now, mixed DTR modes are not supported. So, for example a mode
> like 4S-4D-4D will not work. All phases need to be either DTR or STR.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 305 ++++++++++++++++++++++++++++-------- 
>  drivers/mtd/spi-nor/core.h  |   6 +
>  drivers/mtd/spi-nor/sfdp.c  |   9 +-
>  include/linux/mtd/spi-nor.h |  51 ++++--
>  4 files changed, 295 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0369d98b2d12..22a3832b83a6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -40,6 +40,76 @@
> 
>  #define SPI_NOR_MAX_ADDR_WIDTH 4
> 
> +/**
> + * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
> + *                        extension type.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @op:                        pointer to the 'struct spi_mem_op' whose properties
> + *                     need to be initialized.
> + *
> + * Right now, only "repeat" and "invert" are supported.
> + *
> + * Return: The opcode extension.
> + */
> +static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor,
> +                             const struct spi_mem_op *op)
> +{
> +       switch (nor->cmd_ext_type) {
> +       case SPI_NOR_EXT_INVERT:
> +               return ~op->cmd.opcode;
> +
> +       case SPI_NOR_EXT_REPEAT:
> +               return op->cmd.opcode;
> +
> +       default:
> +               dev_err(nor->dev, "Unknown command extension type\n");
> +               return 0;
> +       }
> +}
> +
> +/**
> + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op.
> + * @nor:               pointer to a 'struct spi_nor'
> + * @op:                        pointer to the 'struct spi_mem_op' whose properties
> + *                     need to be initialized.
> + * @proto:             the protocol from which the properties need to be set.
> + */
> +void spi_nor_spimem_setup_op(const struct spi_nor *nor,
> +                            struct spi_mem_op *op,
> +                            const enum spi_nor_protocol proto)

There's not much to set for the REG operations.

> +{
> +       u8 ext;
> +
> +       op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
> +
> +       if (op->addr.nbytes)
> +               op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +
> +       if (op->dummy.nbytes)
> +               op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> +
> +       if (op->data.nbytes)
> +               op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);

How about getting rid of the above and

> +
> +       if (spi_nor_protocol_is_dtr(proto)) {

introduce a spi_nor_spimem_setup_dtr_op() just for the body of this if?

> +               /*
> +                * spi-mem supports mixed DTR modes, but right now we can only
> +                * have all phases either DTR or STR. IOW, spi-mem can have
nit: SPIMEM
> +                * something like 4S-4D-4D, but spi-nor can't. So, set all 4
nit: SPI NOR
> +                * phases to either DTR or STR.
> +                */
> +               op->cmd.dtr = op->addr.dtr = op->dummy.dtr
> +                              = op->data.dtr = true;
> +
> +               /* 2 bytes per clock cycle in DTR mode. */
> +               op->dummy.nbytes *= 2;
> +
> +               ext = spi_nor_get_cmd_ext(nor, op);
> +               op->cmd.opcode = (op->cmd.opcode << 8) | ext;
> +               op->cmd.nbytes = 2;
> +       }
> +}
> +
>  /**
>   * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data
>   *                           transfer
> @@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
>         ssize_t nbytes;
>         int error;
> 
> -       /* get transfer protocols. */
> -       op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
> -       op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
> -       op.dummy.buswidth = op.addr.buswidth;
> -       op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
> +       spi_nor_spimem_setup_op(nor, &op, nor->read_proto);

Here we would keep the code as it were.
> 
>         /* convert the dummy cycles to the number of bytes */
>         op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> +       if (spi_nor_protocol_is_dtr(nor->read_proto))
> +               op.dummy.nbytes *= 2;

And replace these 2 lines with:
	if (spi_nor_protocol_is_dtr(nor->read_proto))
		spi_nor_spimem_setup_dtr_op(nor, &op, nor->read_proto)
> 
>         usebouncebuf = spi_nor_spimem_bounce(nor, &op);
> 
> @@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
>         ssize_t nbytes;
>         int error;
> 
> -       op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
> -       op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
> -       op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
> -
>         if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
>                 op.addr.nbytes = 0;
> 
> +       spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +
>         if (spi_nor_spimem_bounce(nor, &op))
>                 memcpy(nor->bouncebuf, buf, op.data.nbytes);
> 
> @@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor)
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_NO_DATA);
> 
> +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);

For the reg operation we can get rid of the extra checks that were in
spi_nor_spimem_setup_op and simply do:

		if (spi_nor_protocol_is_dtr(proto))
			spi_nor_spimem_setup_dtr_op()

> +
>                 ret = spi_mem_exec_op(nor->spimem, &op);
>         } else {
> -               ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN,
> -                                                    NULL, 0);
> +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> +                       ret = -ENOTSUPP;
> +               else
> +                       ret = nor->controller_ops->write_reg(nor,
> +                                                            SPINOR_OP_WREN,
> +                                                            NULL, 0);

Would you introduce helpers for the controller ops, like Boris
did in the following patch?
https://patchwork.ozlabs.org/project/linux-mtd/patch/20181012084825.23697-10-boris.brezillon@bootlin.com/

How about spi_nor_controller_ops_read_reg()
and spi_nor_controller_ops_write_reg() instead?

cut

> @@ -1144,7 +1291,11 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_NO_DATA);
> 
> +               spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> +
>                 return spi_mem_exec_op(nor->spimem, &op);
> +       } else if (spi_nor_protocol_is_dtr(nor->write_proto)) {
> +               return -ENOTSUPP;
>         } else if (nor->controller_ops->erase) {
>                 return nor->controller_ops->erase(nor, addr);
>         }

here you would need a helper: spi_nor_controller_ops_erase()

cut

> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>         struct spi_nor_flash_parameter *params = nor->params;
>         unsigned int cap;
> 
> -       /* DTR modes are not supported yet, mask them all. */
> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
> -
>         /* X-X-X modes are not supported yet, mask them all. */
>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
> 
> +       /*
> +        * If the reset line is broken, we do not want to enter a stateful
> +        * mode.
> +        */
> +       if (nor->flags & SNOR_F_BROKEN_RESET)
> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);

A dedicated reset line is not enough for flashes that keep their state
in non-volatile bits. Since we can't protect from unexpected crashes in
the non volatile state case, we should enter these modes only with an
explicit request, i.e. an optional DT property: "update-nonvolatile-state",
or something similar.

For the volatile state case, we can parse the SFDP SCCR map, save if we
can enter stateful modes in a volatile way, and if yes allow the entering.

Do the flashes that you played with define the SFDP SCCR map?

> +
>         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
>                 int rdidx, ppidx;
> 
> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>                  * controller directly implements the spi_nor interface.
>                  * Yet another reason to switch to spi-mem.
>                  */
> -               ignored_mask = SNOR_HWCAPS_X_X_X;
> +               ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
>                 if (shared_mask & ignored_mask) {
>                         dev_dbg(nor->dev,
>                                 "SPI n-n-n protocols are not supported.\n");
> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>                                           SNOR_PROTO_1_1_8);
>         }
> 
> +       if (info->flags & SPI_NOR_OCTAL_DTR_READ) {

Why do we need this flag? Can't we determine if the flash supports
octal DTR by parsing SFDP?

> +               params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> +               spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +                                         0, 20, SPINOR_OP_READ_FAST,
> +                                         SNOR_PROTO_8_8_8_DTR);
> +       }
> +
>         /* Page Program settings. */
>         params->hwcaps.mask |= SNOR_HWCAPS_PP;
>         spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
>                                 SPINOR_OP_PP, SNOR_PROTO_1_1_1);
> 
> +       /*
> +        * Since xSPI Page Program opcode is backward compatible with
> +        * Legacy SPI, use Legacy SPI opcode there as well.
> +        */
> +       spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
> +                               SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
> +

This looks fishy. You haven't updated the hwcaps.mask, these pp settings never
get selected?

>         /*
>          * Sector Erase settings. Sort Erase Types in ascending order, with the
>          * smallest erase size starting at BIT(0).
> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
> 
>         spi_nor_manufacturer_init_params(nor);
> 
> -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> +       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +                                SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
>             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>                 spi_nor_sfdp_init_params(nor);
> 
> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
>                 return err;
>         }
> 
> -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> +       if (nor->addr_width == 4 &&
> +           !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&

Why is the Octal DTR read exempted?

> +           !(nor->flags & SNOR_F_4B_OPCODES)) {
>                 /*
>                  * If the RESET# pin isn't hooked up properly, or the system
>                  * otherwise doesn't perform a reset command in the boot
> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
>  {
>         if (nor->addr_width) {
>                 /* already configured from SFDP */
> +       } else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
> +                /* Always use 4-byte addresses in DTR mode. */
> +               nor->addr_width = 4;

Why? DTR with 3 byte addr width should be possible too.

>         } else if (nor->info->addr_width) {
>                 nor->addr_width = nor->info->addr_width;
>         } else if (nor->mtd.size > 0x1000000) {

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT
  2020-06-23 18:30 ` [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT Pratyush Yadav
@ 2020-07-07 17:53   ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-07 17:53 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Some devices in DTR mode expect an extra command byte called the
> extension. The extension can either be same as the opcode, bitwise
> inverse of the opcode, or another additional byte forming a 16-byte
> opcode. Get the extension type from the BFPT. For now, only flashes with
> "repeat" and "inverse" extensions are supported.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 17 +++++++++++++++++
>  drivers/mtd/spi-nor/sfdp.h |  6 ++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index cb6e93a3560a..3f709de5ea67 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -605,6 +605,23 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>         if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
>                 return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
>                                                 params);
> +       /* 8D-8D-8D command extension. */
> +       switch (bfpt.dwords[BFPT_DWORD(18)] & BFPT_DWORD18_CMD_EXT_MASK) {
> +       case BFPT_DWORD18_CMD_EXT_REP:
> +               nor->cmd_ext_type = SPI_NOR_EXT_REPEAT;
> +               break;
> +
> +       case BFPT_DWORD18_CMD_EXT_INV:
> +               nor->cmd_ext_type = SPI_NOR_EXT_INVERT;
> +               break;
> +
> +       case BFPT_DWORD18_CMD_EXT_RES:

dev_dbg

> +               return -EINVAL;

Do we really want to stop the SFDP parsing here and loose everything that
we gathered?


> +
> +       case BFPT_DWORD18_CMD_EXT_16B:
> +               dev_err(nor->dev, "16-bit opcodes not supported\n");

dev_dbg

> +               return -ENOTSUPP;
> +       }
> 
>         return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params);
>  }
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index 7f9846b3a1ad..6d7243067252 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -90,6 +90,12 @@ struct sfdp_bfpt {
>  #define BFPT_DWORD15_QER_SR2_BIT1_NO_RD                (0x4UL << 20)
>  #define BFPT_DWORD15_QER_SR2_BIT1              (0x5UL << 20) /* Spansion */
> 
> +#define BFPT_DWORD18_CMD_EXT_MASK              GENMASK(30, 29)
> +#define BFPT_DWORD18_CMD_EXT_REP               (0x0UL << 29) /* Repeat */
> +#define BFPT_DWORD18_CMD_EXT_INV               (0x1UL << 29) /* Invert */
> +#define BFPT_DWORD18_CMD_EXT_RES               (0x2UL << 29) /* Reserved */
> +#define BFPT_DWORD18_CMD_EXT_16B               (0x3UL << 29) /* 16-bit opcode */
> +
>  struct sfdp_parameter_header {
>         u8              id_lsb;
>         u8              minor;
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table
  2020-06-23 18:30 ` [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table Pratyush Yadav
@ 2020-07-08 16:01   ` Tudor.Ambarus
  2020-07-20 16:38     ` Pratyush Yadav
  0 siblings, 1 reply; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-08 16:01 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This table is indication that the flash is xSPI compliant and hence
> supports octal DTR mode. Extract information like the fast read opcode,
> dummy cycles, the number of dummy cycles needed for a Read Status
> Register command, and the number of address bytes needed for a Read
> Status Register command.
> 
> We don't know what speed the controller is running at. Find the fast
> read dummy cycles for the fastest frequency the flash can run at to be
> sure we are never short of dummy cycles. If nothing is available,
> default to 20. Flashes that use a different value should update it in
> their fixup hooks.
> 
> Since we want to set read settings, expose spi_nor_set_read_settings()
> in core.h.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.c |  2 +-
>  drivers/mtd/spi-nor/core.h | 10 ++++
>  drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 22a3832b83a6..7d24e63fcca8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2355,7 +2355,7 @@ static int spi_nor_check(struct spi_nor *nor)
>         return 0;
>  }
> 
> -static void
> +void
>  spi_nor_set_read_settings(struct spi_nor_read_command *read,
>                           u8 num_mode_clocks,
>                           u8 num_wait_states,
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index de1e3917889f..7e6df8322da0 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -192,6 +192,9 @@ struct spi_nor_locking_ops {
>   *
>   * @size:              the flash memory density in bytes.
>   * @page_size:         the page size of the SPI NOR flash memory.
> + * @rdsr_dummy:                dummy cycles needed for Read Status Register command.
> + * @rdsr_addr_nbytes:  dummy address bytes needed for Read Status Register
> + *                     command.
>   * @hwcaps:            describes the read and page program hardware
>   *                     capabilities.
>   * @reads:             read capabilities ordered by priority: the higher index
> @@ -214,6 +217,8 @@ struct spi_nor_locking_ops {
>  struct spi_nor_flash_parameter {
>         u64                             size;
>         u32                             page_size;
> +       u8                              rdsr_dummy;
> +       u8                              rdsr_addr_nbytes;
> 
>         struct spi_nor_hwcaps           hwcaps;
>         struct spi_nor_read_command     reads[SNOR_CMD_READ_MAX];
> @@ -424,6 +429,11 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> 
>  int spi_nor_hwcaps_read2cmd(u32 hwcaps);
>  u8 spi_nor_convert_3to4_read(u8 opcode);
> +void spi_nor_set_read_settings(struct spi_nor_read_command *read,
> +                             u8 num_mode_clocks,
> +                             u8 num_wait_states,
> +                             u8 opcode,
> +                             enum spi_nor_protocol proto);
>  void spi_nor_set_pp_settings(struct spi_nor_pp_command *pp, u8 opcode,
>                              enum spi_nor_protocol proto);
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 3f709de5ea67..d5a24e61813c 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -4,12 +4,15 @@
>   * Copyright (C) 2014, Freescale Semiconductor, Inc.
>   */
> 
> +#include <linux/bitfield.h>
>  #include <linux/slab.h>
>  #include <linux/sort.h>
>  #include <linux/mtd/spi-nor.h>
> 
>  #include "core.h"
> 
> +#define ROUND_UP_TO(x, y)      (((x) + (y) - 1) / (y) * (y))

you can use round_up()

> +
>  #define SFDP_PARAM_HEADER_ID(p)        (((p)->id_msb << 8) | (p)->id_lsb)
>  #define SFDP_PARAM_HEADER_PTP(p) \
>         (((p)->parameter_table_pointer[2] << 16) | \
> @@ -19,6 +22,7 @@
>  #define SFDP_BFPT_ID           0xff00  /* Basic Flash Parameter Table */
>  #define SFDP_SECTOR_MAP_ID     0xff81  /* Sector Map Table */
>  #define SFDP_4BAIT_ID          0xff84  /* 4-byte Address Instruction Table */
> +#define SFDP_PROFILE1_ID       0xff05  /* xSPI Profile 1.0 table. */
> 
>  #define SFDP_SIGNATURE         0x50444653U
> 
> @@ -66,6 +70,16 @@ struct sfdp_bfpt_erase {
>         u32                     shift;
>  };
> 
> +/* xSPI Profile 1.0 table (from JESD216D.01). */
> +#define PROFILE1_DWORD1_RD_FAST_CMD            GENMASK(15, 8)
> +#define PROFILE1_DWORD1_RDSR_DUMMY             BIT(28)
> +#define PROFILE1_DWORD1_RDSR_ADDR_BYTES                BIT(29)
> +#define PROFILE1_DWORD4_DUMMY_200MHZ           GENMASK(11, 7)
> +#define PROFILE1_DWORD5_DUMMY_166MHZ           GENMASK(31, 27)
> +#define PROFILE1_DWORD5_DUMMY_133MHZ           GENMASK(21, 17)
> +#define PROFILE1_DWORD5_DUMMY_100MHZ           GENMASK(11, 7)

we should order these macros in a consistent way. I see that previous macros
are declared in order starting from MSB to LSB.

> +#define PROFILE1_DUMMY_DEFAULT                 20

we need to explain why the default dummy value is 20.

How about declaring all these macros immediately above of spi_nor_parse_profile1()?

> +
>  #define SMPT_CMD_ADDRESS_LEN_MASK              GENMASK(23, 22)
>  #define SMPT_CMD_ADDRESS_LEN_0                 (0x0UL << 22)
>  #define SMPT_CMD_ADDRESS_LEN_3                 (0x1UL << 22)
> @@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>         return ret;
>  }
> 
> +/**
> + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
> + * @nor:               pointer to a 'struct spi_nor'
> + * @param_header:      pointer to the 'struct sfdp_parameter_header' describing
> + *                     the 4-Byte Address Instruction Table length and version.
> + * @params:            pointer to the 'struct spi_nor_flash_parameter' to be.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_parse_profile1(struct spi_nor *nor,
> +                                 const struct sfdp_parameter_header *profile1_header,
> +                                 struct spi_nor_flash_parameter *params)
> +{
> +       u32 *table, opcode, addr;

s/table/dwords?

u8 opcode?

> +       size_t len;
> +       int ret, i;
> +       u8 dummy;
> +
> +       len = profile1_header->length * sizeof(*table);
> +       table = kmalloc(len, GFP_KERNEL);
> +       if (!table)
> +               return -ENOMEM;
> +
> +       addr = SFDP_PARAM_HEADER_PTP(profile1_header);
> +       ret = spi_nor_read_sfdp(nor, addr, len, table);
> +       if (ret)
> +               goto out;
> +
> +       /* Fix endianness of the table DWORDs. */
> +       for (i = 0; i < profile1_header->length; i++)
> +               table[i] = le32_to_cpu(table[i]);

le32_to_cpu_array(table, profile1_header->length);

> +
> +       /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> +       opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> +
> +       /*
> +        * We don't know what speed the controller is running at. Find the
> +        * dummy cycles for the fastest frequency the flash can run at to be
> +        * sure we are never short of dummy cycles. A value of 0 means the
> +        * frequency is not supported.
> +        *
> +        * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let
> +        * flashes set the correct value if needed in their fixup hooks.
> +        */
> +       dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]);
> +       if (!dummy)
> +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]);
> +       if (!dummy)
> +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]);
> +       if (!dummy)
> +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]);
> +       if (!dummy)
> +               dummy = PROFILE1_DUMMY_DEFAULT;
> +
> +       /* Round up to an even value to avoid tripping controllers up. */
> +       dummy = ROUND_UP_TO(dummy, 2);
> +
> +       /* Update the fast read settings. */
> +       spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +                                 0, dummy, opcode,
> +                                 SNOR_PROTO_8_8_8_DTR);
> +
> +       /*
> +        * Set the Read Status Register dummy cycles and dummy address bytes.
> +        */

the comment can fit in a single line

> +       if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY)

I would move this above, where opcode is parsed from the same dword

> +               params->rdsr_dummy = 8;
> +       else
> +               params->rdsr_dummy = 4;
> +
> +       if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES)
> +               params->rdsr_addr_nbytes = 4;
> +       else
> +               params->rdsr_addr_nbytes = 0;
> +
> +out:
> +       kfree(table);
> +       return ret;
> +}
> +
>  /**
>   * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
>   * @nor:               pointer to a 'struct spi_nor'
> @@ -1207,6 +1301,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
>                         err = spi_nor_parse_4bait(nor, param_header, params);
>                         break;
> 
> +               case SFDP_PROFILE1_ID:
> +                       err = spi_nor_parse_profile1(nor, param_header, params);
> +                       break;
> +
>                 default:
>                         break;
>                 }
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP
  2020-06-23 18:30 ` [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP Pratyush Yadav
@ 2020-07-08 16:03   ` Tudor.Ambarus
  2020-07-20 16:24     ` Pratyush Yadav
  0 siblings, 1 reply; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-08 16:03 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The xSPI Profile 1.0 table specifies how many dummy cycles and address
> bytes are needed for the Read Status Register command in octal DTR mode.
> Use that information to send the correct Read SR command.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 7d24e63fcca8..f2748f1d9957 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -357,6 +357,8 @@ int spi_nor_write_disable(struct spi_nor *nor)
>  static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>  {
>         int ret;
> +       u8 addr_bytes = nor->params->rdsr_addr_nbytes;
> +       u8 dummy = nor->params->rdsr_dummy;

no need to introduce local variables for a single dereference

> 
>         if (nor->spimem) {
>                 struct spi_mem_op op =
> @@ -365,10 +367,21 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_DATA_IN(1, sr, 1));
> 
> +               if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
> +                       op.addr.nbytes = addr_bytes;
> +                       op.addr.val = 0;

isn't addr already initialized to 0?

> +                       op.dummy.nbytes = dummy;
> +               }
> +
> +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
>                 ret = spi_mem_exec_op(nor->spimem, &op);
>         } else {
> -               ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
> -                                                   sr, 1);
> +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> +                       ret = -ENOTSUPP;
> +               else
> +                       ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
> +                                                           sr, 1);
>         }

doesn't this belong to a previous patch?

> 
>         if (ret)
> @@ -388,6 +401,8 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
>  static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
>  {
>         int ret;
> +       u8 addr_bytes = nor->params->rdsr_addr_nbytes;
> +       u8 dummy = nor->params->rdsr_dummy;
> 
>         if (nor->spimem) {
>                 struct spi_mem_op op =
> @@ -396,6 +411,12 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_DATA_IN(1, fsr, 1));
> 
> +               if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
> +                       op.addr.nbytes = addr_bytes;
> +                       op.addr.val = 0;
> +                       op.dummy.nbytes = dummy;
> +               }
> +
>                 spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> 
>                 ret = spi_mem_exec_op(nor->spimem, &op);
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT
  2020-06-23 18:30 ` [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT Pratyush Yadav
@ 2020-07-08 16:08   ` Tudor.Ambarus
  2020-07-20 16:21     ` Pratyush Yadav
  0 siblings, 1 reply; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-08 16:08 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> A Soft Reset sequence will return the flash to Power-on-Reset (POR)
> state. It consists of two commands: Soft Reset Enable and Soft Reset.
> Find out if the sequence is supported from BFPT DWORD 16.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.h | 1 +
>  drivers/mtd/spi-nor/sfdp.c | 4 ++++
>  drivers/mtd/spi-nor/sfdp.h | 2 ++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 6338d32a0d77..79ce952c0539 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
>         SNOR_F_HAS_SR_TB_BIT6   = BIT(11),
>         SNOR_F_HAS_4BIT_BP      = BIT(12),
>         SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
> +       SNOR_F_SOFT_RESET       = BIT(14),
>  };
> 
>  struct spi_nor_read_command {
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 7983ff431346..8e0741d8bfd3 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -616,6 +616,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>                 break;
>         }
> 
> +       /* Soft Reset support. */
> +       if (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_SOFT_RST)

this can be improved. There are multiple reset methods described and you're
addressing just one of them.

> +               nor->flags |= SNOR_F_SOFT_RESET;
> +
>         /* Stop here if not JESD216 rev C or later. */
>         if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
>                 return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index 6d7243067252..8ae55e98084e 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -90,6 +90,8 @@ struct sfdp_bfpt {
>  #define BFPT_DWORD15_QER_SR2_BIT1_NO_RD                (0x4UL << 20)
>  #define BFPT_DWORD15_QER_SR2_BIT1              (0x5UL << 20) /* Spansion */
> 
> +#define BFPT_DWORD16_SOFT_RST                  BIT(12)
> +
>  #define BFPT_DWORD18_CMD_EXT_MASK              GENMASK(30, 29)
>  #define BFPT_DWORD18_CMD_EXT_REP               (0x0UL << 29) /* Repeat */
>  #define BFPT_DWORD18_CMD_EXT_INV               (0x1UL << 29) /* Invert */
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown
  2020-06-23 18:30 ` [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown Pratyush Yadav
@ 2020-07-08 16:10   ` Tudor.Ambarus
  2020-07-20 16:11     ` Pratyush Yadav
  0 siblings, 1 reply; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-08 16:10 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Perform a Soft Reset on shutdown on flashes that support it so that the
> flash can be reset to its initial state and any configurations made by
> spi-nor (given that they're only done in volatile registers) will be
> reset. This will hand back the flash in pristine state for any further
> operations on it.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 42 +++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h |  2 ++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 4a1f6b343534..27ad9bab06dc 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -40,6 +40,9 @@
> 
>  #define SPI_NOR_MAX_ADDR_WIDTH 4
> 
> +#define SPI_NOR_SRST_SLEEP_MIN 200
> +#define SPI_NOR_SRST_SLEEP_MAX 400
> +
>  /**
>   * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
>   *                        extension type.
> @@ -3201,6 +3204,41 @@ static int spi_nor_init(struct spi_nor *nor)
>         return 0;
>  }
> 
> +static void spi_nor_soft_reset(struct spi_nor *nor)
> +{
> +       struct spi_mem_op op;
> +       int ret;
> +
> +       op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 8),
> +                       SPI_MEM_OP_NO_DUMMY,
> +                       SPI_MEM_OP_NO_ADDR,
> +                       SPI_MEM_OP_NO_DATA);
> +       spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +       ret = spi_mem_exec_op(nor->spimem, &op);
> +       if (ret) {
> +               dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> +               return;
> +       }
> +
> +       op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 8),
> +                       SPI_MEM_OP_NO_DUMMY,
> +                       SPI_MEM_OP_NO_ADDR,
> +                       SPI_MEM_OP_NO_DATA);
> +       spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> +       ret = spi_mem_exec_op(nor->spimem, &op);
> +       if (ret) {
> +               dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> +               return;
> +       }
> +
> +       /*
> +        * Software Reset is not instant, and the delay varies from flash to
> +        * flash. Looking at a few flashes, most range somewhere below 100
> +        * microseconds. So, sleep for a range of 200-400 us.
> +        */
> +       usleep_range(SPI_NOR_SRST_SLEEP_MIN, SPI_NOR_SRST_SLEEP_MAX);
> +}
> +
>  /* mtd resume handler */
>  static void spi_nor_resume(struct mtd_info *mtd)
>  {
> @@ -3220,6 +3258,10 @@ void spi_nor_restore(struct spi_nor *nor)
>         if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
>             nor->flags & SNOR_F_BROKEN_RESET)
>                 nor->params->set_4byte_addr_mode(nor, false);
> +
> +       if (nor->info->flags & SPI_NOR_OCTAL_DTR_READ &&

Why this limitation? Can't we make the software reset available for all
the modes?

> +           nor->flags & SNOR_F_SOFT_RESET)
> +               spi_nor_soft_reset(nor);
>  }
>  EXPORT_SYMBOL_GPL(spi_nor_restore);
> 
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index cd549042c53d..299685d15dc2 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -51,6 +51,8 @@
>  #define SPINOR_OP_CLFSR                0x50    /* Clear flag status register */
>  #define SPINOR_OP_RDEAR                0xc8    /* Read Extended Address Register */
>  #define SPINOR_OP_WREAR                0xc5    /* Write Extended Address Register */
> +#define SPINOR_OP_SRSTEN       0x66    /* Software Reset Enable */
> +#define SPINOR_OP_SRST         0x99    /* Software Reset */
> 
>  /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
>  #define SPINOR_OP_READ_4B      0x13    /* Read data bytes (low frequency) */
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not
  2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
@ 2020-07-08 16:22   ` Tudor.Ambarus
  2020-07-13  3:55   ` Tudor.Ambarus
  1 sibling, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-08 16:22 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Each phase is given a separate 'dtr' field so mixed protocols like
> 4S-4D-4D can be supported.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/spi/spi-mem.c       | 3 +++
>  include/linux/spi/spi-mem.h | 8 ++++++++
>  2 files changed, 11 insertions(+)
>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not
  2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
  2020-07-08 16:22   ` Tudor.Ambarus
@ 2020-07-13  3:55   ` Tudor.Ambarus
  1 sibling, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-13  3:55 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 9a86cc27fcc0..93e255287ab9 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -156,6 +156,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
>                                    op->data.dir == SPI_MEM_DATA_OUT))
>                 return false;
> 
> +       if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> +               return false;


I would put this check the first thing in the function to exit sooner
and avoid the rest of the checks, that would become superfluous.

Anyway this is just a nit.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension
  2020-06-23 18:30 ` [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension Pratyush Yadav
@ 2020-07-13  6:15   ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-13  6:15 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> In xSPI mode, flashes expect 2-byte opcodes. The second byte is called
> the "command extension". There can be 3 types of extensions in xSPI:
> repeat, invert, and hex. When the extension type is "repeat", the same
> opcode is sent twice. When it is "invert", the second byte is the
> inverse of the opcode. When it is "hex" an additional opcode byte based
> is sent with the command whose value can be anything.
> 
> So, make opcode a 16-bit value and add a 'nbytes', similar to how
> multiple address widths are handled.
> 
> Some places use sizeof(op->cmd.opcode). Replace them with op->cmd.nbytes
> 
> The spi-mxic and spi-zynq-qspi drivers directly use op->cmd.opcode as a
> buffer. Now that opcode is a 2-byte field, this can result in different
> behaviour depending on if the machine is little endian or big endian.
> Extract the opcode in a local 1-byte variable and use that as the buffer
> instead. Both these drivers would reject multi-byte opcodes in their
> supports_op() hook anyway, so we only need to worry about single-byte
> opcodes for now.
> 
> The above two changes are put in this commit to keep the series
> bisectable.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/spi/spi-mem.c       | 13 +++++++------
>  drivers/spi/spi-mtk-nor.c   |  4 ++--
>  drivers/spi/spi-mxic.c      |  3 ++-
>  drivers/spi/spi-zynq-qspi.c | 11 ++++++-----
>  include/linux/spi/spi-mem.h |  6 +++++-
>  5 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 93e255287ab9..ef53290b7d24 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -159,6 +159,9 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
>         if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
>                 return false;
> 
> +       if (op->cmd.nbytes != 1)
> +               return false;

I would put this imediately before:

if (spi_check_buswidth_req(mem, op->cmd.buswidth, true))  

to speed up the exit and avoid the rest of the checks that would
become superflous.


> +
>         return true;
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> @@ -173,7 +176,7 @@ static bool spi_mem_buswidth_is_valid(u8 buswidth)
> 
>  static int spi_mem_check_op(const struct spi_mem_op *op)
>  {
> -       if (!op->cmd.buswidth)
> +       if (!op->cmd.buswidth || !op->cmd.nbytes)

we would be more explicit with:
if (!op->cmd.buswidth || !op->cmd.nbytes || op->cmd.nbytes > 2)

With these addressed:

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

>                 return -EINVAL;
> 
>         if ((op->addr.nbytes && !op->addr.buswidth) ||
> @@ -309,8 +312,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>                         return ret;
>         }
> 
> -       tmpbufsize = sizeof(op->cmd.opcode) + op->addr.nbytes +
> -                    op->dummy.nbytes;
> +       tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> 
>         /*
>          * Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so
> @@ -325,7 +327,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
> 
>         tmpbuf[0] = op->cmd.opcode;
>         xfers[xferpos].tx_buf = tmpbuf;
> -       xfers[xferpos].len = sizeof(op->cmd.opcode);
> +       xfers[xferpos].len = op->cmd.nbytes;
>         xfers[xferpos].tx_nbits = op->cmd.buswidth;
>         spi_message_add_tail(&xfers[xferpos], &msg);
>         xferpos++;
> @@ -427,8 +429,7 @@ int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>                 return ctlr->mem_ops->adjust_op_size(mem, op);
> 
>         if (!ctlr->mem_ops || !ctlr->mem_ops->exec_op) {
> -               len = sizeof(op->cmd.opcode) + op->addr.nbytes +
> -                     op->dummy.nbytes;
> +               len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
> 
>                 if (len > spi_max_transfer_size(mem->spi))
>                         return -EINVAL;
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index 7bc302b50396..d5f393871619 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -195,7 +195,7 @@ static int mtk_nor_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op)
>                 }
>         }
> 
> -       len = MTK_NOR_PRG_MAX_SIZE - sizeof(op->cmd.opcode) - op->addr.nbytes -
> +       len = MTK_NOR_PRG_MAX_SIZE - op->cmd.nbytes - op->addr.nbytes -
>               op->dummy.nbytes;
>         if (op->data.nbytes > len)
>                 op->data.nbytes = len;
> @@ -219,7 +219,7 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
>                                (op->dummy.buswidth == 0) &&
>                                (op->data.buswidth == 1);
>         }
> -       len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
> +       len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
>         if ((len > MTK_NOR_PRG_MAX_SIZE) ||
>             ((op->data.nbytes) && (len == MTK_NOR_PRG_MAX_SIZE)))
>                 return false;
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index 69491f3a515d..8c630acb0110 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -356,6 +356,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>         int nio = 1, i, ret;
>         u32 ss_ctrl;
>         u8 addr[8];
> +       u8 opcode = op->cmd.opcode;
> 
>         ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
>         if (ret)
> @@ -393,7 +394,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
>         writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
>                mxic->regs + HC_CFG);
> 
> -       ret = mxic_spi_data_xfer(mxic, &op->cmd.opcode, NULL, 1);
> +       ret = mxic_spi_data_xfer(mxic, &opcode, NULL, 1);
>         if (ret)
>                 goto out;
> 
> diff --git a/drivers/spi/spi-zynq-qspi.c b/drivers/spi/spi-zynq-qspi.c
> index 17641157354d..bbf3d90561f5 100644
> --- a/drivers/spi/spi-zynq-qspi.c
> +++ b/drivers/spi/spi-zynq-qspi.c
> @@ -527,20 +527,21 @@ static int zynq_qspi_exec_mem_op(struct spi_mem *mem,
>         struct zynq_qspi *xqspi = spi_controller_get_devdata(mem->spi->master);
>         int err = 0, i;
>         u8 *tmpbuf;
> +       u8 opcode = op->cmd.opcode;
> 
>         dev_dbg(xqspi->dev, "cmd:%#x mode:%d.%d.%d.%d\n",
> -               op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
> +               opcode, op->cmd.buswidth, op->addr.buswidth,
>                 op->dummy.buswidth, op->data.buswidth);
> 
>         zynq_qspi_chipselect(mem->spi, true);
>         zynq_qspi_config_op(xqspi, mem->spi);
> 
> -       if (op->cmd.opcode) {
> +       if (op->cmd.nbytes) {
>                 reinit_completion(&xqspi->data_completion);
> -               xqspi->txbuf = (u8 *)&op->cmd.opcode;
> +               xqspi->txbuf = &opcode;
>                 xqspi->rxbuf = NULL;
> -               xqspi->tx_bytes = sizeof(op->cmd.opcode);
> -               xqspi->rx_bytes = sizeof(op->cmd.opcode);
> +               xqspi->tx_bytes = op->cmd.nbytes;
> +               xqspi->rx_bytes = op->cmd.nbytes;
>                 zynq_qspi_write_op(xqspi, ZYNQ_QSPI_FIFO_DEPTH, true);
>                 zynq_qspi_write(xqspi, ZYNQ_QSPI_IEN_OFFSET,
>                                 ZYNQ_QSPI_IXR_RXTX_MASK);
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index e3dcb956bf61..159463cc659c 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -17,6 +17,7 @@
>         {                                                       \
>                 .buswidth = __buswidth,                         \
>                 .opcode = __opcode,                             \
> +               .nbytes = 1,                                    \
>         }
> 
>  #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth)           \
> @@ -69,6 +70,8 @@ enum spi_mem_data_dir {
> 
>  /**
>   * struct spi_mem_op - describes a SPI memory operation
> + * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
> + *             sent MSB-first.
>   * @cmd.buswidth: number of IO lines used to transmit the command
>   * @cmd.opcode: operation opcode
>   * @cmd.dtr: whether the command opcode should be sent in DTR mode or not
> @@ -94,9 +97,10 @@ enum spi_mem_data_dir {
>   */
>  struct spi_mem_op {
>         struct {
> +               u8 nbytes;
>                 u8 buswidth;
>                 u8 dtr : 1;
> -               u8 opcode;
> +               u16 opcode;
>         } cmd;
> 
>         struct {
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops
  2020-06-23 18:30 ` [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops Pratyush Yadav
@ 2020-07-13  6:19   ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-13  6:19 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Double Transfer Rate (DTR) ops are added in spi-mem. But this controller
> doesn't support DTR transactions. Since we don't use the default

the sam9x60 version of the controller supports DTR ops.

> supports_op(), which rejects all DTR ops, do that explicitly in our
> supports_op().
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/spi/atmel-quadspi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spi/atmel-quadspi.c b/drivers/spi/atmel-quadspi.c
> index cb44d1e169aa..a898755fb41e 100644
> --- a/drivers/spi/atmel-quadspi.c
> +++ b/drivers/spi/atmel-quadspi.c
> @@ -285,6 +285,12 @@ static bool atmel_qspi_supports_op(struct spi_mem *mem,
>                 op->dummy.nbytes == 0)
>                 return false;
> 
> +       /* DTR ops not supported. */

Better to say that "DTR ops are not implemented."

> +       if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> +               return false;
> +       if (op->cmd.nbytes != 1)
> +               return false;
> +

I would move these the first thing in the function, to speed up the exit.

With these addressed:

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

>         return true;
>  }
> 
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 04/17] spi: spi-mtk-nor: reject DTR ops
  2020-06-23 18:30 ` [PATCH v10 04/17] spi: spi-mtk-nor: " Pratyush Yadav
@ 2020-07-13  6:24   ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-13  6:24 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Double Transfer Rate (DTR) ops are added in spi-mem. But this controller
> doesn't support DTR transactions. Since we don't use the default
> supports_op(), which rejects all DTR ops, do that explicitly in our
> supports_op().
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/spi/spi-mtk-nor.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/spi/spi-mtk-nor.c b/drivers/spi/spi-mtk-nor.c
> index d5f393871619..b08d8e9a8ee9 100644
> --- a/drivers/spi/spi-mtk-nor.c
> +++ b/drivers/spi/spi-mtk-nor.c
> @@ -211,6 +211,12 @@ static bool mtk_nor_supports_op(struct spi_mem *mem,
>         if (op->cmd.buswidth != 1)
>                 return false;
> 
> +       /* DTR ops not supported. */
> +       if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> +               return false;
> +       if (op->cmd.nbytes != 1)
> +               return false;
> +
>         if ((op->addr.nbytes == 3) || (op->addr.nbytes == 4)) {
>                 if ((op->data.dir == SPI_MEM_DATA_IN) && mtk_nor_match_read(op))
>                         return true;
> --
> 2.27.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (16 preceding siblings ...)
  2020-06-23 18:30 ` [PATCH v10 17/17] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode Pratyush Yadav
@ 2020-07-13  6:34 ` Tudor.Ambarus
  2020-07-14 19:19   ` Mark Brown
  2020-07-14 16:40 ` Mark Brown
  18 siblings, 1 reply; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-13  6:34 UTC (permalink / raw)
  To: p.yadav, vigneshr, broonie, linux-spi, boris.brezillon
  Cc: alexandre.belloni, richard, nsekhar, Nicolas.Ferre, michal.simek,
	Ludovic.Desroches, linux-mtd, linux-arm-kernel, miquel.raynal,
	matthias.bgg, linux-mediatek, linux-kernel

Hi, Mark,

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> Pratyush Yadav (17):
>   spi: spi-mem: allow specifying whether an op is DTR or not
>   spi: spi-mem: allow specifying a command's extension
>   spi: atmel-quadspi: reject DTR ops
>   spi: spi-mtk-nor: reject DTR ops

These four patches are looking good, I had just few minor comments.
If you too think that they are ok, would you take them through the
SPI tree? If so, I would need an immutable tag on top of v5.8-rc1
preferably, so I can merge them back to SPI NOR and continue the
development on top of them.

Let us know if you want us to resubmit for those minor comments.

Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal
  2020-06-23 18:30 ` [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal Pratyush Yadav
@ 2020-07-13  9:33   ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-13  9:33 UTC (permalink / raw)
  To: p.yadav, miquel.raynal, richard, vigneshr, broonie,
	Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches,
	matthias.bgg, michal.simek, linux-mtd, linux-kernel, linux-spi,
	linux-arm-kernel, linux-mediatek
  Cc: boris.brezillon, nsekhar

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> The Micron MT35XU512ABA flash does not support the quad enable bit. But
> instead of programming the Quad Enable Require field to 000b ("Device
> does not have a QE bit"), it is programmed to 111b ("Reserved").
> 
> While this is technically incorrect, it is not reason enough to abort
> BFPT parsing. Instead, continue BFPT parsing and let flashes set it in
> their fixup hooks.
> 
> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Applied, thanks!
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support
  2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
                   ` (17 preceding siblings ...)
  2020-07-13  6:34 ` [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Tudor.Ambarus
@ 2020-07-14 16:40 ` Mark Brown
  18 siblings, 0 replies; 43+ messages in thread
From: Mark Brown @ 2020-07-14 16:40 UTC (permalink / raw)
  To: Michal Simek, Pratyush Yadav, Tudor Ambarus, Nicolas Ferre,
	Vignesh Raghavendra, linux-mediatek, linux-kernel,
	linux-arm-kernel, Matthias Brugger, Alexandre Belloni,
	Miquel Raynal, linux-spi, Ludovic Desroches, linux-mtd,
	Richard Weinberger
  Cc: Boris Brezillon, Sekhar Nori

On Wed, 24 Jun 2020 00:00:13 +0530, Pratyush Yadav wrote:
> This series adds support for octal DTR flashes in the spi-nor framework,
> and then adds hooks for the Cypress Semper and Mircom Xcella flashes to
> allow running them in octal DTR mode. This series assumes that the flash
> is handed to the kernel in Legacy SPI mode.
> 
> Tested on TI J721e EVM with 1-bit ECC on the Cypress flash.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/4] spi: spi-mem: allow specifying whether an op is DTR or not
      commit: 4c5e2bba30e49b970a0fd07b43e0b7a3b5fd5ea7
[2/4] spi: spi-mem: allow specifying a command's extension
      commit: caf72df48be32c39f74287976ae843501ae06949
[3/4] spi: atmel-quadspi: reject DTR ops
      commit: 5c81c275582c9d9c66d2f928591a2065f2528231
[4/4] spi: spi-mtk-nor: reject DTR ops
      commit: 4728f073bfc66b8b555274ef0d7741d7f5a48947

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support
  2020-07-13  6:34 ` [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Tudor.Ambarus
@ 2020-07-14 19:19   ` Mark Brown
  2020-07-15  3:40     ` Tudor.Ambarus
  0 siblings, 1 reply; 43+ messages in thread
From: Mark Brown @ 2020-07-14 19:19 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, michal.simek, vigneshr, richard, nsekhar,
	Nicolas.Ferre, linux-spi, Ludovic.Desroches, boris.brezillon,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, p.yadav, linux-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1472 bytes --]

On Mon, Jul 13, 2020 at 06:34:12AM +0000, Tudor.Ambarus@microchip.com wrote:

> These four patches are looking good, I had just few minor comments.
> If you too think that they are ok, would you take them through the
> SPI tree? If so, I would need an immutable tag on top of v5.8-rc1
> preferably, so I can merge them back to SPI NOR and continue the
> development on top of them.

The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407:

  Linux 5.8-rc1 (2020-06-14 12:45:04 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git tags/spi-mem-dtr

for you to fetch changes up to 4728f073bfc66b8b555274ef0d7741d7f5a48947:

  spi: spi-mtk-nor: reject DTR ops (2020-07-14 17:29:40 +0100)

----------------------------------------------------------------
spi: Support for DTR ops

----------------------------------------------------------------
Pratyush Yadav (4):
      spi: spi-mem: allow specifying whether an op is DTR or not
      spi: spi-mem: allow specifying a command's extension
      spi: atmel-quadspi: reject DTR ops
      spi: spi-mtk-nor: reject DTR ops

 drivers/spi/atmel-quadspi.c |  6 ++++++
 drivers/spi/spi-mem.c       | 16 ++++++++++------
 drivers/spi/spi-mtk-nor.c   | 10 ++++++++--
 drivers/spi/spi-mxic.c      |  3 ++-
 drivers/spi/spi-zynq-qspi.c | 11 ++++++-----
 include/linux/spi/spi-mem.h | 14 +++++++++++++-
 6 files changed, 45 insertions(+), 15 deletions(-)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: Re: [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support
  2020-07-14 19:19   ` Mark Brown
@ 2020-07-15  3:40     ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-07-15  3:40 UTC (permalink / raw)
  To: broonie
  Cc: alexandre.belloni, michal.simek, vigneshr, richard, nsekhar,
	Nicolas.Ferre, linux-spi, Ludovic.Desroches, boris.brezillon,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, p.yadav, linux-kernel

On 7/14/20 10:19 PM, Mark Brown wrote:
> On Mon, Jul 13, 2020 at 06:34:12AM +0000, Tudor.Ambarus@microchip.com wrote:
> 
>> These four patches are looking good, I had just few minor comments.
>> If you too think that they are ok, would you take them through the
>> SPI tree? If so, I would need an immutable tag on top of v5.8-rc1
>> preferably, so I can merge them back to SPI NOR and continue the
>> development on top of them.
> 
> The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407:
> 
>   Linux 5.8-rc1 (2020-06-14 12:45:04 -0700)
> 
> are available in the Git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git tags/spi-mem-dtr
> 
> for you to fetch changes up to 4728f073bfc66b8b555274ef0d7741d7f5a48947:
> 
>   spi: spi-mtk-nor: reject DTR ops (2020-07-14 17:29:40 +0100)
> 
> ----------------------------------------------------------------
> spi: Support for DTR ops
> 
> ----------------------------------------------------------------
> Pratyush Yadav (4):
>       spi: spi-mem: allow specifying whether an op is DTR or not
>       spi: spi-mem: allow specifying a command's extension
>       spi: atmel-quadspi: reject DTR ops
>       spi: spi-mtk-nor: reject DTR ops
> 
>  drivers/spi/atmel-quadspi.c |  6 ++++++
>  drivers/spi/spi-mem.c       | 16 ++++++++++------
>  drivers/spi/spi-mtk-nor.c   | 10 ++++++++--
>  drivers/spi/spi-mxic.c      |  3 ++-
>  drivers/spi/spi-zynq-qspi.c | 11 ++++++-----
>  include/linux/spi/spi-mem.h | 14 +++++++++++++-
>  6 files changed, 45 insertions(+), 15 deletions(-)
> 

Merged into spi-nor/next. Thank you!


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown
  2020-07-08 16:10   ` Tudor.Ambarus
@ 2020-07-20 16:11     ` Pratyush Yadav
  0 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-07-20 16:11 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, vigneshr, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel

Hi Tudor,

On 08/07/20 04:10PM, Tudor.Ambarus@microchip.com wrote:
> On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Perform a Soft Reset on shutdown on flashes that support it so that the
> > flash can be reset to its initial state and any configurations made by
> > spi-nor (given that they're only done in volatile registers) will be
> > reset. This will hand back the flash in pristine state for any further
> > operations on it.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/mtd/spi-nor/core.c  | 42 +++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/spi-nor.h |  2 ++
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 4a1f6b343534..27ad9bab06dc 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -40,6 +40,9 @@
> > 
> >  #define SPI_NOR_MAX_ADDR_WIDTH 4
> > 
> > +#define SPI_NOR_SRST_SLEEP_MIN 200
> > +#define SPI_NOR_SRST_SLEEP_MAX 400
> > +
> >  /**
> >   * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
> >   *                        extension type.
> > @@ -3201,6 +3204,41 @@ static int spi_nor_init(struct spi_nor *nor)
> >         return 0;
> >  }
> > 
> > +static void spi_nor_soft_reset(struct spi_nor *nor)
> > +{
> > +       struct spi_mem_op op;
> > +       int ret;
> > +
> > +       op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRSTEN, 8),
> > +                       SPI_MEM_OP_NO_DUMMY,
> > +                       SPI_MEM_OP_NO_ADDR,
> > +                       SPI_MEM_OP_NO_DATA);
> > +       spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> > +       ret = spi_mem_exec_op(nor->spimem, &op);
> > +       if (ret) {
> > +               dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> > +               return;
> > +       }
> > +
> > +       op = (struct spi_mem_op)SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_SRST, 8),
> > +                       SPI_MEM_OP_NO_DUMMY,
> > +                       SPI_MEM_OP_NO_ADDR,
> > +                       SPI_MEM_OP_NO_DATA);
> > +       spi_nor_spimem_setup_op(nor, &op, SNOR_PROTO_8_8_8_DTR);
> > +       ret = spi_mem_exec_op(nor->spimem, &op);
> > +       if (ret) {
> > +               dev_warn(nor->dev, "Software reset failed: %d\n", ret);
> > +               return;
> > +       }
> > +
> > +       /*
> > +        * Software Reset is not instant, and the delay varies from flash to
> > +        * flash. Looking at a few flashes, most range somewhere below 100
> > +        * microseconds. So, sleep for a range of 200-400 us.
> > +        */
> > +       usleep_range(SPI_NOR_SRST_SLEEP_MIN, SPI_NOR_SRST_SLEEP_MAX);
> > +}
> > +
> >  /* mtd resume handler */
> >  static void spi_nor_resume(struct mtd_info *mtd)
> >  {
> > @@ -3220,6 +3258,10 @@ void spi_nor_restore(struct spi_nor *nor)
> >         if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
> >             nor->flags & SNOR_F_BROKEN_RESET)
> >                 nor->params->set_4byte_addr_mode(nor, false);
> > +
> > +       if (nor->info->flags & SPI_NOR_OCTAL_DTR_READ &&
> 
> Why this limitation? Can't we make the software reset available for all
> the modes?

Because I wrote the function spi_nor_soft_reset() from the perspective 
of xSPI support, and the xSPI spec only cares about the 8D-8D-8D version 
of the soft reset.

BFPT says we can execute it on 1, 2, 4, or 8 wires depending on the mode 
so I guess we can support a generalized version as well. Will fix.

> > +           nor->flags & SNOR_F_SOFT_RESET)
> > +               spi_nor_soft_reset(nor);
> >  }
> >  EXPORT_SYMBOL_GPL(spi_nor_restore);
> > 
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index cd549042c53d..299685d15dc2 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -51,6 +51,8 @@
> >  #define SPINOR_OP_CLFSR                0x50    /* Clear flag status register */
> >  #define SPINOR_OP_RDEAR                0xc8    /* Read Extended Address Register */
> >  #define SPINOR_OP_WREAR                0xc5    /* Write Extended Address Register */
> > +#define SPINOR_OP_SRSTEN       0x66    /* Software Reset Enable */
> > +#define SPINOR_OP_SRST         0x99    /* Software Reset */
> > 
> >  /* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
> >  #define SPINOR_OP_READ_4B      0x13    /* Read data bytes (low frequency) */

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT
  2020-07-08 16:08   ` Tudor.Ambarus
@ 2020-07-20 16:21     ` Pratyush Yadav
  0 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-07-20 16:21 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, vigneshr, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel

Hi Tudor,

On 08/07/20 04:08PM, Tudor.Ambarus@microchip.com wrote:
> On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > A Soft Reset sequence will return the flash to Power-on-Reset (POR)
> > state. It consists of two commands: Soft Reset Enable and Soft Reset.
> > Find out if the sequence is supported from BFPT DWORD 16.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/mtd/spi-nor/core.h | 1 +
> >  drivers/mtd/spi-nor/sfdp.c | 4 ++++
> >  drivers/mtd/spi-nor/sfdp.h | 2 ++
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index 6338d32a0d77..79ce952c0539 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -26,6 +26,7 @@ enum spi_nor_option_flags {
> >         SNOR_F_HAS_SR_TB_BIT6   = BIT(11),
> >         SNOR_F_HAS_4BIT_BP      = BIT(12),
> >         SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
> > +       SNOR_F_SOFT_RESET       = BIT(14),
> >  };
> > 
> >  struct spi_nor_read_command {
> > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > index 7983ff431346..8e0741d8bfd3 100644
> > --- a/drivers/mtd/spi-nor/sfdp.c
> > +++ b/drivers/mtd/spi-nor/sfdp.c
> > @@ -616,6 +616,10 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
> >                 break;
> >         }
> > 
> > +       /* Soft Reset support. */
> > +       if (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_SOFT_RST)
> 
> this can be improved. There are multiple reset methods described and you're
> addressing just one of them.

Yes, it can be. But xSPI only cares about the 0x66 and 0x99 reset 
sequence and that is what I implemented. Others can be added if they are 
needed in the future. In addition, I don't have hardware that supports 
these resets so I can't test them. IMO if someone needs other reset 
modes, they should send a separate patch for it.

If you are worried about future work needed to support multiple soft 
reset modes, I can introduce a nor->soft_reset() hook that can be 
populated when parsing BFPT. But I think that is a bit premature. The 
work needed to do that is not a lot so I think we should hold off until 
the need really comes up.
 
> > +               nor->flags |= SNOR_F_SOFT_RESET;
> > +
> >         /* Stop here if not JESD216 rev C or later. */
> >         if (bfpt_header->length == BFPT_DWORD_MAX_JESD216B)
> >                 return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
> > diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> > index 6d7243067252..8ae55e98084e 100644
> > --- a/drivers/mtd/spi-nor/sfdp.h
> > +++ b/drivers/mtd/spi-nor/sfdp.h
> > @@ -90,6 +90,8 @@ struct sfdp_bfpt {
> >  #define BFPT_DWORD15_QER_SR2_BIT1_NO_RD                (0x4UL << 20)
> >  #define BFPT_DWORD15_QER_SR2_BIT1              (0x5UL << 20) /* Spansion */
> > 
> > +#define BFPT_DWORD16_SOFT_RST                  BIT(12)
> > +
> >  #define BFPT_DWORD18_CMD_EXT_MASK              GENMASK(30, 29)
> >  #define BFPT_DWORD18_CMD_EXT_REP               (0x0UL << 29) /* Repeat */
> >  #define BFPT_DWORD18_CMD_EXT_INV               (0x1UL << 29) /* Invert */

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP
  2020-07-08 16:03   ` Tudor.Ambarus
@ 2020-07-20 16:24     ` Pratyush Yadav
  0 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-07-20 16:24 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, vigneshr, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel

Hi Tudor,

On 08/07/20 04:03PM, Tudor.Ambarus@microchip.com wrote:
> On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > The xSPI Profile 1.0 table specifies how many dummy cycles and address
> > bytes are needed for the Read Status Register command in octal DTR mode.
> > Use that information to send the correct Read SR command.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/mtd/spi-nor/core.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 7d24e63fcca8..f2748f1d9957 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -357,6 +357,8 @@ int spi_nor_write_disable(struct spi_nor *nor)
> >  static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
> >  {
> >         int ret;
> > +       u8 addr_bytes = nor->params->rdsr_addr_nbytes;
> > +       u8 dummy = nor->params->rdsr_dummy;
> 
> no need to introduce local variables for a single dereference

Ok.
 
> > 
> >         if (nor->spimem) {
> >                 struct spi_mem_op op =
> > @@ -365,10 +367,21 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
> >                                    SPI_MEM_OP_NO_DUMMY,
> >                                    SPI_MEM_OP_DATA_IN(1, sr, 1));
> > 
> > +               if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
> > +                       op.addr.nbytes = addr_bytes;
> > +                       op.addr.val = 0;
> 
> isn't addr already initialized to 0?

Yes, it is. But I figured it won't hurt to be explicit about what we 
intend the address to be.

> > +                       op.dummy.nbytes = dummy;
> > +               }
> > +
> > +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > +
> >                 ret = spi_mem_exec_op(nor->spimem, &op);
> >         } else {
> > -               ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
> > -                                                   sr, 1);
> > +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> > +                       ret = -ENOTSUPP;
> > +               else
> > +                       ret = nor->controller_ops->read_reg(nor, SPINOR_OP_RDSR,
> > +                                                           sr, 1);
> >         }
> 
> doesn't this belong to a previous patch?

It does. Will fix.
 
> > 
> >         if (ret)
> > @@ -388,6 +401,8 @@ static int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
> >  static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
> >  {
> >         int ret;
> > +       u8 addr_bytes = nor->params->rdsr_addr_nbytes;
> > +       u8 dummy = nor->params->rdsr_dummy;
> > 
> >         if (nor->spimem) {
> >                 struct spi_mem_op op =
> > @@ -396,6 +411,12 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
> >                                    SPI_MEM_OP_NO_DUMMY,
> >                                    SPI_MEM_OP_DATA_IN(1, fsr, 1));
> > 
> > +               if (spi_nor_protocol_is_dtr(nor->reg_proto)) {
> > +                       op.addr.nbytes = addr_bytes;
> > +                       op.addr.val = 0;
> > +                       op.dummy.nbytes = dummy;
> > +               }
> > +
> >                 spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > 
> >                 ret = spi_mem_exec_op(nor->spimem, &op);

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table
  2020-07-08 16:01   ` Tudor.Ambarus
@ 2020-07-20 16:38     ` Pratyush Yadav
  0 siblings, 0 replies; 43+ messages in thread
From: Pratyush Yadav @ 2020-07-20 16:38 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, vigneshr, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel

Hi Tudor,

On 08/07/20 04:01PM, Tudor.Ambarus@microchip.com wrote:
> On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > This table is indication that the flash is xSPI compliant and hence
> > supports octal DTR mode. Extract information like the fast read opcode,
> > dummy cycles, the number of dummy cycles needed for a Read Status
> > Register command, and the number of address bytes needed for a Read
> > Status Register command.
> > 
> > We don't know what speed the controller is running at. Find the fast
> > read dummy cycles for the fastest frequency the flash can run at to be
> > sure we are never short of dummy cycles. If nothing is available,
> > default to 20. Flashes that use a different value should update it in
> > their fixup hooks.
> > 
> > Since we want to set read settings, expose spi_nor_set_read_settings()
> > in core.h.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/mtd/spi-nor/core.c |  2 +-
> >  drivers/mtd/spi-nor/core.h | 10 ++++
> >  drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 109 insertions(+), 1 deletion(-)
> > 
[...]
> > diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> > index 3f709de5ea67..d5a24e61813c 100644
> > --- a/drivers/mtd/spi-nor/sfdp.c
> > +++ b/drivers/mtd/spi-nor/sfdp.c
[...]
> > @@ -66,6 +70,16 @@ struct sfdp_bfpt_erase {
> >         u32                     shift;
> >  };
> > 
> > +/* xSPI Profile 1.0 table (from JESD216D.01). */
> > +#define PROFILE1_DWORD1_RD_FAST_CMD            GENMASK(15, 8)
> > +#define PROFILE1_DWORD1_RDSR_DUMMY             BIT(28)
> > +#define PROFILE1_DWORD1_RDSR_ADDR_BYTES                BIT(29)
> > +#define PROFILE1_DWORD4_DUMMY_200MHZ           GENMASK(11, 7)
> > +#define PROFILE1_DWORD5_DUMMY_166MHZ           GENMASK(31, 27)
> > +#define PROFILE1_DWORD5_DUMMY_133MHZ           GENMASK(21, 17)
> > +#define PROFILE1_DWORD5_DUMMY_100MHZ           GENMASK(11, 7)
> 
> we should order these macros in a consistent way. I see that previous macros
> are declared in order starting from MSB to LSB.
> 
> > +#define PROFILE1_DUMMY_DEFAULT                 20
> 
> we need to explain why the default dummy value is 20.

No reason other than the fact that it is the default for the first flash 
that uses Profile 1.0 parsing (S28HS512T). AFAIK a similar reasoning is 
followed for the default being 8 for 1-1-4 or 1-1-8 modes.

I can't think of any reasonable way of deciding on a default value since 
it varies from flash to flash.
 
> How about declaring all these macros immediately above of spi_nor_parse_profile1()?
> 
> > +
> >  #define SMPT_CMD_ADDRESS_LEN_MASK              GENMASK(23, 22)
> >  #define SMPT_CMD_ADDRESS_LEN_0                 (0x0UL << 22)
> >  #define SMPT_CMD_ADDRESS_LEN_3                 (0x1UL << 22)
> > @@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
> >         return ret;
> >  }
> > 
> > +/**
> > + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
> > + * @nor:               pointer to a 'struct spi_nor'
> > + * @param_header:      pointer to the 'struct sfdp_parameter_header' describing
> > + *                     the 4-Byte Address Instruction Table length and version.
> > + * @params:            pointer to the 'struct spi_nor_flash_parameter' to be.
> > + *
> > + * Return: 0 on success, -errno otherwise.
> > + */
> > +static int spi_nor_parse_profile1(struct spi_nor *nor,
> > +                                 const struct sfdp_parameter_header *profile1_header,
> > +                                 struct spi_nor_flash_parameter *params)
> > +{
> > +       u32 *table, opcode, addr;
> 
> s/table/dwords?
> 
> u8 opcode?
> 
> > +       size_t len;
> > +       int ret, i;
> > +       u8 dummy;
> > +
> > +       len = profile1_header->length * sizeof(*table);
> > +       table = kmalloc(len, GFP_KERNEL);
> > +       if (!table)
> > +               return -ENOMEM;
> > +
> > +       addr = SFDP_PARAM_HEADER_PTP(profile1_header);
> > +       ret = spi_nor_read_sfdp(nor, addr, len, table);
> > +       if (ret)
> > +               goto out;
> > +
> > +       /* Fix endianness of the table DWORDs. */
> > +       for (i = 0; i < profile1_header->length; i++)
> > +               table[i] = le32_to_cpu(table[i]);
> 
> le32_to_cpu_array(table, profile1_header->length);
> 
> > +
> > +       /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> > +       opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> > +
> > +       /*
> > +        * We don't know what speed the controller is running at. Find the
> > +        * dummy cycles for the fastest frequency the flash can run at to be
> > +        * sure we are never short of dummy cycles. A value of 0 means the
> > +        * frequency is not supported.
> > +        *
> > +        * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let
> > +        * flashes set the correct value if needed in their fixup hooks.
> > +        */
> > +       dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]);
> > +       if (!dummy)
> > +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]);
> > +       if (!dummy)
> > +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]);
> > +       if (!dummy)
> > +               dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]);
> > +       if (!dummy)
> > +               dummy = PROFILE1_DUMMY_DEFAULT;
> > +
> > +       /* Round up to an even value to avoid tripping controllers up. */
> > +       dummy = ROUND_UP_TO(dummy, 2);
> > +
[...]

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-07-07 17:37   ` Tudor.Ambarus
@ 2020-07-21 11:29     ` Pratyush Yadav
  2020-09-29 15:42       ` Tudor.Ambarus
  0 siblings, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-07-21 11:29 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, vigneshr, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel

Hi Tudor,

On 07/07/20 05:37PM, Tudor.Ambarus@microchip.com wrote:
> Hi, Pratyush,
> 
> On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Double Transfer Rate (DTR) is SPI protocol in which data is transferred
> > on each clock edge as opposed to on each clock cycle. Make
> > framework-level changes to allow supporting flashes in DTR mode.
> > 
> > Right now, mixed DTR modes are not supported. So, for example a mode
> > like 4S-4D-4D will not work. All phases need to be either DTR or STR.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > ---
> >  drivers/mtd/spi-nor/core.c  | 305 ++++++++++++++++++++++++++++-------- 
> >  drivers/mtd/spi-nor/core.h  |   6 +
> >  drivers/mtd/spi-nor/sfdp.c  |   9 +-
> >  include/linux/mtd/spi-nor.h |  51 ++++--
> >  4 files changed, 295 insertions(+), 76 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 0369d98b2d12..22a3832b83a6 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -40,6 +40,76 @@
> > 
> >  #define SPI_NOR_MAX_ADDR_WIDTH 4
> > 
> > +/**
> > + * spi_nor_get_cmd_ext() - Get the command opcode extension based on the
> > + *                        extension type.
> > + * @nor:               pointer to a 'struct spi_nor'
> > + * @op:                        pointer to the 'struct spi_mem_op' whose properties
> > + *                     need to be initialized.
> > + *
> > + * Right now, only "repeat" and "invert" are supported.
> > + *
> > + * Return: The opcode extension.
> > + */
> > +static u8 spi_nor_get_cmd_ext(const struct spi_nor *nor,
> > +                             const struct spi_mem_op *op)
> > +{
> > +       switch (nor->cmd_ext_type) {
> > +       case SPI_NOR_EXT_INVERT:
> > +               return ~op->cmd.opcode;
> > +
> > +       case SPI_NOR_EXT_REPEAT:
> > +               return op->cmd.opcode;
> > +
> > +       default:
> > +               dev_err(nor->dev, "Unknown command extension type\n");
> > +               return 0;
> > +       }
> > +}
> > +
> > +/**
> > + * spi_nor_spimem_setup_op() - Set up common properties of a spi-mem op.
> > + * @nor:               pointer to a 'struct spi_nor'
> > + * @op:                        pointer to the 'struct spi_mem_op' whose properties
> > + *                     need to be initialized.
> > + * @proto:             the protocol from which the properties need to be set.
> > + */
> > +void spi_nor_spimem_setup_op(const struct spi_nor *nor,
> > +                            struct spi_mem_op *op,
> > +                            const enum spi_nor_protocol proto)
> 
> There's not much to set for the REG operations.
> 
> > +{
> > +       u8 ext;
> > +
> > +       op->cmd.buswidth = spi_nor_get_protocol_inst_nbits(proto);
> > +
> > +       if (op->addr.nbytes)
> > +               op->addr.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> > +
> > +       if (op->dummy.nbytes)
> > +               op->dummy.buswidth = spi_nor_get_protocol_addr_nbits(proto);
> > +
> > +       if (op->data.nbytes)
> > +               op->data.buswidth = spi_nor_get_protocol_data_nbits(proto);
> 
> How about getting rid of the above and
> 
> > +
> > +       if (spi_nor_protocol_is_dtr(proto)) {
> 
> introduce a spi_nor_spimem_setup_dtr_op() just for the body of this if?

What benefit do we get with that other than skipping a couple of if() 
checks? The downside is that we would have to then replicate all this 
code to assign buswidth everywhere, including in spi_nor_read_sr() and 
spi_nor_read_fsr() adding to more boilerplate.

If we change anything about spi-mem ops in the future we would then 
again have to hunt and peck all places where we create spi-mem ops and 
update them.

For example, I was recently experimenting with a mechanism to tell 
controllers the maximum supported frequency for an op (xSPI says read 
SFDP should support at least 50MHz operation so we want to make sure 
controllers don't exceed that speed). A max speed of 0 would mean 
controllers can go as fast as they wish (how it is done currently). 
Having a central function to set up ops made it a 1 line change to set 
the speed to 0 for all ops, and then we can set it to 50MHz for read 
SFDP. The same thing without it would have me copying that line in 10-15 
places.

So unless there are any significant reasons to avoid having this, I 
think it is a good idea to keep it.

> > +               /*
> > +                * spi-mem supports mixed DTR modes, but right now we can only
> > +                * have all phases either DTR or STR. IOW, spi-mem can have
> nit: SPIMEM
> > +                * something like 4S-4D-4D, but spi-nor can't. So, set all 4
> nit: SPI NOR
> > +                * phases to either DTR or STR.
> > +                */
> > +               op->cmd.dtr = op->addr.dtr = op->dummy.dtr
> > +                              = op->data.dtr = true;
> > +
> > +               /* 2 bytes per clock cycle in DTR mode. */
> > +               op->dummy.nbytes *= 2;
> > +
> > +               ext = spi_nor_get_cmd_ext(nor, op);
> > +               op->cmd.opcode = (op->cmd.opcode << 8) | ext;
> > +               op->cmd.nbytes = 2;
> > +       }
> > +}
> > +
> >  /**
> >   * spi_nor_spimem_bounce() - check if a bounce buffer is needed for the data
> >   *                           transfer
> > @@ -104,14 +174,12 @@ static ssize_t spi_nor_spimem_read_data(struct spi_nor *nor, loff_t from,
> >         ssize_t nbytes;
> >         int error;
> > 
> > -       /* get transfer protocols. */
> > -       op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->read_proto);
> > -       op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->read_proto);
> > -       op.dummy.buswidth = op.addr.buswidth;
> > -       op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->read_proto);
> > +       spi_nor_spimem_setup_op(nor, &op, nor->read_proto);
> 
> Here we would keep the code as it were.
> > 
> >         /* convert the dummy cycles to the number of bytes */
> >         op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> > +       if (spi_nor_protocol_is_dtr(nor->read_proto))
> > +               op.dummy.nbytes *= 2;
> 
> And replace these 2 lines with:
> 	if (spi_nor_protocol_is_dtr(nor->read_proto))
> 		spi_nor_spimem_setup_dtr_op(nor, &op, nor->read_proto)
> > 
> >         usebouncebuf = spi_nor_spimem_bounce(nor, &op);
> > 
> > @@ -169,13 +237,11 @@ static ssize_t spi_nor_spimem_write_data(struct spi_nor *nor, loff_t to,
> >         ssize_t nbytes;
> >         int error;
> > 
> > -       op.cmd.buswidth = spi_nor_get_protocol_inst_nbits(nor->write_proto);
> > -       op.addr.buswidth = spi_nor_get_protocol_addr_nbits(nor->write_proto);
> > -       op.data.buswidth = spi_nor_get_protocol_data_nbits(nor->write_proto);
> > -
> >         if (nor->program_opcode == SPINOR_OP_AAI_WP && nor->sst_write_second)
> >                 op.addr.nbytes = 0;
> > 
> > +       spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> > +
> >         if (spi_nor_spimem_bounce(nor, &op))
> >                 memcpy(nor->bouncebuf, buf, op.data.nbytes);
> > 
> > @@ -227,10 +293,16 @@ int spi_nor_write_enable(struct spi_nor *nor)
> >                                    SPI_MEM_OP_NO_DUMMY,
> >                                    SPI_MEM_OP_NO_DATA);
> > 
> > +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> 
> For the reg operation we can get rid of the extra checks that were in
> spi_nor_spimem_setup_op and simply do:
> 
> 		if (spi_nor_protocol_is_dtr(proto))
> 			spi_nor_spimem_setup_dtr_op()
> 
> > +
> >                 ret = spi_mem_exec_op(nor->spimem, &op);
> >         } else {
> > -               ret = nor->controller_ops->write_reg(nor, SPINOR_OP_WREN,
> > -                                                    NULL, 0);
> > +               if (spi_nor_protocol_is_dtr(nor->reg_proto))
> > +                       ret = -ENOTSUPP;
> > +               else
> > +                       ret = nor->controller_ops->write_reg(nor,
> > +                                                            SPINOR_OP_WREN,
> > +                                                            NULL, 0);
> 
> Would you introduce helpers for the controller ops, like Boris
> did in the following patch?
> https://patchwork.ozlabs.org/project/linux-mtd/patch/20181012084825.23697-10-boris.brezillon@bootlin.com/
> 
> How about spi_nor_controller_ops_read_reg()
> and spi_nor_controller_ops_write_reg() instead?

It would get rid of the boilerplate so I think it is a good idea.

> cut
> 
> > @@ -1144,7 +1291,11 @@ static int spi_nor_erase_sector(struct spi_nor *nor, u32 addr)
> >                                    SPI_MEM_OP_NO_DUMMY,
> >                                    SPI_MEM_OP_NO_DATA);
> > 
> > +               spi_nor_spimem_setup_op(nor, &op, nor->write_proto);
> > +
> >                 return spi_mem_exec_op(nor->spimem, &op);
> > +       } else if (spi_nor_protocol_is_dtr(nor->write_proto)) {
> > +               return -ENOTSUPP;
> >         } else if (nor->controller_ops->erase) {
> >                 return nor->controller_ops->erase(nor, addr);
> >         }
> 
> here you would need a helper: spi_nor_controller_ops_erase()

Ok.
 
> cut
> 
> > @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
> >         struct spi_nor_flash_parameter *params = nor->params;
> >         unsigned int cap;
> > 
> > -       /* DTR modes are not supported yet, mask them all. */
> > -       *hwcaps &= ~SNOR_HWCAPS_DTR;
> > -
> >         /* X-X-X modes are not supported yet, mask them all. */
> >         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
> > 
> > +       /*
> > +        * If the reset line is broken, we do not want to enter a stateful
> > +        * mode.
> > +        */
> > +       if (nor->flags & SNOR_F_BROKEN_RESET)
> > +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
> 
> A dedicated reset line is not enough for flashes that keep their state
> in non-volatile bits. Since we can't protect from unexpected crashes in
> the non volatile state case, we should enter these modes only with an
> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
> or something similar.

I wrote this patch with the assumption that we won't be supporting 
non-volatile configuration as of now. In the previous discussions we 
came to the conclusion that it is not easy to detect the flash if it 
boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile 
state, the flash would be useless after a reboot because we won't be 
able to detect it in 8D mode. It doesn't matter if the reset line is 
connected or not because it will reset the flash to the non-volatile 
state, and we can't detect it from the non-volatile state.

> For the volatile state case, we can parse the SFDP SCCR map, save if we
> can enter stateful modes in a volatile way, and if yes allow the entering.

If we are not support volatile configurations, the reset line is enough 
to take care of unexpected resets, no? I don't see any need to parse 
SCCR map just for this.

> Do the flashes that you played with define the SFDP SCCR map?

FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does 
not.
 
> > +
> >         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
> >                 int rdidx, ppidx;
> > 
> > @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
> >                  * controller directly implements the spi_nor interface.
> >                  * Yet another reason to switch to spi-mem.
> >                  */
> > -               ignored_mask = SNOR_HWCAPS_X_X_X;
> > +               ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
> >                 if (shared_mask & ignored_mask) {
> >                         dev_dbg(nor->dev,
> >                                 "SPI n-n-n protocols are not supported.\n");
> > @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
> >                                           SNOR_PROTO_1_1_8);
> >         }
> > 
> > +       if (info->flags & SPI_NOR_OCTAL_DTR_READ) {
> 
> Why do we need this flag? Can't we determine if the flash supports
> octal DTR by parsing SFDP?

For Cypress S28HS512T, we can since it is xSPI compliant. We can't do 
that for Micron MT35XU512ABA since it is not xSPI compliant.

> > +               params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> > +               spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> > +                                         0, 20, SPINOR_OP_READ_FAST,
> > +                                         SNOR_PROTO_8_8_8_DTR);
> > +       }
> > +
> >         /* Page Program settings. */
> >         params->hwcaps.mask |= SNOR_HWCAPS_PP;
> >         spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
> >                                 SPINOR_OP_PP, SNOR_PROTO_1_1_1);
> > 
> > +       /*
> > +        * Since xSPI Page Program opcode is backward compatible with
> > +        * Legacy SPI, use Legacy SPI opcode there as well.
> > +        */
> > +       spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
> > +                               SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
> > +
> 
> This looks fishy. You haven't updated the hwcaps.mask, these pp settings never
> get selected?

The problem here is that I don't see any field/table in SFDP that can 
tell us {if,which} 8D-8D-8D program commands are supported. The xSPI 
spec says that "The program commands provide SPI backward compatible 
commands for programming data...".

So we populate the 8D page program opcodes here (and in 4bait parsing) 
using the 1S opcodes. The flashes have to enable the hwcap in fixup 
hooks.

As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag 
that can enable the hwcap here? Thoughts?

> >         /*
> >          * Sector Erase settings. Sort Erase Types in ascending order, with the
> >          * smallest erase size starting at BIT(0).
> > @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
> > 
> >         spi_nor_manufacturer_init_params(nor);
> > 
> > -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> > +       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> > +                                SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
> >             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> >                 spi_nor_sfdp_init_params(nor);
> > 
> > @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
> >                 return err;
> >         }
> > 
> > -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> > +       if (nor->addr_width == 4 &&
> > +           !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&
> 
> Why is the Octal DTR read exempted?

It is based on the assumption explained below that 8D mode will always 
use 4-byte addresses so we don't need to explicitly enable 8D mode. 
Although I think maybe we should exempt all flashes that support DTR 
mode?

> > +           !(nor->flags & SNOR_F_4B_OPCODES)) {
> >                 /*
> >                  * If the RESET# pin isn't hooked up properly, or the system
> >                  * otherwise doesn't perform a reset command in the boot
> > @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
> >  {
> >         if (nor->addr_width) {
> >                 /* already configured from SFDP */
> > +       } else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
> > +                /* Always use 4-byte addresses in DTR mode. */
> > +               nor->addr_width = 4;
> 
> Why? DTR with 3 byte addr width should be possible too.

Should it be? What would happen to the half cycle left over? Do we then 
start the dummy phase in the middle of the cycle? We would also have to 
start the data phase in the middle of a cycle as well and end the 
transaction with half a cycle left over.

AFAIK, the controller I tested with (Cadence QSPI) does not support 
this. Similarly, the two flashes this series adds support for, Cypress 
S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D 
mode. I'm not sure if there are any flashes or controllers that do.
 
> >         } else if (nor->info->addr_width) {
> >                 nor->addr_width = nor->info->addr_width;
> >         } else if (nor->mtd.size > 0x1000000) {
> 
> Cheers,
> ta

[0] https://lore.kernel.org/linux-mtd/20200228093658.zc3uifqg4zruokq3@ti.com/

-- 
Regards,
Pratyush Yadav

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-07-21 11:29     ` Pratyush Yadav
@ 2020-09-29 15:42       ` Tudor.Ambarus
  2020-09-29 16:29         ` Pratyush Yadav
  2020-09-29 16:57         ` Vignesh Raghavendra
  0 siblings, 2 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-09-29 15:42 UTC (permalink / raw)
  To: me
  Cc: alexandre.belloni, vigneshr, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel

Hi, Pratyush,

I'm replying to v10 so that we continue the discussion, but this applies to v13 as well.

On 7/21/20 2:29 PM, Pratyush Yadav wrote:

>>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>>>         struct spi_nor_flash_parameter *params = nor->params;
>>>         unsigned int cap;
>>>
>>> -       /* DTR modes are not supported yet, mask them all. */
>>> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
>>> -
>>>         /* X-X-X modes are not supported yet, mask them all. */
>>>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
>>>
>>> +       /*
>>> +        * If the reset line is broken, we do not want to enter a stateful
>>> +        * mode.
>>> +        */
>>> +       if (nor->flags & SNOR_F_BROKEN_RESET)
>>> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
>>
>> A dedicated reset line is not enough for flashes that keep their state
>> in non-volatile bits. Since we can't protect from unexpected crashes in
>> the non volatile state case, we should enter these modes only with an
>> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
>> or something similar.
> 
> I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we

I think we have to take care of the stateful flashes now, otherwise we'll risk
to end up with users that let their flashes in a mode from which they can't recover.
I made some small RFC patches in reply to your v13, let me know what you think.

> came to the conclusion that it is not easy to detect the flash if it
> boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile
> state, the flash would be useless after a reboot because we won't be
> able to detect it in 8D mode. It doesn't matter if the reset line is
> connected or not because it will reset the flash to the non-volatile
> state, and we can't detect it from the non-volatile state.

correct, so a reset line for stateful modes doesn't help and the comment from the
code should be updated. s/stateful/stateless
> 
>> For the volatile state case, we can parse the SFDP SCCR map, save if we
>> can enter stateful modes in a volatile way, and if yes allow the entering.
> 
> If we are not support volatile configurations, the reset line is enough
> to take care of unexpected resets, no? I don't see any need to parse

the reset line is excellent for the stateless flashes, it guarantees that the
volatile bits will return to their default state. Disabling the clock, waiting
for a period and re-enabling it again should do the trick too, but probably
a dedicated reset line is safer.

> SCCR map just for this.

This fits the RFC that I sent to your v13. We need to parse as much as we can
from the SFDP tables so that we don't abuse the flash info flags.

> 
>> Do the flashes that you played with define the SFDP SCCR map?
> 
> FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does
> not.
> 
>>> +
>>>         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
>>>                 int rdidx, ppidx;
>>>
>>> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
>>>                  * controller directly implements the spi_nor interface.
>>>                  * Yet another reason to switch to spi-mem.
>>>                  */
>>> -               ignored_mask = SNOR_HWCAPS_X_X_X;
>>> +               ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
>>>                 if (shared_mask & ignored_mask) {
>>>                         dev_dbg(nor->dev,
>>>                                 "SPI n-n-n protocols are not supported.\n");
>>> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>>>                                           SNOR_PROTO_1_1_8);
>>>         }
>>>
>>> +       if (info->flags & SPI_NOR_OCTAL_DTR_READ) {
>>
>> Why do we need this flag? Can't we determine if the flash supports
>> octal DTR by parsing SFDP?
> 
> For Cypress S28HS512T, we can since it is xSPI compliant. We can't do
> that for Micron MT35XU512ABA since it is not xSPI compliant.

Ok

> 
>>> +               params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
>>> +               spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
>>> +                                         0, 20, SPINOR_OP_READ_FAST,
>>> +                                         SNOR_PROTO_8_8_8_DTR);
>>> +       }
>>> +
>>>         /* Page Program settings. */
>>>         params->hwcaps.mask |= SNOR_HWCAPS_PP;
>>>         spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
>>>                                 SPINOR_OP_PP, SNOR_PROTO_1_1_1);
>>>
>>> +       /*
>>> +        * Since xSPI Page Program opcode is backward compatible with
>>> +        * Legacy SPI, use Legacy SPI opcode there as well.
>>> +        */
>>> +       spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
>>> +                               SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
>>> +
>>
>> This looks fishy. You haven't updated the hwcaps.mask, these pp settings never
>> get selected?
> 
> The problem here is that I don't see any field/table in SFDP that can
> tell us {if,which} 8D-8D-8D program commands are supported. The xSPI
> spec says that "The program commands provide SPI backward compatible
> commands for programming data...".
> 
> So we populate the 8D page program opcodes here (and in 4bait parsing)
> using the 1S opcodes. The flashes have to enable the hwcap in fixup
> hooks.

I see. Would be good if you write this description as a comment, or in the commit
message.

> 
> As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag
> that can enable the hwcap here? Thoughts?

Should be fine the way that you did. We can change this later on if needed.

> 
>>>         /*
>>>          * Sector Erase settings. Sort Erase Types in ascending order, with the
>>>          * smallest erase size starting at BIT(0).
>>> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>>
>>>         spi_nor_manufacturer_init_params(nor);
>>>
>>> -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
>>> +       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>>> +                                SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
>>>             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>>>                 spi_nor_sfdp_init_params(nor);
>>>
>>> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
>>>                 return err;
>>>         }
>>>
>>> -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
>>> +       if (nor->addr_width == 4 &&
>>> +           !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&
>>
>> Why is the Octal DTR read exempted?
> 
> It is based on the assumption explained below that 8D mode will always
> use 4-byte addresses so we don't need to explicitly enable 8D mode.
> Although I think maybe we should exempt all flashes that support DTR
> mode?

4-4-4-dtr can work with 3-byte addresses, check MX25L25673G. 2-2-2-dtr should work too.


> 
>>> +           !(nor->flags & SNOR_F_4B_OPCODES)) {
>>>                 /*
>>>                  * If the RESET# pin isn't hooked up properly, or the system
>>>                  * otherwise doesn't perform a reset command in the boot
>>> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
>>>  {
>>>         if (nor->addr_width) {
>>>                 /* already configured from SFDP */
>>> +       } else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
>>> +                /* Always use 4-byte addresses in DTR mode. */
>>> +               nor->addr_width = 4;
>>
>> Why? DTR with 3 byte addr width should be possible too.
> 
> Should it be? What would happen to the half cycle left over? Do we then
> start the dummy phase in the middle of the cycle? We would also have to
> start the data phase in the middle of a cycle as well and end the
> transaction with half a cycle left over.
> 
> AFAIK, the controller I tested with (Cadence QSPI) does not support
> this. Similarly, the two flashes this series adds support for, Cypress
> S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D
> mode. I'm not sure if there are any flashes or controllers that do.

how about conditioning this only for 8-8-8-dtr?

I'll now jump to v13 and continue the review there.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-09-29 15:42       ` Tudor.Ambarus
@ 2020-09-29 16:29         ` Pratyush Yadav
  2020-09-29 18:34           ` Tudor.Ambarus
  2020-09-29 16:57         ` Vignesh Raghavendra
  1 sibling, 1 reply; 43+ messages in thread
From: Pratyush Yadav @ 2020-09-29 16:29 UTC (permalink / raw)
  To: Tudor.Ambarus
  Cc: alexandre.belloni, vigneshr, richard, me, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, linux-kernel

On 29/09/20 03:42PM, Tudor.Ambarus@microchip.com wrote:
> Hi, Pratyush,
> 
> I'm replying to v10 so that we continue the discussion, but this applies to v13 as well.
> 
> On 7/21/20 2:29 PM, Pratyush Yadav wrote:
> 
> >>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
> >>>         struct spi_nor_flash_parameter *params = nor->params;
> >>>         unsigned int cap;
> >>>
> >>> -       /* DTR modes are not supported yet, mask them all. */
> >>> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
> >>> -
> >>>         /* X-X-X modes are not supported yet, mask them all. */
> >>>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
> >>>
> >>> +       /*
> >>> +        * If the reset line is broken, we do not want to enter a stateful
> >>> +        * mode.
> >>> +        */
> >>> +       if (nor->flags & SNOR_F_BROKEN_RESET)
> >>> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
> >>
> >> A dedicated reset line is not enough for flashes that keep their state
> >> in non-volatile bits. Since we can't protect from unexpected crashes in
> >> the non volatile state case, we should enter these modes only with an
> >> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
> >> or something similar.
> > 
> > I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we
> 
> I think we have to take care of the stateful flashes now, otherwise we'll risk
> to end up with users that let their flashes in a mode from which they can't recover.
> I made some small RFC patches in reply to your v13, let me know what you think.

I haven't gone through them yet. Will check tomorrow.
 
> > came to the conclusion that it is not easy to detect the flash if it
> > boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile
> > state, the flash would be useless after a reboot because we won't be
> > able to detect it in 8D mode. It doesn't matter if the reset line is
> > connected or not because it will reset the flash to the non-volatile
> > state, and we can't detect it from the non-volatile state.
> 
> correct, so a reset line for stateful modes doesn't help and the comment from the
> code should be updated. s/stateful/stateless

We are talking about two different kinds of "state" here. The state you 
are talking about is the persistent state of the flash configured via 
non-volatile registers. Yes, a reset line doesn't help in that case at 
all.

The other state is the non-persistent state we set on the flash. Using 
1S-1S-8D mode is stateless in the sense that we didn't change any state 
on the flash to be able to use this mode, and only had to use the 
correct opcode. If we execute a 1S-1S-1S command next it will also work 
because the flash is still interpreting opcodes in 1S mode. Using 
8D-8D-8D or 4S-4S-4S mode is stateful because we did have to configure 
some state on the flash (which can very well be volatile). Once 8D-8D-8D 
or 4S-4S-4S mode is entered, we cannot execute 1S-1S-1S commands until 
we reset the flash because now the flash is interpreting commands in 4S 
or 8D mode. This means we introduced some state on the flash.

Having a reset line will not help against the former but will help 
against the latter. If the flash is in a stateful mode like 8D-8D-8D 
without a reset line, an unexpected reset could leave bootloader unable 
to boot because it issues the commands in 1S-1S-1S mode that the flash 
cannot interpret. So even if the state we set is volatile, we still want 
to avoid doing it if there is no reset line.

So I think the code and comment should stay as they are.

> > 
> >> For the volatile state case, we can parse the SFDP SCCR map, save if we
> >> can enter stateful modes in a volatile way, and if yes allow the entering.
> > 
> > If we are not support volatile configurations, the reset line is enough
> > to take care of unexpected resets, no? I don't see any need to parse
> 
> the reset line is excellent for the stateless flashes, it guarantees that the
> volatile bits will return to their default state. Disabling the clock, waiting
> for a period and re-enabling it again should do the trick too, but probably
> a dedicated reset line is safer.
> 
> > SCCR map just for this.
> 
> This fits the RFC that I sent to your v13. We need to parse as much as we can
> from the SFDP tables so that we don't abuse the flash info flags.
> 
> > 
> >> Do the flashes that you played with define the SFDP SCCR map?
> > 
> > FWIW, the Cypress S28HS512T flash does but the Micron MT35XU512ABA does
> > not.
> > 
> >>> +
> >>>         for (cap = 0; cap < sizeof(*hwcaps) * BITS_PER_BYTE; cap++) {
> >>>                 int rdidx, ppidx;
> >>>
> >>> @@ -2628,7 +2781,7 @@ static int spi_nor_default_setup(struct spi_nor *nor,
> >>>                  * controller directly implements the spi_nor interface.
> >>>                  * Yet another reason to switch to spi-mem.
> >>>                  */
> >>> -               ignored_mask = SNOR_HWCAPS_X_X_X;
> >>> +               ignored_mask = SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR;
> >>>                 if (shared_mask & ignored_mask) {
> >>>                         dev_dbg(nor->dev,
> >>>                                 "SPI n-n-n protocols are not supported.\n");
> >>> @@ -2774,11 +2927,25 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
> >>>                                           SNOR_PROTO_1_1_8);
> >>>         }
> >>>
> >>> +       if (info->flags & SPI_NOR_OCTAL_DTR_READ) {
> >>
> >> Why do we need this flag? Can't we determine if the flash supports
> >> octal DTR by parsing SFDP?
> > 
> > For Cypress S28HS512T, we can since it is xSPI compliant. We can't do
> > that for Micron MT35XU512ABA since it is not xSPI compliant.
> 
> Ok
> 
> > 
> >>> +               params->hwcaps.mask |= SNOR_HWCAPS_READ_8_8_8_DTR;
> >>> +               spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> >>> +                                         0, 20, SPINOR_OP_READ_FAST,
> >>> +                                         SNOR_PROTO_8_8_8_DTR);
> >>> +       }
> >>> +
> >>>         /* Page Program settings. */
> >>>         params->hwcaps.mask |= SNOR_HWCAPS_PP;
> >>>         spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
> >>>                                 SPINOR_OP_PP, SNOR_PROTO_1_1_1);
> >>>
> >>> +       /*
> >>> +        * Since xSPI Page Program opcode is backward compatible with
> >>> +        * Legacy SPI, use Legacy SPI opcode there as well.
> >>> +        */
> >>> +       spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP_8_8_8_DTR],
> >>> +                               SPINOR_OP_PP, SNOR_PROTO_8_8_8_DTR);
> >>> +
> >>
> >> This looks fishy. You haven't updated the hwcaps.mask, these pp settings never
> >> get selected?
> > 
> > The problem here is that I don't see any field/table in SFDP that can
> > tell us {if,which} 8D-8D-8D program commands are supported. The xSPI
> > spec says that "The program commands provide SPI backward compatible
> > commands for programming data...".
> > 
> > So we populate the 8D page program opcodes here (and in 4bait parsing)
> > using the 1S opcodes. The flashes have to enable the hwcap in fixup
> > hooks.
> 
> I see. Would be good if you write this description as a comment, or in the commit
> message.
> 
> > 
> > As an alternative, maybe we can introduce the SPI_NOR_OCTAL_DTR_PP flag
> > that can enable the hwcap here? Thoughts?
> 
> Should be fine the way that you did. We can change this later on if needed.

I have already added it in v11 and later. Since it is already there I 
suppose it can stay.
 
> > 
> >>>         /*
> >>>          * Sector Erase settings. Sort Erase Types in ascending order, with the
> >>>          * smallest erase size starting at BIT(0).
> >>> @@ -2886,7 +3053,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
> >>>
> >>>         spi_nor_manufacturer_init_params(nor);
> >>>
> >>> -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> >>> +       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> >>> +                                SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
> >>>             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> >>>                 spi_nor_sfdp_init_params(nor);
> >>>
> >>> @@ -2948,7 +3116,9 @@ static int spi_nor_init(struct spi_nor *nor)
> >>>                 return err;
> >>>         }
> >>>
> >>> -       if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> >>> +       if (nor->addr_width == 4 &&
> >>> +           !(nor->info->flags & SPI_NOR_OCTAL_DTR_READ) &&
> >>
> >> Why is the Octal DTR read exempted?
> > 
> > It is based on the assumption explained below that 8D mode will always
> > use 4-byte addresses so we don't need to explicitly enable 8D mode.
> > Although I think maybe we should exempt all flashes that support DTR
> > mode?
> 
> 4-4-4-dtr can work with 3-byte addresses, check MX25L25673G. 2-2-2-dtr should work too.

Yes, I didn't catch this before.
 
> 
> > 
> >>> +           !(nor->flags & SNOR_F_4B_OPCODES)) {
> >>>                 /*
> >>>                  * If the RESET# pin isn't hooked up properly, or the system
> >>>                  * otherwise doesn't perform a reset command in the boot
> >>> @@ -3007,6 +3177,9 @@ static int spi_nor_set_addr_width(struct spi_nor *nor)
> >>>  {
> >>>         if (nor->addr_width) {
> >>>                 /* already configured from SFDP */
> >>> +       } else if (spi_nor_protocol_is_dtr(nor->read_proto)) {
> >>> +                /* Always use 4-byte addresses in DTR mode. */
> >>> +               nor->addr_width = 4;
> >>
> >> Why? DTR with 3 byte addr width should be possible too.
> > 
> > Should it be? What would happen to the half cycle left over? Do we then
> > start the dummy phase in the middle of the cycle? We would also have to
> > start the data phase in the middle of a cycle as well and end the
> > transaction with half a cycle left over.
> > 
> > AFAIK, the controller I tested with (Cadence QSPI) does not support
> > this. Similarly, the two flashes this series adds support for, Cypress
> > S28HS512T and Micron MT35XU512ABA, don't support 3-byte address in 8D
> > mode. I'm not sure if there are any flashes or controllers that do.
> 
> how about conditioning this only for 8-8-8-dtr?

Yes, it should only apply for 8D-8D-8D. Will fix.
 
> I'll now jump to v13 and continue the review there.

-- 
Regards,
Pratyush Yadav
Texas Instruments India

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-09-29 15:42       ` Tudor.Ambarus
  2020-09-29 16:29         ` Pratyush Yadav
@ 2020-09-29 16:57         ` Vignesh Raghavendra
  1 sibling, 0 replies; 43+ messages in thread
From: Vignesh Raghavendra @ 2020-09-29 16:57 UTC (permalink / raw)
  To: Tudor.Ambarus, me
  Cc: alexandre.belloni, richard, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, p.yadav, linux-kernel



On 9/29/20 9:12 PM, Tudor.Ambarus@microchip.com wrote:
> Hi, Pratyush,
> 
> I'm replying to v10 so that we continue the discussion, but this applies to v13 as well.
> 
> On 7/21/20 2:29 PM, Pratyush Yadav wrote:
> 
>>>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>>>>         struct spi_nor_flash_parameter *params = nor->params;
>>>>         unsigned int cap;
>>>>
>>>> -       /* DTR modes are not supported yet, mask them all. */
>>>> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
>>>> -
>>>>         /* X-X-X modes are not supported yet, mask them all. */
>>>>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
>>>>
>>>> +       /*
>>>> +        * If the reset line is broken, we do not want to enter a stateful
>>>> +        * mode.
>>>> +        */
>>>> +       if (nor->flags & SNOR_F_BROKEN_RESET)
>>>> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
>>>
>>> A dedicated reset line is not enough for flashes that keep their state
>>> in non-volatile bits. Since we can't protect from unexpected crashes in
>>> the non volatile state case, we should enter these modes only with an
>>> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
>>> or something similar.
>>
>> I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we
> 
> I think we have to take care of the stateful flashes now, otherwise we'll risk
> to end up with users that let their flashes in a mode from which they can't recover.
> I made some small RFC patches in reply to your v13, let me know what you think.
> 
>> came to the conclusion that it is not easy to detect the flash if it
>> boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile
>> state, the flash would be useless after a reboot because we won't be
>> able to detect it in 8D mode. It doesn't matter if the reset line is
>> connected or not because it will reset the flash to the non-volatile
>> state, and we can't detect it from the non-volatile state.
> 
> correct, so a reset line for stateful modes doesn't help and the comment from the
> code should be updated. s/stateful/stateless

Entering an IO mode even using volatile bits (which gets cleared on SW
or HW reset) creates a "state" that SW needs to keep track of which is
why "stateful" term is used. I think that's what is implied here

AFAIU, Boris's original RFC[1] introducing X-X-X mode also used
"stateful mode" term in the same context .


>>
>>> For the volatile state case, we can parse the SFDP SCCR map, save if we
>>> can enter stateful modes in a volatile way, and if yes allow the entering.
>>
>> If we are not support volatile configurations, the reset line is enough
>> to take care of unexpected resets, no? I don't see any need to parse
> 
> the reset line is excellent for the stateless flashes, it guarantees that the
> volatile bits will return to their default state. Disabling the clock, waiting
> for a period and re-enabling it again should do the trick too, but probably
> a dedicated reset line is safer.
> 

I don't think disable-wait-enable sequence of clock input to flash will
trigger a reset... You have to take down the power and thus force flash
to go through power-down/power-up sequence or use HW or SW reset sequences

[1]
https://patchwork.ozlabs.org/project/linux-mtd/cover/20181012084825.23697-1-boris.brezillon@bootlin.com/

Regards
Vignesh

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

* Re: [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol
  2020-09-29 16:29         ` Pratyush Yadav
@ 2020-09-29 18:34           ` Tudor.Ambarus
  0 siblings, 0 replies; 43+ messages in thread
From: Tudor.Ambarus @ 2020-09-29 18:34 UTC (permalink / raw)
  To: p.yadav
  Cc: alexandre.belloni, vigneshr, richard, me, nsekhar, Nicolas.Ferre,
	boris.brezillon, michal.simek, Ludovic.Desroches, broonie,
	linux-mtd, linux-arm-kernel, miquel.raynal, matthias.bgg,
	linux-mediatek, linux-spi, linux-kernel

On 9/29/20 7:29 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 29/09/20 03:42PM, Tudor.Ambarus@microchip.com wrote:
>> Hi, Pratyush,
>>
>> I'm replying to v10 so that we continue the discussion, but this applies to v13 as well.
>>
>> On 7/21/20 2:29 PM, Pratyush Yadav wrote:
>>
>>>>> @@ -2368,12 +2517,16 @@ spi_nor_spimem_adjust_hwcaps(struct spi_nor *nor, u32 *hwcaps)
>>>>>         struct spi_nor_flash_parameter *params = nor->params;
>>>>>         unsigned int cap;
>>>>>
>>>>> -       /* DTR modes are not supported yet, mask them all. */
>>>>> -       *hwcaps &= ~SNOR_HWCAPS_DTR;
>>>>> -
>>>>>         /* X-X-X modes are not supported yet, mask them all. */
>>>>>         *hwcaps &= ~SNOR_HWCAPS_X_X_X;
>>>>>
>>>>> +       /*
>>>>> +        * If the reset line is broken, we do not want to enter a stateful
>>>>> +        * mode.
>>>>> +        */
>>>>> +       if (nor->flags & SNOR_F_BROKEN_RESET)
>>>>> +               *hwcaps &= ~(SNOR_HWCAPS_X_X_X | SNOR_HWCAPS_X_X_X_DTR);
>>>>
>>>> A dedicated reset line is not enough for flashes that keep their state
>>>> in non-volatile bits. Since we can't protect from unexpected crashes in
>>>> the non volatile state case, we should enter these modes only with an
>>>> explicit request, i.e. an optional DT property: "update-nonvolatile-state",
>>>> or something similar.
>>>
>>> I wrote this patch with the assumption that we won't be supporting> non-volatile configuration as of now. In the previous discussions we
>>
>> I think we have to take care of the stateful flashes now, otherwise we'll risk
>> to end up with users that let their flashes in a mode from which they can't recover.
>> I made some small RFC patches in reply to your v13, let me know what you think.
> 
> I haven't gone through them yet. Will check tomorrow.
> 
>>> came to the conclusion that it is not easy to detect the flash if it
>>> boots in any mode other than 1S-1S-1S [0]. So if we update non-volatile
>>> state, the flash would be useless after a reboot because we won't be
>>> able to detect it in 8D mode. It doesn't matter if the reset line is
>>> connected or not because it will reset the flash to the non-volatile
>>> state, and we can't detect it from the non-volatile state.
>>
>> correct, so a reset line for stateful modes doesn't help and the comment from the
>> code should be updated. s/stateful/stateless
> 
> We are talking about two different kinds of "state" here. The state you

Right, I used 'stateful' for flashes that enter in a X-X-X mode by setting a
non-volatile bit and 'stateless' for those that enter in a X-X-X mode
via volatile bits.

> are talking about is the persistent state of the flash configured via
> non-volatile registers. Yes, a reset line doesn't help in that case at
> all.
> > The other state is the non-persistent state we set on the flash. Using
> 1S-1S-8D mode is stateless in the sense that we didn't change any state
> on the flash to be able to use this mode, and only had to use the
> correct opcode. If we execute a 1S-1S-1S command next it will also work
> because the flash is still interpreting opcodes in 1S mode. Using
> 8D-8D-8D or 4S-4S-4S mode is stateful because we did have to configure
> some state on the flash (which can very well be volatile). Once 8D-8D-8D
> or 4S-4S-4S mode is entered, we cannot execute 1S-1S-1S commands until
> we reset the flash because now the flash is interpreting commands in 4S
> or 8D mode. This means we introduced some state on the flash.
> 
> Having a reset line will not help against the former but will help
> against the latter. If the flash is in a stateful mode like 8D-8D-8D
> without a reset line, an unexpected reset could leave bootloader unable
> to boot because it issues the commands in 1S-1S-1S mode that the flash
> cannot interpret. So even if the state we set is volatile, we still want
> to avoid doing it if there is no reset line.
> 
> So I think the code and comment should stay as they are.

Ok. Cheers,
ta
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 43+ messages in thread

end of thread, other threads:[~2020-09-29 18:35 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-23 18:30 [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 01/17] spi: spi-mem: allow specifying whether an op is DTR or not Pratyush Yadav
2020-07-08 16:22   ` Tudor.Ambarus
2020-07-13  3:55   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 02/17] spi: spi-mem: allow specifying a command's extension Pratyush Yadav
2020-07-13  6:15   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 03/17] spi: atmel-quadspi: reject DTR ops Pratyush Yadav
2020-07-13  6:19   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 04/17] spi: spi-mtk-nor: " Pratyush Yadav
2020-07-13  6:24   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 05/17] mtd: spi-nor: add support for DTR protocol Pratyush Yadav
2020-07-07 17:37   ` Tudor.Ambarus
2020-07-21 11:29     ` Pratyush Yadav
2020-09-29 15:42       ` Tudor.Ambarus
2020-09-29 16:29         ` Pratyush Yadav
2020-09-29 18:34           ` Tudor.Ambarus
2020-09-29 16:57         ` Vignesh Raghavendra
2020-06-23 18:30 ` [PATCH v10 06/17] mtd: spi-nor: sfdp: get command opcode extension type from BFPT Pratyush Yadav
2020-07-07 17:53   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table Pratyush Yadav
2020-07-08 16:01   ` Tudor.Ambarus
2020-07-20 16:38     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 08/17] mtd: spi-nor: core: use dummy cycle and address width info from SFDP Pratyush Yadav
2020-07-08 16:03   ` Tudor.Ambarus
2020-07-20 16:24     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 09/17] mtd: spi-nor: core: do 2 byte reads for SR and FSR in DTR mode Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 10/17] mtd: spi-nor: core: enable octal DTR mode when possible Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 11/17] mtd: spi-nor: sfdp: do not make invalid quad enable fatal Pratyush Yadav
2020-07-13  9:33   ` Tudor.Ambarus
2020-06-23 18:30 ` [PATCH v10 12/17] mtd: spi-nor: sfdp: detect Soft Reset sequence support from BFPT Pratyush Yadav
2020-07-08 16:08   ` Tudor.Ambarus
2020-07-20 16:21     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 13/17] mtd: spi-nor: core: perform a Soft Reset on shutdown Pratyush Yadav
2020-07-08 16:10   ` Tudor.Ambarus
2020-07-20 16:11     ` Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 14/17] mtd: spi-nor: core: disable Octal DTR mode on suspend Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 15/17] mtd: spi-nor: core: expose spi_nor_default_setup() in core.h Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 16/17] mtd: spi-nor: spansion: add support for Cypress Semper flash Pratyush Yadav
2020-06-23 18:30 ` [PATCH v10 17/17] mtd: spi-nor: micron-st: allow using MT35XU512ABA in Octal DTR mode Pratyush Yadav
2020-07-13  6:34 ` [PATCH v10 00/17] mtd: spi-nor: add xSPI Octal DTR support Tudor.Ambarus
2020-07-14 19:19   ` Mark Brown
2020-07-15  3:40     ` Tudor.Ambarus
2020-07-14 16:40 ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).