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

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