[PATCH v7 00/14] External ECC engines & Macronix support

[PATCH v7 00/14] External ECC engines & Macronix support

[Miquel Raynal]

Hello all,

As I received the last missing ack from Rob for the first halve of the
series (bindings + ECC part) I've applied the beginning of the series
in a branch named spi-mem-ecc on the MTD korg repository, hence now I am
resending only the second halve, including spi-mem and spi changes,
mostly. When that second part will be validated, I will apply it on top
of the spi-mem-ecc branch and set an immutable tag, shared with the spi
tree.

Cheers,
Miquèl

Changes in v7:
* Added a macro to check if the caps are present or not before accessing
  them. This allows for optional caps.
* Dropped the 'no-caps' instance created in v6.
* Reworked a bit all the patches using these caps to have a nice and
  bisectable series, like adding missing static keywords.

Changes in v6:
* Re-include the first patches because a few things have changed in the
  bindings. These are only style changes as Rob asked to group every
  property above or below the description field, which I applied to all
  the binding commits, but without any further update.
* Created a spi-mem capabilities structure. Put that one in the spi-mem
  ops strucure and ensured that all the controllers provided one.
* Created a default "no-caps" empty instance that controller drivers can
  point to by default.
* Dropped the spi_mem_generic_defaults_op() intermediate helper entirely
  (not needed anymore).

Changes in v5:
* Moved a helper in the core as it seems that it will be useful for
  other ECC engines as well (Xiangsheng Hou for Mediatek will need it).
* Changed the parameters of the spi_mem_generic_supports_op() function
  in order to take a structure as input instead of a list of arguments,
  which will be much easier to complement in the future if ever needed.

Changes in v4:
* The first half of the series has been left aside (all the binding
  changes + the external mode in the Macronix driver), now let's focus
  on the pipelined mode.
* Added the ecc_en spi_mem_op structure parameter in a dedicated commit.
* Introduced a new helper for supporting generically the supported ops.
* Used this new helper in the macronix driver.
* By default all the other drivers would refuse a spi_mem_op with ecc_en
  enabled.

Changes in v3:
* Added Mark's R-by.
* Added a commit changing the initialization order between the dirmaps
  and the ECC engine so that the core might now if we are using a
  pipelined engine or not.
* Stopped creating additional dirmaps with ECC if the engine is not a
  pipelined engine.
* Solved the kernel test robot reports. In particular, I added a
  dependency on MTD_NAND_ECC to Macronix SPI controller driver.
* Added a patch to clean the NAND controller yaml file before moving
  some bits to nand-chip.yaml. This addresses the comments made by Rob
  about the useless allOf's.
* Used platform_get_irq_byname_optional() in order to avoid useless
  warnings when there is no IRQ.

Changes in v2:
* Fixed the bindings and added Rob's acks when relevant.
* Added locking in the ECC engine driver.
* Brought more changes in the core in order to bring the ECC information
  into the spi_mem_op structure with the idea of avoiding any races
  between parallel calls on the same engine.
* Reorganized the ECC driver entirely in order to have a per-engine mxic
  structure plus a per-NAND context. This lead to a number of changes
  internally which cannot all be listed.

Changes since the RFC:
* Rebased on top of v5.15-rc1.
* Fixed the dirmap configuration.
* Added the various tags received.
* Fixed the bindings as reported by the robots.
* Fixed the return value of the helper counting bitflips.
* Included a fix from Jaime Liao in the external pattern logic.
* Added the yaml conversion of Macronix SPI controller description.
* Added the yaml conversion of the SPI-NAND description.
* Created a nand-chip.yaml file to share properties between SPI-NAND and
  raw NAND.

Miquel Raynal (14):
  spi: spi-mem: Fix a DTR related check in spi_mem_dtr_supports_op()
  spi: spi-mem: Introduce a capability structure
  spi: spi-mem: Check the controller extra capabilities
  spi: cadence: Provide a capability structure
  spi: mxic: Provide a capability structure
  spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper
  spi: spi-mem: Add an ecc_en parameter to the spi_mem_op structure
  mtd: spinand: Delay a little bit the dirmap creation
  mtd: spinand: Create direct mapping descriptors for ECC operations
  spi: mxic: Fix the transmit path
  spi: mxic: Create a helper to configure the controller before an
    operation
  spi: mxic: Create a helper to ease the start of an operation
  spi: mxic: Add support for direct mapping
  spi: mxic: Add support for pipelined ECC operations

 drivers/mtd/nand/spi/core.c       |  51 ++++-
 drivers/spi/Kconfig               |   2 +-
 drivers/spi/spi-cadence-quadspi.c |  10 +-
 drivers/spi/spi-mem.c             |  32 +--
 drivers/spi/spi-mxic.c            | 340 ++++++++++++++++++++++++------
 include/linux/mtd/spinand.h       |   2 +
 include/linux/spi/spi-mem.h       |  29 ++-
 7 files changed, 366 insertions(+), 100 deletions(-)

-- 
2.27.0

[PATCH v7 01/14] spi: spi-mem: Fix a DTR related check in spi_mem_dtr_supports_op()

[Miquel Raynal]

It seems that the number of command bytes must be "2" only when the
command itself is sent in DTR mode. The current logic checks if the
number of command bytes is "2" when any of the cycles is a DTR cycle. It
is likely that so far no device was actually mixing DTR/non-DTR cycles
in the same operation, explaining why this was left undetected until
now.

Fixes: 539cf68cd51b ("spi: spi-mem: add spi_mem_dtr_supports_op()")
Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 37f4443ce9a0..c4da0c9b05e9 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -163,7 +163,7 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
 bool spi_mem_dtr_supports_op(struct spi_mem *mem,
 			     const struct spi_mem_op *op)
 {
-	if (op->cmd.nbytes != 2)
+	if (op->cmd.dtr && op->cmd.nbytes != 2)
 		return false;
 
 	return spi_mem_check_buswidth(mem, op);
-- 
2.27.0

[PATCH v7 02/14] spi: spi-mem: Introduce a capability structure

[Miquel Raynal]

Create a spi_controller_mem_caps structure and put it within the
spi_controller_mem_ops structure as these are highly related. So far the
only field in this structure is the support for dtr operations, but soon
we will add another parameter.

Also create a helper to parse the capabilities and check if the
requested capability has been set or not.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/spi/spi-mem.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 85e2ff7b840d..045ecb7c6f50 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -220,6 +220,17 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
 	return mem->drvpriv;
 }
 
+/**
+ * struct spi_controller_mem_caps - SPI memory controller capabilities
+ * @dtr: Supports DTR operations
+ */
+struct spi_controller_mem_caps {
+	bool dtr;
+};
+
+#define spi_mem_controller_is_capable(ctlr, cap)		\
+	((ctlr)->mem_ops->caps && (ctlr)->mem_ops->caps->cap)	\
+
 /**
  * struct spi_controller_mem_ops - SPI memory operations
  * @adjust_op_size: shrink the data xfer of an operation to match controller's
@@ -253,6 +264,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
  * @poll_status: poll memory device status until (status & mask) == match or
  *               when the timeout has expired. It fills the data buffer with
  *               the last status value.
+ * @caps: controller capabilities for the handling of the above operations.
  *
  * This interface should be implemented by SPI controllers providing an
  * high-level interface to execute SPI memory operation, which is usually the
@@ -283,6 +295,7 @@ struct spi_controller_mem_ops {
 			   unsigned long initial_delay_us,
 			   unsigned long polling_rate_us,
 			   unsigned long timeout_ms);
+	const struct spi_controller_mem_caps *caps;
 };
 
 /**
-- 
2.27.0

[PATCH v7 03/14] spi: spi-mem: Check the controller extra capabilities

[Miquel Raynal]

Controllers can now provide a spi-mem capabilities structure. Let's make
use of it in spi_mem_controller_default_supports_op(). As we want to
check for DTR operations as well as normal operations in a single
helper, let's pull the necessary checks from spi_mem_dtr_supports_op()
for now.

However, because no controller provide these extra capabilities, this
change has no effect so far.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mem.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index c4da0c9b05e9..677e54221ebc 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -173,11 +173,20 @@ EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
 bool spi_mem_default_supports_op(struct spi_mem *mem,
 				 const struct spi_mem_op *op)
 {
-	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
-		return false;
+	struct spi_controller *ctlr = mem->spi->controller;
+	bool op_is_dtr =
+		op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;
 
-	if (op->cmd.nbytes != 1)
-		return false;
+	if (op_is_dtr) {
+		if (!spi_mem_controller_is_capable(ctlr, dtr))
+			return false;
+
+		if (op->cmd.dtr && op->cmd.nbytes != 2)
+			return false;
+	} else {
+		if (op->cmd.nbytes != 1)
+			return false;
+	}
 
 	return spi_mem_check_buswidth(mem, op);
 }
-- 
2.27.0

[PATCH v7 04/14] spi: cadence: Provide a capability structure

[Miquel Raynal]

This controller has DTR support, so advertize it with a capability now
that the spi_controller_mem_ops structure contains this new field. This
will later be used by the core to discriminate whether an operation is
supported or not, in a more generic way than having different helpers.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 101cc71bffa7..98e0cc4236e3 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem)
 	return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select);
 }
 
+static const struct spi_controller_mem_caps cqspi_mem_caps = {
+	.dtr = true,
+};
+
 static const struct spi_controller_mem_ops cqspi_mem_ops = {
 	.exec_op = cqspi_exec_mem_op,
 	.get_name = cqspi_get_name,
 	.supports_op = cqspi_supports_mem_op,
+	.caps = &cqspi_mem_caps,
 };
 
 static int cqspi_setup_flash(struct cqspi_st *cqspi)
-- 
2.27.0

[PATCH v7 05/14] spi: mxic: Provide a capability structure

[Miquel Raynal]

This controller has DTR support, so advertize it with a capability now
that the spi_controller_mem_ops structure contains this new field. This
will later be used by the core to discriminate whether an operation is
supported or not, in a more generic way than having different helpers.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mxic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 45889947afed..e137b1ec85d4 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -443,9 +443,14 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	return ret;
 }
 
+static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
+	.dtr = true,
+};
+
 static const struct spi_controller_mem_ops mxic_spi_mem_ops = {
 	.supports_op = mxic_spi_mem_supports_op,
 	.exec_op = mxic_spi_mem_exec_op,
+	.caps = &mxic_spi_mem_caps,
 };
 
 static void mxic_spi_set_cs(struct spi_device *spi, bool lvl)
-- 
2.27.0

[PATCH v7 06/14] spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper

[Miquel Raynal]

Now that spi_mem_default_supports_op() has access to the static
controller capabilities (related to memory operations), and now that
these capabilities have been filled by the impacted controllers, there
is no need for a specific helper checking only DTR operations, so let's
just kill spi_mem_dtr_supports_op() and simply use
spi_mem_default_supports_op() instead.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-cadence-quadspi.c |  5 +----
 drivers/spi/spi-mem.c             | 10 ----------
 drivers/spi/spi-mxic.c            | 10 +---------
 include/linux/spi/spi-mem.h       | 11 -----------
 4 files changed, 2 insertions(+), 34 deletions(-)

diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
index 98e0cc4236e3..bc6b33bce0bb 100644
--- a/drivers/spi/spi-cadence-quadspi.c
+++ b/drivers/spi/spi-cadence-quadspi.c
@@ -1252,10 +1252,7 @@ static bool cqspi_supports_mem_op(struct spi_mem *mem,
 	if (!(all_true || all_false))
 		return false;
 
-	if (all_true)
-		return spi_mem_dtr_supports_op(mem, op);
-	else
-		return spi_mem_default_supports_op(mem, op);
+	return spi_mem_default_supports_op(mem, op);
 }
 
 static int cqspi_of_get_flash_pdata(struct platform_device *pdev,
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index 677e54221ebc..cfe1c99db5f3 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -160,16 +160,6 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem,
 	return true;
 }
 
-bool spi_mem_dtr_supports_op(struct spi_mem *mem,
-			     const struct spi_mem_op *op)
-{
-	if (op->cmd.dtr && op->cmd.nbytes != 2)
-		return false;
-
-	return spi_mem_check_buswidth(mem, op);
-}
-EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
-
 bool spi_mem_default_supports_op(struct spi_mem *mem,
 				 const struct spi_mem_op *op)
 {
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index e137b1ec85d4..67d05ee8d6a0 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
 static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 				     const struct spi_mem_op *op)
 {
-	bool all_false;
-
 	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
 	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
 		return false;
@@ -348,13 +346,7 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 	if (op->addr.nbytes > 7)
 		return false;
 
-	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
-		    !op->data.dtr;
-
-	if (all_false)
-		return spi_mem_default_supports_op(mem, op);
-	else
-		return spi_mem_dtr_supports_op(mem, op);
+	return spi_mem_default_supports_op(mem, op);
 }
 
 static int mxic_spi_mem_exec_op(struct spi_mem *mem,
diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 045ecb7c6f50..d7787c8f3746 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -332,10 +332,6 @@ void spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
 
 bool spi_mem_default_supports_op(struct spi_mem *mem,
 				 const struct spi_mem_op *op);
-
-bool spi_mem_dtr_supports_op(struct spi_mem *mem,
-			     const struct spi_mem_op *op);
-
 #else
 static inline int
 spi_controller_dma_map_mem_op_data(struct spi_controller *ctlr,
@@ -358,13 +354,6 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 {
 	return false;
 }
-
-static inline
-bool spi_mem_dtr_supports_op(struct spi_mem *mem,
-			     const struct spi_mem_op *op)
-{
-	return false;
-}
 #endif /* CONFIG_SPI_MEM */
 
 int spi_mem_adjust_op_size(struct spi_mem *mem, struct spi_mem_op *op);
