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

[Stefan Roese]

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 at 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 at denx.de>
>> Cc: Miquel Raynal <miquel.raynal at bootlin.com>
>> Cc: Boris Brezillon <boris.brezillon at bootlin.com>
>> Cc: Jagan Teki <jagan at 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