All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers
@ 2020-11-29 10:39 Jean Pihet
  2020-11-29 10:39 ` [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read Jean Pihet
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Jean Pihet @ 2020-11-29 10:39 UTC (permalink / raw)
  To: u-boot

Use the flags field of the SPI slave struct to pass the dual and quad
read properties, from the SPI NOR layer to the low level driver.
Tested with TI QSPI in 1, 2 and 4 bits modes.

Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
---
 drivers/mtd/spi/spi-nor-core.c | 34 +++++++++++++++++++++++++++++++---
 drivers/spi/spi-mem.c          |  8 +++++++-
 include/spi.h                  |  2 ++
 3 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index e16b0e1462..54569b3cba 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -94,22 +94,50 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 	/* convert the dummy cycles to the number of bytes */
 	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
 
+	/* Check for dual and quad I/O read */
+	switch (op.cmd.opcode) {
+	case SPINOR_OP_READ_1_1_2:
+	case SPINOR_OP_READ_1_2_2:
+	case SPINOR_OP_READ_1_1_2_4B:
+	case SPINOR_OP_READ_1_2_2_4B:
+	case SPINOR_OP_READ_1_2_2_DTR:
+	case SPINOR_OP_READ_1_2_2_DTR_4B:
+		nor->spi->flags |= SPI_XFER_DUAL_READ;
+		break;
+	case SPINOR_OP_READ_1_1_4:
+	case SPINOR_OP_READ_1_4_4:
+	case SPINOR_OP_READ_1_1_4_4B:
+	case SPINOR_OP_READ_1_4_4_4B:
+	case SPINOR_OP_READ_1_4_4_DTR:
+	case SPINOR_OP_READ_1_4_4_DTR_4B:
+		nor->spi->flags |= SPI_XFER_QUAD_READ;
+		break;
+	default:
+		break;
+	}
+
 	while (remaining) {
 		op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
 		ret = spi_mem_adjust_op_size(nor->spi, &op);
 		if (ret)
-			return ret;
+			goto out;
 
 		ret = spi_mem_exec_op(nor->spi, &op);
 		if (ret)
-			return ret;
+			goto out;
 
 		op.addr.val += op.data.nbytes;
 		remaining -= op.data.nbytes;
 		op.data.buf.in += op.data.nbytes;
 	}
 
-	return len;
+	ret = len;
+
+out:
+	/* Reset dual and quad I/O read flags for upcoming transactions */
+	nor->spi->flags &= ~(SPI_XFER_DUAL_READ | SPI_XFER_QUAD_READ);
+
+	return ret;
 }
 
 static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index c095ae9505..24e38ea95e 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -387,9 +387,15 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
 		return ret;
 
 	/* 2nd transfer: rx or tx data path */
+	flag = SPI_XFER_END;
+	/* Check for dual and quad I/O reads */
+	if (slave->flags & SPI_XFER_DUAL_READ)
+		flag |= SPI_XFER_DUAL_READ;
+	if (slave->flags & SPI_XFER_QUAD_READ)
+		flag |= SPI_XFER_QUAD_READ;
 	if (tx_buf || rx_buf) {
 		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
-			       rx_buf, SPI_XFER_END);
+			       rx_buf, flag);
 		if (ret)
 			return ret;
 	}
diff --git a/include/spi.h b/include/spi.h
index ef8c1f6692..cf5f05e526 100644
--- a/include/spi.h
+++ b/include/spi.h
@@ -146,6 +146,8 @@ struct spi_slave {
 #define SPI_XFER_BEGIN		BIT(0)	/* Assert CS before transfer */
 #define SPI_XFER_END		BIT(1)	/* Deassert CS after transfer */
 #define SPI_XFER_ONCE		(SPI_XFER_BEGIN | SPI_XFER_END)
+#define SPI_XFER_DUAL_READ	BIT(2)  /* Dual I/O read */
+#define SPI_XFER_QUAD_READ	BIT(3)  /* Quad I/O read */
 };
 
 /**
-- 
2.26.2

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

* [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read
  2020-11-29 10:39 [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Jean Pihet
@ 2020-11-29 10:39 ` Jean Pihet
  2020-11-30 13:09   ` Pratyush Yadav
  2020-11-29 10:39 ` [PATCH 3/3] TI QSPI: prevent a 4-byte write to the flash on read access Jean Pihet
  2020-11-30 12:37 ` [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Pratyush Yadav
  2 siblings, 1 reply; 7+ messages in thread
From: Jean Pihet @ 2020-11-29 10:39 UTC (permalink / raw)
  To: u-boot

TI QSPI: the TI QSPI IP has a limitation of 64MB of MMIO so use
the MMIO mode to read below 64MB and fallback to the SW generated
sequences to read above 64MB.
The TI QSPI IP has another limitation of 4096 words per frame,
so split the accesses into smaller ones.
Also fix the TI QSPI operating frequency in order to correctly
generate the required SPI CLK frequency.

Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
---
 drivers/spi/Kconfig   |  9 ++++++
 drivers/spi/ti_qspi.c | 68 ++++++++++++++++++++++++++++++++++++-------
 2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index f7a9852565..5d24029ff0 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -395,10 +395,19 @@ config TEGRA210_QSPI
 config TI_QSPI
 	bool "TI QSPI driver"
 	imply TI_EDMA3
+	imply TI_QSPI_MQX_FRAME_SIZE
 	help
 	  Enable the TI Quad-SPI (QSPI) driver for DRA7xx and AM43xx evms.
 	  This driver support spi flash single, quad and memory reads.
 
+config TI_QSPI_MAX_FRAME_SIZE
+	int "TI QSPI max frame size"
+	default 4096
+	help
+	  TI Quad-SPI (QSPI) IP block has a maximum number of 4096 words in a
+	  frame, in non-memory mapped mode. A frame includes the command,
+	  arguments and dummy bytes.
+
 config UNIPHIER_SPI
 	bool "Socionext UniPhier SPI driver"
 	depends on ARCH_UNIPHIER
diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
index 5fdbb49442..57192b9c0a 100644
--- a/drivers/spi/ti_qspi.c
+++ b/drivers/spi/ti_qspi.c
@@ -29,7 +29,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 /* ti qpsi register bit masks */
 #define QSPI_TIMEOUT                    2000000
-#define QSPI_FCLK			192000000
+#define QSPI_FCLK                       48000000
 #define QSPI_DRA7XX_FCLK                76800000
 #define QSPI_WLEN_MAX_BITS		128
 #define QSPI_WLEN_MAX_BYTES		(QSPI_WLEN_MAX_BITS >> 3)
@@ -41,10 +41,11 @@ DECLARE_GLOBAL_DATA_PTR;
 #define QSPI_EN_CS(n)                   (n << 28)
 #define QSPI_WLEN(n)                    ((n-1) << 19)
 #define QSPI_3_PIN                      BIT(18)
-#define QSPI_RD_SNGL                    BIT(16)
+#define QSPI_RD_SNGL                    (1 << 16)
+#define QSPI_RD_DUAL                    (3 << 16)
+#define QSPI_RD_QUAD                    (7 << 16)
 #define QSPI_WR_SNGL                    (2 << 16)
 #define QSPI_INVAL                      (4 << 16)
-#define QSPI_RD_QUAD                    (7 << 16)
 /* device control */
 #define QSPI_CKPHA(n)                   (1 << (2 + n*8))
 #define QSPI_CSPOL(n)                   (1 << (1 + n*8))
@@ -155,6 +156,7 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
 			const void *dout, void *din, unsigned long flags)
 {
 	struct dm_spi_slave_platdata *slave = dev_get_parent_platdata(dev);
+	struct spi_slave *spi_slave = dev_get_parent_priv(dev);
 	struct ti_qspi_priv *priv;
 	struct udevice *bus;
 	uint words = bitlen >> 3; /* fixed 8-bit word length */
@@ -182,7 +184,6 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
 
 	/* Setup command reg */
 	priv->cmd = 0;
-	priv->cmd |= QSPI_WLEN(8);
 	priv->cmd |= QSPI_EN_CS(cs);
 	if (priv->mode & SPI_3WIRE)
 		priv->cmd |= QSPI_3_PIN;
@@ -206,15 +207,16 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
 				writel(data, &priv->base->data1);
 				data = cpu_to_be32(*txbuf++);
 				writel(data, &priv->base->data);
-				cmd &= ~QSPI_WLEN_MASK;
 				cmd |= QSPI_WLEN(QSPI_WLEN_MAX_BITS);
 				xfer_len = QSPI_WLEN_MAX_BYTES;
 			} else {
 				writeb(*txp, &priv->base->data);
+				cmd |= QSPI_WLEN(8);
 				xfer_len = 1;
 			}
 			debug("tx cmd %08x dc %08x\n",
 			      cmd | QSPI_WR_SNGL, priv->dc);
+			// Only 1-bit write is supported
 			writel(cmd | QSPI_WR_SNGL, &priv->base->cmd);
 			status = readl(&priv->base->status);
 			timeout = QSPI_TIMEOUT;
@@ -229,9 +231,17 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
 			debug("tx done, status %08x\n", status);
 		}
 		if (rxp) {
+			u32 rx;
+			/* Check for dual and quad reads */
+			u32 readcmd_cmd = QSPI_RD_SNGL | QSPI_WLEN(8);
+			if (spi_slave->flags & SPI_XFER_DUAL_READ)
+				readcmd_cmd = QSPI_RD_DUAL | QSPI_WLEN(16);
+			if (spi_slave->flags & SPI_XFER_QUAD_READ)
+				readcmd_cmd = QSPI_RD_QUAD | QSPI_WLEN(32);
+
 			debug("rx cmd %08x dc %08x\n",
-			      ((u32)(priv->cmd | QSPI_RD_SNGL)), priv->dc);
-			writel(priv->cmd | QSPI_RD_SNGL, &priv->base->cmd);
+			      ((u32)(priv->cmd | readcmd_cmd)), priv->dc);
+			writel(priv->cmd | readcmd_cmd, &priv->base->cmd);
 			status = readl(&priv->base->status);
 			timeout = QSPI_TIMEOUT;
 			while ((status & QSPI_WC_BUSY) != QSPI_XFER_DONE) {
@@ -241,10 +251,28 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
 				}
 				status = readl(&priv->base->status);
 			}
-			*rxp++ = readl(&priv->base->data);
-			xfer_len = 1;
-			debug("rx done, status %08x, read %02x\n",
-			      status, *(rxp-1));
+			rx = readl(&priv->base->data);
+			if (spi_slave->flags & SPI_XFER_QUAD_READ) {
+				if (words >= 1)
+					*rxp++ = rx >> 24;
+				if (words >= 2)
+					*rxp++ = rx >> 16;
+				if (words >= 3)
+					*rxp++ = rx >> 8;
+				if (words >= 4)
+					*rxp++ = rx;
+				xfer_len = (words >= 4) ? 4 : words;
+			} else if (spi_slave->flags & SPI_XFER_DUAL_READ) {
+				if (words >= 1)
+					*rxp++ = rx >> 8;
+				if (words >= 2)
+					*rxp++ = rx;
+				xfer_len = (words >= 2) ? 2 : words;
+			} else {
+				*rxp++ = rx;
+				xfer_len = 1;
+			}
+			debug("rx done, status %08x, rx=%08x\n", status, rx);
 		}
 		words -= xfer_len;
 	}
@@ -353,6 +381,23 @@ static int ti_qspi_exec_mem_op(struct spi_slave *slave,
 	return ret;
 }
 
+static int ti_qspi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
+{
+#ifdef CONFIG_TI_QSPI_MAX_FRAME_SIZE
+	// Adjust the data length to fit in the QSPI max frame length
+	if (op->data.dir == SPI_MEM_DATA_IN) {
+		unsigned int data_len = CONFIG_TI_QSPI_MAX_FRAME_SIZE -
+								sizeof(op->cmd.opcode) - op->addr.nbytes -
+								op->dummy.nbytes;
+		// Align the data on cache line
+		data_len = data_len & ~(ARCH_DMA_MINALIGN - 1);
+		op->data.nbytes = min(op->data.nbytes, data_len);
+	}
+#endif
+
+	return 0;
+}
+
 static int ti_qspi_claim_bus(struct udevice *dev)
 {
 	struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
@@ -482,6 +527,7 @@ static int ti_qspi_ofdata_to_platdata(struct udevice *bus)
 
 static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
 	.exec_op = ti_qspi_exec_mem_op,
+	.adjust_op_size = ti_qspi_adjust_size,
 };
 
 static const struct dm_spi_ops ti_qspi_ops = {
-- 
2.26.2

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

* [PATCH 3/3] TI QSPI: prevent a 4-byte write to the flash on read access
  2020-11-29 10:39 [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Jean Pihet
  2020-11-29 10:39 ` [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read Jean Pihet
@ 2020-11-29 10:39 ` Jean Pihet
  2020-11-30 12:37 ` [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Pratyush Yadav
  2 siblings, 0 replies; 7+ messages in thread
From: Jean Pihet @ 2020-11-29 10:39 UTC (permalink / raw)
  To: u-boot

In MMIO mode, a write to the flash generates write commands to the device.
Prevent this from happening when reading from the device.

Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
---
 drivers/spi/ti_qspi.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
index 57192b9c0a..40e1988df5 100644
--- a/drivers/spi/ti_qspi.c
+++ b/drivers/spi/ti_qspi.c
@@ -305,8 +305,6 @@ static void ti_qspi_copy_mmap(void *data, void *offset, size_t len)
 #else
 	memcpy_fromio(data, offset, len);
 #endif
-
-	*((unsigned int *)offset) += len;
 }
 
 static void ti_qspi_setup_mmap_read(struct ti_qspi_priv *priv, int cs,
-- 
2.26.2

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

* [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers
  2020-11-29 10:39 [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Jean Pihet
  2020-11-29 10:39 ` [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read Jean Pihet
  2020-11-29 10:39 ` [PATCH 3/3] TI QSPI: prevent a 4-byte write to the flash on read access Jean Pihet
@ 2020-11-30 12:37 ` Pratyush Yadav
  2020-12-01 19:03   ` Jean Pihet
  2 siblings, 1 reply; 7+ messages in thread
From: Pratyush Yadav @ 2020-11-30 12:37 UTC (permalink / raw)
  To: u-boot

Hi Jean,

On 29/11/20 11:39AM, Jean Pihet wrote:
> Use the flags field of the SPI slave struct to pass the dual and quad
> read properties, from the SPI NOR layer to the low level driver.
> Tested with TI QSPI in 1, 2 and 4 bits modes.
> 
> Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
> ---
>  drivers/mtd/spi/spi-nor-core.c | 34 +++++++++++++++++++++++++++++++---
>  drivers/spi/spi-mem.c          |  8 +++++++-
>  include/spi.h                  |  2 ++
>  3 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index e16b0e1462..54569b3cba 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -94,22 +94,50 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>  	/* convert the dummy cycles to the number of bytes */
>  	op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
>  
> +	/* Check for dual and quad I/O read */
> +	switch (op.cmd.opcode) {
> +	case SPINOR_OP_READ_1_1_2:
> +	case SPINOR_OP_READ_1_2_2:
> +	case SPINOR_OP_READ_1_1_2_4B:
> +	case SPINOR_OP_READ_1_2_2_4B:
> +	case SPINOR_OP_READ_1_2_2_DTR:
> +	case SPINOR_OP_READ_1_2_2_DTR_4B:
> +		nor->spi->flags |= SPI_XFER_DUAL_READ;
> +		break;
> +	case SPINOR_OP_READ_1_1_4:
> +	case SPINOR_OP_READ_1_4_4:
> +	case SPINOR_OP_READ_1_1_4_4B:
> +	case SPINOR_OP_READ_1_4_4_4B:
> +	case SPINOR_OP_READ_1_4_4_DTR:
> +	case SPINOR_OP_READ_1_4_4_DTR_4B:
> +		nor->spi->flags |= SPI_XFER_QUAD_READ;
> +		break;
> +	default:
> +		break;
> +	}
> +

NACK. What if a flash uses some other opcode for dual or quad reads?

Why not use op->data.buswidth in ti_qspi_exec_mem_op() to enable dual or 
quad mode? In fact, ti_qspi_setup_mmap_read() already checks this and 
enables dual or quad mode. What problem does this patch solve then?

>  	while (remaining) {
>  		op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
>  		ret = spi_mem_adjust_op_size(nor->spi, &op);
>  		if (ret)
> -			return ret;
> +			goto out;
>  
>  		ret = spi_mem_exec_op(nor->spi, &op);
>  		if (ret)
> -			return ret;
> +			goto out;
>  
>  		op.addr.val += op.data.nbytes;
>  		remaining -= op.data.nbytes;
>  		op.data.buf.in += op.data.nbytes;
>  	}
>  
> -	return len;
> +	ret = len;
> +
> +out:
> +	/* Reset dual and quad I/O read flags for upcoming transactions */
> +	nor->spi->flags &= ~(SPI_XFER_DUAL_READ | SPI_XFER_QUAD_READ);
> +
> +	return ret;
>  }
>  
>  static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index c095ae9505..24e38ea95e 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -387,9 +387,15 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
>  		return ret;
>  
>  	/* 2nd transfer: rx or tx data path */
> +	flag = SPI_XFER_END;
> +	/* Check for dual and quad I/O reads */
> +	if (slave->flags & SPI_XFER_DUAL_READ)
> +		flag |= SPI_XFER_DUAL_READ;
> +	if (slave->flags & SPI_XFER_QUAD_READ)
> +		flag |= SPI_XFER_QUAD_READ;

Ah, so you are targeting the "legacy" path. Seeing that ti_qspi does 
support the SPI MEM path, this would be executed when the read exceeds 
priv->mmap_size, correct?

In any case, I don't see why you need slave->flags. Why can't you set 
these flags by checking op->data.buswidth and op->data.dir? This would 
make your fix transparent to SPI NOR, which IMO is a much better idea.

>  	if (tx_buf || rx_buf) {
>  		ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
> -			       rx_buf, SPI_XFER_END);
> +			       rx_buf, flag);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/include/spi.h b/include/spi.h
> index ef8c1f6692..cf5f05e526 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -146,6 +146,8 @@ struct spi_slave {
>  #define SPI_XFER_BEGIN		BIT(0)	/* Assert CS before transfer */
>  #define SPI_XFER_END		BIT(1)	/* Deassert CS after transfer */
>  #define SPI_XFER_ONCE		(SPI_XFER_BEGIN | SPI_XFER_END)
> +#define SPI_XFER_DUAL_READ	BIT(2)  /* Dual I/O read */
> +#define SPI_XFER_QUAD_READ	BIT(3)  /* Quad I/O read */
>  };
>  
>  /**
> -- 
> 2.26.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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

* [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read
  2020-11-29 10:39 ` [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read Jean Pihet
@ 2020-11-30 13:09   ` Pratyush Yadav
  2020-12-01 19:06     ` Jean Pihet
  0 siblings, 1 reply; 7+ messages in thread
From: Pratyush Yadav @ 2020-11-30 13:09 UTC (permalink / raw)
  To: u-boot

Hi Jean,

Quick disclaimer: I am not familiar with the IP or the driver so I just 
gave this patch a brief read and did not dig into it a lot. This is in 
no way a comprehensive review.

On 29/11/20 11:39AM, Jean Pihet wrote:
> TI QSPI: the TI QSPI IP has a limitation of 64MB of MMIO so use
> the MMIO mode to read below 64MB and fallback to the SW generated
> sequences to read above 64MB.
> The TI QSPI IP has another limitation of 4096 words per frame,
> so split the accesses into smaller ones.

Please split this change...

> Also fix the TI QSPI operating frequency in order to correctly
> generate the required SPI CLK frequency.

... and this one into a separate patch. If they cause any bugs or 
regressions it would be easier to bisect and revert.

> 
> Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
> ---
>  drivers/spi/Kconfig   |  9 ++++++
>  drivers/spi/ti_qspi.c | 68 ++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index f7a9852565..5d24029ff0 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -395,10 +395,19 @@ config TEGRA210_QSPI
>  config TI_QSPI
>  	bool "TI QSPI driver"
>  	imply TI_EDMA3
> +	imply TI_QSPI_MQX_FRAME_SIZE
Typo?                  ^

>  	help
>  	  Enable the TI Quad-SPI (QSPI) driver for DRA7xx and AM43xx evms.
>  	  This driver support spi flash single, quad and memory reads.
>  
> +config TI_QSPI_MAX_FRAME_SIZE
> +	int "TI QSPI max frame size"
> +	default 4096
> +	help
> +	  TI Quad-SPI (QSPI) IP block has a maximum number of 4096 words in a
> +	  frame, in non-memory mapped mode. A frame includes the command,
> +	  arguments and dummy bytes.

Can the frame size change between IP versions/implementations? If it can 
not then I don't see a need for defining a config here. Just use 4096 
directly in the driver.

> +
>  config UNIPHIER_SPI
>  	bool "Socionext UniPhier SPI driver"
>  	depends on ARCH_UNIPHIER
> diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
> index 5fdbb49442..57192b9c0a 100644
> --- a/drivers/spi/ti_qspi.c
> +++ b/drivers/spi/ti_qspi.c
> @@ -29,7 +29,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  
>  /* ti qpsi register bit masks */
>  #define QSPI_TIMEOUT                    2000000
> -#define QSPI_FCLK			192000000
> +#define QSPI_FCLK                       48000000

This is fishy. Different platforms can have different FCLK values. 
Looking at ti_qspi_ids[], it looks like am4372 uses 192 MHz and dra7xxx 
uses 76.8 MHz. Changing QSPI_FCLK like this might break am4372.

Do what dra7xx does and add an entry for your platform in the ids table 
and set .data to the FCLK value for your platform.

>  #define QSPI_DRA7XX_FCLK                76800000
>  #define QSPI_WLEN_MAX_BITS		128
>  #define QSPI_WLEN_MAX_BYTES		(QSPI_WLEN_MAX_BITS >> 3)
> @@ -41,10 +41,11 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define QSPI_EN_CS(n)                   (n << 28)
>  #define QSPI_WLEN(n)                    ((n-1) << 19)
>  #define QSPI_3_PIN                      BIT(18)
> -#define QSPI_RD_SNGL                    BIT(16)
> +#define QSPI_RD_SNGL                    (1 << 16)
> +#define QSPI_RD_DUAL                    (3 << 16)
> +#define QSPI_RD_QUAD                    (7 << 16)
>  #define QSPI_WR_SNGL                    (2 << 16)
>  #define QSPI_INVAL                      (4 << 16)
> -#define QSPI_RD_QUAD                    (7 << 16)
>  /* device control */
>  #define QSPI_CKPHA(n)                   (1 << (2 + n*8))
>  #define QSPI_CSPOL(n)                   (1 << (1 + n*8))
> @@ -155,6 +156,7 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
>  			const void *dout, void *din, unsigned long flags)
>  {
>  	struct dm_spi_slave_platdata *slave = dev_get_parent_platdata(dev);
> +	struct spi_slave *spi_slave = dev_get_parent_priv(dev);
>  	struct ti_qspi_priv *priv;
>  	struct udevice *bus;
>  	uint words = bitlen >> 3; /* fixed 8-bit word length */
> @@ -182,7 +184,6 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
>  
>  	/* Setup command reg */
>  	priv->cmd = 0;
> -	priv->cmd |= QSPI_WLEN(8);
>  	priv->cmd |= QSPI_EN_CS(cs);
>  	if (priv->mode & SPI_3WIRE)
>  		priv->cmd |= QSPI_3_PIN;
> @@ -206,15 +207,16 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
>  				writel(data, &priv->base->data1);
>  				data = cpu_to_be32(*txbuf++);
>  				writel(data, &priv->base->data);
> -				cmd &= ~QSPI_WLEN_MASK;
>  				cmd |= QSPI_WLEN(QSPI_WLEN_MAX_BITS);
>  				xfer_len = QSPI_WLEN_MAX_BYTES;
>  			} else {
>  				writeb(*txp, &priv->base->data);
> +				cmd |= QSPI_WLEN(8);
>  				xfer_len = 1;
>  			}
>  			debug("tx cmd %08x dc %08x\n",
>  			      cmd | QSPI_WR_SNGL, priv->dc);
> +			// Only 1-bit write is supported

Do not use C++ style comments. Same for the other such comments below.

>  			writel(cmd | QSPI_WR_SNGL, &priv->base->cmd);
>  			status = readl(&priv->base->status);
>  			timeout = QSPI_TIMEOUT;
> @@ -229,9 +231,17 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
>  			debug("tx done, status %08x\n", status);
>  		}
>  		if (rxp) {
> +			u32 rx;
> +			/* Check for dual and quad reads */
> +			u32 readcmd_cmd = QSPI_RD_SNGL | QSPI_WLEN(8);
> +			if (spi_slave->flags & SPI_XFER_DUAL_READ)
> +				readcmd_cmd = QSPI_RD_DUAL | QSPI_WLEN(16);
> +			if (spi_slave->flags & SPI_XFER_QUAD_READ)
> +				readcmd_cmd = QSPI_RD_QUAD | QSPI_WLEN(32);
> +
>  			debug("rx cmd %08x dc %08x\n",
> -			      ((u32)(priv->cmd | QSPI_RD_SNGL)), priv->dc);
> -			writel(priv->cmd | QSPI_RD_SNGL, &priv->base->cmd);
> +			      ((u32)(priv->cmd | readcmd_cmd)), priv->dc);
> +			writel(priv->cmd | readcmd_cmd, &priv->base->cmd);
>  			status = readl(&priv->base->status);
>  			timeout = QSPI_TIMEOUT;
>  			while ((status & QSPI_WC_BUSY) != QSPI_XFER_DONE) {
> @@ -241,10 +251,28 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
>  				}
>  				status = readl(&priv->base->status);
>  			}
> -			*rxp++ = readl(&priv->base->data);
> -			xfer_len = 1;
> -			debug("rx done, status %08x, read %02x\n",
> -			      status, *(rxp-1));
> +			rx = readl(&priv->base->data);
> +			if (spi_slave->flags & SPI_XFER_QUAD_READ) {
> +				if (words >= 1)
> +					*rxp++ = rx >> 24;
> +				if (words >= 2)
> +					*rxp++ = rx >> 16;
> +				if (words >= 3)
> +					*rxp++ = rx >> 8;
> +				if (words >= 4)
> +					*rxp++ = rx;
> +				xfer_len = (words >= 4) ? 4 : words;
> +			} else if (spi_slave->flags & SPI_XFER_DUAL_READ) {
> +				if (words >= 1)
> +					*rxp++ = rx >> 8;
> +				if (words >= 2)
> +					*rxp++ = rx;
> +				xfer_len = (words >= 2) ? 2 : words;
> +			} else {
> +				*rxp++ = rx;
> +				xfer_len = 1;
> +			}
> +			debug("rx done, status %08x, rx=%08x\n", status, rx);
>  		}
>  		words -= xfer_len;
>  	}
> @@ -353,6 +381,23 @@ static int ti_qspi_exec_mem_op(struct spi_slave *slave,
>  	return ret;
>  }
>  
> +static int ti_qspi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
> +{
> +#ifdef CONFIG_TI_QSPI_MAX_FRAME_SIZE

Try to avoid conditional compilation. Using `if (CONFIG_IS_ENABLED(TI_QSPI_MQX_FRAME_SIZE))`
should work just as fine I think.

> +	// Adjust the data length to fit in the QSPI max frame length
> +	if (op->data.dir == SPI_MEM_DATA_IN) {
> +		unsigned int data_len = CONFIG_TI_QSPI_MAX_FRAME_SIZE -
> +								sizeof(op->cmd.opcode) - op->addr.nbytes -
> +								op->dummy.nbytes;
> +		// Align the data on cache line
> +		data_len = data_len & ~(ARCH_DMA_MINALIGN - 1);
> +		op->data.nbytes = min(op->data.nbytes, data_len);
> +	}
> +#endif
> +
> +	return 0;
> +}
> +
>  static int ti_qspi_claim_bus(struct udevice *dev)
>  {
>  	struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
> @@ -482,6 +527,7 @@ static int ti_qspi_ofdata_to_platdata(struct udevice *bus)
>  
>  static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
>  	.exec_op = ti_qspi_exec_mem_op,
> +	.adjust_op_size = ti_qspi_adjust_size,
>  };
>  
>  static const struct dm_spi_ops ti_qspi_ops = {
> -- 
> 2.26.2
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments India

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

* [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers
  2020-11-30 12:37 ` [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Pratyush Yadav
@ 2020-12-01 19:03   ` Jean Pihet
  0 siblings, 0 replies; 7+ messages in thread
From: Jean Pihet @ 2020-12-01 19:03 UTC (permalink / raw)
  To: u-boot

Hi Pratyush,

On Mon, Nov 30, 2020 at 1:37 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Jean,
>
> On 29/11/20 11:39AM, Jean Pihet wrote:
> > Use the flags field of the SPI slave struct to pass the dual and quad
> > read properties, from the SPI NOR layer to the low level driver.
> > Tested with TI QSPI in 1, 2 and 4 bits modes.
> >
> > Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
> > ---
> >  drivers/mtd/spi/spi-nor-core.c | 34 +++++++++++++++++++++++++++++++---
> >  drivers/spi/spi-mem.c          |  8 +++++++-
> >  include/spi.h                  |  2 ++
> >  3 files changed, 40 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > index e16b0e1462..54569b3cba 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -94,22 +94,50 @@ static ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
> >       /* convert the dummy cycles to the number of bytes */
> >       op.dummy.nbytes = (nor->read_dummy * op.dummy.buswidth) / 8;
> >
> > +     /* Check for dual and quad I/O read */
> > +     switch (op.cmd.opcode) {
> > +     case SPINOR_OP_READ_1_1_2:
> > +     case SPINOR_OP_READ_1_2_2:
> > +     case SPINOR_OP_READ_1_1_2_4B:
> > +     case SPINOR_OP_READ_1_2_2_4B:
> > +     case SPINOR_OP_READ_1_2_2_DTR:
> > +     case SPINOR_OP_READ_1_2_2_DTR_4B:
> > +             nor->spi->flags |= SPI_XFER_DUAL_READ;
> > +             break;
> > +     case SPINOR_OP_READ_1_1_4:
> > +     case SPINOR_OP_READ_1_4_4:
> > +     case SPINOR_OP_READ_1_1_4_4B:
> > +     case SPINOR_OP_READ_1_4_4_4B:
> > +     case SPINOR_OP_READ_1_4_4_DTR:
> > +     case SPINOR_OP_READ_1_4_4_DTR_4B:
> > +             nor->spi->flags |= SPI_XFER_QUAD_READ;
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
>
> NACK. What if a flash uses some other opcode for dual or quad reads?
>
> Why not use op->data.buswidth in ti_qspi_exec_mem_op() to enable dual or
> quad mode? In fact, ti_qspi_setup_mmap_read() already checks this and
> enables dual or quad mode. What problem does this patch solve then?

Ok I will rework this that way.
The goal is to enable bigger flashes (>64MB) and to optimize the
transfer speed. I will add a cover letter to the patch series to
better describe the goals.

>
> >       while (remaining) {
> >               op.data.nbytes = remaining < UINT_MAX ? remaining : UINT_MAX;
> >               ret = spi_mem_adjust_op_size(nor->spi, &op);
> >               if (ret)
> > -                     return ret;
> > +                     goto out;
> >
> >               ret = spi_mem_exec_op(nor->spi, &op);
> >               if (ret)
> > -                     return ret;
> > +                     goto out;
> >
> >               op.addr.val += op.data.nbytes;
> >               remaining -= op.data.nbytes;
> >               op.data.buf.in += op.data.nbytes;
> >       }
> >
> > -     return len;
> > +     ret = len;
> > +
> > +out:
> > +     /* Reset dual and quad I/O read flags for upcoming transactions */
> > +     nor->spi->flags &= ~(SPI_XFER_DUAL_READ | SPI_XFER_QUAD_READ);
> > +
> > +     return ret;
> >  }
> >
> >  static ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> > index c095ae9505..24e38ea95e 100644
> > --- a/drivers/spi/spi-mem.c
> > +++ b/drivers/spi/spi-mem.c
> > @@ -387,9 +387,15 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op)
> >               return ret;
> >
> >       /* 2nd transfer: rx or tx data path */
> > +     flag = SPI_XFER_END;
> > +     /* Check for dual and quad I/O reads */
> > +     if (slave->flags & SPI_XFER_DUAL_READ)
> > +             flag |= SPI_XFER_DUAL_READ;
> > +     if (slave->flags & SPI_XFER_QUAD_READ)
> > +             flag |= SPI_XFER_QUAD_READ;
>
> Ah, so you are targeting the "legacy" path. Seeing that ti_qspi does
> support the SPI MEM path, this would be executed when the read exceeds
> priv->mmap_size, correct?
Yes exactly. The TI QSPI controller can only mmap 64MB.

>
> In any case, I don't see why you need slave->flags. Why can't you set
> these flags by checking op->data.buswidth and op->data.dir? This would
> make your fix transparent to SPI NOR, which IMO is a much better idea.

Ok let me rework this.

>
> >       if (tx_buf || rx_buf) {
> >               ret = spi_xfer(slave, op->data.nbytes * 8, tx_buf,
> > -                            rx_buf, SPI_XFER_END);
> > +                            rx_buf, flag);
> >               if (ret)
> >                       return ret;
> >       }
> > diff --git a/include/spi.h b/include/spi.h
> > index ef8c1f6692..cf5f05e526 100644
> > --- a/include/spi.h
> > +++ b/include/spi.h
> > @@ -146,6 +146,8 @@ struct spi_slave {
> >  #define SPI_XFER_BEGIN               BIT(0)  /* Assert CS before transfer */
> >  #define SPI_XFER_END         BIT(1)  /* Deassert CS after transfer */
> >  #define SPI_XFER_ONCE                (SPI_XFER_BEGIN | SPI_XFER_END)
> > +#define SPI_XFER_DUAL_READ   BIT(2)  /* Dual I/O read */
> > +#define SPI_XFER_QUAD_READ   BIT(3)  /* Quad I/O read */
> >  };
> >
> >  /**
> > --
> > 2.26.2
> >
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India

Thanks for reviewing!

BR,
Jean

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

* [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read
  2020-11-30 13:09   ` Pratyush Yadav
@ 2020-12-01 19:06     ` Jean Pihet
  0 siblings, 0 replies; 7+ messages in thread
From: Jean Pihet @ 2020-12-01 19:06 UTC (permalink / raw)
  To: u-boot

Pratyush,

On Mon, Nov 30, 2020 at 2:09 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Jean,
>
> Quick disclaimer: I am not familiar with the IP or the driver so I just
> gave this patch a brief read and did not dig into it a lot. This is in
> no way a comprehensive review.
>
> On 29/11/20 11:39AM, Jean Pihet wrote:
> > TI QSPI: the TI QSPI IP has a limitation of 64MB of MMIO so use
> > the MMIO mode to read below 64MB and fallback to the SW generated
> > sequences to read above 64MB.
> > The TI QSPI IP has another limitation of 4096 words per frame,
> > so split the accesses into smaller ones.
>
> Please split this change...

Ok

>
> > Also fix the TI QSPI operating frequency in order to correctly
> > generate the required SPI CLK frequency.
>
> ... and this one into a separate patch. If they cause any bugs or
> regressions it would be easier to bisect and revert.
>
> >
> > Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com>
> > ---
> >  drivers/spi/Kconfig   |  9 ++++++
> >  drivers/spi/ti_qspi.c | 68 ++++++++++++++++++++++++++++++++++++-------
> >  2 files changed, 66 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index f7a9852565..5d24029ff0 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -395,10 +395,19 @@ config TEGRA210_QSPI
> >  config TI_QSPI
> >       bool "TI QSPI driver"
> >       imply TI_EDMA3
> > +     imply TI_QSPI_MQX_FRAME_SIZE
> Typo?                  ^

Oh yes, this is a leftover.

>
> >       help
> >         Enable the TI Quad-SPI (QSPI) driver for DRA7xx and AM43xx evms.
> >         This driver support spi flash single, quad and memory reads.
> >
> > +config TI_QSPI_MAX_FRAME_SIZE
> > +     int "TI QSPI max frame size"
> > +     default 4096
> > +     help
> > +       TI Quad-SPI (QSPI) IP block has a maximum number of 4096 words in a
> > +       frame, in non-memory mapped mode. A frame includes the command,
> > +       arguments and dummy bytes.
>
> Can the frame size change between IP versions/implementations? If it can
> not then I don't see a need for defining a config here. Just use 4096
> directly in the driver.

I am not sure about that. The TRM has a fixed frame size, are there
other sizes in other chip implementation.

>
> > +
> >  config UNIPHIER_SPI
> >       bool "Socionext UniPhier SPI driver"
> >       depends on ARCH_UNIPHIER
> > diff --git a/drivers/spi/ti_qspi.c b/drivers/spi/ti_qspi.c
> > index 5fdbb49442..57192b9c0a 100644
> > --- a/drivers/spi/ti_qspi.c
> > +++ b/drivers/spi/ti_qspi.c
> > @@ -29,7 +29,7 @@ DECLARE_GLOBAL_DATA_PTR;
> >
> >  /* ti qpsi register bit masks */
> >  #define QSPI_TIMEOUT                    2000000
> > -#define QSPI_FCLK                    192000000
> > +#define QSPI_FCLK                       48000000
>
> This is fishy. Different platforms can have different FCLK values.
> Looking at ti_qspi_ids[], it looks like am4372 uses 192 MHz and dra7xxx
> uses 76.8 MHz. Changing QSPI_FCLK like this might break am4372.
>
> Do what dra7xx does and add an entry for your platform in the ids table
> and set .data to the FCLK value for your platform.