-- 
2.27.0

[PATCH v7 07/14] spi: spi-mem: Add an ecc_en parameter to the spi_mem_op structure

[Miquel Raynal]

Soon the SPI-NAND core will need a way to request a SPI controller to
enable ECC support for a given operation. This is because of the
pipelined integration of certain ECC engines, which are directly managed
by the SPI controller itself.

Introduce a spi_mem_op additional field for this purpose: ecc_en.

So far this field is left unset and checked to be false by all
the SPI controller drivers in their ->supports_op() hook, as they all
call spi_mem_default_supports_op().

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-mem.c       | 5 +++++
 include/linux/spi/spi-mem.h | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index cfe1c99db5f3..94758e7e747d 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -178,6 +178,11 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
 			return false;
 	}
 
+	if (op->ecc_en) {
+		if (!spi_mem_controller_is_capable(ctlr, ecc))
+			return false;
+	}
+
 	return spi_mem_check_buswidth(mem, op);
 }
 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 d7787c8f3746..e9238a858109 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -94,6 +94,7 @@ enum spi_mem_data_dir {
  *		 operation does not involve transferring data
  * @data.buf.in: input buffer (must be DMA-able)
  * @data.buf.out: output buffer (must be DMA-able)
+ * @ecc_en: error correction is required
  */
 struct spi_mem_op {
 	struct {
@@ -126,6 +127,8 @@ struct spi_mem_op {
 			const void *out;
 		} buf;
 	} data;
+
+	bool ecc_en;
 };
 
 #define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\
@@ -223,9 +226,11 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
 /**
  * struct spi_controller_mem_caps - SPI memory controller capabilities
  * @dtr: Supports DTR operations
+ * @ecc: Supports operations with error correction
  */
 struct spi_controller_mem_caps {
 	bool dtr;
+	bool ecc;
 };
 
 #define spi_mem_controller_is_capable(ctlr, cap)		\
-- 
2.27.0

[PATCH v7 08/14] mtd: spinand: Delay a little bit the dirmap creation

[Miquel Raynal]

As we will soon tweak the dirmap creation to act a little bit
differently depending on the picked ECC engine, we need to initialize
dirmaps after ECC engines. This should not have any effect as dirmaps
are not yet used at this point.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/spi/core.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 7027c09925e2..715cad26fdef 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1209,14 +1209,6 @@ static int spinand_init(struct spinand_device *spinand)
 	if (ret)
 		goto err_free_bufs;
 
-	ret = spinand_create_dirmaps(spinand);
-	if (ret) {
-		dev_err(dev,
-			"Failed to create direct mappings for read/write operations (err = %d)\n",
-			ret);
-		goto err_manuf_cleanup;
-	}
-
 	ret = nanddev_init(nand, &spinand_ops, THIS_MODULE);
 	if (ret)
 		goto err_manuf_cleanup;
@@ -1251,6 +1243,14 @@ static int spinand_init(struct spinand_device *spinand)
 	mtd->ecc_strength = nanddev_get_ecc_conf(nand)->strength;
 	mtd->ecc_step_size = nanddev_get_ecc_conf(nand)->step_size;
 
+	ret = spinand_create_dirmaps(spinand);
+	if (ret) {
+		dev_err(dev,
+			"Failed to create direct mappings for read/write operations (err = %d)\n",
+			ret);
+		goto err_cleanup_ecc_engine;
+	}
+
 	return 0;
 
 err_cleanup_ecc_engine:
-- 
2.27.0

[PATCH v7 09/14] mtd: spinand: Create direct mapping descriptors for ECC operations

[Miquel Raynal]

In order for pipelined ECC engines to be able to enable/disable the ECC
engine only when needed and avoid races when future parallel-operations
will be supported, we need to provide the information about the use of
the ECC engine in the direct mapping hooks. As direct mapping
configurations are meant to be static, it is best to create two new
mappings: one for regular 'raw' accesses and one for accesses involving
correction. It is up to the driver to use or not the new ECC enable
boolean contained in the spi-mem operation.

As dirmaps are not free (they consume a few pages of MMIO address space)
and because these extra entries are only meant to be used by pipelined
engines, let's limit their use to this specific type of engine and save
a bit of memory with all the other setups.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/spi/core.c | 35 +++++++++++++++++++++++++++++++++--
 include/linux/mtd/spinand.h |  2 ++
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 715cad26fdef..eb999e47e978 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -381,7 +381,10 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
 		}
 	}
 
-	rdesc = spinand->dirmaps[req->pos.plane].rdesc;
+	if (req->mode == MTD_OPS_RAW)
+		rdesc = spinand->dirmaps[req->pos.plane].rdesc;
+	else
+		rdesc = spinand->dirmaps[req->pos.plane].rdesc_ecc;
 
 	while (nbytes) {
 		ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf);
@@ -452,7 +455,10 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
 			       req->ooblen);
 	}
 
