[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[Sean Anderson]

This series adds support for enhanced SPI modes. It was tested on a K210 (DWC
SSI with QSPI flash).

If anyone has a designware device with QSPI flash attached (especially a DW SSI
APB device), I'd greatly appreciate them testing out this patch series. Given
that there has been no testing of v2 over the past month, I don't think lack of
testing should hold up this series.

Changes in v3:
- Dropped merged patches
- Rebased on u-boot/master

Changes in v2:
- Add more information to exec_op debug message
- Actually mask interrupts
- Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED
- Fix some inconsistencies in register naming and usage
- Moved some hunks between commits so things make more sense

Sean Anderson (10):
  mtd: spi-mem: Export spi_mem_default_supports_op
  spi: spi-mem: Add debug message for spi-mem ops
  spi: dw: Log status register on timeout
  spi: dw: Actually mask interrupts
  spi: dw: Switch to capabilities
  spi: dw: Rewrite poll_transfer logic
  spi: dw: Add ENHANCED cap
  spi: dw: Define registers for enhanced mode
  spi: dw: Support enhanced SPI
  spi: dw: Support clock stretching

 drivers/spi/designware_spi.c | 647 ++++++++++++++++++++++++-----------
 drivers/spi/spi-mem.c        |   7 +
 include/spi-mem.h            |   3 +
 3 files changed, 451 insertions(+), 206 deletions(-)

-- 
2.31.0

[PATCH v2 01/10] mtd: spi-mem: Export spi_mem_default_supports_op

[Sean Anderson]

This is useful for extending the default functionality. This mirrors the
change in Linux commit 46109648052f ("spi: spi-mem: export
spi_mem_default_supports_op()").

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---

(no changes since v1)

 include/spi-mem.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/spi-mem.h b/include/spi-mem.h
index 8be3e2bf6b..a404d3bbee 100644
--- a/include/spi-mem.h
+++ b/include/spi-mem.h
@@ -236,6 +236,9 @@ spi_controller_dma_unmap_mem_op_data(struct spi_controller *ctlr,
 
 int spi_mem_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op);
 
+bool spi_mem_default_supports_op(struct spi_slave *slave,
+				 const struct spi_mem_op *op);
+
 bool spi_mem_supports_op(struct spi_slave *slave, const struct spi_mem_op *op);
 
 int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op);
-- 
2.31.0

[PATCH v2 02/10] spi: spi-mem: Add debug message for spi-mem ops

[Sean Anderson]

This prints some basic metadata about the SPI memory op. This information
may be used to debug SPI drivers (e.g. determining the expected SPI mode).
It is also helpful for verifying that the data on the wire matches the data
intended to be transmitted (e.g. with a logic analyzer). The opcode is
printed with a format of %02Xh to match the notation commonly used in flash
datasheets.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---

(no changes since v2)

Changes in v2:
- Add more information to exec_op debug message

 drivers/spi/spi-mem.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index c095ae9505..6772367ef7 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -220,6 +220,13 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	int ret;
 	int i;
 
+	dev_dbg(slave->dev,
+		"exec %02Xh %u-%u-%u addr=%llx dummy cycles=%u data bytes=%u\n",
+		op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
+		op->data.buswidth, op->addr.val,
+		op->dummy.buswidth ? op->dummy.nbytes * 8 / op->dummy.buswidth : 0,
+		op->data.nbytes);
+
 	if (!spi_mem_supports_op(slave, op))
 		return -ENOTSUPP;
 
-- 
2.31.0

[PATCH v2 03/10] spi: dw: Log status register on timeout

[Sean Anderson]

This logs the status register on timeout, so it is easier to determine the
cause of the failure.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/spi/designware_spi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 742121140d..519d6e32bd 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -552,6 +552,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
 			       (val & SR_TF_EMPT) && !(val & SR_BUSY),
 			       RX_TIMEOUT * 1000)) {
+		dev_dbg(bus, "timed out; sr=%x\n", dw_read(priv, DW_SPI_SR));
 		ret = -ETIMEDOUT;
 	}
 
@@ -639,6 +640,8 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
 				       (val & SR_TF_EMPT) && !(val & SR_BUSY),
 				       RX_TIMEOUT * 1000)) {
+			dev_dbg(bus, "timed out; sr=%x\n",
+				dw_read(priv, DW_SPI_SR));
 			ret = -ETIMEDOUT;
 		}
 	}
-- 
2.31.0

[PATCH v2 04/10] spi: dw: Actually mask interrupts

[Sean Anderson]

Writing 1s to this register *unmasks* interrupts. Mask them instead.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- Actually mask interrupts

 drivers/spi/designware_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 519d6e32bd..4ef948a0b9 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -252,7 +252,7 @@ static int dw_spi_of_to_plat(struct udevice *bus)
 static void spi_hw_init(struct udevice *bus, struct dw_spi_priv *priv)
 {
 	dw_write(priv, DW_SPI_SSIENR, 0);
-	dw_write(priv, DW_SPI_IMR, 0xff);
+	dw_write(priv, DW_SPI_IMR, 0);
 	dw_write(priv, DW_SPI_SSIENR, 1);
 
 	/*
-- 
2.31.0

[PATCH v2 05/10] spi: dw: Switch to capabilities

[Sean Anderson]

Since Linux commit cc760f3143f5 ("spi: dw: Convert CS-override to DW SPI
capabilities"), the Linux driver has used capability flags instead of
using ad-hoc flags and functions. This is a great idea, and we should use
it as well.

The .data field in the compatible array has switched from being an
initialization function to being a set of default capabilities. This is
necessary since some capabilities cannot be determined at runtime.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/spi/designware_spi.c | 151 +++++++++++++++++------------------
 1 file changed, 73 insertions(+), 78 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 4ef948a0b9..6375e6d778 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -123,9 +123,13 @@ struct dw_spi_priv {
 	struct reset_ctl_bulk resets;
 	struct gpio_desc cs_gpio;	/* External chip-select gpio */
 
-	u32 (*update_cr0)(struct dw_spi_priv *priv);
-
 	void __iomem *regs;
+/* DW SPI capabilities */
+#define DW_SPI_CAP_CS_OVERRIDE		BIT(0) /* Unimplemented */
+#define DW_SPI_CAP_KEEMBAY_MST		BIT(1) /* Unimplemented */
+#define DW_SPI_CAP_DWC_SSI		BIT(2)
+#define DW_SPI_CAP_DFS32		BIT(3)
+	unsigned long caps;
 	unsigned long bus_clk_rate;
 	unsigned int freq;		/* Default frequency */
 	unsigned int mode;
@@ -135,7 +139,6 @@ struct dw_spi_priv {
 	void *rx;
 	void *rx_end;
 	u32 fifo_len;			/* depth of the FIFO buffer */
-	u32 max_xfer;			/* Maximum transfer size (in bits) */
 
 	int bits_per_word;
 	int len;
@@ -154,51 +157,30 @@ static inline void dw_write(struct dw_spi_priv *priv, u32 offset, u32 val)
 	__raw_writel(val, priv->regs + offset);
 }
 
-static u32 dw_spi_dw16_update_cr0(struct dw_spi_priv *priv)
+static u32 dw_spi_update_cr0(struct dw_spi_priv *priv)
 {
-	return FIELD_PREP(CTRLR0_DFS_MASK, priv->bits_per_word - 1)
-	     | FIELD_PREP(CTRLR0_FRF_MASK, priv->type)
-	     | FIELD_PREP(CTRLR0_MODE_MASK, priv->mode)
-	     | FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode);
-}
+	u32 cr0;
 
-static u32 dw_spi_dw32_update_cr0(struct dw_spi_priv *priv)
-{
-	return FIELD_PREP(CTRLR0_DFS_32_MASK, priv->bits_per_word - 1)
-	     | FIELD_PREP(CTRLR0_FRF_MASK, priv->type)
-	     | FIELD_PREP(CTRLR0_MODE_MASK, priv->mode)
-	     | FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode);
-}
-
-static u32 dw_spi_dwc_update_cr0(struct dw_spi_priv *priv)
-{
-	return FIELD_PREP(DWC_SSI_CTRLR0_DFS_MASK, priv->bits_per_word - 1)
-	     | FIELD_PREP(DWC_SSI_CTRLR0_FRF_MASK, priv->type)
-	     | FIELD_PREP(DWC_SSI_CTRLR0_MODE_MASK, priv->mode)
-	     | FIELD_PREP(DWC_SSI_CTRLR0_TMOD_MASK, priv->tmode);
-}
-
-static int dw_spi_apb_init(struct udevice *bus, struct dw_spi_priv *priv)
-{
-	/* If we read zeros from DFS, then we need to use DFS_32 instead */
-	dw_write(priv, DW_SPI_SSIENR, 0);
-	dw_write(priv, DW_SPI_CTRLR0, 0xffffffff);
-	if (FIELD_GET(CTRLR0_DFS_MASK, dw_read(priv, DW_SPI_CTRLR0))) {
-		priv->max_xfer = 16;
-		priv->update_cr0 = dw_spi_dw16_update_cr0;
+	if (priv->caps & DW_SPI_CAP_DWC_SSI) {
+		cr0 = FIELD_PREP(DWC_SSI_CTRLR0_DFS_MASK,
+				 priv->bits_per_word - 1)
+		    | FIELD_PREP(DWC_SSI_CTRLR0_FRF_MASK, priv->type)
+		    | FIELD_PREP(DWC_SSI_CTRLR0_MODE_MASK, priv->mode)
+		    | FIELD_PREP(DWC_SSI_CTRLR0_TMOD_MASK, priv->tmode);
 	} else {
-		priv->max_xfer = 32;
-		priv->update_cr0 = dw_spi_dw32_update_cr0;
+		if (priv->caps & DW_SPI_CAP_DFS32)
+			cr0 = FIELD_PREP(CTRLR0_DFS_32_MASK,
+					 priv->bits_per_word - 1);
+		else
+			cr0 = FIELD_PREP(CTRLR0_DFS_MASK,
+					 priv->bits_per_word - 1);
+
+		cr0 |= FIELD_PREP(CTRLR0_FRF_MASK, priv->type)
+		    |  FIELD_PREP(CTRLR0_MODE_MASK, priv->mode)
+		    |  FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode);
 	}
 
-	return 0;
-}
-
-static int dw_spi_dwc_init(struct udevice *bus, struct dw_spi_priv *priv)
-{
-	priv->max_xfer = 32;
-	priv->update_cr0 = dw_spi_dwc_update_cr0;
-	return 0;
+	return cr0;
 }
 
 static int request_gpio_cs(struct udevice *bus)
@@ -251,8 +233,26 @@ static int dw_spi_of_to_plat(struct udevice *bus)
 /* Restart the controller, disable all interrupts, clean rx fifo */
 static void spi_hw_init(struct udevice *bus, struct dw_spi_priv *priv)
 {
+	u32 cr0;
+
 	dw_write(priv, DW_SPI_SSIENR, 0);
 	dw_write(priv, DW_SPI_IMR, 0);
+
+	/*
+	 * Detect features by writing CTRLR0 and seeing which fields remain
+	 * zeroed.
+	 */
+	dw_write(priv, DW_SPI_SSIENR, 0);
+	dw_write(priv, DW_SPI_CTRLR0, 0xffffffff);
+	cr0 = dw_read(priv, DW_SPI_CTRLR0);
+
+	/*
+	 * DWC_SPI always has DFS_32. If we read zeros from DFS, then we need to
+	 * use DFS_32 instead
+	 */
+	if (priv->caps & DW_SPI_CAP_DWC_SSI || !FIELD_GET(CTRLR0_DFS_MASK, cr0))
+		priv->caps |= DW_SPI_CAP_DFS32;
+
 	dw_write(priv, DW_SPI_SSIENR, 1);
 
 	/*
@@ -271,7 +271,6 @@ static void spi_hw_init(struct udevice *bus, struct dw_spi_priv *priv)
 		priv->fifo_len = (fifo == 1) ? 0 : fifo;
 		dw_write(priv, DW_SPI_TXFTLR, 0);
 	}
-	dev_dbg(bus, "fifo_len=%d\n", priv->fifo_len);
 }
 
 /*
@@ -337,11 +336,8 @@ static int dw_spi_reset(struct udevice *bus)
 	return 0;
 }
 
-typedef int (*dw_spi_init_t)(struct udevice *bus, struct dw_spi_priv *priv);
-
 static int dw_spi_probe(struct udevice *bus)
 {
-	dw_spi_init_t init = (dw_spi_init_t)dev_get_driver_data(bus);
 	struct dw_spi_plat *plat = dev_get_plat(bus);
 	struct dw_spi_priv *priv = dev_get_priv(bus);
 	int ret;
@@ -358,25 +354,21 @@ static int dw_spi_probe(struct udevice *bus)
 	if (ret)
 		return ret;
 
-	if (!init)
-		return -EINVAL;
-	ret = init(bus, priv);
-	if (ret)
-		return ret;
-
-	version = dw_read(priv, DW_SPI_VERSION);
-	dev_dbg(bus, "ssi_version_id=%c.%c%c%c ssi_max_xfer_size=%u\n",
-		version >> 24, version >> 16, version >> 8, version,
-		priv->max_xfer);
-
 	/* Currently only bits_per_word == 8 supported */
 	priv->bits_per_word = 8;
 
 	priv->tmode = 0; /* Tx & Rx */
 
 	/* Basic HW init */
+	priv->caps = dev_get_driver_data(bus);
 	spi_hw_init(bus, priv);
 
+	version = dw_read(priv, DW_SPI_VERSION);
+	dev_dbg(bus,
+		"ssi_version_id=%c.%c%c%c ssi_rx_fifo_depth=%u ssi_max_xfer_size=%u\n",
+		version >> 24, version >> 16, version >> 8, version,
+		priv->fifo_len, priv->caps & DW_SPI_CAP_DFS32 ? 32 : 16);
+
 	return 0;
 }
 
