Re: [PATCH v8 02/28] spi: spi-mem: allow specifying a command's extension

On 02/04/21 06:29PM, Sean Anderson wrote:

On 4/1/21 3:31 PM, Pratyush Yadav wrote:

...

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

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

All usages of sizeof(op->cmd.opcode) also need to be changed to be op->cmd.nbytes because that is the actual indicator of opcode size.

Signed-off-by: Pratyush Yadav p.yadav@ti.com

drivers/spi/mtk_snfi_spi.c | 3 +-- drivers/spi/spi-mem-nodm.c | 4 ++-- drivers/spi/spi-mem.c | 13 +++++++------ include/spi-mem.h | 6 +++++- 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/mtk_snfi_spi.c b/drivers/spi/mtk_snfi_spi.c index b6ab5fa3ad..65d0ce0981 100644 --- a/drivers/spi/mtk_snfi_spi.c +++ b/drivers/spi/mtk_snfi_spi.c @@ -64,8 +64,7 @@ static int mtk_snfi_adjust_op_size(struct spi_slave *slave, * or the output+input data must not exceed the GPRAM size. */

• nbytes = sizeof(op->cmd.opcode) + op->addr.nbytes +

• op->dummy.nbytes;
  

• nbytes = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; if (nbytes + op->data.nbytes <= SNFI_GPRAM_SIZE) return 0;

diff --git a/drivers/spi/spi-mem-nodm.c b/drivers/spi/spi-mem-nodm.c index 765f05fe54..db54101383 100644 --- a/drivers/spi/spi-mem-nodm.c +++ b/drivers/spi/spi-mem-nodm.c @@ -27,7 +27,7 @@ int spi_mem_exec_op(struct spi_slave *slave, tx_buf = op->data.buf.out; }

• op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;

• op_len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; op_buf = calloc(1, op_len); ret = spi_claim_bus(slave);

@@ -89,7 +89,7 @@ int spi_mem_adjust_op_size(struct spi_slave *slave, { unsigned int len;

• len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;

• len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; if (slave->max_write_size && len > slave->max_write_size) return -EINVAL;

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 427f7c13c5..541cd0e5a7 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -167,6 +167,9 @@ bool spi_mem_default_supports_op(struct spi_slave *slave, if (op->cmd.dtr || op->addr.dtr || op->dummy.dtr || op->data.dtr) return false;

• if (op->cmd.nbytes != 1)

• return false;
  

• return true; } EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);

@@ -273,8 +276,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) } #ifndef __UBOOT__

• tmpbufsize = sizeof(op->cmd.opcode) + op->addr.nbytes +

•      op->dummy.nbytes;
  

• tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; /*
  • Allocate a buffer to transmit the CMD, ADDR cycles with kmalloc() so

@@ -289,7 +291,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) tmpbuf[0] = op->cmd.opcode; xfers[xferpos].tx_buf = tmpbuf;

• xfers[xferpos].len = sizeof(op->cmd.opcode);

• xfers[xferpos].len = op->cmd.nbytes; xfers[xferpos].tx_nbits = op->cmd.buswidth; spi_message_add_tail(&xfers[xferpos], &msg); xferpos++;

@@ -353,7 +355,7 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) tx_buf = op->data.buf.out; }

• op_len = sizeof(op->cmd.opcode) + op->addr.nbytes + op->dummy.nbytes;

• op_len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; /*
  • Avoid using malloc() here so that we can use this code in SPL where

@@ -442,8 +444,7 @@ int spi_mem_adjust_op_size(struct spi_slave *slave, struct spi_mem_op *op) if (!ops->mem_ops || !ops->mem_ops->exec_op) { unsigned int len;

• len = sizeof(op->cmd.opcode) + op->addr.nbytes +
  

• 	op->dummy.nbytes;
  

• len = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
  

  if (slave->max_write_size && len > slave->max_write_size) return -EINVAL;

diff --git a/include/spi-mem.h b/include/spi-mem.h index 9e6b044548..3e5b771045 100644 --- a/include/spi-mem.h +++ b/include/spi-mem.h @@ -17,6 +17,7 @@ struct udevice; { \ .buswidth = __buswidth, \ .opcode = __opcode, \

• .nbytes = 1,					\
  

  } #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth) \

@@ -69,6 +70,8 @@ enum spi_mem_data_dir { /**

• struct spi_mem_op - describes a SPI memory operation

• • @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is

• • sent MSB-first.
    

  • @cmd.buswidth: number of IO lines used to transmit the command
  • @cmd.opcode: operation opcode
  • @cmd.dtr: whether the command opcode should be sent in DTR mode or not

@@ -92,9 +95,10 @@ enum spi_mem_data_dir { */ struct spi_mem_op { struct {

• u8 nbytes;
  

  u8 buswidth;

• u8 opcode;
  

  u8 dtr : 1;

• u16 opcode;
  

Shouldn't the opcode go first for alignment?

Right. I just put all the 'u8's together. But this order is now in mainline Linux so I'm not sure if we should change that here. The spi-mem subsystem in U-Boot generally tries to mirror what Linux does.

--Sean

...

} cmd; struct {