-	wdesc = spinand->dirmaps[req->pos.plane].wdesc;
+	if (req->mode == MTD_OPS_RAW)
+		wdesc = spinand->dirmaps[req->pos.plane].wdesc;
+	else
+		wdesc = spinand->dirmaps[req->pos.plane].wdesc_ecc;
 
 	while (nbytes) {
 		ret = spi_mem_dirmap_write(wdesc, column, nbytes, buf);
@@ -866,6 +872,31 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
 
 	spinand->dirmaps[plane].rdesc = desc;
 
+	if (nand->ecc.engine->integration != NAND_ECC_ENGINE_INTEGRATION_PIPELINED) {
+		spinand->dirmaps[plane].wdesc_ecc = spinand->dirmaps[plane].wdesc;
+		spinand->dirmaps[plane].rdesc_ecc = spinand->dirmaps[plane].rdesc;
+
+		return 0;
+	}
+
+	info.op_tmpl = *spinand->op_templates.update_cache;
+	info.op_tmpl.ecc_en = true;
+	desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
+					  spinand->spimem, &info);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	spinand->dirmaps[plane].wdesc_ecc = desc;
+
+	info.op_tmpl = *spinand->op_templates.read_cache;
+	info.op_tmpl.ecc_en = true;
+	desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
+					  spinand->spimem, &info);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	spinand->dirmaps[plane].rdesc_ecc = desc;
+
 	return 0;
 }
 
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 6988956b8492..3aa28240a77f 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -389,6 +389,8 @@ struct spinand_info {
 struct spinand_dirmap {
 	struct spi_mem_dirmap_desc *wdesc;
 	struct spi_mem_dirmap_desc *rdesc;
+	struct spi_mem_dirmap_desc *wdesc_ecc;
+	struct spi_mem_dirmap_desc *rdesc_ecc;
 };
 
 /**
-- 
2.27.0

[PATCH v7 10/14] spi: mxic: Fix the transmit path

[Miquel Raynal]

By working with external hardware ECC engines, we figured out that
Under certain circumstances, it is needed for the SPI controller to
check INT_TX_EMPTY and INT_RX_NOT_EMPTY in both receive and transmit
path (not only in the receive path). The delay penalty being
negligible, move this code in the common path.

Fixes: b942d80b0a39 ("spi: Add MXIC controller driver")
Cc: stable@vger.kernel.org
Suggested-by: Mason Yang <masonccyang@mxic.com.tw>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Zhengxun Li <zhengxunli@mxic.com.tw>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mxic.c | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 67d05ee8d6a0..aedc9a144b82 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -304,25 +304,21 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
 
 		writel(data, mxic->regs + TXD(nbytes % 4));
 
+		ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
+		if (ret)
+			return ret;
+
+		ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+					 sts & INT_RX_NOT_EMPTY, 0,
+					 USEC_PER_SEC);
+		if (ret)
+			return ret;
+
+		data = readl(mxic->regs + RXD);
 		if (rxbuf) {
-			ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
-						 sts & INT_TX_EMPTY, 0,
-						 USEC_PER_SEC);
-			if (ret)
-				return ret;
-
-			ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
-						 sts & INT_RX_NOT_EMPTY, 0,
-						 USEC_PER_SEC);
-			if (ret)
-				return ret;
-
-			data = readl(mxic->regs + RXD);
 			data >>= (8 * (4 - nbytes));
 			memcpy(rxbuf + pos, &data, nbytes);
-			WARN_ON(readl(mxic->regs + INT_STS) & INT_RX_NOT_EMPTY);
-		} else {
-			readl(mxic->regs + RXD);
 		}
 		WARN_ON(readl(mxic->regs + INT_STS) & INT_RX_NOT_EMPTY);
 
-- 
2.27.0

[PATCH v7 11/14] spi: mxic: Create a helper to configure the controller before an operation

[Miquel Raynal]

Create the mxic_spi_set_hc_cfg() helper to configure the HC_CFG
register. This helper will soon be used by the dirmap implementation and
having this code factorized out earlier will clarify this addition.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mxic.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index aedc9a144b82..7d0ec223622a 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -280,6 +280,22 @@ static void mxic_spi_hw_init(struct mxic_spi *mxic)
 	       mxic->regs + HC_CFG);
 }
 
+static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags)
+{
+	int nio = 1;
+
+	if (spi->mode & (SPI_TX_OCTAL | SPI_RX_OCTAL))
+		nio = 8;
+	else if (spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
+		nio = 4;
+	else if (spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
+		nio = 2;
+
+	return flags | HC_CFG_NIO(nio) |
+	       HC_CFG_TYPE(spi->chip_select, HC_CFG_TYPE_SPI_NOR) |
+	       HC_CFG_SLV_ACT(spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1);
+}
+
 static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
 			      void *rxbuf, unsigned int len)
 {
@@ -349,7 +365,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 				const struct spi_mem_op *op)
 {
 	struct mxic_spi *mxic = spi_master_get_devdata(mem->spi->master);
-	int nio = 1, i, ret;
+	int i, ret;
 	u32 ss_ctrl;
 	u8 addr[8], cmd[2];
 
@@ -357,18 +373,9 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 	if (ret)
 		return ret;
 
-	if (mem->spi->mode & (SPI_TX_OCTAL | SPI_RX_OCTAL))
-		nio = 8;
-	else if (mem->spi->mode & (SPI_TX_QUAD | SPI_RX_QUAD))
-		nio = 4;
-	else if (mem->spi->mode & (SPI_TX_DUAL | SPI_RX_DUAL))
-		nio = 2;
-
-	writel(HC_CFG_NIO(nio) |
-	       HC_CFG_TYPE(mem->spi->chip_select, HC_CFG_TYPE_SPI_NOR) |
-	       HC_CFG_SLV_ACT(mem->spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1) |
-	       HC_CFG_MAN_CS_EN,
+	writel(mxic_spi_prep_hc_cfg(mem->spi, HC_CFG_MAN_CS_EN),
 	       mxic->regs + HC_CFG);
+
 	writel(HC_EN_BIT, mxic->regs + HC_EN);
 
 	ss_ctrl = OP_CMD_BYTES(op->cmd.nbytes) |
-- 
2.27.0

[PATCH v7 12/14] spi: mxic: Create a helper to ease the start of an operation

[Miquel Raynal]

Create the mxic_spi_mem_prep_op_cfg() helper to provide the content to
write to the register controlling the next IO command. This helper will
soon be used by the dirmap implementation and having this code
factorized out earlier will clarify this addition.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mxic.c | 53 +++++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 7d0ec223622a..52fe1060d940 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -296,6 +296,33 @@ static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags)
 	       HC_CFG_SLV_ACT(spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1);
 }
 
+static u32 mxic_spi_mem_prep_op_cfg(const struct spi_mem_op *op)
+{
+	u32 cfg = OP_CMD_BYTES(op->cmd.nbytes) |
+		  OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
+		  (op->cmd.dtr ? OP_CMD_DDR : 0);
+
+	if (op->addr.nbytes)
+		cfg |= OP_ADDR_BYTES(op->addr.nbytes) |
+		       OP_ADDR_BUSW(fls(op->addr.buswidth) - 1) |
+		       (op->addr.dtr ? OP_ADDR_DDR : 0);
+
+	if (op->dummy.nbytes)
+		cfg |= OP_DUMMY_CYC(op->dummy.nbytes);
+
+	if (op->data.nbytes) {
+		cfg |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
+		       (op->data.dtr ? OP_DATA_DDR : 0);
+		if (op->data.dir == SPI_MEM_DATA_IN) {
+			cfg |= OP_READ;
+			if (op->data.dtr)
+				cfg |= OP_DQS_EN;
+		}
+	}
+
+	return cfg;
+}
+
 static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
 			      void *rxbuf, unsigned int len)
 {
@@ -366,7 +393,6 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 {
 	struct mxic_spi *mxic = spi_master_get_devdata(mem->spi->master);
 	int i, ret;
-	u32 ss_ctrl;
 	u8 addr[8], cmd[2];
 
 	ret = mxic_spi_set_freq(mxic, mem->spi->max_speed_hz);
@@ -378,29 +404,8 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 
 	writel(HC_EN_BIT, mxic->regs + HC_EN);
 
-	ss_ctrl = OP_CMD_BYTES(op->cmd.nbytes) |
-		  OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
-		  (op->cmd.dtr ? OP_CMD_DDR : 0);
-
-	if (op->addr.nbytes)
-		ss_ctrl |= OP_ADDR_BYTES(op->addr.nbytes) |
-			   OP_ADDR_BUSW(fls(op->addr.buswidth) - 1) |
-			   (op->addr.dtr ? OP_ADDR_DDR : 0);
-
-	if (op->dummy.nbytes)
-		ss_ctrl |= OP_DUMMY_CYC(op->dummy.nbytes);
-
-	if (op->data.nbytes) {
-		ss_ctrl |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
-			   (op->data.dtr ? OP_DATA_DDR : 0);
-		if (op->data.dir == SPI_MEM_DATA_IN) {
-			ss_ctrl |= OP_READ;
-			if (op->data.dtr)
-				ss_ctrl |= OP_DQS_EN;
-		}
-	}
-
-	writel(ss_ctrl, mxic->regs + SS_CTRL(mem->spi->chip_select));
+	writel(mxic_spi_mem_prep_op_cfg(op),
+	       mxic->regs + SS_CTRL(mem->spi->chip_select));
 
 	writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
 	       mxic->regs + HC_CFG);
-- 
2.27.0

[PATCH v7 13/14] spi: mxic: Add support for direct mapping

[Miquel Raynal]

Implement the ->dirmap_create() and ->dirmap_read/write() hooks to
provide a fast path for read and write accesses.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Tested-by: Zhengxun Li <zhengxunli@mxic.com.tw>
Reviewed-by: Zhengxun Li <zhengxunli@mxic.com.tw>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mxic.c | 112 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 109 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 52fe1060d940..97de058d2f80 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -172,6 +172,11 @@ struct mxic_spi {
 	struct clk *send_dly_clk;
 	void __iomem *regs;
 	u32 cur_speed_hz;
+	struct {
+		void __iomem *map;
+		dma_addr_t dma;
+		size_t size;
+	} linear;
 };
 
 static int mxic_spi_clk_enable(struct mxic_spi *mxic)
@@ -296,7 +301,8 @@ static u32 mxic_spi_prep_hc_cfg(struct spi_device *spi, u32 flags)
 	       HC_CFG_SLV_ACT(spi->chip_select) | HC_CFG_IDLE_SIO_LVL(1);
 }
 
-static u32 mxic_spi_mem_prep_op_cfg(const struct spi_mem_op *op)
+static u32 mxic_spi_mem_prep_op_cfg(const struct spi_mem_op *op,
+				    unsigned int data_len)
 {
 	u32 cfg = OP_CMD_BYTES(op->cmd.nbytes) |
 		  OP_CMD_BUSW(fls(op->cmd.buswidth) - 1) |
@@ -310,7 +316,8 @@ static u32 mxic_spi_mem_prep_op_cfg(const struct spi_mem_op *op)
 	if (op->dummy.nbytes)
 		cfg |= OP_DUMMY_CYC(op->dummy.nbytes);
 
-	if (op->data.nbytes) {
+	/* Direct mapping data.nbytes field is not populated */
+	if (data_len) {
 		cfg |= OP_DATA_BUSW(fls(op->data.buswidth) - 1) |
 		       (op->data.dtr ? OP_DATA_DDR : 0);
 		if (op->data.dir == SPI_MEM_DATA_IN) {
@@ -371,6 +378,77 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
 	return 0;
 }
 
+static ssize_t mxic_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
+					u64 offs, size_t len, void *buf)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(desc->mem->spi->master);
+	int ret;
+	u32 sts;
+
+	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
+		return -EINVAL;
+
+	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0), mxic->regs + HC_CFG);
+
+	writel(mxic_spi_mem_prep_op_cfg(&desc->info.op_tmpl, len),
+	       mxic->regs + LRD_CFG);
+	writel(desc->info.offset + offs, mxic->regs + LRD_ADDR);
+	len = min_t(size_t, len, mxic->linear.size);
+	writel(len, mxic->regs + LRD_RANGE);
+	writel(LMODE_CMD0(desc->info.op_tmpl.cmd.opcode) |
+	       LMODE_SLV_ACT(desc->mem->spi->chip_select) |
+	       LMODE_EN,
+	       mxic->regs + LRD_CTRL);
+
+	memcpy_fromio(buf, mxic->linear.map, len);
+
+	writel(INT_LRD_DIS, mxic->regs + INT_STS);
+	writel(0, mxic->regs + LRD_CTRL);
+
+	ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+				 sts & INT_LRD_DIS, 0, USEC_PER_SEC);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
+					 u64 offs, size_t len,
+					 const void *buf)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(desc->mem->spi->master);
+	u32 sts;
+	int ret;
+
+	if (WARN_ON(offs + desc->info.offset + len > U32_MAX))
+		return -EINVAL;
+
+	writel(mxic_spi_prep_hc_cfg(desc->mem->spi, 0), mxic->regs + HC_CFG);
+
+	writel(mxic_spi_mem_prep_op_cfg(&desc->info.op_tmpl, len),
+	       mxic->regs + LWR_CFG);
+	writel(desc->info.offset + offs, mxic->regs + LWR_ADDR);
+	len = min_t(size_t, len, mxic->linear.size);
+	writel(len, mxic->regs + LWR_RANGE);
+	writel(LMODE_CMD0(desc->info.op_tmpl.cmd.opcode) |
+	       LMODE_SLV_ACT(desc->mem->spi->chip_select) |
+	       LMODE_EN,
+	       mxic->regs + LWR_CTRL);
+
+	memcpy_toio(mxic->linear.map, buf, len);
+
+	writel(INT_LWR_DIS, mxic->regs + INT_STS);
+	writel(0, mxic->regs + LWR_CTRL);
+
+	ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
+				 sts & INT_LWR_DIS, 0, USEC_PER_SEC);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
 static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 				     const struct spi_mem_op *op)
 {
@@ -388,6 +466,22 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
 	return spi_mem_default_supports_op(mem, op);
 }
 