@@ -510,7 +502,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		 */
 		priv->tmode = CTRLR0_TMOD_TR;
 
-	cr0 = priv->update_cr0(priv);
+	cr0 = dw_spi_update_cr0(priv);
 
 	priv->len = bitlen >> 3;
 
@@ -582,7 +574,7 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	else
 		priv->tmode = CTRLR0_TMOD_TO;
 
-	cr0 = priv->update_cr0(priv);
+	cr0 = dw_spi_update_cr0(priv);
 	dev_dbg(bus, "cr0=%08x buf=%p len=%u [bytes]\n", cr0, op->data.buf.in,
 		op->data.nbytes);
 
@@ -742,15 +734,15 @@ static const struct dm_spi_ops dw_spi_ops = {
 static const struct udevice_id dw_spi_ids[] = {
 	/* Generic compatible strings */
 
-	{ .compatible = "snps,dw-apb-ssi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "snps,dw-apb-ssi-3.20a", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "snps,dw-apb-ssi-3.22a", .data = (ulong)dw_spi_apb_init },
+	{ .compatible = "snps,dw-apb-ssi" },
+	{ .compatible = "snps,dw-apb-ssi-3.20a" },
+	{ .compatible = "snps,dw-apb-ssi-3.22a" },
 	/* First version with SSI_MAX_XFER_SIZE */
-	{ .compatible = "snps,dw-apb-ssi-3.23a", .data = (ulong)dw_spi_apb_init },
-	/* First version with Dual/Quad SPI; unused by this driver */
-	{ .compatible = "snps,dw-apb-ssi-4.00a", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "snps,dw-apb-ssi-4.01", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "snps,dwc-ssi-1.01a", .data = (ulong)dw_spi_dwc_init },
+	{ .compatible = "snps,dw-apb-ssi-3.23a" },
+	/* First version with Dual/Quad SPI */
+	{ .compatible = "snps,dw-apb-ssi-4.00a" },
+	{ .compatible = "snps,dw-apb-ssi-4.01" },
+	{ .compatible = "snps,dwc-ssi-1.01a", .data = DW_SPI_CAP_DWC_SSI },
 
 	/* Compatible strings for specific SoCs */
 
@@ -759,16 +751,19 @@ static const struct udevice_id dw_spi_ids[] = {
 	 * version of this device. This compatible string is used for those
 	 * devices, and is not used for sofpgas in general.
 	 */
-	{ .compatible = "altr,socfpga-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "altr,socfpga-arria10-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "canaan,kendryte-k210-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "canaan,kendryte-k210-ssi", .data = (ulong)dw_spi_dwc_init },
-	{ .compatible = "intel,stratix10-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "intel,agilex-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "mscc,ocelot-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "mscc,jaguar2-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "snps,axs10x-spi", .data = (ulong)dw_spi_apb_init },
-	{ .compatible = "snps,hsdk-spi", .data = (ulong)dw_spi_apb_init },
+	{ .compatible = "altr,socfpga-spi" },
+	{ .compatible = "altr,socfpga-arria10-spi" },
+	{ .compatible = "canaan,kendryte-k210-spi" },
+	{
+		.compatible = "canaan,kendryte-k210-ssi",
+		.data = DW_SPI_CAP_DWC_SSI,
+	},
+	{ .compatible = "intel,stratix10-spi" },
+	{ .compatible = "intel,agilex-spi" },
+	{ .compatible = "mscc,ocelot-spi" },
+	{ .compatible = "mscc,jaguar2-spi" },
+	{ .compatible = "snps,axs10x-spi" },
+	{ .compatible = "snps,hsdk-spi" },
 	{ }
 };
 
-- 
2.31.0

[PATCH v2 06/10] spi: dw: Rewrite poll_transfer logic

[Sean Anderson]

This rewrites poll_transfer, dw_writer, and dw_reader.

* We now use RO transfers (instead of always using TR). This eliminates the
  need to send out dummy words, and simplifies the transmit logic.
* All parameters (except regs and bits_per_word) are passed explicitly.
* Most parameters have been made explicit (instead of being recalculated on
  every loop).
* Transfers are measured in units of frames instead of bytes. This matches
  the measurements used by the device and eliminates several divisions by
  bits_per_word.
* We now check if we have over-/under-flowed the FIFO. This should help
  prevent hangs when the device stops the transfer and U-Boot doesn't
  realize it (and then waits for the remaining data forever). TXUI is not
  present in DW_APB_SSI, but in the worst case we just don't write data.

  Unfortunately, this doesn't seem to solve all problems, and there are
  some remaining bugs (such as some transfers containing all 1s or all 0s)
  when we have many fifo overflows. This is solved for DWC devices by
  enabling clock stretching.
* poll_transfer is now used by dw_spi_exec_op as well as xfer.
* There are separate inner loops for 8-, 16-, and 32-bit frame sizes. This
  should hopefully reduce the amount of over-/under-flows. However, I
  haven't done exhaustive testing to ensure this is really necessary. In
  particular, I was never able to prevent read overflows at 50MHz clock
  speed.

These changes should probably have been split up into several commits, but
several depend on each other, and it would be difficult to break this up
while preserving bisectability.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/spi/designware_spi.c | 284 +++++++++++++++++++++--------------
 1 file changed, 171 insertions(+), 113 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 6375e6d778..72cca14887 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -134,14 +134,10 @@ struct dw_spi_priv {
 	unsigned int freq;		/* Default frequency */
 	unsigned int mode;
 
-	const void *tx;
-	const void *tx_end;
-	void *rx;
-	void *rx_end;
 	u32 fifo_len;			/* depth of the FIFO buffer */
 
 	int bits_per_word;
-	int len;
+	int frames;			/* Number of frames in the transfer */
 	u8 cs;				/* chip select pin */
 	u8 tmode;			/* TR/TO/RO/EEPROM */
 	u8 type;			/* SPI/SSP/MicroWire */
@@ -372,14 +368,24 @@ static int dw_spi_probe(struct udevice *bus)
 	return 0;
 }
 
-/* Return the max entries we can fill into tx fifo */
-static inline u32 tx_max(struct dw_spi_priv *priv)
+/**
+ * dw_writer() - Write data frames to the tx fifo
+ * @priv: Driver private info
+ * @tx: The tx buffer
+ * @idx: The number of data frames already transmitted
+ * @tx_frames: The number of data frames left to transmit
+ * @rx_frames: The number of data frames left to receive (0 if only
+ *             transmitting)
+ * @frame_bytes: The number of bytes taken up by one data frame
+ *
+ * This function writes up to @tx_frames data frames using data from @tx[@idx].
+ *
+ * Return: The number of frames read
+ */
+static uint dw_writer(struct dw_spi_priv *priv, const void *tx, uint idx,
+		      uint tx_frames, uint rx_frames, uint frame_bytes)
 {
-	u32 tx_left, tx_room, rxtx_gap;
-
-	tx_left = (priv->tx_end - priv->tx) / (priv->bits_per_word >> 3);
-	tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
-
+	u32 tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
 	/*
 	 * Another concern is about the tx/rx mismatch, we
 	 * thought about using (priv->fifo_len - rxflr - txflr) as
@@ -388,67 +394,131 @@ static inline u32 tx_max(struct dw_spi_priv *priv)
 	 * shift registers. So a control from sw point of
 	 * view is taken.
 	 */
-	rxtx_gap = ((priv->rx_end - priv->rx) - (priv->tx_end - priv->tx)) /
-		(priv->bits_per_word >> 3);
+	u32 rxtx_gap = rx_frames - tx_frames;
+	u32 count = min3(tx_frames, tx_room, (u32)(priv->fifo_len - rxtx_gap));
+	u32 *dr = priv->regs + DW_SPI_DR;
 
-	return min3(tx_left, tx_room, (u32)(priv->fifo_len - rxtx_gap));
-}
+	if (!count)
+		return 0;
 
-/* Return the max entries we should read out of rx fifo */
-static inline u32 rx_max(struct dw_spi_priv *priv)
-{
-	u32 rx_left = (priv->rx_end - priv->rx) / (priv->bits_per_word >> 3);
-
-	return min_t(u32, rx_left, dw_read(priv, DW_SPI_RXFLR));
-}
-
-static void dw_writer(struct dw_spi_priv *priv)
-{
-	u32 max = tx_max(priv);
-	u32 txw = 0xFFFFFFFF;
-
-	while (max--) {
-		/* Set the tx word if the transfer's original "tx" is not null */
-		if (priv->tx_end - priv->len) {
-			if (priv->bits_per_word == 8)
-				txw = *(u8 *)(priv->tx);
-			else
-				txw = *(u16 *)(priv->tx);
-		}
-		dw_write(priv, DW_SPI_DR, txw);
-		log_content("tx=0x%02x\n", txw);
-		priv->tx += priv->bits_per_word >> 3;
+#define do_write(type) do { \
+	type *start = ((type *)tx) + idx; \
+	type *end = start + count; \
+	do { \
+		__raw_writel(*start++, dr); \
+	} while (start < end); \
+} while (0)
+	switch (frame_bytes) {
+	case 1:
+		do_write(u8);
+		break;
+	case 2:
+		do_write(u16);
+		break;
+	case 3:
+	case 4:
+	default:
+		do_write(u32);
+		break;
 	}
+#undef do_write
+
+	return count;
 }
 
-static void dw_reader(struct dw_spi_priv *priv)
+/**
+ * dw_reader() - Read data frames from the rx fifo
+ * @priv: Driver private data
+ * @rx: The rx buffer
+ * @idx: The number of data frames already received
+ * @frames: The number of data frames left to receive
+ * @frame_bytes: The size of a data frame in bytes
+ *
+ * This function reads up to @frames data frames from @rx[@idx].
+ *
+ * Return: The number of frames read
+ */
+static uint dw_reader(struct dw_spi_priv *priv, void *rx, uint idx, uint frames,
+		      uint frame_bytes)
 {
-	u32 max = rx_max(priv);
-	u16 rxw;
+	u32 rx_lvl = dw_read(priv, DW_SPI_RXFLR);
+	u32 count = min(frames, rx_lvl);
+	u32 *dr = priv->regs + DW_SPI_DR;
 
-	while (max--) {
-		rxw = dw_read(priv, DW_SPI_DR);
-		log_content("rx=0x%02x\n", rxw);
+	if (!count)
+		return 0;
 
-		/* Care about rx if the transfer's original "rx" is not null */
-		if (priv->rx_end - priv->len) {
-			if (priv->bits_per_word == 8)
-				*(u8 *)(priv->rx) = rxw;
-			else
-				*(u16 *)(priv->rx) = rxw;
-		}
-		priv->rx += priv->bits_per_word >> 3;
+#define do_read(type) do { \
+	type *start = ((type *)rx) + idx; \
+	type *end = start + count; \
+	do { \
+		*start++ = __raw_readl(dr); \
+	} while (start < end); \
+} while (0)
+	switch (frame_bytes) {
+	case 1:
+		do_read(u8);
+		break;
+	case 2:
+		do_read(u16);
+		break;
+	case 3:
+	case 4:
+	default:
+		do_read(u32);
+		break;
 	}
+#undef do_read
+
+	return count;
 }
 
-static int poll_transfer(struct dw_spi_priv *priv)
+/**
+ * poll_transfer() - Transmit and receive data frames
+ * @priv: Driver private data
+ * @tx: The tx buffer. May be %NULL to only receive.
+ * @rx: The rx buffer. May be %NULL to discard read data.
+ * @frames: The number of data frames to transfer
+ *
+ * Transmit @tx, while recieving @rx.
+ *
+ * Return: The lesser of the number of frames transmitted or received.
+ */
+static uint poll_transfer(struct dw_spi_priv *priv, const void *tx, void *rx,
+			  uint frames)
 {
-	do {
-		dw_writer(priv);
-		dw_reader(priv);
-	} while (priv->rx_end > priv->rx);
+	uint frame_bytes = priv->bits_per_word >> 3;
+	uint tx_idx = 0;
+	uint rx_idx = 0;
+	uint tx_frames = tx ? frames : 0;
+	uint rx_frames = rx ? frames : 0;
 
-	return 0;
+	while (tx_frames || rx_frames) {
+		if (tx_frames) {
+			uint tx_diff = dw_writer(priv, tx, tx_idx, tx_frames,
+						 rx_frames, frame_bytes);
+
+			tx_idx += tx_diff;
+			tx_frames -= tx_diff;
+		}
+
+		if (rx_frames) {
+			uint rx_diff = dw_reader(priv, rx, rx_idx, rx_frames,
+						 frame_bytes);
+
+			rx_idx += rx_diff;
+			rx_frames -= rx_diff;
+		}
+
+		/*
+		 * If we don't read/write fast enough, the transfer stops.
+		 * Don't bother reading out what's left in the FIFO; it's
+		 * garbage.
+		 */
+		if (dw_read(priv, DW_SPI_RISR) & (ISR_RXOI | ISR_TXUI))
+			break;
+	}
+	return min(tx ? tx_idx : rx_idx, rx ? rx_idx : tx_idx);
 }
 
 /*
@@ -474,19 +544,21 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 {
 	struct udevice *bus = dev->parent;
 	struct dw_spi_priv *priv = dev_get_priv(bus);
-	const u8 *tx = dout;
+	const void *tx = dout;
 	u8 *rx = din;
 	int ret = 0;
 	u32 cr0 = 0;
-	u32 val;
-	u32 cs;
+	u32 val, cs;
+	uint frames;
 
 	/* spi core configured to do 8 bit transfers */
-	if (bitlen % 8) {
+	if (bitlen % priv->bits_per_word) {
 		dev_err(dev, "Non byte aligned SPI transfer.\n");
 		return -1;
 	}
 
+	frames = bitlen / priv->bits_per_word;
+
 	/* Start the transaction if necessary. */
 	if (flags & SPI_XFER_BEGIN)
 		external_cs_manage(dev, false);
@@ -496,30 +568,22 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	else if (rx)
 		priv->tmode = CTRLR0_TMOD_RO;
 	else
-		/*
-		 * In transmit only mode (CTRL0_TMOD_TO) input FIFO never gets
-		 * any data which breaks our logic in poll_transfer() above.
-		 */
-		priv->tmode = CTRLR0_TMOD_TR;
+		priv->tmode = CTRLR0_TMOD_TO;
 
 	cr0 = dw_spi_update_cr0(priv);
 
-	priv->len = bitlen >> 3;
-
-	priv->tx = (void *)tx;
-	priv->tx_end = priv->tx + priv->len;
-	priv->rx = rx;
-	priv->rx_end = priv->rx + priv->len;
-
 	/* Disable controller before writing control registers */
 	dw_write(priv, DW_SPI_SSIENR, 0);
 
-	dev_dbg(dev, "cr0=%08x rx=%p tx=%p len=%d [bytes]\n", cr0, rx, tx,
-		priv->len);
+	dev_dbg(dev, "cr0=%08x rx=%p tx=%p frames=%d\n", cr0, rx, tx,
+		frames);
 	/* Reprogram cr0 only if changed */
 	if (dw_read(priv, DW_SPI_CTRLR0) != cr0)
 		dw_write(priv, DW_SPI_CTRLR0, cr0);
 
+	if (rx)
+		dw_write(priv, DW_SPI_CTRLR1, frames - 1);
+
 	/*
 	 * Configure the desired SS (slave select 0...3) in the controller
 	 * The DW SPI controller will activate and deactivate this CS
@@ -531,8 +595,15 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	/* Enable controller after writing control registers */
 	dw_write(priv, DW_SPI_SSIENR, 1);
 
+	/*
+	 * Prime the pump. RO-mode doesn't work unless something gets written to
+	 * the FIFO
+	 */
+	if (rx && !tx)
+		dw_write(priv, DW_SPI_DR, 0xFFFFFFFF);
+
 	/* Start transfer in a polling loop */
-	ret = poll_transfer(priv);
+	poll_transfer(priv, tx, rx, frames);
 
 	/*
 	 * Wait for current transmit operation to complete.
@@ -565,9 +636,10 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	int pos, i, ret = 0;
 	struct udevice *bus = slave->dev->parent;
 	struct dw_spi_priv *priv = dev_get_priv(bus);
+	struct spi_mem_op *mut_op = (struct spi_mem_op *)op;
 	u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
 	u8 op_buf[op_len];
-	u32 cr0;
+	u32 cr0, val;
 
 	if (read)
 		priv->tmode = CTRLR0_TMOD_EPROMREAD;
@@ -597,47 +669,33 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	if (op->dummy.nbytes)
 		memset(op_buf + pos, 0xff, op->dummy.nbytes);
 
-	external_cs_manage(slave->dev, false);
+	dw_writer(priv, &op_buf, 0, op_len, 0, sizeof(u8));
 
-	priv->tx = &op_buf;
-	priv->tx_end = priv->tx + op_len;
-	priv->rx = NULL;
-	priv->rx_end = NULL;
-	while (priv->tx != priv->tx_end)
-		dw_writer(priv);
+	external_cs_manage(slave->dev, false);
+	dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
 
 	/*
 	 * XXX: The following are tight loops! Enabling debug messages may cause
 	 * them to fail because we are not reading/writing the fifo fast enough.
 	 */
-	if (read) {
-		priv->rx = op->data.buf.in;
-		priv->rx_end = priv->rx + op->data.nbytes;
+	if (read)
+		mut_op->data.nbytes = poll_transfer(priv, NULL, op->data.buf.in,
+						    op->data.nbytes);
+	else
+		mut_op->data.nbytes = poll_transfer(priv, op->data.buf.out,
+						    NULL, op->data.nbytes);
 
-		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
-		while (priv->rx != priv->rx_end)
-			dw_reader(priv);
-	} else {
-		u32 val;
-
-		priv->tx = op->data.buf.out;
-		priv->tx_end = priv->tx + op->data.nbytes;
-
-		/* Fill up the write fifo before starting the transfer */
-		dw_writer(priv);
-		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
-		while (priv->tx != priv->tx_end)
-			dw_writer(priv);
-
-		if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
-				       (val & SR_TF_EMPT) && !(val & SR_BUSY),
-				       RX_TIMEOUT * 1000)) {
-			dev_dbg(bus, "timed out; sr=%x\n",
-				dw_read(priv, DW_SPI_SR));
-			ret = -ETIMEDOUT;
-		}
+	/*
+	 * Ensure the data (or the instruction for zero-data instructions) has
+	 * been transmitted from the fifo/shift register before disabling the
+	 * device.
+	 */
+	if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
+			       (val & SR_TF_EMPT) && !(val & SR_BUSY),
+			       RX_TIMEOUT * 1000)) {
+		dev_dbg(bus, "timed out; sr=%x\n", dw_read(priv, DW_SPI_SR));
+		ret = -ETIMEDOUT;
 	}
-
 	dw_write(priv, DW_SPI_SER, 0);
 	external_cs_manage(slave->dev, true);
 
-- 
2.31.0

[PATCH v2 06/10] spi: dw: Rewrite poll_transfer logic

[Damien Le Moal]

On 2021/04/03 8:05, Sean Anderson wrote:
> This rewrites poll_transfer, dw_writer, and dw_reader.
> 
> * We now use RO transfers (instead of always using TR). This eliminates the
>   need to send out dummy words, and simplifies the transmit logic.
> * All parameters (except regs and bits_per_word) are passed explicitly.
> * Most parameters have been made explicit (instead of being recalculated on
>   every loop).
> * Transfers are measured in units of frames instead of bytes. This matches
>   the measurements used by the device and eliminates several divisions by
>   bits_per_word.
> * We now check if we have over-/under-flowed the FIFO. This should help
>   prevent hangs when the device stops the transfer and U-Boot doesn't
>   realize it (and then waits for the remaining data forever). TXUI is not
>   present in DW_APB_SSI, but in the worst case we just don't write data.
> 
>   Unfortunately, this doesn't seem to solve all problems, and there are
>   some remaining bugs (such as some transfers containing all 1s or all 0s)
>   when we have many fifo overflows. This is solved for DWC devices by
>   enabling clock stretching.
> * poll_transfer is now used by dw_spi_exec_op as well as xfer.
> * There are separate inner loops for 8-, 16-, and 32-bit frame sizes. This
>   should hopefully reduce the amount of over-/under-flows. However, I
>   haven't done exhaustive testing to ensure this is really necessary. In
>   particular, I was never able to prevent read overflows at 50MHz clock
>   speed.

That sounds like the long fight I had on Linux side... Did you account for the
fact that the K210 has a buggy fifo depth (it reports 32 but is in fact 31) ?

> 
> These changes should probably have been split up into several commits, but
> several depend on each other, and it would be difficult to break this up
> while preserving bisectability.
> 
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/spi/designware_spi.c | 284 +++++++++++++++++++++--------------
>  1 file changed, 171 insertions(+), 113 deletions(-)
> 
> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
> index 6375e6d778..72cca14887 100644
> --- a/drivers/spi/designware_spi.c
> +++ b/drivers/spi/designware_spi.c
> @@ -134,14 +134,10 @@ struct dw_spi_priv {
>  	unsigned int freq;		/* Default frequency */
>  	unsigned int mode;
>  
> -	const void *tx;
> -	const void *tx_end;
> -	void *rx;
> -	void *rx_end;
>  	u32 fifo_len;			/* depth of the FIFO buffer */
>  
>  	int bits_per_word;
> -	int len;
> +	int frames;			/* Number of frames in the transfer */
>  	u8 cs;				/* chip select pin */
>  	u8 tmode;			/* TR/TO/RO/EEPROM */
>  	u8 type;			/* SPI/SSP/MicroWire */
> @@ -372,14 +368,24 @@ static int dw_spi_probe(struct udevice *bus)
>  	return 0;
>  }
>  
> -/* Return the max entries we can fill into tx fifo */
> -static inline u32 tx_max(struct dw_spi_priv *priv)
> +/**
> + * dw_writer() - Write data frames to the tx fifo
> + * @priv: Driver private info
> + * @tx: The tx buffer
> + * @idx: The number of data frames already transmitted
> + * @tx_frames: The number of data frames left to transmit
> + * @rx_frames: The number of data frames left to receive (0 if only
> + *             transmitting)
> + * @frame_bytes: The number of bytes taken up by one data frame
> + *
> + * This function writes up to @tx_frames data frames using data from @tx[@idx].
> + *
> + * Return: The number of frames read
> + */
> +static uint dw_writer(struct dw_spi_priv *priv, const void *tx, uint idx,
> +		      uint tx_frames, uint rx_frames, uint frame_bytes)
>  {
> -	u32 tx_left, tx_room, rxtx_gap;
> -
> -	tx_left = (priv->tx_end - priv->tx) / (priv->bits_per_word >> 3);
> -	tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
> -
> +	u32 tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
>  	/*
>  	 * Another concern is about the tx/rx mismatch, we
>  	 * thought about using (priv->fifo_len - rxflr - txflr) as
> @@ -388,67 +394,131 @@ static inline u32 tx_max(struct dw_spi_priv *priv)
>  	 * shift registers. So a control from sw point of
>  	 * view is taken.
>  	 */
> -	rxtx_gap = ((priv->rx_end - priv->rx) - (priv->tx_end - priv->tx)) /
> -		(priv->bits_per_word >> 3);
> +	u32 rxtx_gap = rx_frames - tx_frames;
> +	u32 count = min3(tx_frames, tx_room, (u32)(priv->fifo_len - rxtx_gap));
> +	u32 *dr = priv->regs + DW_SPI_DR;
>  
> -	return min3(tx_left, tx_room, (u32)(priv->fifo_len - rxtx_gap));
> -}
> +	if (!count)
> +		return 0;
>  
> -/* Return the max entries we should read out of rx fifo */
> -static inline u32 rx_max(struct dw_spi_priv *priv)
> -{
> -	u32 rx_left = (priv->rx_end - priv->rx) / (priv->bits_per_word >> 3);
> -
> -	return min_t(u32, rx_left, dw_read(priv, DW_SPI_RXFLR));
> -}
> -
> -static void dw_writer(struct dw_spi_priv *priv)
> -{
> -	u32 max = tx_max(priv);
> -	u32 txw = 0xFFFFFFFF;
> -
> -	while (max--) {
> -		/* Set the tx word if the transfer's original "tx" is not null */
> -		if (priv->tx_end - priv->len) {
> -			if (priv->bits_per_word == 8)
> -				txw = *(u8 *)(priv->tx);
> -			else
> -				txw = *(u16 *)(priv->tx);
> -		}
> -		dw_write(priv, DW_SPI_DR, txw);
> -		log_content("tx=0x%02x\n", txw);
> -		priv->tx += priv->bits_per_word >> 3;
> +#define do_write(type) do { \
> +	type *start = ((type *)tx) + idx; \
> +	type *end = start + count; \
> +	do { \
> +		__raw_writel(*start++, dr); \
> +	} while (start < end); \
> +} while (0)
> +	switch (frame_bytes) {
> +	case 1:
> +		do_write(u8);
> +		break;
> +	case 2:
> +		do_write(u16);
> +		break;
> +	case 3:
> +	case 4:
> +	default:
> +		do_write(u32);
> +		break;
>  	}
> +#undef do_write
> +
> +	return count;
>  }
>  
> -static void dw_reader(struct dw_spi_priv *priv)
> +/**
> + * dw_reader() - Read data frames from the rx fifo
> + * @priv: Driver private data
> + * @rx: The rx buffer
> + * @idx: The number of data frames already received
> + * @frames: The number of data frames left to receive
> + * @frame_bytes: The size of a data frame in bytes
> + *
> + * This function reads up to @frames data frames from @rx[@idx].
> + *
> + * Return: The number of frames read
> + */
> +static uint dw_reader(struct dw_spi_priv *priv, void *rx, uint idx, uint frames,
> +		      uint frame_bytes)
>  {
> -	u32 max = rx_max(priv);
> -	u16 rxw;
> +	u32 rx_lvl = dw_read(priv, DW_SPI_RXFLR);
> +	u32 count = min(frames, rx_lvl);
> +	u32 *dr = priv->regs + DW_SPI_DR;
>  
> -	while (max--) {
> -		rxw = dw_read(priv, DW_SPI_DR);
> -		log_content("rx=0x%02x\n", rxw);
> +	if (!count)
> +		return 0;
>  
> -		/* Care about rx if the transfer's original "rx" is not null */
> -		if (priv->rx_end - priv->len) {
> -			if (priv->bits_per_word == 8)
> -				*(u8 *)(priv->rx) = rxw;
> -			else
> -				*(u16 *)(priv->rx) = rxw;
> -		}
> -		priv->rx += priv->bits_per_word >> 3;
> +#define do_read(type) do { \
> +	type *start = ((type *)rx) + idx; \
> +	type *end = start + count; \
> +	do { \
> +		*start++ = __raw_readl(dr); \
> +	} while (start < end); \
> +} while (0)
> +	switch (frame_bytes) {
> +	case 1:
> +		do_read(u8);
> +		break;
> +	case 2:
> +		do_read(u16);
> +		break;
> +	case 3:
> +	case 4:
> +	default:
> +		do_read(u32);
> +		break;
>  	}
> +#undef do_read
> +
> +	return count;
>  }
>  
> -static int poll_transfer(struct dw_spi_priv *priv)
> +/**
> + * poll_transfer() - Transmit and receive data frames
> + * @priv: Driver private data
> + * @tx: The tx buffer. May be %NULL to only receive.
> + * @rx: The rx buffer. May be %NULL to discard read data.
> + * @frames: The number of data frames to transfer
> + *
> + * Transmit @tx, while recieving @rx.
> + *
> + * Return: The lesser of the number of frames transmitted or received.
> + */
> +static uint poll_transfer(struct dw_spi_priv *priv, const void *tx, void *rx,
> +			  uint frames)
>  {
> -	do {
> -		dw_writer(priv);
> -		dw_reader(priv);
> -	} while (priv->rx_end > priv->rx);
> +	uint frame_bytes = priv->bits_per_word >> 3;
> +	uint tx_idx = 0;
> +	uint rx_idx = 0;
> +	uint tx_frames = tx ? frames : 0;
> +	uint rx_frames = rx ? frames : 0;
>  
> -	return 0;
> +	while (tx_frames || rx_frames) {
> +		if (tx_frames) {
> +			uint tx_diff = dw_writer(priv, tx, tx_idx, tx_frames,
> +						 rx_frames, frame_bytes);
> +
> +			tx_idx += tx_diff;
> +			tx_frames -= tx_diff;
> +		}
> +
> +		if (rx_frames) {
> +			uint rx_diff = dw_reader(priv, rx, rx_idx, rx_frames,
> +						 frame_bytes);
> +
> +			rx_idx += rx_diff;
> +			rx_frames -= rx_diff;
> +		}
> +
> +		/*
> +		 * If we don't read/write fast enough, the transfer stops.
> +		 * Don't bother reading out what's left in the FIFO; it's
> +		 * garbage.
> +		 */
> +		if (dw_read(priv, DW_SPI_RISR) & (ISR_RXOI | ISR_TXUI))
> +			break;
> +	}
> +	return min(tx ? tx_idx : rx_idx, rx ? rx_idx : tx_idx);
>  }
>  
>  /*
> @@ -474,19 +544,21 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  {
>  	struct udevice *bus = dev->parent;
>  	struct dw_spi_priv *priv = dev_get_priv(bus);
> -	const u8 *tx = dout;
> +	const void *tx = dout;
>  	u8 *rx = din;
>  	int ret = 0;
>  	u32 cr0 = 0;
> -	u32 val;
> -	u32 cs;
> +	u32 val, cs;
> +	uint frames;
>  
>  	/* spi core configured to do 8 bit transfers */
> -	if (bitlen % 8) {
> +	if (bitlen % priv->bits_per_word) {
>  		dev_err(dev, "Non byte aligned SPI transfer.\n");
>  		return -1;
>  	}
>  
> +	frames = bitlen / priv->bits_per_word;
> +
>  	/* Start the transaction if necessary. */
>  	if (flags & SPI_XFER_BEGIN)
>  		external_cs_manage(dev, false);
> @@ -496,30 +568,22 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	else if (rx)
>  		priv->tmode = CTRLR0_TMOD_RO;
>  	else
> -		/*
> -		 * In transmit only mode (CTRL0_TMOD_TO) input FIFO never gets
> -		 * any data which breaks our logic in poll_transfer() above.
> -		 */
> -		priv->tmode = CTRLR0_TMOD_TR;
> +		priv->tmode = CTRLR0_TMOD_TO;
>  
>  	cr0 = dw_spi_update_cr0(priv);
>  
> -	priv->len = bitlen >> 3;
> -
> -	priv->tx = (void *)tx;
> -	priv->tx_end = priv->tx + priv->len;
> -	priv->rx = rx;
> -	priv->rx_end = priv->rx + priv->len;
> -
>  	/* Disable controller before writing control registers */
>  	dw_write(priv, DW_SPI_SSIENR, 0);
>  
> -	dev_dbg(dev, "cr0=%08x rx=%p tx=%p len=%d [bytes]\n", cr0, rx, tx,
> -		priv->len);
> +	dev_dbg(dev, "cr0=%08x rx=%p tx=%p frames=%d\n", cr0, rx, tx,
> +		frames);
>  	/* Reprogram cr0 only if changed */
>  	if (dw_read(priv, DW_SPI_CTRLR0) != cr0)
>  		dw_write(priv, DW_SPI_CTRLR0, cr0);
>  
> +	if (rx)
> +		dw_write(priv, DW_SPI_CTRLR1, frames - 1);
> +
>  	/*
>  	 * Configure the desired SS (slave select 0...3) in the controller
>  	 * The DW SPI controller will activate and deactivate this CS
> @@ -531,8 +595,15 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>  	/* Enable controller after writing control registers */
>  	dw_write(priv, DW_SPI_SSIENR, 1);
>  
> +	/*
> +	 * Prime the pump. RO-mode doesn't work unless something gets written to
> +	 * the FIFO
> +	 */
> +	if (rx && !tx)
> +		dw_write(priv, DW_SPI_DR, 0xFFFFFFFF);
> +
>  	/* Start transfer in a polling loop */
> -	ret = poll_transfer(priv);
> +	poll_transfer(priv, tx, rx, frames);
>  
>  	/*
>  	 * Wait for current transmit operation to complete.
> @@ -565,9 +636,10 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>  	int pos, i, ret = 0;
>  	struct udevice *bus = slave->dev->parent;
>  	struct dw_spi_priv *priv = dev_get_priv(bus);
> +	struct spi_mem_op *mut_op = (struct spi_mem_op *)op;
>  	u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
>  	u8 op_buf[op_len];
> -	u32 cr0;
> +	u32 cr0, val;
>  
>  	if (read)
>  		priv->tmode = CTRLR0_TMOD_EPROMREAD;
> @@ -597,47 +669,33 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>  	if (op->dummy.nbytes)
>  		memset(op_buf + pos, 0xff, op->dummy.nbytes);
>  
> -	external_cs_manage(slave->dev, false);
> +	dw_writer(priv, &op_buf, 0, op_len, 0, sizeof(u8));
>  
> -	priv->tx = &op_buf;
> -	priv->tx_end = priv->tx + op_len;
> -	priv->rx = NULL;
> -	priv->rx_end = NULL;
> -	while (priv->tx != priv->tx_end)
> -		dw_writer(priv);
> +	external_cs_manage(slave->dev, false);
> +	dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
>  
>  	/*
>  	 * XXX: The following are tight loops! Enabling debug messages may cause
>  	 * them to fail because we are not reading/writing the fifo fast enough.
>  	 */
> -	if (read) {
> -		priv->rx = op->data.buf.in;
> -		priv->rx_end = priv->rx + op->data.nbytes;
> +	if (read)
> +		mut_op->data.nbytes = poll_transfer(priv, NULL, op->data.buf.in,
> +						    op->data.nbytes);
> +	else
> +		mut_op->data.nbytes = poll_transfer(priv, op->data.buf.out,
> +						    NULL, op->data.nbytes);
>  
> -		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
> -		while (priv->rx != priv->rx_end)
> -			dw_reader(priv);
> -	} else {
> -		u32 val;
> -
> -		priv->tx = op->data.buf.out;
> -		priv->tx_end = priv->tx + op->data.nbytes;
> -
> -		/* Fill up the write fifo before starting the transfer */
> -		dw_writer(priv);
> -		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
> -		while (priv->tx != priv->tx_end)
> -			dw_writer(priv);
> -
> -		if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
> -				       (val & SR_TF_EMPT) && !(val & SR_BUSY),
> -				       RX_TIMEOUT * 1000)) {
> -			dev_dbg(bus, "timed out; sr=%x\n",
> -				dw_read(priv, DW_SPI_SR));
> -			ret = -ETIMEDOUT;
> -		}
> +	/*
> +	 * Ensure the data (or the instruction for zero-data instructions) has
> +	 * been transmitted from the fifo/shift register before disabling the
> +	 * device.
> +	 */
> +	if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
> +			       (val & SR_TF_EMPT) && !(val & SR_BUSY),
> +			       RX_TIMEOUT * 1000)) {
> +		dev_dbg(bus, "timed out; sr=%x\n", dw_read(priv, DW_SPI_SR));
> +		ret = -ETIMEDOUT;
>  	}
> -
>  	dw_write(priv, DW_SPI_SER, 0);
>  	external_cs_manage(slave->dev, true);
>  
> 