Ok!

>
> >  #define QSPI_DRA7XX_FCLK                76800000
> >  #define QSPI_WLEN_MAX_BITS           128
> >  #define QSPI_WLEN_MAX_BYTES          (QSPI_WLEN_MAX_BITS >> 3)
> > @@ -41,10 +41,11 @@ DECLARE_GLOBAL_DATA_PTR;
> >  #define QSPI_EN_CS(n)                   (n << 28)
> >  #define QSPI_WLEN(n)                    ((n-1) << 19)
> >  #define QSPI_3_PIN                      BIT(18)
> > -#define QSPI_RD_SNGL                    BIT(16)
> > +#define QSPI_RD_SNGL                    (1 << 16)
> > +#define QSPI_RD_DUAL                    (3 << 16)
> > +#define QSPI_RD_QUAD                    (7 << 16)
> >  #define QSPI_WR_SNGL                    (2 << 16)
> >  #define QSPI_INVAL                      (4 << 16)
> > -#define QSPI_RD_QUAD                    (7 << 16)
> >  /* device control */
> >  #define QSPI_CKPHA(n)                   (1 << (2 + n*8))
> >  #define QSPI_CSPOL(n)                   (1 << (1 + n*8))
> > @@ -155,6 +156,7 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> >                       const void *dout, void *din, unsigned long flags)
> >  {
> >       struct dm_spi_slave_platdata *slave = dev_get_parent_platdata(dev);
> > +     struct spi_slave *spi_slave = dev_get_parent_priv(dev);
> >       struct ti_qspi_priv *priv;
> >       struct udevice *bus;
> >       uint words = bitlen >> 3; /* fixed 8-bit word length */
> > @@ -182,7 +184,6 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> >
> >       /* Setup command reg */
> >       priv->cmd = 0;
> > -     priv->cmd |= QSPI_WLEN(8);
> >       priv->cmd |= QSPI_EN_CS(cs);
> >       if (priv->mode & SPI_3WIRE)
> >               priv->cmd |= QSPI_3_PIN;
> > @@ -206,15 +207,16 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> >                               writel(data, &priv->base->data1);
> >                               data = cpu_to_be32(*txbuf++);
> >                               writel(data, &priv->base->data);
> > -                             cmd &= ~QSPI_WLEN_MASK;
> >                               cmd |= QSPI_WLEN(QSPI_WLEN_MAX_BITS);
> >                               xfer_len = QSPI_WLEN_MAX_BYTES;
> >                       } else {
> >                               writeb(*txp, &priv->base->data);
> > +                             cmd |= QSPI_WLEN(8);
> >                               xfer_len = 1;
> >                       }
> >                       debug("tx cmd %08x dc %08x\n",
> >                             cmd | QSPI_WR_SNGL, priv->dc);
> > +                     // Only 1-bit write is supported
>
> Do not use C++ style comments. Same for the other such comments below.
>
> >                       writel(cmd | QSPI_WR_SNGL, &priv->base->cmd);
> >                       status = readl(&priv->base->status);
> >                       timeout = QSPI_TIMEOUT;
> > @@ -229,9 +231,17 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> >                       debug("tx done, status %08x\n", status);
> >               }
> >               if (rxp) {
> > +                     u32 rx;
> > +                     /* Check for dual and quad reads */
> > +                     u32 readcmd_cmd = QSPI_RD_SNGL | QSPI_WLEN(8);
> > +                     if (spi_slave->flags & SPI_XFER_DUAL_READ)
> > +                             readcmd_cmd = QSPI_RD_DUAL | QSPI_WLEN(16);
> > +                     if (spi_slave->flags & SPI_XFER_QUAD_READ)
> > +                             readcmd_cmd = QSPI_RD_QUAD | QSPI_WLEN(32);
> > +
> >                       debug("rx cmd %08x dc %08x\n",
> > -                           ((u32)(priv->cmd | QSPI_RD_SNGL)), priv->dc);
> > -                     writel(priv->cmd | QSPI_RD_SNGL, &priv->base->cmd);
> > +                           ((u32)(priv->cmd | readcmd_cmd)), priv->dc);
> > +                     writel(priv->cmd | readcmd_cmd, &priv->base->cmd);
> >                       status = readl(&priv->base->status);
> >                       timeout = QSPI_TIMEOUT;
> >                       while ((status & QSPI_WC_BUSY) != QSPI_XFER_DONE) {
> > @@ -241,10 +251,28 @@ static int ti_qspi_xfer(struct udevice *dev, unsigned int bitlen,
> >                               }
> >                               status = readl(&priv->base->status);
> >                       }
> > -                     *rxp++ = readl(&priv->base->data);
> > -                     xfer_len = 1;
> > -                     debug("rx done, status %08x, read %02x\n",
> > -                           status, *(rxp-1));
> > +                     rx = readl(&priv->base->data);
> > +                     if (spi_slave->flags & SPI_XFER_QUAD_READ) {
> > +                             if (words >= 1)
> > +                                     *rxp++ = rx >> 24;
> > +                             if (words >= 2)
> > +                                     *rxp++ = rx >> 16;
> > +                             if (words >= 3)
> > +                                     *rxp++ = rx >> 8;
> > +                             if (words >= 4)
> > +                                     *rxp++ = rx;
> > +                             xfer_len = (words >= 4) ? 4 : words;
> > +                     } else if (spi_slave->flags & SPI_XFER_DUAL_READ) {
> > +                             if (words >= 1)
> > +                                     *rxp++ = rx >> 8;
> > +                             if (words >= 2)
> > +                                     *rxp++ = rx;
> > +                             xfer_len = (words >= 2) ? 2 : words;
> > +                     } else {
> > +                             *rxp++ = rx;
> > +                             xfer_len = 1;
> > +                     }
> > +                     debug("rx done, status %08x, rx=%08x\n", status, rx);
> >               }
> >               words -= xfer_len;
> >       }
> > @@ -353,6 +381,23 @@ static int ti_qspi_exec_mem_op(struct spi_slave *slave,
> >       return ret;
> >  }
> >
> > +static int ti_qspi_adjust_size(struct spi_slave *slave, struct spi_mem_op *op)
> > +{
> > +#ifdef CONFIG_TI_QSPI_MAX_FRAME_SIZE
>
> Try to avoid conditional compilation. Using `if (CONFIG_IS_ENABLED(TI_QSPI_MQX_FRAME_SIZE))`
> should work just as fine I think.

Ok. If the frame size is fixed to 4096, there is no need for the conditional.

>
> > +     // Adjust the data length to fit in the QSPI max frame length
> > +     if (op->data.dir == SPI_MEM_DATA_IN) {
> > +             unsigned int data_len = CONFIG_TI_QSPI_MAX_FRAME_SIZE -
> > +                                                             sizeof(op->cmd.opcode) - op->addr.nbytes -
> > +                                                             op->dummy.nbytes;
> > +             // Align the data on cache line
> > +             data_len = data_len & ~(ARCH_DMA_MINALIGN - 1);
> > +             op->data.nbytes = min(op->data.nbytes, data_len);
> > +     }
> > +#endif
> > +
> > +     return 0;
> > +}
> > +
> >  static int ti_qspi_claim_bus(struct udevice *dev)
> >  {
> >       struct dm_spi_slave_platdata *slave_plat = dev_get_parent_platdata(dev);
> > @@ -482,6 +527,7 @@ static int ti_qspi_ofdata_to_platdata(struct udevice *bus)
> >
> >  static const struct spi_controller_mem_ops ti_qspi_mem_ops = {
> >       .exec_op = ti_qspi_exec_mem_op,
> > +     .adjust_op_size = ti_qspi_adjust_size,
> >  };
> >
> >  static const struct dm_spi_ops ti_qspi_ops = {
> > --
> > 2.26.2
> >
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments India

Thanks!

BR,
Jean

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

end of thread, other threads:[~2020-12-01 19:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-29 10:39 [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Jean Pihet
2020-11-29 10:39 ` [PATCH 2/3] TI QSPI: add support for dual and quad-bit I/O read Jean Pihet
2020-11-30 13:09   ` Pratyush Yadav
2020-12-01 19:06     ` Jean Pihet
2020-11-29 10:39 ` [PATCH 3/3] TI QSPI: prevent a 4-byte write to the flash on read access Jean Pihet
2020-11-30 12:37 ` [PATCH 1/3] mtd: spi nor: add support for dual and quad bit transfers Pratyush Yadav
2020-12-01 19:03   ` Jean Pihet

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.