+static int mxic_spi_mem_dirmap_create(struct spi_mem_dirmap_desc *desc)
+{
+	struct mxic_spi *mxic = spi_master_get_devdata(desc->mem->spi->master);
+
+	if (!mxic->linear.map)
+		return -EINVAL;
+
+	if (desc->info.offset + desc->info.length > U32_MAX)
+		return -EINVAL;
+
+	if (!mxic_spi_mem_supports_op(desc->mem, &desc->info.op_tmpl))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 				const struct spi_mem_op *op)
 {
@@ -404,7 +498,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 
 	writel(HC_EN_BIT, mxic->regs + HC_EN);
 
-	writel(mxic_spi_mem_prep_op_cfg(op),
+	writel(mxic_spi_mem_prep_op_cfg(op, op->data.nbytes),
 	       mxic->regs + SS_CTRL(mem->spi->chip_select));
 
 	writel(readl(mxic->regs + HC_CFG) | HC_CFG_MAN_CS_ASSERT,
@@ -450,6 +544,9 @@ static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
 static const struct spi_controller_mem_ops mxic_spi_mem_ops = {
 	.supports_op = mxic_spi_mem_supports_op,
 	.exec_op = mxic_spi_mem_exec_op,
+	.dirmap_create = mxic_spi_mem_dirmap_create,
+	.dirmap_read = mxic_spi_mem_dirmap_read,
+	.dirmap_write = mxic_spi_mem_dirmap_write,
 	.caps = &mxic_spi_mem_caps,
 };
 
@@ -580,6 +677,15 @@ static int mxic_spi_probe(struct platform_device *pdev)
 	if (IS_ERR(mxic->regs))
 		return PTR_ERR(mxic->regs);
 
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dirmap");
+	mxic->linear.map = devm_ioremap_resource(&pdev->dev, res);
+	if (!IS_ERR(mxic->linear.map)) {
+		mxic->linear.dma = res->start;
+		mxic->linear.size = resource_size(res);
+	} else {
+		mxic->linear.map = NULL;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	master->auto_runtime_pm = true;
 
-- 
2.27.0

[PATCH v7 14/14] spi: mxic: Add support for pipelined ECC operations

[Miquel Raynal]

Some SPI-NAND chips do not have a proper on-die ECC engine providing
error correction/detection. This is particularly an issue on embedded
devices with limited resources because all the computations must
happen in software, unless an external hardware engine is provided.

These external engines are new and can be of two categories: external
or pipelined. Macronix is providing both, the former being already
supported. The second, however, is very SoC implementation dependent
and must be instantiated by the SPI host controller directly.

An entire subsystem has been contributed to support these engines which
makes the insertion into another subsystem such as SPI quite
straightforward without the need for a lot of specific functions.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/Kconfig    |   2 +-
 drivers/spi/spi-mxic.c | 113 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 112 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83e352b0c8f9..bff75629e872 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -856,7 +856,7 @@ config SPI_SYNQUACER
 
 config SPI_MXIC
 	tristate "Macronix MX25F0A SPI controller"
-	depends on SPI_MASTER
+	depends on SPI_MASTER && MTD_NAND_ECC
 	help
 	  This selects the Macronix MX25F0A SPI controller driver.
 
diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
index 97de058d2f80..7fc57e94b8b4 100644
--- a/drivers/spi/spi-mxic.c
+++ b/drivers/spi/spi-mxic.c
@@ -12,6 +12,8 @@
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/nand-ecc-mxic.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
@@ -167,6 +169,7 @@
 #define HW_TEST(x)		(0xe0 + ((x) * 4))
 
 struct mxic_spi {
+	struct device *dev;
 	struct clk *ps_clk;
 	struct clk *send_clk;
 	struct clk *send_dly_clk;
@@ -177,6 +180,12 @@ struct mxic_spi {
 		dma_addr_t dma;
 		size_t size;
 	} linear;
+
+	struct {
+		bool use_pipelined_conf;
+		struct nand_ecc_engine *pipelined_engine;
+		void *ctx;
+	} ecc;
 };
 
 static int mxic_spi_clk_enable(struct mxic_spi *mxic)
@@ -400,7 +409,15 @@ static ssize_t mxic_spi_mem_dirmap_read(struct spi_mem_dirmap_desc *desc,
 	       LMODE_EN,
 	       mxic->regs + LRD_CTRL);
 
-	memcpy_fromio(buf, mxic->linear.map, len);
+	if (mxic->ecc.use_pipelined_conf && desc->info.op_tmpl.ecc_en) {
+		ret = mxic_ecc_process_data_pipelined(mxic->ecc.pipelined_engine,
+						      NAND_PAGE_READ,
+						      mxic->linear.dma + offs);
+		if (ret)
+			return ret;
+	} else {
+		memcpy_fromio(buf, mxic->linear.map, len);
+	}
 
 	writel(INT_LRD_DIS, mxic->regs + INT_STS);
 	writel(0, mxic->regs + LRD_CTRL);
@@ -436,7 +453,15 @@ static ssize_t mxic_spi_mem_dirmap_write(struct spi_mem_dirmap_desc *desc,
 	       LMODE_EN,
 	       mxic->regs + LWR_CTRL);
 
-	memcpy_toio(mxic->linear.map, buf, len);
+	if (mxic->ecc.use_pipelined_conf && desc->info.op_tmpl.ecc_en) {
+		ret = mxic_ecc_process_data_pipelined(mxic->ecc.pipelined_engine,
+						      NAND_PAGE_WRITE,
+						      mxic->linear.dma + offs);
+		if (ret)
+			return ret;
+	} else {
+		memcpy_toio(mxic->linear.map, buf, len);
+	}
 
 	writel(INT_LWR_DIS, mxic->regs + INT_STS);
 	writel(0, mxic->regs + LWR_CTRL);
@@ -539,6 +564,7 @@ static int mxic_spi_mem_exec_op(struct spi_mem *mem,
 
 static const struct spi_controller_mem_caps mxic_spi_mem_caps = {
 	.dtr = true,
+	.ecc = true,
 };
 
 static const struct spi_controller_mem_ops mxic_spi_mem_ops = {
@@ -612,6 +638,80 @@ static int mxic_spi_transfer_one(struct spi_master *master,
 	return 0;
 }
 
+/* ECC wrapper */
+static int mxic_spi_mem_ecc_init_ctx(struct nand_device *nand)
+{
+	struct nand_ecc_engine_ops *ops = mxic_ecc_get_pipelined_ops();
+	struct mxic_spi *mxic = nand->ecc.engine->priv;
+
+	mxic->ecc.use_pipelined_conf = true;
+
+	return ops->init_ctx(nand);
+}
+
+static void mxic_spi_mem_ecc_cleanup_ctx(struct nand_device *nand)
+{
+	struct nand_ecc_engine_ops *ops = mxic_ecc_get_pipelined_ops();
+	struct mxic_spi *mxic = nand->ecc.engine->priv;
+
+	mxic->ecc.use_pipelined_conf = false;
+
+	ops->cleanup_ctx(nand);
+}
+
+static int mxic_spi_mem_ecc_prepare_io_req(struct nand_device *nand,
+					   struct nand_page_io_req *req)
+{
+	struct nand_ecc_engine_ops *ops = mxic_ecc_get_pipelined_ops();
+
+	return ops->prepare_io_req(nand, req);
+}
+
+static int mxic_spi_mem_ecc_finish_io_req(struct nand_device *nand,
+					  struct nand_page_io_req *req)
+{
+	struct nand_ecc_engine_ops *ops = mxic_ecc_get_pipelined_ops();
+
+	return ops->finish_io_req(nand, req);
+}
+
+static struct nand_ecc_engine_ops mxic_spi_mem_ecc_engine_pipelined_ops = {
+	.init_ctx = mxic_spi_mem_ecc_init_ctx,
+	.cleanup_ctx = mxic_spi_mem_ecc_cleanup_ctx,
+	.prepare_io_req = mxic_spi_mem_ecc_prepare_io_req,
+	.finish_io_req = mxic_spi_mem_ecc_finish_io_req,
+};
+
+static void mxic_spi_mem_ecc_remove(struct mxic_spi *mxic)
+{
+	if (mxic->ecc.pipelined_engine) {
+		mxic_ecc_put_pipelined_engine(mxic->ecc.pipelined_engine);
+		nand_ecc_unregister_on_host_hw_engine(mxic->ecc.pipelined_engine);
+	}
+}
+
+static int mxic_spi_mem_ecc_probe(struct platform_device *pdev,
+				  struct mxic_spi *mxic)
+{
+	struct nand_ecc_engine *eng;
+
+	if (!mxic_ecc_get_pipelined_ops())
+		return -EOPNOTSUPP;
+
+	eng = mxic_ecc_get_pipelined_engine(pdev);
+	if (IS_ERR(eng))
+		return PTR_ERR(eng);
+
+	eng->dev = &pdev->dev;
+	eng->integration = NAND_ECC_ENGINE_INTEGRATION_PIPELINED;
+	eng->ops = &mxic_spi_mem_ecc_engine_pipelined_ops;
+	eng->priv = mxic;
+	mxic->ecc.pipelined_engine = eng;
+	nand_ecc_register_on_host_hw_engine(eng);
+
+	return 0;
+}
+
 static int __maybe_unused mxic_spi_runtime_suspend(struct device *dev)
 {
 	struct spi_master *master = dev_get_drvdata(dev);
@@ -657,6 +757,7 @@ static int mxic_spi_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, master);
 
 	mxic = spi_master_get_devdata(master);
+	mxic->dev = &pdev->dev;
 
 	master->dev.of_node = pdev->dev.of_node;
 
@@ -702,6 +803,12 @@ static int mxic_spi_probe(struct platform_device *pdev)
 
 	mxic_spi_hw_init(mxic);
 
+	ret = mxic_spi_mem_ecc_probe(pdev, mxic);
+	if (ret == -EPROBE_DEFER) {
+		pm_runtime_disable(&pdev->dev);
+		return ret;
+	}
+
 	ret = spi_register_master(master);
 	if (ret) {
 		dev_err(&pdev->dev, "spi_register_master failed\n");
@@ -714,8 +821,10 @@ static int mxic_spi_probe(struct platform_device *pdev)
 static int mxic_spi_remove(struct platform_device *pdev)
 {
 	struct spi_master *master = platform_get_drvdata(pdev);
+	struct mxic_spi *mxic = spi_master_get_devdata(master);
 
 	pm_runtime_disable(&pdev->dev);
+	mxic_spi_mem_ecc_remove(mxic);
 	spi_unregister_master(master);
 
 	return 0;
-- 
2.27.0

Re: [PATCH v7 01/14] spi: spi-mem: Fix a DTR related check in spi_mem_dtr_supports_op()

[Pratyush Yadav]

On 17/12/21 05:16PM, Miquel Raynal wrote:
> It seems that the number of command bytes must be "2" only when the
> command itself is sent in DTR mode. The current logic checks if the
> number of command bytes is "2" when any of the cycles is a DTR cycle. It
> is likely that so far no device was actually mixing DTR/non-DTR cycles
> in the same operation, explaining why this was left undetected until
> now.

This was intentional. spi_mem_dtr_supports_op() must only be called when 
the operation is DTR in all phases so I did not add any sanity checks if 
someone was using it for non-DTR ops. In fact, I added on to this 
function in [0] to check nbytes for other phases as well. The patch fell 
off my radar unfortunately, and it didn't get merged.

I would like to keep this as it is since we have no user of mixed 
DTR/non-DTR modes yet. But if you really want to support it, please 
apply my patch first to make sure we check for every phase, not just 
command.

[0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@ti.com/

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 02/14] spi: spi-mem: Introduce a capability structure

[Pratyush Yadav]

On 17/12/21 05:16PM, Miquel Raynal wrote:
> Create a spi_controller_mem_caps structure and put it within the
> spi_controller_mem_ops structure as these are highly related. So far the
> only field in this structure is the support for dtr operations, but soon
> we will add another parameter.
> 
> Also create a helper to parse the capabilities and check if the
> requested capability has been set or not.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  include/linux/spi/spi-mem.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 85e2ff7b840d..045ecb7c6f50 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -220,6 +220,17 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
>  	return mem->drvpriv;
>  }
>  
> +/**
> + * struct spi_controller_mem_caps - SPI memory controller capabilities
> + * @dtr: Supports DTR operations
> + */
> +struct spi_controller_mem_caps {
> +	bool dtr;

I assume this would mean DTR is supported on _all_ phases? I am not sure 
if we would ever need to encode DTR support per-phase, but that can 
probably come later. Or the controller's supports_op() hook can do those 
checks before calling spi_mem_default_supports_op().

> +};
> +
> +#define spi_mem_controller_is_capable(ctlr, cap)		\
> +	((ctlr)->mem_ops->caps && (ctlr)->mem_ops->caps->cap)	\
> +
>  /**
>   * struct spi_controller_mem_ops - SPI memory operations
>   * @adjust_op_size: shrink the data xfer of an operation to match controller's
> @@ -253,6 +264,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
>   * @poll_status: poll memory device status until (status & mask) == match or
>   *               when the timeout has expired. It fills the data buffer with
>   *               the last status value.
> + * @caps: controller capabilities for the handling of the above operations.
>   *
>   * This interface should be implemented by SPI controllers providing an
>   * high-level interface to execute SPI memory operation, which is usually the
> @@ -283,6 +295,7 @@ struct spi_controller_mem_ops {
>  			   unsigned long initial_delay_us,
>  			   unsigned long polling_rate_us,
>  			   unsigned long timeout_ms);
> +	const struct spi_controller_mem_caps *caps;

I feel like this would be better passed in as an argument to the 
spi_mem_default_supports_op() function. But I see that Mark and you feel 
differently so I won't insist on it.

>  };
>  
>  /**
> -- 
> 2.27.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 03/14] spi: spi-mem: Check the controller extra capabilities

[Pratyush Yadav]

On 17/12/21 05:16PM, Miquel Raynal wrote:
> Controllers can now provide a spi-mem capabilities structure. Let's make
> use of it in spi_mem_controller_default_supports_op(). As we want to
> check for DTR operations as well as normal operations in a single
> helper, let's pull the necessary checks from spi_mem_dtr_supports_op()
> for now.
> 
> However, because no controller provide these extra capabilities, this
> change has no effect so far.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-mem.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index c4da0c9b05e9..677e54221ebc 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -173,11 +173,20 @@ EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
>  bool spi_mem_default_supports_op(struct spi_mem *mem,
>  				 const struct spi_mem_op *op)
>  {
> -	if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr)
> -		return false;
> +	struct spi_controller *ctlr = mem->spi->controller;
> +	bool op_is_dtr =
> +		op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr;
>  
> -	if (op->cmd.nbytes != 1)
> -		return false;
> +	if (op_is_dtr) {
> +		if (!spi_mem_controller_is_capable(ctlr, dtr))
> +			return false;
> +
> +		if (op->cmd.dtr && op->cmd.nbytes != 2)
> +			return false;

As I mentioned in patch 1, you want to do this check for all phases. For 
controllers that do not support mixed DTR modes, this does not allow the 
controller to make sure those ops are rejected. So that check would have 
to then move in the controller's supports_op() before 
spi_mem_default_supports_op() is called.

> +	} else {
> +		if (op->cmd.nbytes != 1)
> +			return false;

Technically speaking there is nothing stopping a device from using 2 or 
3 or even 4 byte opcodes. But that is a different topic that we don't 
really need to look at until the need comes up.

> +	}
>  
>  	return spi_mem_check_buswidth(mem, op);
>  }
> -- 
> 2.27.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Pratyush Yadav]

> Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure

s/cadence/cadence-quadspi/

On 17/12/21 05:16PM, Miquel Raynal wrote:
> This controller has DTR support, so advertize it with a capability now
> that the spi_controller_mem_ops structure contains this new field. This
> will later be used by the core to discriminate whether an operation is
> supported or not, in a more generic way than having different helpers.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> index 101cc71bffa7..98e0cc4236e3 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem)
>  	return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select);
>  }
>  
> +static const struct spi_controller_mem_caps cqspi_mem_caps = {
> +	.dtr = true,
> +};
> +
>  static const struct spi_controller_mem_ops cqspi_mem_ops = {
>  	.exec_op = cqspi_exec_mem_op,
>  	.get_name = cqspi_get_name,
>  	.supports_op = cqspi_supports_mem_op,
> +	.caps = &cqspi_mem_caps,

I just noticed you put it under struct spi_mem_ops, not under struct 
spi_mem. This is not an operation per se so wouldn't it be better if it 
is moved to struct spi_mem?

Anyway, the change itself looks good to me. The cqspi_supports_mem_op()
already checks for mixed DTR modes so we should be good.

>  };
>  
>  static int cqspi_setup_flash(struct cqspi_st *cqspi)
> -- 
> 2.27.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 06/14] spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper

[Pratyush Yadav]

On 17/12/21 05:16PM, Miquel Raynal wrote:
> Now that spi_mem_default_supports_op() has access to the static
> controller capabilities (related to memory operations), and now that
> these capabilities have been filled by the impacted controllers, there
> is no need for a specific helper checking only DTR operations, so let's
> just kill spi_mem_dtr_supports_op() and simply use
> spi_mem_default_supports_op() instead.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-cadence-quadspi.c |  5 +----
>  drivers/spi/spi-mem.c             | 10 ----------
>  drivers/spi/spi-mxic.c            | 10 +---------
>  include/linux/spi/spi-mem.h       | 11 -----------
>  4 files changed, 2 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> index e137b1ec85d4..67d05ee8d6a0 100644
> --- a/drivers/spi/spi-mxic.c
> +++ b/drivers/spi/spi-mxic.c
> @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
>  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  				     const struct spi_mem_op *op)
>  {
> -	bool all_false;
> -
>  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
>  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
>  		return false;
> @@ -348,13 +346,7 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
>  	if (op->addr.nbytes > 7)
>  		return false;
>  
> -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> -		    !op->data.dtr;
> -
> -	if (all_false)
> -		return spi_mem_default_supports_op(mem, op);
> -	else
> -		return spi_mem_dtr_supports_op(mem, op);
> +	return spi_mem_default_supports_op(mem, op);

Does this controller support mixed DTR modes? If it doesn't then it 
should reject mixed DTR ops before calling 
spi_mem_default_supports_op(). Anyway, the current driver doesn't check 
for it either so this change does not make anything worse at the very 
least.

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>


-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 07/14] spi: spi-mem: Add an ecc_en parameter to the spi_mem_op structure

[Pratyush Yadav]

On 17/12/21 05:16PM, Miquel Raynal wrote:
> Soon the SPI-NAND core will need a way to request a SPI controller to
> enable ECC support for a given operation. This is because of the
> pipelined integration of certain ECC engines, which are directly managed
> by the SPI controller itself.
> 
> Introduce a spi_mem_op additional field for this purpose: ecc_en.
> 
> So far this field is left unset and checked to be false by all
> the SPI controller drivers in their ->supports_op() hook, as they all
> call spi_mem_default_supports_op().
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-mem.c       | 5 +++++
>  include/linux/spi/spi-mem.h | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index cfe1c99db5f3..94758e7e747d 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -178,6 +178,11 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
>  			return false;
>  	}
>  
> +	if (op->ecc_en) {
> +		if (!spi_mem_controller_is_capable(ctlr, ecc))
> +			return false;
> +	}
> +
>  	return spi_mem_check_buswidth(mem, op);
>  }
>  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 d7787c8f3746..e9238a858109 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -94,6 +94,7 @@ enum spi_mem_data_dir {
>   *		 operation does not involve transferring data
>   * @data.buf.in: input buffer (must be DMA-able)
>   * @data.buf.out: output buffer (must be DMA-able)
> + * @ecc_en: error correction is required
>   */
>  struct spi_mem_op {
>  	struct {
> @@ -126,6 +127,8 @@ struct spi_mem_op {
>  			const void *out;
>  		} buf;
>  	} data;
> +
> +	bool ecc_en;

ECC should only concern the data phase right? Would it make more sense 
to move this field under data?

Anyway, I don't know much about NAND or ECC so either way,

Acked-by: Pratyush Yadav <p.yadav@ti.com>

>  };
>  
>  #define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\
> @@ -223,9 +226,11 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
>  /**
>   * struct spi_controller_mem_caps - SPI memory controller capabilities
>   * @dtr: Supports DTR operations
> + * @ecc: Supports operations with error correction
>   */
>  struct spi_controller_mem_caps {
>  	bool dtr;
> +	bool ecc;
>  };
>  
>  #define spi_mem_controller_is_capable(ctlr, cap)		\
> -- 
> 2.27.0
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 02/14] spi: spi-mem: Introduce a capability structure

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:13:25 +0530:

> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > Create a spi_controller_mem_caps structure and put it within the
> > spi_controller_mem_ops structure as these are highly related. So far the
> > only field in this structure is the support for dtr operations, but soon
> > we will add another parameter.
> > 
> > Also create a helper to parse the capabilities and check if the
> > requested capability has been set or not.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  include/linux/spi/spi-mem.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > index 85e2ff7b840d..045ecb7c6f50 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -220,6 +220,17 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
> >  	return mem->drvpriv;
> >  }
> >  
> > +/**
> > + * struct spi_controller_mem_caps - SPI memory controller capabilities
> > + * @dtr: Supports DTR operations
> > + */
> > +struct spi_controller_mem_caps {
> > +	bool dtr;  
> 
> I assume this would mean DTR is supported on _all_ phases? I am not sure 
> if we would ever need to encode DTR support per-phase, but that can 
> probably come later. Or the controller's supports_op() hook can do those 
> checks before calling spi_mem_default_supports_op().

If we ever need this there is no problem: the idea here is to provide a
default (and for this one the naming is rather good =) ) helper to do
basic checks. A controller either supports DTR ops or not. If it does
not support them, then this check will fail. But like the Cadence SPI
controller driver does, if only one specific configuration is not
supported by the driver, then the ->supports_op() hook of the driver
should check that beforehand and return an error [1].


[1] https://elixir.bootlin.com/linux/latest/source/drivers/spi/spi-cadence-quadspi.c#L1253

> > +};
> > +
> > +#define spi_mem_controller_is_capable(ctlr, cap)		\
> > +	((ctlr)->mem_ops->caps && (ctlr)->mem_ops->caps->cap)	\
> > +
> >  /**
> >   * struct spi_controller_mem_ops - SPI memory operations
> >   * @adjust_op_size: shrink the data xfer of an operation to match controller's
> > @@ -253,6 +264,7 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
> >   * @poll_status: poll memory device status until (status & mask) == match or
> >   *               when the timeout has expired. It fills the data buffer with
> >   *               the last status value.
> > + * @caps: controller capabilities for the handling of the above operations.
> >   *
> >   * This interface should be implemented by SPI controllers providing an
> >   * high-level interface to execute SPI memory operation, which is usually the
> > @@ -283,6 +295,7 @@ struct spi_controller_mem_ops {
> >  			   unsigned long initial_delay_us,
> >  			   unsigned long polling_rate_us,
> >  			   unsigned long timeout_ms);
> > +	const struct spi_controller_mem_caps *caps;  
> 
> I feel like this would be better passed in as an argument to the 
> spi_mem_default_supports_op() function. But I see that Mark and you feel 
> differently so I won't insist on it.

As these properties are supposes to be more or less static over the
lifetime of the controller we assumed there was no need for something
more dynamic. Anyway this is a default helper, drivers are pleased to
implement their own if needed. Plus, doing so prevents the need for
hacking into dozens of drivers, which is certainly the reason I
personally like the most :p 

Thanks,
Miquèl

Re: [PATCH v7 01/14] spi: spi-mem: Fix a DTR related check in spi_mem_dtr_supports_op()

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:09:19 +0530:

> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > It seems that the number of command bytes must be "2" only when the
> > command itself is sent in DTR mode. The current logic checks if the
> > number of command bytes is "2" when any of the cycles is a DTR cycle. It
> > is likely that so far no device was actually mixing DTR/non-DTR cycles
> > in the same operation, explaining why this was left undetected until
> > now.  
> 
> This was intentional. spi_mem_dtr_supports_op() must only be called when 
> the operation is DTR in all phases so I did not add any sanity checks if 
> someone was using it for non-DTR ops.

Maybe that was the original intention but since then the Macronix
driver has been merged and supports (at lest does not reject) these
modes.

> In fact, I added on to this 
> function in [0] to check nbytes for other phases as well. The patch fell 
> off my radar unfortunately, and it didn't get merged.
> 
> I would like to keep this as it is since we have no user of mixed 
> DTR/non-DTR modes yet.

I don't know if the Macronix driver really supports it or if it is the
driver that is doing the wrong checks but in appearance such mixed mode
could be used.

> But if you really want to support it, please 
> apply my patch first to make sure we check for every phase, not just 
> command.
> 
> [0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@ti.com/

Nice, I might take that as well indeed in order to make the checks a
little bit more robust.

Thanks,
Miquèl

Re: [PATCH v7 06/14] spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530:

> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > Now that spi_mem_default_supports_op() has access to the static
> > controller capabilities (related to memory operations), and now that
> > these capabilities have been filled by the impacted controllers, there
> > is no need for a specific helper checking only DTR operations, so let's
> > just kill spi_mem_dtr_supports_op() and simply use
> > spi_mem_default_supports_op() instead.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/spi/spi-cadence-quadspi.c |  5 +----
> >  drivers/spi/spi-mem.c             | 10 ----------
> >  drivers/spi/spi-mxic.c            | 10 +---------
> >  include/linux/spi/spi-mem.h       | 11 -----------
> >  4 files changed, 2 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > index e137b1ec85d4..67d05ee8d6a0 100644
> > --- a/drivers/spi/spi-mxic.c
> > +++ b/drivers/spi/spi-mxic.c
> > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> >  				     const struct spi_mem_op *op)
> >  {
> > -	bool all_false;
> > -
> >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> >  		return false;
> > @@ -348,13 +346,7 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> >  	if (op->addr.nbytes > 7)
> >  		return false;
> >  
> > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > -		    !op->data.dtr;
> > -
> > -	if (all_false)
> > -		return spi_mem_default_supports_op(mem, op);
> > -	else
> > -		return spi_mem_dtr_supports_op(mem, op);
> > +	return spi_mem_default_supports_op(mem, op);  
> 
> Does this controller support mixed DTR modes? If it doesn't then it 
> should reject mixed DTR ops before calling 
> spi_mem_default_supports_op(). Anyway, the current driver doesn't check 
> for it either so this change does not make anything worse at the very 
> least.

The Cadence SPI driver does something like:

all_true = cmd.dtr && addr.dtr ...;
all_false = !cmd.dtr && !addr.dtr ...;
if (!all_true || !all_false)
	return false;

This basically rules out any mixed DTR operation.

I believe Macronix code is inspired from this function, but they
intentionally dropped the all_true check, making the situation boolean:
either there is at least one DTR op, or there is none. So I believe
this was done on purpose and this controller supports mixed DTR ops.

> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Thanks,
Miquèl

Re: [PATCH v7 06/14] spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper

[Pratyush Yadav]

On 21/12/21 10:58AM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530:
> 
> > On 17/12/21 05:16PM, Miquel Raynal wrote:
> > > Now that spi_mem_default_supports_op() has access to the static
> > > controller capabilities (related to memory operations), and now that
> > > these capabilities have been filled by the impacted controllers, there
> > > is no need for a specific helper checking only DTR operations, so let's
> > > just kill spi_mem_dtr_supports_op() and simply use
> > > spi_mem_default_supports_op() instead.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/spi/spi-cadence-quadspi.c |  5 +----
> > >  drivers/spi/spi-mem.c             | 10 ----------
> > >  drivers/spi/spi-mxic.c            | 10 +---------
> > >  include/linux/spi/spi-mem.h       | 11 -----------
> > >  4 files changed, 2 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > > index e137b1ec85d4..67d05ee8d6a0 100644
> > > --- a/drivers/spi/spi-mxic.c
> > > +++ b/drivers/spi/spi-mxic.c
> > > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> > >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > >  				     const struct spi_mem_op *op)
> > >  {
> > > -	bool all_false;
> > > -
> > >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> > >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > >  		return false;
> > > @@ -348,13 +346,7 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > >  	if (op->addr.nbytes > 7)
> > >  		return false;
> > >  
> > > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > > -		    !op->data.dtr;
> > > -
> > > -	if (all_false)
> > > -		return spi_mem_default_supports_op(mem, op);
> > > -	else
> > > -		return spi_mem_dtr_supports_op(mem, op);
> > > +	return spi_mem_default_supports_op(mem, op);  
> > 
> > Does this controller support mixed DTR modes? If it doesn't then it 
> > should reject mixed DTR ops before calling 
> > spi_mem_default_supports_op(). Anyway, the current driver doesn't check 
> > for it either so this change does not make anything worse at the very 
> > least.
> 
> The Cadence SPI driver does something like:
> 
> all_true = cmd.dtr && addr.dtr ...;
> all_false = !cmd.dtr && !addr.dtr ...;
> if (!all_true || !all_false)
> 	return false;
> 
> This basically rules out any mixed DTR operation.
> 
> I believe Macronix code is inspired from this function, but they
> intentionally dropped the all_true check, making the situation boolean:
> either there is at least one DTR op, or there is none. So I believe
> this was done on purpose and this controller supports mixed DTR ops.

I see that your other patches in this series touch this driver so you 
should have the datasheet right? Can you look and see for sure if it 
does? I am fine with the patch as it is but if we can make the check 
stricter it would be nice.

> 
> > Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> 
> Thanks,
> Miquèl

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 01/14] spi: spi-mem: Fix a DTR related check in spi_mem_dtr_supports_op()

[Pratyush Yadav]

On 21/12/21 10:50AM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:09:19 +0530:
> 
> > On 17/12/21 05:16PM, Miquel Raynal wrote:
> > > It seems that the number of command bytes must be "2" only when the
> > > command itself is sent in DTR mode. The current logic checks if the
> > > number of command bytes is "2" when any of the cycles is a DTR cycle. It
> > > is likely that so far no device was actually mixing DTR/non-DTR cycles
> > > in the same operation, explaining why this was left undetected until
> > > now.  
> > 
> > This was intentional. spi_mem_dtr_supports_op() must only be called when 
> > the operation is DTR in all phases so I did not add any sanity checks if 
> > someone was using it for non-DTR ops.
> 
> Maybe that was the original intention but since then the Macronix
> driver has been merged and supports (at lest does not reject) these
> modes.

But I don't see any code that actually creates mixed DTR ops. SPI NOR 
does not do it for sure, and SPI NAND does not have DTR support yet. 
Those are the only two users of SPI MEM I can see. So we are solving at 
a problem that doesn't exist yet. Which is fine of course, I just want 
to point it out.

> 
> > In fact, I added on to this 
> > function in [0] to check nbytes for other phases as well. The patch fell 
> > off my radar unfortunately, and it didn't get merged.
> > 
> > I would like to keep this as it is since we have no user of mixed 
> > DTR/non-DTR modes yet.
> 
> I don't know if the Macronix driver really supports it or if it is the
> driver that is doing the wrong checks but in appearance such mixed mode
> could be used.
> 
> > But if you really want to support it, please 
> > apply my patch first to make sure we check for every phase, not just 
> > command.
> > 
> > [0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@ti.com/
> 
> Nice, I might take that as well indeed in order to make the checks a
> little bit more robust.

Thanks.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530:

> > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure  
> 
> s/cadence/cadence-quadspi/

Right.

> 
> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > This controller has DTR support, so advertize it with a capability now
> > that the spi_controller_mem_ops structure contains this new field. This
> > will later be used by the core to discriminate whether an operation is
> > supported or not, in a more generic way than having different helpers.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/spi/spi-cadence-quadspi.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > index 101cc71bffa7..98e0cc4236e3 100644
> > --- a/drivers/spi/spi-cadence-quadspi.c
> > +++ b/drivers/spi/spi-cadence-quadspi.c
> > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem)
> >  	return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select);
> >  }
> >  
> > +static const struct spi_controller_mem_caps cqspi_mem_caps = {
> > +	.dtr = true,
> > +};
> > +
> >  static const struct spi_controller_mem_ops cqspi_mem_ops = {
> >  	.exec_op = cqspi_exec_mem_op,
> >  	.get_name = cqspi_get_name,
> >  	.supports_op = cqspi_supports_mem_op,
> > +	.caps = &cqspi_mem_caps,  
> 
> I just noticed you put it under struct spi_mem_ops, not under struct 
> spi_mem. This is not an operation per se so wouldn't it be better if it 
> is moved to struct spi_mem?

I had a hard time taking a decision but my conclusion was that these
caps are static controller capabilities and exclusively tight to the
controller. The spi_mem structure defines a SPI peripheral. The
spi_mem_ops structure is the only spi-mem related field of the
spi-controller structure. I could have added my own field there but
as these caps are only meant to be used by the spi_mem_ops anyway
(exclusively ->supports_op() for now), it seemed to be a good location,
at least better than the spi-mem structure.

> Anyway, the change itself looks good to me. The cqspi_supports_mem_op()
> already checks for mixed DTR modes so we should be good.

Yep!

> 
> >  };
> >  
> >  static int cqspi_setup_flash(struct cqspi_st *cqspi)
> > -- 
> > 2.27.0
> >   
> 


Thanks,
Miquèl

Re: [PATCH v7 06/14] spi: spi-mem: Kill the spi_mem_dtr_supports_op() helper

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 15:40:54 +0530:

> On 21/12/21 10:58AM, Miquel Raynal wrote:
> > Hi Pratyush,
> > 
> > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:28:42 +0530:
> >   
> > > On 17/12/21 05:16PM, Miquel Raynal wrote:  
> > > > Now that spi_mem_default_supports_op() has access to the static
> > > > controller capabilities (related to memory operations), and now that
> > > > these capabilities have been filled by the impacted controllers, there
> > > > is no need for a specific helper checking only DTR operations, so let's
> > > > just kill spi_mem_dtr_supports_op() and simply use
> > > > spi_mem_default_supports_op() instead.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/spi/spi-cadence-quadspi.c |  5 +----
> > > >  drivers/spi/spi-mem.c             | 10 ----------
> > > >  drivers/spi/spi-mxic.c            | 10 +---------
> > > >  include/linux/spi/spi-mem.h       | 11 -----------
> > > >  4 files changed, 2 insertions(+), 34 deletions(-)
> > > > 
> > > > diff --git a/drivers/spi/spi-mxic.c b/drivers/spi/spi-mxic.c
> > > > index e137b1ec85d4..67d05ee8d6a0 100644
> > > > --- a/drivers/spi/spi-mxic.c
> > > > +++ b/drivers/spi/spi-mxic.c
> > > > @@ -335,8 +335,6 @@ static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> > > >  static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > > >  				     const struct spi_mem_op *op)
> > > >  {
> > > > -	bool all_false;
> > > > -
> > > >  	if (op->data.buswidth > 8 || op->addr.buswidth > 8 ||
> > > >  	    op->dummy.buswidth > 8 || op->cmd.buswidth > 8)
> > > >  		return false;
> > > > @@ -348,13 +346,7 @@ static bool mxic_spi_mem_supports_op(struct spi_mem *mem,
> > > >  	if (op->addr.nbytes > 7)
> > > >  		return false;
> > > >  
> > > > -	all_false = !op->cmd.dtr && !op->addr.dtr && !op->dummy.dtr &&
> > > > -		    !op->data.dtr;
> > > > -
> > > > -	if (all_false)
> > > > -		return spi_mem_default_supports_op(mem, op);
> > > > -	else
> > > > -		return spi_mem_dtr_supports_op(mem, op);
> > > > +	return spi_mem_default_supports_op(mem, op);    
> > > 
> > > Does this controller support mixed DTR modes? If it doesn't then it 
> > > should reject mixed DTR ops before calling 
> > > spi_mem_default_supports_op(). Anyway, the current driver doesn't check 
> > > for it either so this change does not make anything worse at the very 
> > > least.  
> > 
> > The Cadence SPI driver does something like:
> > 
> > all_true = cmd.dtr && addr.dtr ...;
> > all_false = !cmd.dtr && !addr.dtr ...;
> > if (!all_true || !all_false)
> > 	return false;
> > 
> > This basically rules out any mixed DTR operation.
> > 
> > I believe Macronix code is inspired from this function, but they
> > intentionally dropped the all_true check, making the situation boolean:
> > either there is at least one DTR op, or there is none. So I believe
> > this was done on purpose and this controller supports mixed DTR ops.  
> 
> I see that your other patches in this series touch this driver so you 
> should have the datasheet right? Can you look and see for sure if it 
> does? I am fine with the patch as it is but if we can make the check 
> stricter it would be nice.

As discussed on IRC, I've checked the datasheet and indeed the
following modes are supported:
* 1S-1D-1D
* 1S-2D-2D
* 1S-4D-4D
* 4S-4D-4D

So yes, even if there is no use of these modes yet, certain mixed modes
are supported by this controller.

> 
> >   
> > > Reviewed-by: Pratyush Yadav <p.yadav@ti.com>  

Thanks,
Miquèl

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Pratyush Yadav]

On 21/12/21 11:16AM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530:
> 
> > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure  
> > 
> > s/cadence/cadence-quadspi/
> 
> Right.
> 
> > 
> > On 17/12/21 05:16PM, Miquel Raynal wrote:
> > > This controller has DTR support, so advertize it with a capability now
> > > that the spi_controller_mem_ops structure contains this new field. This
> > > will later be used by the core to discriminate whether an operation is
> > > supported or not, in a more generic way than having different helpers.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/spi/spi-cadence-quadspi.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > > index 101cc71bffa7..98e0cc4236e3 100644
> > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem)
> > >  	return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select);
> > >  }
> > >  
> > > +static const struct spi_controller_mem_caps cqspi_mem_caps = {
> > > +	.dtr = true,
> > > +};
> > > +
> > >  static const struct spi_controller_mem_ops cqspi_mem_ops = {
> > >  	.exec_op = cqspi_exec_mem_op,
> > >  	.get_name = cqspi_get_name,
> > >  	.supports_op = cqspi_supports_mem_op,
> > > +	.caps = &cqspi_mem_caps,  
> > 
> > I just noticed you put it under struct spi_mem_ops, not under struct 
> > spi_mem. This is not an operation per se so wouldn't it be better if it 
> > is moved to struct spi_mem?
> 
> I had a hard time taking a decision but my conclusion was that these
> caps are static controller capabilities and exclusively tight to the
> controller. The spi_mem structure defines a SPI peripheral. The
> spi_mem_ops structure is the only spi-mem related field of the
> spi-controller structure. I could have added my own field there but
> as these caps are only meant to be used by the spi_mem_ops anyway
> (exclusively ->supports_op() for now), it seemed to be a good location,
> at least better than the spi-mem structure.

Can we have a 3rd person chime in and break the tie? :-)

> 
> > Anyway, the change itself looks good to me. The cqspi_supports_mem_op()
> > already checks for mixed DTR modes so we should be good.
> 
> Yep!
> 
> > 
> > >  };
> > >  
> > >  static int cqspi_setup_flash(struct cqspi_st *cqspi)
> > > -- 
> > > 2.27.0
> > >   
> > 
> 
> 
> Thanks,
> Miquèl

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 16:11:10 +0530:

> On 21/12/21 11:16AM, Miquel Raynal wrote:
> > Hi Pratyush,
> > 
> > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530:
> >   
> > > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure    
> > > 
> > > s/cadence/cadence-quadspi/  
> > 
> > Right.
> >   
> > > 
> > > On 17/12/21 05:16PM, Miquel Raynal wrote:  
> > > > This controller has DTR support, so advertize it with a capability now
> > > > that the spi_controller_mem_ops structure contains this new field. This
> > > > will later be used by the core to discriminate whether an operation is
> > > > supported or not, in a more generic way than having different helpers.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/spi/spi-cadence-quadspi.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > > > index 101cc71bffa7..98e0cc4236e3 100644
> > > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem)
> > > >  	return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select);
> > > >  }
> > > >  
> > > > +static const struct spi_controller_mem_caps cqspi_mem_caps = {
> > > > +	.dtr = true,
> > > > +};
> > > > +
> > > >  static const struct spi_controller_mem_ops cqspi_mem_ops = {
> > > >  	.exec_op = cqspi_exec_mem_op,
> > > >  	.get_name = cqspi_get_name,
> > > >  	.supports_op = cqspi_supports_mem_op,
> > > > +	.caps = &cqspi_mem_caps,    
> > > 
> > > I just noticed you put it under struct spi_mem_ops, not under struct 
> > > spi_mem. This is not an operation per se so wouldn't it be better if it 
> > > is moved to struct spi_mem?  
> > 
> > I had a hard time taking a decision but my conclusion was that these
> > caps are static controller capabilities and exclusively tight to the
> > controller. The spi_mem structure defines a SPI peripheral. The
> > spi_mem_ops structure is the only spi-mem related field of the
> > spi-controller structure. I could have added my own field there but
> > as these caps are only meant to be used by the spi_mem_ops anyway
> > (exclusively ->supports_op() for now), it seemed to be a good location,
> > at least better than the spi-mem structure.  
> 
> Can we have a 3rd person chime in and break the tie? :-)

I don't quite get why we should put controller specific information
into the memory device structure?

Anyway, do you mind if we move forward first? Not that I don't think
that this choice should be discussed further, but I think this can
easily be changed in the near future if there is a desire to
reorganize spi-mem objects. In fact, these capabilities are accessed
through a helper so that hypothetic change would be almost transparent.

Thanks,
Miquèl

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Pratyush Yadav]

On 21/12/21 12:19PM, Miquel Raynal wrote:
> Hi Pratyush,
> 
> p.yadav@ti.com wrote on Tue, 21 Dec 2021 16:11:10 +0530:
> 
> > On 21/12/21 11:16AM, Miquel Raynal wrote:
> > > Hi Pratyush,
> > > 
> > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:25:18 +0530:
> > >   
> > > > > Subject: [PATCH v7 04/14] spi: cadence: Provide a capability structure    
> > > > 
> > > > s/cadence/cadence-quadspi/  
> > > 
> > > Right.
> > >   
> > > > 
> > > > On 17/12/21 05:16PM, Miquel Raynal wrote:  
> > > > > This controller has DTR support, so advertize it with a capability now
> > > > > that the spi_controller_mem_ops structure contains this new field. This
> > > > > will later be used by the core to discriminate whether an operation is
> > > > > supported or not, in a more generic way than having different helpers.
> > > > > 
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > ---
> > > > >  drivers/spi/spi-cadence-quadspi.c | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c
> > > > > index 101cc71bffa7..98e0cc4236e3 100644
> > > > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > > > @@ -1388,10 +1388,15 @@ static const char *cqspi_get_name(struct spi_mem *mem)
> > > > >  	return devm_kasprintf(dev, GFP_KERNEL, "%s.%d", dev_name(dev), mem->spi->chip_select);
> > > > >  }
> > > > >  
> > > > > +static const struct spi_controller_mem_caps cqspi_mem_caps = {
> > > > > +	.dtr = true,
> > > > > +};
> > > > > +
> > > > >  static const struct spi_controller_mem_ops cqspi_mem_ops = {
> > > > >  	.exec_op = cqspi_exec_mem_op,
> > > > >  	.get_name = cqspi_get_name,
> > > > >  	.supports_op = cqspi_supports_mem_op,
> > > > > +	.caps = &cqspi_mem_caps,    
> > > > 
> > > > I just noticed you put it under struct spi_mem_ops, not under struct 
> > > > spi_mem. This is not an operation per se so wouldn't it be better if it 
> > > > is moved to struct spi_mem?  
> > > 
> > > I had a hard time taking a decision but my conclusion was that these
> > > caps are static controller capabilities and exclusively tight to the
> > > controller. The spi_mem structure defines a SPI peripheral. The
> > > spi_mem_ops structure is the only spi-mem related field of the
> > > spi-controller structure. I could have added my own field there but
> > > as these caps are only meant to be used by the spi_mem_ops anyway
> > > (exclusively ->supports_op() for now), it seemed to be a good location,
> > > at least better than the spi-mem structure.  
> > 
> > Can we have a 3rd person chime in and break the tie? :-)
> 
> I don't quite get why we should put controller specific information
> into the memory device structure?

Hmm, you're right. struct spi_controller would probably be a better 
place.

> 
> Anyway, do you mind if we move forward first? Not that I don't think
> that this choice should be discussed further, but I think this can
> easily be changed in the near future if there is a desire to
> reorganize spi-mem objects. In fact, these capabilities are accessed
> through a helper so that hypothetic change would be almost transparent.

Okay. I would still like to hear other opinions on this, but fine by me 
if you want to take this in as-is.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v7 07/14] spi: spi-mem: Add an ecc_en parameter to the spi_mem_op structure

[Miquel Raynal]

Hi Pratyush,

p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:32:29 +0530:

> On 17/12/21 05:16PM, Miquel Raynal wrote:
> > Soon the SPI-NAND core will need a way to request a SPI controller to
> > enable ECC support for a given operation. This is because of the
> > pipelined integration of certain ECC engines, which are directly managed
> > by the SPI controller itself.
> > 
> > Introduce a spi_mem_op additional field for this purpose: ecc_en.
> > 
> > So far this field is left unset and checked to be false by all
> > the SPI controller drivers in their ->supports_op() hook, as they all
> > call spi_mem_default_supports_op().
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/spi/spi-mem.c       | 5 +++++
> >  include/linux/spi/spi-mem.h | 5 +++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index cfe1c99db5f3..94758e7e747d 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -178,6 +178,11 @@ bool spi_mem_default_supports_op(struct spi_mem *mem,
> >  			return false;
> >  	}
> >  
> > +	if (op->ecc_en) {
> > +		if (!spi_mem_controller_is_capable(ctlr, ecc))
> > +			return false;
> > +	}
> > +
> >  	return spi_mem_check_buswidth(mem, op);
> >  }
> >  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 d7787c8f3746..e9238a858109 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -94,6 +94,7 @@ enum spi_mem_data_dir {
> >   *		 operation does not involve transferring data
> >   * @data.buf.in: input buffer (must be DMA-able)
> >   * @data.buf.out: output buffer (must be DMA-able)
> > + * @ecc_en: error correction is required
> >   */
> >  struct spi_mem_op {
> >  	struct {
> > @@ -126,6 +127,8 @@ struct spi_mem_op {
> >  			const void *out;
> >  		} buf;
> >  	} data;
> > +
> > +	bool ecc_en;  
> 
> ECC should only concern the data phase right? Would it make more sense 
> to move this field under data?
> 
> Anyway, I don't know much about NAND or ECC so either way,

I've moved the field to the data struct because it makes indeed more
sense. I've also changed the type and name to "u8 ecc : 1;" to match
the style used for the dtr parameter.

> 
> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> 
> >  };
> >  
> >  #define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\
> > @@ -223,9 +226,11 @@ static inline void *spi_mem_get_drvdata(struct spi_mem *mem)
> >  /**
> >   * struct spi_controller_mem_caps - SPI memory controller capabilities
> >   * @dtr: Supports DTR operations
> > + * @ecc: Supports operations with error correction
> >   */
> >  struct spi_controller_mem_caps {
> >  	bool dtr;
> > +	bool ecc;
> >  };
> >  
> >  #define spi_mem_controller_is_capable(ctlr, cap)		\
> > -- 
> > 2.27.0
> >   
> 


Thanks,
Miquèl

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Boris Brezillon]

On Tue, 21 Dec 2021 17:35:00 +0530
Pratyush Yadav <p.yadav@ti.com> wrote:

> > 
> > Anyway, do you mind if we move forward first? Not that I don't think
> > that this choice should be discussed further, but I think this can
> > easily be changed in the near future if there is a desire to
> > reorganize spi-mem objects. In fact, these capabilities are accessed
> > through a helper so that hypothetic change would be almost transparent.  
> 
> Okay. I would still like to hear other opinions on this, but fine by me 
> if you want to take this in as-is.

I think we discussed that with Miquel, and I remember complaining about
mixing function pointers and actual data in the spi_mem_ops struct, but
honestly, it's just cosmetic concern, and I don't think it matters much
in practice. So I'm fine either way, make it a field of spi_controller
or spi_mem_ops, spi_mem is definitely not the right place though.

Re: [PATCH v7 04/14] spi: cadence: Provide a capability structure

[Miquel Raynal]

Hello,

boris.brezillon@collabora.com wrote on Mon, 3 Jan 2022 09:38:19 +0100:

> On Tue, 21 Dec 2021 17:35:00 +0530
> Pratyush Yadav <p.yadav@ti.com> wrote:
> 
> > > 
> > > Anyway, do you mind if we move forward first? Not that I don't think
> > > that this choice should be discussed further, but I think this can
> > > easily be changed in the near future if there is a desire to
> > > reorganize spi-mem objects. In fact, these capabilities are accessed
> > > through a helper so that hypothetic change would be almost transparent.    
> > 
> > Okay. I would still like to hear other opinions on this, but fine by me 
> > if you want to take this in as-is.  
> 
> I think we discussed that with Miquel, and I remember complaining about
> mixing function pointers and actual data in the spi_mem_ops struct, but
> honestly, it's just cosmetic concern, and I don't think it matters much
> in practice. So I'm fine either way, make it a field of spi_controller
> or spi_mem_ops, spi_mem is definitely not the right place though.

Yeah, I don't like the idea of leaking spi-mem information into the spi
controller structure, while there is a structure (so far only
containing hooks) that is dedicated to spi-mem operations. Extending
this structure to contain capabilities appeared the right choice to me.
But on the other hand this is a controller information anyway so if you
both prefer moving this data into the SPI controller structure I'll find
a way to do it.

Thanks,
Miquèl