-- 
Damien Le Moal
Western Digital Research

[PATCH v2 06/10] spi: dw: Rewrite poll_transfer logic

[Sean Anderson]

On 4/4/21 8:47 PM, Damien Le Moal wrote:
> On 2021/04/03 8:05, Sean Anderson wrote:
>> This rewrites poll_transfer, dw_writer, and dw_reader.
>>
>> * We now use RO transfers (instead of always using TR). This eliminates the
>>    need to send out dummy words, and simplifies the transmit logic.
>> * All parameters (except regs and bits_per_word) are passed explicitly.
>> * Most parameters have been made explicit (instead of being recalculated on
>>    every loop).
>> * Transfers are measured in units of frames instead of bytes. This matches
>>    the measurements used by the device and eliminates several divisions by
>>    bits_per_word.
>> * We now check if we have over-/under-flowed the FIFO. This should help
>>    prevent hangs when the device stops the transfer and U-Boot doesn't
>>    realize it (and then waits for the remaining data forever). TXUI is not
>>    present in DW_APB_SSI, but in the worst case we just don't write data.
>>
>>    Unfortunately, this doesn't seem to solve all problems, and there are
>>    some remaining bugs (such as some transfers containing all 1s or all 0s)
>>    when we have many fifo overflows. This is solved for DWC devices by
>>    enabling clock stretching.
>> * poll_transfer is now used by dw_spi_exec_op as well as xfer.
>> * There are separate inner loops for 8-, 16-, and 32-bit frame sizes. This
>>    should hopefully reduce the amount of over-/under-flows. However, I
>>    haven't done exhaustive testing to ensure this is really necessary. In
>>    particular, I was never able to prevent read overflows at 50MHz clock
>>    speed.
> 
> That sounds like the long fight I had on Linux side... Did you account for the
> fact that the K210 has a buggy fifo depth (it reports 32 but is in fact 31) ?

