Re: [U-Boot] [PATCH 1/4] spi: spi-mem: Add optional half-duplex SPI transfer mode

Hi Boris,

On 06.08.2018 21:53, Boris Brezillon wrote:

Hi Stefan,

On Mon, 6 Aug 2018 17:12:50 +0200 Stefan Roese sr@denx.de wrote:

...

Some SPI controller might now support full-duplex SPI transfers. This option can be enabled to use half-duplex operation mode for such SPI controllers.

Signed-off-by: Stefan Roese sr@denx.de Cc: Miquel Raynal miquel.raynal@bootlin.com Cc: Boris Brezillon boris.brezillon@bootlin.com Cc: Jagan Teki jagan@openedev.com

---

drivers/spi/Kconfig | 8 ++++++++ drivers/spi/spi-mem.c | 31 ++++++++++++++++++++++++++++--- 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 9fbd26740d..5bd8289284 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -25,6 +25,14 @@ config SPI_MEM This extension is meant to simplify interaction with SPI memories by providing an high-level interface to send memory-like commands.

+config SPI_MEM_HALF_DUPLEX

• bool "Use half-duplex SPI transfer"
• depends on SPI_MEM
• help

•  Some SPI controller might not support full-duplex SPI transfers.
  

•  This option can be enabled to use half-duplex operation mode for
  

•  such SPI controllers.
  

• config ALTERA_SPI bool "Altera SPI driver" help

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 07ce799170..617fbbfe09 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -202,6 +202,10 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) struct dm_spi_ops *ops = spi_get_ops(bus); unsigned int xfer_len, pos = 0; u8 *tx_buf, *rx_buf = NULL;

• unsigned int pos2;
• int tx_len;
• int rx_len;
• u32 flag; int ret; int i;

@@ -370,8 +374,29 @@ int spi_mem_exec_op(struct spi_slave *slave, const struct spi_mem_op *op) if (tx_data) memcpy(tx_buf + pos, op->data.buf.out, op->data.nbytes);

• ret = spi_xfer(slave, xfer_len * 8, tx_buf, rx_buf,

•        SPI_XFER_BEGIN | SPI_XFER_END);
  

• pos2 = pos;
• if (CONFIG_IS_ENABLED(SPI_MEM_HALF_DUPLEX)) {

• if (rx_data) {
  

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

• 		  op->addr.nbytes + op->dummy.nbytes);
  

• 	rx_len = op->data.nbytes;
  

• 	flag = SPI_XFER_BEGIN;
  

• 	pos2 = 0;
  

• } else {
  

• 	tx_len = xfer_len;
  

• 	rx_len = 0;
  

• 	flag = SPI_XFER_BEGIN | SPI_XFER_END;
  

• }
  

• ret = spi_xfer(slave, tx_len * 8, tx_buf, NULL, flag);
  

• if (rx_data) {
  

• 	ret = spi_xfer(slave, rx_len * 8, NULL,
  

• 		       rx_buf, SPI_XFER_END);
  

• }
  

Why not doing that all the time and split things in 3 transfers instead of 2:

1/ the opcode + address + dummy cycles 2/ the tx data 3/ the rx data (optional)

This way you should have to allocate the rx/tx buf and you'd get rid of the memcpy.

I also thought about this but was not "brave" enough to drop the current single xfer. If you and Miquel think this is good idea, then I will gladly work on such an implementation. Getting rid of this compile time option would be good.

Of course, that only works if all SPI controllers are capable of dealing with transfers that have only SPI_XFER_BEGIN or SPI_XFER_END set.

I'm pretty sure that we can assume this is the case. Otherwise such a driver should be easy to fix to support this type of xfer.

Thanks, Stefan