Hm, I forgot about that. I think that's a good line of investigation.

--Sean

> 
>>
>> These changes should probably have been split up into several commits, but
>> several depend on each other, and it would be difficult to break this up
>> while preserving bisectability.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>> (no changes since v1)
>>
>>   drivers/spi/designware_spi.c | 284 +++++++++++++++++++++--------------
>>   1 file changed, 171 insertions(+), 113 deletions(-)
>>
>> diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
>> index 6375e6d778..72cca14887 100644
>> --- a/drivers/spi/designware_spi.c
>> +++ b/drivers/spi/designware_spi.c
>> @@ -134,14 +134,10 @@ struct dw_spi_priv {
>>   	unsigned int freq;		/* Default frequency */
>>   	unsigned int mode;
>>   
>> -	const void *tx;
>> -	const void *tx_end;
>> -	void *rx;
>> -	void *rx_end;
>>   	u32 fifo_len;			/* depth of the FIFO buffer */
>>   
>>   	int bits_per_word;
>> -	int len;
>> +	int frames;			/* Number of frames in the transfer */
>>   	u8 cs;				/* chip select pin */
>>   	u8 tmode;			/* TR/TO/RO/EEPROM */
>>   	u8 type;			/* SPI/SSP/MicroWire */
>> @@ -372,14 +368,24 @@ static int dw_spi_probe(struct udevice *bus)
>>   	return 0;
>>   }
>>   
>> -/* Return the max entries we can fill into tx fifo */
>> -static inline u32 tx_max(struct dw_spi_priv *priv)
>> +/**
>> + * dw_writer() - Write data frames to the tx fifo
>> + * @priv: Driver private info
>> + * @tx: The tx buffer
>> + * @idx: The number of data frames already transmitted
>> + * @tx_frames: The number of data frames left to transmit
>> + * @rx_frames: The number of data frames left to receive (0 if only
>> + *             transmitting)
>> + * @frame_bytes: The number of bytes taken up by one data frame
>> + *
>> + * This function writes up to @tx_frames data frames using data from @tx[@idx].
>> + *
>> + * Return: The number of frames read
>> + */
>> +static uint dw_writer(struct dw_spi_priv *priv, const void *tx, uint idx,
>> +		      uint tx_frames, uint rx_frames, uint frame_bytes)
>>   {
>> -	u32 tx_left, tx_room, rxtx_gap;
>> -
>> -	tx_left = (priv->tx_end - priv->tx) / (priv->bits_per_word >> 3);
>> -	tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
>> -
>> +	u32 tx_room = priv->fifo_len - dw_read(priv, DW_SPI_TXFLR);
>>   	/*
>>   	 * Another concern is about the tx/rx mismatch, we
>>   	 * thought about using (priv->fifo_len - rxflr - txflr) as
>> @@ -388,67 +394,131 @@ static inline u32 tx_max(struct dw_spi_priv *priv)
>>   	 * shift registers. So a control from sw point of
>>   	 * view is taken.
>>   	 */
>> -	rxtx_gap = ((priv->rx_end - priv->rx) - (priv->tx_end - priv->tx)) /
>> -		(priv->bits_per_word >> 3);
>> +	u32 rxtx_gap = rx_frames - tx_frames;
>> +	u32 count = min3(tx_frames, tx_room, (u32)(priv->fifo_len - rxtx_gap));
>> +	u32 *dr = priv->regs + DW_SPI_DR;
>>   
>> -	return min3(tx_left, tx_room, (u32)(priv->fifo_len - rxtx_gap));
>> -}
>> +	if (!count)
>> +		return 0;
>>   
>> -/* Return the max entries we should read out of rx fifo */
>> -static inline u32 rx_max(struct dw_spi_priv *priv)
>> -{
>> -	u32 rx_left = (priv->rx_end - priv->rx) / (priv->bits_per_word >> 3);
>> -
>> -	return min_t(u32, rx_left, dw_read(priv, DW_SPI_RXFLR));
>> -}
>> -
>> -static void dw_writer(struct dw_spi_priv *priv)
>> -{
>> -	u32 max = tx_max(priv);
>> -	u32 txw = 0xFFFFFFFF;
>> -
>> -	while (max--) {
>> -		/* Set the tx word if the transfer's original "tx" is not null */
>> -		if (priv->tx_end - priv->len) {
>> -			if (priv->bits_per_word == 8)
>> -				txw = *(u8 *)(priv->tx);
>> -			else
>> -				txw = *(u16 *)(priv->tx);
>> -		}
>> -		dw_write(priv, DW_SPI_DR, txw);
>> -		log_content("tx=0x%02x\n", txw);
>> -		priv->tx += priv->bits_per_word >> 3;
>> +#define do_write(type) do { \
>> +	type *start = ((type *)tx) + idx; \
>> +	type *end = start + count; \
>> +	do { \
>> +		__raw_writel(*start++, dr); \
>> +	} while (start < end); \
>> +} while (0)
>> +	switch (frame_bytes) {
>> +	case 1:
>> +		do_write(u8);
>> +		break;
>> +	case 2:
>> +		do_write(u16);
>> +		break;
>> +	case 3:
>> +	case 4:
>> +	default:
>> +		do_write(u32);
>> +		break;
>>   	}
>> +#undef do_write
>> +
>> +	return count;
>>   }
>>   
>> -static void dw_reader(struct dw_spi_priv *priv)
>> +/**
>> + * dw_reader() - Read data frames from the rx fifo
>> + * @priv: Driver private data
>> + * @rx: The rx buffer
>> + * @idx: The number of data frames already received
>> + * @frames: The number of data frames left to receive
>> + * @frame_bytes: The size of a data frame in bytes
>> + *
>> + * This function reads up to @frames data frames from @rx[@idx].
>> + *
>> + * Return: The number of frames read
>> + */
>> +static uint dw_reader(struct dw_spi_priv *priv, void *rx, uint idx, uint frames,
>> +		      uint frame_bytes)
>>   {
>> -	u32 max = rx_max(priv);
>> -	u16 rxw;
>> +	u32 rx_lvl = dw_read(priv, DW_SPI_RXFLR);
>> +	u32 count = min(frames, rx_lvl);
>> +	u32 *dr = priv->regs + DW_SPI_DR;
>>   
>> -	while (max--) {
>> -		rxw = dw_read(priv, DW_SPI_DR);
>> -		log_content("rx=0x%02x\n", rxw);
>> +	if (!count)
>> +		return 0;
>>   
>> -		/* Care about rx if the transfer's original "rx" is not null */
>> -		if (priv->rx_end - priv->len) {
>> -			if (priv->bits_per_word == 8)
>> -				*(u8 *)(priv->rx) = rxw;
>> -			else
>> -				*(u16 *)(priv->rx) = rxw;
>> -		}
>> -		priv->rx += priv->bits_per_word >> 3;
>> +#define do_read(type) do { \
>> +	type *start = ((type *)rx) + idx; \
>> +	type *end = start + count; \
>> +	do { \
>> +		*start++ = __raw_readl(dr); \
>> +	} while (start < end); \
>> +} while (0)
>> +	switch (frame_bytes) {
>> +	case 1:
>> +		do_read(u8);
>> +		break;
>> +	case 2:
>> +		do_read(u16);
>> +		break;
>> +	case 3:
>> +	case 4:
>> +	default:
>> +		do_read(u32);
>> +		break;
>>   	}
>> +#undef do_read
>> +
>> +	return count;
>>   }
>>   
>> -static int poll_transfer(struct dw_spi_priv *priv)
>> +/**
>> + * poll_transfer() - Transmit and receive data frames
>> + * @priv: Driver private data
>> + * @tx: The tx buffer. May be %NULL to only receive.
>> + * @rx: The rx buffer. May be %NULL to discard read data.
>> + * @frames: The number of data frames to transfer
>> + *
>> + * Transmit @tx, while recieving @rx.
>> + *
>> + * Return: The lesser of the number of frames transmitted or received.
>> + */
>> +static uint poll_transfer(struct dw_spi_priv *priv, const void *tx, void *rx,
>> +			  uint frames)
>>   {
>> -	do {
>> -		dw_writer(priv);
>> -		dw_reader(priv);
>> -	} while (priv->rx_end > priv->rx);
>> +	uint frame_bytes = priv->bits_per_word >> 3;
>> +	uint tx_idx = 0;
>> +	uint rx_idx = 0;
>> +	uint tx_frames = tx ? frames : 0;
>> +	uint rx_frames = rx ? frames : 0;
>>   
>> -	return 0;
>> +	while (tx_frames || rx_frames) {
>> +		if (tx_frames) {
>> +			uint tx_diff = dw_writer(priv, tx, tx_idx, tx_frames,
>> +						 rx_frames, frame_bytes);
>> +
>> +			tx_idx += tx_diff;
>> +			tx_frames -= tx_diff;
>> +		}
>> +
>> +		if (rx_frames) {
>> +			uint rx_diff = dw_reader(priv, rx, rx_idx, rx_frames,
>> +						 frame_bytes);
>> +
>> +			rx_idx += rx_diff;
>> +			rx_frames -= rx_diff;
>> +		}
>> +
>> +		/*
>> +		 * If we don't read/write fast enough, the transfer stops.
>> +		 * Don't bother reading out what's left in the FIFO; it's
>> +		 * garbage.
>> +		 */
>> +		if (dw_read(priv, DW_SPI_RISR) & (ISR_RXOI | ISR_TXUI))
>> +			break;
>> +	}
>> +	return min(tx ? tx_idx : rx_idx, rx ? rx_idx : tx_idx);
>>   }
>>   
>>   /*
>> @@ -474,19 +544,21 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>   {
>>   	struct udevice *bus = dev->parent;
>>   	struct dw_spi_priv *priv = dev_get_priv(bus);
>> -	const u8 *tx = dout;
>> +	const void *tx = dout;
>>   	u8 *rx = din;
>>   	int ret = 0;
>>   	u32 cr0 = 0;
>> -	u32 val;
>> -	u32 cs;
>> +	u32 val, cs;
>> +	uint frames;
>>   
>>   	/* spi core configured to do 8 bit transfers */
>> -	if (bitlen % 8) {
>> +	if (bitlen % priv->bits_per_word) {
>>   		dev_err(dev, "Non byte aligned SPI transfer.\n");
>>   		return -1;
>>   	}
>>   
>> +	frames = bitlen / priv->bits_per_word;
>> +
>>   	/* Start the transaction if necessary. */
>>   	if (flags & SPI_XFER_BEGIN)
>>   		external_cs_manage(dev, false);
>> @@ -496,30 +568,22 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>   	else if (rx)
>>   		priv->tmode = CTRLR0_TMOD_RO;
>>   	else
>> -		/*
>> -		 * In transmit only mode (CTRL0_TMOD_TO) input FIFO never gets
>> -		 * any data which breaks our logic in poll_transfer() above.
>> -		 */
>> -		priv->tmode = CTRLR0_TMOD_TR;
>> +		priv->tmode = CTRLR0_TMOD_TO;
>>   
>>   	cr0 = dw_spi_update_cr0(priv);
>>   
>> -	priv->len = bitlen >> 3;
>> -
>> -	priv->tx = (void *)tx;
>> -	priv->tx_end = priv->tx + priv->len;
>> -	priv->rx = rx;
>> -	priv->rx_end = priv->rx + priv->len;
>> -
>>   	/* Disable controller before writing control registers */
>>   	dw_write(priv, DW_SPI_SSIENR, 0);
>>   
>> -	dev_dbg(dev, "cr0=%08x rx=%p tx=%p len=%d [bytes]\n", cr0, rx, tx,
>> -		priv->len);
>> +	dev_dbg(dev, "cr0=%08x rx=%p tx=%p frames=%d\n", cr0, rx, tx,
>> +		frames);
>>   	/* Reprogram cr0 only if changed */
>>   	if (dw_read(priv, DW_SPI_CTRLR0) != cr0)
>>   		dw_write(priv, DW_SPI_CTRLR0, cr0);
>>   
>> +	if (rx)
>> +		dw_write(priv, DW_SPI_CTRLR1, frames - 1);
>> +
>>   	/*
>>   	 * Configure the desired SS (slave select 0...3) in the controller
>>   	 * The DW SPI controller will activate and deactivate this CS
>> @@ -531,8 +595,15 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>   	/* Enable controller after writing control registers */
>>   	dw_write(priv, DW_SPI_SSIENR, 1);
>>   
>> +	/*
>> +	 * Prime the pump. RO-mode doesn't work unless something gets written to
>> +	 * the FIFO
>> +	 */
>> +	if (rx && !tx)
>> +		dw_write(priv, DW_SPI_DR, 0xFFFFFFFF);
>> +
>>   	/* Start transfer in a polling loop */
>> -	ret = poll_transfer(priv);
>> +	poll_transfer(priv, tx, rx, frames);
>>   
>>   	/*
>>   	 * Wait for current transmit operation to complete.
>> @@ -565,9 +636,10 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>>   	int pos, i, ret = 0;
>>   	struct udevice *bus = slave->dev->parent;
>>   	struct dw_spi_priv *priv = dev_get_priv(bus);
>> +	struct spi_mem_op *mut_op = (struct spi_mem_op *)op;
>>   	u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
>>   	u8 op_buf[op_len];
>> -	u32 cr0;
>> +	u32 cr0, val;
>>   
>>   	if (read)
>>   		priv->tmode = CTRLR0_TMOD_EPROMREAD;
>> @@ -597,47 +669,33 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>>   	if (op->dummy.nbytes)
>>   		memset(op_buf + pos, 0xff, op->dummy.nbytes);
>>   
>> -	external_cs_manage(slave->dev, false);
>> +	dw_writer(priv, &op_buf, 0, op_len, 0, sizeof(u8));
>>   
>> -	priv->tx = &op_buf;
>> -	priv->tx_end = priv->tx + op_len;
>> -	priv->rx = NULL;
>> -	priv->rx_end = NULL;
>> -	while (priv->tx != priv->tx_end)
>> -		dw_writer(priv);
>> +	external_cs_manage(slave->dev, false);
>> +	dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
>>   
>>   	/*
>>   	 * XXX: The following are tight loops! Enabling debug messages may cause
>>   	 * them to fail because we are not reading/writing the fifo fast enough.
>>   	 */
>> -	if (read) {
>> -		priv->rx = op->data.buf.in;
>> -		priv->rx_end = priv->rx + op->data.nbytes;
>> +	if (read)
>> +		mut_op->data.nbytes = poll_transfer(priv, NULL, op->data.buf.in,
>> +						    op->data.nbytes);
>> +	else
>> +		mut_op->data.nbytes = poll_transfer(priv, op->data.buf.out,
>> +						    NULL, op->data.nbytes);
>>   
>> -		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
>> -		while (priv->rx != priv->rx_end)
>> -			dw_reader(priv);
>> -	} else {
>> -		u32 val;
>> -
>> -		priv->tx = op->data.buf.out;
>> -		priv->tx_end = priv->tx + op->data.nbytes;
>> -
>> -		/* Fill up the write fifo before starting the transfer */
>> -		dw_writer(priv);
>> -		dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
>> -		while (priv->tx != priv->tx_end)
>> -			dw_writer(priv);
>> -
>> -		if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
>> -				       (val & SR_TF_EMPT) && !(val & SR_BUSY),
>> -				       RX_TIMEOUT * 1000)) {
>> -			dev_dbg(bus, "timed out; sr=%x\n",
>> -				dw_read(priv, DW_SPI_SR));
>> -			ret = -ETIMEDOUT;
>> -		}
>> +	/*
>> +	 * Ensure the data (or the instruction for zero-data instructions) has
>> +	 * been transmitted from the fifo/shift register before disabling the
>> +	 * device.
>> +	 */
>> +	if (readl_poll_timeout(priv->regs + DW_SPI_SR, val,
>> +			       (val & SR_TF_EMPT) && !(val & SR_BUSY),
>> +			       RX_TIMEOUT * 1000)) {
>> +		dev_dbg(bus, "timed out; sr=%x\n", dw_read(priv, DW_SPI_SR));
>> +		ret = -ETIMEDOUT;
>>   	}
>> -
>>   	dw_write(priv, DW_SPI_SER, 0);
>>   	external_cs_manage(slave->dev, true);
>>   
>>
> 
> 

[PATCH v2 07/10] spi: dw: Add ENHANCED cap

[Sean Anderson]

This capability corresponds to an SSIC_SPI_MODE of 1, 2, or 3.  This
doesn't do much yet, but it does add support for detection and for
disallowing unsupported modes. Unfortunately, we cannot discriminate
between these modes (only that SSIC_SPI_MODE != 0), so we just pretend to
have DUAL+QUAD+OCTAL and let the slave determine what we actually use.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED

 drivers/spi/designware_spi.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 72cca14887..c2639141c6 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -129,6 +129,7 @@ struct dw_spi_priv {
 #define DW_SPI_CAP_KEEMBAY_MST		BIT(1) /* Unimplemented */
 #define DW_SPI_CAP_DWC_SSI		BIT(2)
 #define DW_SPI_CAP_DFS32		BIT(3)
+#define DW_SPI_CAP_ENHANCED		BIT(4)
 	unsigned long caps;
 	unsigned long bus_clk_rate;
 	unsigned int freq;		/* Default frequency */
@@ -141,6 +142,7 @@ struct dw_spi_priv {
 	u8 cs;				/* chip select pin */
 	u8 tmode;			/* TR/TO/RO/EEPROM */
 	u8 type;			/* SPI/SSP/MicroWire */
+	u8 spi_frf;			/* BYTE/DUAL/QUAD/OCTAL */
 };
 
 static inline u32 dw_read(struct dw_spi_priv *priv, u32 offset)
@@ -249,6 +251,18 @@ static void spi_hw_init(struct udevice *bus, struct dw_spi_priv *priv)
 	if (priv->caps & DW_SPI_CAP_DWC_SSI || !FIELD_GET(CTRLR0_DFS_MASK, cr0))
 		priv->caps |= DW_SPI_CAP_DFS32;
 
+	/*
+	 * If SPI_FRF exists that means we have DUAL, QUAD, or OCTAL. Since we
+	 * can't differentiate, just set a general ENHANCED cap and let the
+	 * slave decide what to use.
+	 */
+	if (priv->caps & DW_SPI_CAP_DWC_SSI) {
+		if (FIELD_GET(DWC_SSI_CTRLR0_SPI_FRF_MASK, cr0))
+			priv->caps |= DW_SPI_CAP_ENHANCED;
+	} else if (FIELD_GET(CTRLR0_SPI_FRF_MASK, cr0)) {
+		priv->caps |= DW_SPI_CAP_ENHANCED;
+	}
+
 	dw_write(priv, DW_SPI_SSIENR, 1);
 
 	/*
@@ -746,13 +760,19 @@ static int dw_spi_set_mode(struct udevice *bus, uint mode)
 {
 	struct dw_spi_priv *priv = dev_get_priv(bus);
 
+	if (!(priv->caps & DW_SPI_CAP_ENHANCED) &&
+	    (mode & (SPI_RX_DUAL | SPI_TX_DUAL |
+		     SPI_RX_QUAD | SPI_TX_QUAD |
+		     SPI_RX_OCTAL | SPI_TX_OCTAL)))
+		return -EINVAL;
+
 	/*
 	 * Can't set mode yet. Since this depends on if rx, tx, or
 	 * rx & tx is requested. So we have to defer this to the
 	 * real transfer function.
 	 */
 	priv->mode = mode;
-	dev_dbg(bus, "mode=%d\n", priv->mode);
+	dev_dbg(bus, "mode=%x\n", mode);
 
 	return 0;
 }
-- 
2.31.0

[PATCH v2 08/10] spi: dw: Define registers for enhanced mode

[Sean Anderson]

This adds some registers needed for DUAL/QUAD/OCTAL modes. It also adds the
fields in (R)ISR so we can check for over-/under-flow.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v2)

Changes in v2:
- Fix some inconsistencies in register naming and usage

 drivers/spi/designware_spi.c | 60 ++++++++++++++++++++++++++++++++++--
 1 file changed, 57 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index c2639141c6..e110c5bca1 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -56,6 +56,8 @@
 #define DW_SPI_IDR			0x58
 #define DW_SPI_VERSION			0x5c
 #define DW_SPI_DR			0x60
+#define DW_SPI_RX_SAMPLE_DLY		0xf0
+#define DW_SPI_SPI_CTRL0		0xf4
 
 /* Bit fields in CTRLR0 */
 /*
@@ -80,8 +82,8 @@
 #define CTRLR0_TMOD_RO			0x2		/* recv only */
 #define CTRLR0_TMOD_EPROMREAD		0x3		/* eeprom read mode */
 
-#define CTRLR0_SLVOE_OFFSET		10
-#define CTRLR0_SRL_OFFSET		11
+#define CTRLR0_SLVOE_OFFSET		BIT(10)
+#define CTRLR0_SRL			BIT(11)
 #define CTRLR0_CFS_MASK			GENMASK(15, 12)
 
 /* Only present when SSI_MAX_XFER_SIZE=32 */
@@ -92,13 +94,15 @@
 #define CTRLR0_SPI_FRF_BYTE		0x0
 #define	CTRLR0_SPI_FRF_DUAL		0x1
 #define	CTRLR0_SPI_FRF_QUAD		0x2
+#define	CTRLR0_SPI_FRF_OCTAL		0x3
 
 /* Bit fields in CTRLR0 based on DWC_ssi_databook.pdf v1.01a */
 #define DWC_SSI_CTRLR0_DFS_MASK		GENMASK(4, 0)
 #define DWC_SSI_CTRLR0_FRF_MASK		GENMASK(7, 6)
 #define DWC_SSI_CTRLR0_MODE_MASK	GENMASK(9, 8)
 #define DWC_SSI_CTRLR0_TMOD_MASK	GENMASK(11, 10)
-#define DWC_SSI_CTRLR0_SRL_OFFSET	13
+#define DWC_SSI_CTRLR0_SRL		BIT(13)
+#define DWC_SSI_CTRLR0_SSTE		BIT(14)
 #define DWC_SSI_CTRLR0_SPI_FRF_MASK	GENMASK(23, 22)
 
 /* Bit fields in SR, 7 bits */
@@ -111,6 +115,56 @@
 #define SR_TX_ERR			BIT(5)
 #define SR_DCOL				BIT(6)
 
+/* Bit fields in (R)ISR */
+
+/* TX FIFO Empty */
+#define ISR_TXEI			BIT(0)
+/* TX FIFO Overflow */
+#define ISR_TXOI			BIT(1)
+/* RX FIFO Underflow */
+#define ISR_RXUI			BIT(2)
+/* RX FIFO Overflow */
+#define ISR_RXOI			BIT(3)
+/* RX FIFO Full */
+#define ISR_RXFI			BIT(4)
+/* Multi-master contention */
+#define ISR_MSTI			BIT(5)
+/* XIP Receive FIFO Overflow */
+#define ISR_XRXOI			BIT(6)
+/* TX FIFO Underflow */
+#define ISR_TXUI			BIT(7)
+/* AXI Error */
+#define ISR_AXIE			BIT(8)
+/* SPI TX Error */
+#define ISR_SPITE			BIT(10)
+/* SSI Done */
+#define ISR_DONE			BIT(11)
+
+/* Bit fields in SPI_CTRLR0 */
+
+/*
+ * Whether the instruction or address use the value of SPI_FRF or use
+ * FRF_BYTE
+ */
+#define SPI_CTRLR0_TRANS_TYPE_MASK	GENMASK(1, 0)
+#define SPI_CTRLR0_TRANS_TYPE_1_1_X	0x0
+#define SPI_CTRLR0_TRANS_TYPE_1_X_X	0x1
+#define SPI_CTRLR0_TRANS_TYPE_X_X_X	0x2
+/* Address length in 4-bit units */
+#define SPI_CTRLR0_ADDR_L_MASK		GENMASK(5, 2)
+/* Enable mode bits after address in XIP mode */
+#define SPI_CTRLR0_XIP_MD_BIT_EN	BIT(7)
+/* Instruction length */
+#define SPI_CTRLR0_INST_L_MASK		GENMASK(9, 8)
+#define INST_L_0			0x0
+#define INST_L_4			0x1
+#define INST_L_8			0x2
+#define INST_L_16			0x3
+/* Number of "dummy" cycles */
+#define SPI_CTRLR0_WAIT_CYCLES_MASK	GENMASK(15, 11)
+/* Stretch the clock if the FIFO over/underflows */
+#define SPI_CTRLR0_CLK_STRETCH_EN	BIT(30)
+
 #define RX_TIMEOUT			1000		/* timeout in ms */
 
 struct dw_spi_plat {
-- 
2.31.0

[PATCH v2 09/10] spi: dw: Support enhanced SPI

[Sean Anderson]

This adds support for DUAL/QUAD/OCTAL transfers. This adds
dw_spi_supports_op to do some sanity checks which would otherwise live in
exec_op. We only support byte transfers, but as far as I could tell only
bytes are supported by mem_ops (e.g. every part of the opcode has nbytes).

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/spi/designware_spi.c | 136 ++++++++++++++++++++++++++++++-----
 1 file changed, 119 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index e110c5bca1..64a3a8556b 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -218,7 +218,8 @@ static u32 dw_spi_update_cr0(struct dw_spi_priv *priv)
 				 priv->bits_per_word - 1)
 		    | FIELD_PREP(DWC_SSI_CTRLR0_FRF_MASK, priv->type)
 		    | FIELD_PREP(DWC_SSI_CTRLR0_MODE_MASK, priv->mode)
-		    | FIELD_PREP(DWC_SSI_CTRLR0_TMOD_MASK, priv->tmode);
+		    | FIELD_PREP(DWC_SSI_CTRLR0_TMOD_MASK, priv->tmode)
+		    | FIELD_PREP(DWC_SSI_CTRLR0_SPI_FRF_MASK, priv->spi_frf);
 	} else {
 		if (priv->caps & DW_SPI_CAP_DFS32)
 			cr0 = FIELD_PREP(CTRLR0_DFS_32_MASK,
@@ -229,12 +230,36 @@ static u32 dw_spi_update_cr0(struct dw_spi_priv *priv)
 
 		cr0 |= FIELD_PREP(CTRLR0_FRF_MASK, priv->type)
 		    |  FIELD_PREP(CTRLR0_MODE_MASK, priv->mode)
-		    |  FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode);
+		    |  FIELD_PREP(CTRLR0_TMOD_MASK, priv->tmode)
+		    |  FIELD_PREP(CTRLR0_SPI_FRF_MASK, priv->spi_frf);
 	}
 
 	return cr0;
 }
 
+static u32 dw_spi_update_spi_cr0(const struct spi_mem_op *op)
+{
+	uint trans_type, wait_cycles;
+
+	/* This assumes support_op has filtered invalid types */
+	if (op->addr.buswidth == 1)
+		trans_type = SPI_CTRLR0_TRANS_TYPE_1_1_X;
+	else if (op->cmd.buswidth == 1)
+		trans_type = SPI_CTRLR0_TRANS_TYPE_1_X_X;
+	else
+		trans_type = SPI_CTRLR0_TRANS_TYPE_X_X_X;
+
+	if (op->dummy.buswidth)
+		wait_cycles = op->dummy.nbytes * 8 / op->dummy.buswidth;
+	else
+		wait_cycles = 0;
+
+	return FIELD_PREP(SPI_CTRLR0_TRANS_TYPE_MASK, trans_type)
+	       | FIELD_PREP(SPI_CTRLR0_ADDR_L_MASK, op->addr.nbytes * 2)
+	       | FIELD_PREP(SPI_CTRLR0_INST_L_MASK, INST_L_8)
+	       | FIELD_PREP(SPI_CTRLR0_WAIT_CYCLES_MASK, wait_cycles)
+}
+
 static int request_gpio_cs(struct udevice *bus)
 {
 #if CONFIG_IS_ENABLED(DM_GPIO) && !defined(CONFIG_SPL_BUILD)
@@ -619,6 +644,13 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	u32 val, cs;
 	uint frames;
 
+	/* DUAL/QUAD/OCTAL only supported by exec_op for now */
+	if (priv->mode & (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
+			  SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL))
+		return -1;
+
+	priv->spi_frf = CTRLR0_SPI_FRF_BYTE;
+
 	/* spi core configured to do 8 bit transfers */
 	if (bitlen % priv->bits_per_word) {
 		dev_err(dev, "Non byte aligned SPI transfer.\n");
@@ -697,6 +729,9 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen,
 /*
  * This function is necessary for reading SPI flash with the native CS
  * c.f. https://lkml.org/lkml/2015/12/23/132
+ *
+ * It also lets us handle DUAL/QUAD/OCTAL transfers in a much more idiomatic
+ * way.
  */
 static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 {
@@ -707,37 +742,72 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	struct spi_mem_op *mut_op = (struct spi_mem_op *)op;
 	u8 op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;
 	u8 op_buf[op_len];
-	u32 cr0, val;
+	u32 cr0, spi_cr0, val;
+
+	/* Only bytes are supported for spi-mem transfers */
+	if (priv->bits_per_word != 8)
+		return -EINVAL;
+
+	switch (op->data.buswidth) {
+	case 0:
+	case 1:
+		priv->spi_frf = CTRLR0_SPI_FRF_BYTE;
+		break;
+	case 2:
+		priv->spi_frf = CTRLR0_SPI_FRF_DUAL;
+		break;
+	case 4:
+		priv->spi_frf = CTRLR0_SPI_FRF_QUAD;
+		break;
+	case 8:
+		priv->spi_frf = CTRLR0_SPI_FRF_OCTAL;
+		break;
+	/* BUG: should have been filtered out by supports_op */
+	default:
+		return -EINVAL;
+	}
 
 	if (read)
-		priv->tmode = CTRLR0_TMOD_EPROMREAD;
+		if (priv->spi_frf == CTRLR0_SPI_FRF_BYTE)
+			priv->tmode = CTRLR0_TMOD_EPROMREAD;
+		else
+			priv->tmode = CTRLR0_TMOD_RO;
 	else
 		priv->tmode = CTRLR0_TMOD_TO;
 
 	cr0 = dw_spi_update_cr0(priv);
-	dev_dbg(bus, "cr0=%08x buf=%p len=%u [bytes]\n", cr0, op->data.buf.in,
-		op->data.nbytes);
+	spi_cr0 = dw_spi_update_spi_cr0(op);
+	dev_dbg(bus, "cr0=%08x spi_cr0=%08x buf=%p len=%u [bytes]\n", cr0,
+		spi_cr0, op->data.buf.in, op->data.nbytes);
 
 	dw_write(priv, DW_SPI_SSIENR, 0);
 	dw_write(priv, DW_SPI_CTRLR0, cr0);
 	if (read)
 		dw_write(priv, DW_SPI_CTRLR1, op->data.nbytes - 1);
+	if (priv->spi_frf != CTRLR0_SPI_FRF_BYTE)
+		dw_write(priv, DW_SPI_SPI_CTRL0, spi_cr0);
 	dw_write(priv, DW_SPI_SSIENR, 1);
 
-	/* From spi_mem_exec_op */
-	pos = 0;
-	op_buf[pos++] = op->cmd.opcode;
-	if (op->addr.nbytes) {
-		for (i = 0; i < op->addr.nbytes; i++)
-			op_buf[pos + i] = op->addr.val >>
-				(8 * (op->addr.nbytes - i - 1));
+	/* Write out the instruction */
+	if (priv->spi_frf == CTRLR0_SPI_FRF_BYTE) {
+		/* From spi_mem_exec_op */
+		pos = 0;
+		op_buf[pos++] = op->cmd.opcode;
+		if (op->addr.nbytes) {
+			for (i = 0; i < op->addr.nbytes; i++)
+				op_buf[pos + i] = op->addr.val >>
+					(8 * (op->addr.nbytes - i - 1));
 
-		pos += op->addr.nbytes;
-	}
-	if (op->dummy.nbytes)
+			pos += op->addr.nbytes;
+		}
 		memset(op_buf + pos, 0xff, op->dummy.nbytes);
 
-	dw_writer(priv, &op_buf, 0, op_len, 0, sizeof(u8));
+		dw_writer(priv, &op_buf, 0, op_len, 0, sizeof(u8));
+	} else {
+		/* MUST be written as byte/long; don't ask me why */
+		writeb(op->cmd.opcode, priv->regs + DW_SPI_DR);
+		writel(op->addr.val, priv->regs + DW_SPI_DR);
+	}
 
 	external_cs_manage(slave->dev, false);
 	dw_write(priv, DW_SPI_SER, 1 << spi_chip_select(slave->dev));
@@ -771,6 +841,37 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 	return ret;
 }
 
+bool dw_spi_supports_op(struct spi_slave *slave, const struct spi_mem_op *op)
+{
+	struct dw_spi_priv *priv = dev_get_priv(slave->dev->parent);
+
+	if (!spi_mem_default_supports_op(slave, op))
+		return false;
+
+	/*
+	 * Everything before the data must fit in the fifo.
+	 * In EEPROM mode we also need to fit the dummy.
+	 */
+	if (1 + op->addr.nbytes +
+	    (op->data.buswidth == 1 ? op->dummy.nbytes : 0) > priv->fifo_len)
+		return false;
+
+	/* We only support 1_1_X, 1_X_X, and X_X_X formats */
+	if (op->cmd.buswidth == 1 &&
+	    (!op->addr.nbytes || op->addr.buswidth == 1))
+		return true;
+
+	if (op->cmd.buswidth == 1 &&
+	    (!op->addr.nbytes || op->addr.buswidth == op->data.buswidth))
+		return true;
+
+	if (op->cmd.buswidth == op->data.buswidth &&
+	    (!op->addr.nbytes || op->addr.buswidth == op->data.buswidth))
+		return true;
+
+	return false;
+}
+
 /* The size of ctrl1 limits data transfers to 64K */
 static int dw_spi_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op)
 {
@@ -781,6 +882,7 @@ static int dw_spi_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op)
 
 static const struct spi_controller_mem_ops dw_spi_mem_ops = {
 	.exec_op = dw_spi_exec_op,
+	.supports_op = dw_spi_supports_op,
 	.adjust_op_size = dw_spi_adjust_op_size,
 };
 
-- 
2.31.0

[PATCH v2 10/10] spi: dw: Support clock stretching

[Sean Anderson]

We don't always read/write to the FIFO fast enough. Enable clock stretching
for enhanced SPI transfers. This is only possible with DWC SSI devices more
recent than 1.01a. We also need to set the RXFTLR register to tell the
device when to start reciving again. In particular, the default of 0 will
result in the device never restarting reception if there is an overflow. On
the transmit side, we need to set CTRL1 so that the device knows when to
keep stretching the clock if the FIFO is empty.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 drivers/spi/designware_spi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c
index 64a3a8556b..44fb679fdb 100644
--- a/drivers/spi/designware_spi.c
+++ b/drivers/spi/designware_spi.c
@@ -258,6 +258,7 @@ static u32 dw_spi_update_spi_cr0(const struct spi_mem_op *op)
 	       | FIELD_PREP(SPI_CTRLR0_ADDR_L_MASK, op->addr.nbytes * 2)
 	       | FIELD_PREP(SPI_CTRLR0_INST_L_MASK, INST_L_8)
 	       | FIELD_PREP(SPI_CTRLR0_WAIT_CYCLES_MASK, wait_cycles)
+	       | SPI_CTRLR0_CLK_STRETCH_EN;
 }
 
 static int request_gpio_cs(struct udevice *bus)
@@ -360,6 +361,9 @@ static void spi_hw_init(struct udevice *bus, struct dw_spi_priv *priv)
 		priv->fifo_len = (fifo == 1) ? 0 : fifo;
 		dw_write(priv, DW_SPI_TXFTLR, 0);
 	}
+
+	/* Set receive fifo interrupt level register for clock stretching */
+	dw_write(priv, DW_SPI_RXFTLR, priv->fifo_len - 1);
 }
 
 /*
@@ -782,8 +786,7 @@ static int dw_spi_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 
 	dw_write(priv, DW_SPI_SSIENR, 0);
 	dw_write(priv, DW_SPI_CTRLR0, cr0);
-	if (read)
-		dw_write(priv, DW_SPI_CTRLR1, op->data.nbytes - 1);
+	dw_write(priv, DW_SPI_CTRLR1, op->data.nbytes - 1);
 	if (priv->spi_frf != CTRLR0_SPI_FRF_BYTE)
 		dw_write(priv, DW_SPI_SPI_CTRL0, spi_cr0);
 	dw_write(priv, DW_SPI_SSIENR, 1);
-- 
2.31.0

[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[Sean Anderson]

On 4/2/21 7:05 PM, Sean Anderson wrote:
> This series adds support for enhanced SPI modes. It was tested on a K210 (DWC
> SSI with QSPI flash).
> 
> If anyone has a designware device with QSPI flash attached (especially a DW SSI
> APB device), I'd greatly appreciate them testing out this patch series. Given
> that there has been no testing of v2 over the past month, I don't think lack of
> testing should hold up this series.
> 
> Changes in v3:
> - Dropped merged patches
> - Rebased on u-boot/master
> 
> Changes in v2:
> - Add more information to exec_op debug message
> - Actually mask interrupts
> - Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED
> - Fix some inconsistencies in register naming and usage
> - Moved some hunks between commits so things make more sense
> 
> Sean Anderson (10):
>    mtd: spi-mem: Export spi_mem_default_supports_op
>    spi: spi-mem: Add debug message for spi-mem ops
>    spi: dw: Log status register on timeout
>    spi: dw: Actually mask interrupts
>    spi: dw: Switch to capabilities
>    spi: dw: Rewrite poll_transfer logic
>    spi: dw: Add ENHANCED cap
>    spi: dw: Define registers for enhanced mode
>    spi: dw: Support enhanced SPI
>    spi: dw: Support clock stretching
> 
>   drivers/spi/designware_spi.c | 647 ++++++++++++++++++++++++-----------
>   drivers/spi/spi-mem.c        |   7 +
>   include/spi-mem.h            |   3 +
>   3 files changed, 451 insertions(+), 206 deletions(-)
> 

Looks like I forgot to bump the version. This should be v3. I can resend if necessary.

--Sean

[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[Jagan Teki]

On Sat, Apr 3, 2021 at 4:37 AM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 4/2/21 7:05 PM, Sean Anderson wrote:
> > This series adds support for enhanced SPI modes. It was tested on a K210 (DWC
> > SSI with QSPI flash).
> >
> > If anyone has a designware device with QSPI flash attached (especially a DW SSI
> > APB device), I'd greatly appreciate them testing out this patch series. Given
> > that there has been no testing of v2 over the past month, I don't think lack of
> > testing should hold up this series.
> >
> > Changes in v3:
> > - Dropped merged patches
> > - Rebased on u-boot/master
> >
> > Changes in v2:
> > - Add more information to exec_op debug message
> > - Actually mask interrupts
> > - Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED
> > - Fix some inconsistencies in register naming and usage
> > - Moved some hunks between commits so things make more sense
> >
> > Sean Anderson (10):
> >    mtd: spi-mem: Export spi_mem_default_supports_op
> >    spi: spi-mem: Add debug message for spi-mem ops
> >    spi: dw: Log status register on timeout
> >    spi: dw: Actually mask interrupts
> >    spi: dw: Switch to capabilities
> >    spi: dw: Rewrite poll_transfer logic
> >    spi: dw: Add ENHANCED cap
> >    spi: dw: Define registers for enhanced mode
> >    spi: dw: Support enhanced SPI
> >    spi: dw: Support clock stretching
> >
> >   drivers/spi/designware_spi.c | 647 ++++++++++++++++++++++++-----------
> >   drivers/spi/spi-mem.c        |   7 +
> >   include/spi-mem.h            |   3 +
> >   3 files changed, 451 insertions(+), 206 deletions(-)
> >
>
> Looks like I forgot to bump the version. This should be v3. I can resend if necessary.

Yes, Please.

There are few comments,

1. Patch "spi: spi-mem: Add debug message for spi-mem ops"

As we discussed in the previous version, drop the unnecessary debug
statements after ops execution as this patch trying to add more
verbose to before ops execution.

2. Comments in Patch v2,06/10

Jagan.

[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[Sean Anderson]

On 4/19/21 3:06 AM, Jagan Teki wrote:
> On Sat, Apr 3, 2021 at 4:37 AM Sean Anderson <seanga2@gmail.com> wrote:
>>
>> On 4/2/21 7:05 PM, Sean Anderson wrote:
>>> This series adds support for enhanced SPI modes. It was tested on a K210 (DWC
>>> SSI with QSPI flash).
>>>
>>> If anyone has a designware device with QSPI flash attached (especially a DW SSI
>>> APB device), I'd greatly appreciate them testing out this patch series. Given
>>> that there has been no testing of v2 over the past month, I don't think lack of
>>> testing should hold up this series.
>>>
>>> Changes in v3:
>>> - Dropped merged patches
>>> - Rebased on u-boot/master
>>>
>>> Changes in v2:
>>> - Add more information to exec_op debug message
>>> - Actually mask interrupts
>>> - Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED
>>> - Fix some inconsistencies in register naming and usage
>>> - Moved some hunks between commits so things make more sense
>>>
>>> Sean Anderson (10):
>>>     mtd: spi-mem: Export spi_mem_default_supports_op
>>>     spi: spi-mem: Add debug message for spi-mem ops
>>>     spi: dw: Log status register on timeout
>>>     spi: dw: Actually mask interrupts
>>>     spi: dw: Switch to capabilities
>>>     spi: dw: Rewrite poll_transfer logic
>>>     spi: dw: Add ENHANCED cap
>>>     spi: dw: Define registers for enhanced mode
>>>     spi: dw: Support enhanced SPI
>>>     spi: dw: Support clock stretching
>>>
>>>    drivers/spi/designware_spi.c | 647 ++++++++++++++++++++++++-----------
>>>    drivers/spi/spi-mem.c        |   7 +
>>>    include/spi-mem.h            |   3 +
>>>    3 files changed, 451 insertions(+), 206 deletions(-)
>>>
>>
>> Looks like I forgot to bump the version. This should be v3. I can resend if necessary.
> 
> Yes, Please.
> 
> There are few comments,
> 
> 1. Patch "spi: spi-mem: Add debug message for spi-mem ops"
> 
> As we discussed in the previous version, drop the unnecessary debug
> statements after ops execution as this patch trying to add more
> verbose to before ops execution.

 From last time:

>>> +       dev_dbg(slave->dev,
>>> +               "exec %02Xh %u-%u-%u addr=%llx dummy cycles=%u data bytes=%u\n",
>>> +               op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
>>> +               op->data.buswidth, op->addr.val,
>>> +               op->dummy.buswidth ? op->dummy.nbytes * 8 / op->dummy.buswidth : 0,
>>> +               op->data.nbytes);
>>
>> This looks unnecessary to me, we have debug prints at the end of the
>> function, which indeed sufficient.
>>
> 
> This is insufficient. First, that information is printed after the op
> is executed. This is too late if one is trying to debug (e.g.) a hang in
> the spi driver. It's also missing opcode, widths, address, and dummy
> bytes. The only duplicated info here is nbytes. Since this is debugging
> information, I think we should print what is most useful for people
> debugging this subsystem.

I would like if you could respond to my response before I remove this.

> 
> 2. Comments in Patch v2,06/10

I will look into that before resending.

--Sean

> 
> Jagan.
> 

[PATCH v2 00/10] spi: dw: Add support for DUAL/QUAD/OCTAL modes

[Jagan Teki]

On Mon, Apr 19, 2021 at 6:49 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> On 4/19/21 3:06 AM, Jagan Teki wrote:
> > On Sat, Apr 3, 2021 at 4:37 AM Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> On 4/2/21 7:05 PM, Sean Anderson wrote:
> >>> This series adds support for enhanced SPI modes. It was tested on a K210 (DWC
> >>> SSI with QSPI flash).
> >>>
> >>> If anyone has a designware device with QSPI flash attached (especially a DW SSI
> >>> APB device), I'd greatly appreciate them testing out this patch series. Given
> >>> that there has been no testing of v2 over the past month, I don't think lack of
> >>> testing should hold up this series.
> >>>
> >>> Changes in v3:
> >>> - Dropped merged patches
> >>> - Rebased on u-boot/master
> >>>
> >>> Changes in v2:
> >>> - Add more information to exec_op debug message
> >>> - Actually mask interrupts
> >>> - Merge CAP_{DUAL,QUAD,OCTAL} into CAP_ENHANCED
> >>> - Fix some inconsistencies in register naming and usage
> >>> - Moved some hunks between commits so things make more sense
> >>>
> >>> Sean Anderson (10):
> >>>     mtd: spi-mem: Export spi_mem_default_supports_op
> >>>     spi: spi-mem: Add debug message for spi-mem ops
> >>>     spi: dw: Log status register on timeout
> >>>     spi: dw: Actually mask interrupts
> >>>     spi: dw: Switch to capabilities
> >>>     spi: dw: Rewrite poll_transfer logic
> >>>     spi: dw: Add ENHANCED cap
> >>>     spi: dw: Define registers for enhanced mode
> >>>     spi: dw: Support enhanced SPI
> >>>     spi: dw: Support clock stretching
> >>>
> >>>    drivers/spi/designware_spi.c | 647 ++++++++++++++++++++++++-----------
> >>>    drivers/spi/spi-mem.c        |   7 +
> >>>    include/spi-mem.h            |   3 +
> >>>    3 files changed, 451 insertions(+), 206 deletions(-)
> >>>
> >>
> >> Looks like I forgot to bump the version. This should be v3. I can resend if necessary.
> >
> > Yes, Please.
> >
> > There are few comments,
> >
> > 1. Patch "spi: spi-mem: Add debug message for spi-mem ops"
> >
> > As we discussed in the previous version, drop the unnecessary debug
> > statements after ops execution as this patch trying to add more
> > verbose to before ops execution.
>
>  From last time:
>
> >>> +       dev_dbg(slave->dev,
> >>> +               "exec %02Xh %u-%u-%u addr=%llx dummy cycles=%u data bytes=%u\n",
> >>> +               op->cmd.opcode, op->cmd.buswidth, op->addr.buswidth,
> >>> +               op->data.buswidth, op->addr.val,
> >>> +               op->dummy.buswidth ? op->dummy.nbytes * 8 / op->dummy.buswidth : 0,
> >>> +               op->data.nbytes);
> >>
> >> This looks unnecessary to me, we have debug prints at the end of the
> >> function, which indeed sufficient.
> >>
> >
> > This is insufficient. First, that information is printed after the op
> > is executed. This is too late if one is trying to debug (e.g.) a hang in
> > the spi driver. It's also missing opcode, widths, address, and dummy
> > bytes. The only duplicated info here is nbytes. Since this is debugging
> > information, I think we should print what is most useful for people
> > debugging this subsystem.
>
> I would like if you could respond to my response before I remove this.

Yes, I have commented based on your previous version comments. What
I'm saying here is to adjust even the exiting debug statements in
order to not duplicate if your patch changes.

Jagan.