Re: [PATCH v3 5/9] spi: spi-mem: Allow masters to transfer dummy cycles directly by hardware

[Boris Brezillon]

On Sun, 13 Dec 2020 10:54:26 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

> On Sat, 12 Dec 2020 09:28:50 -0800
> Sowjanya Komatineni <skomatineni@xxxxxxxxxx> wrote:
> 
> > On 12/12/20 2:57 AM, Boris Brezillon wrote:  
> > > On Fri, 11 Dec 2020 13:15:59 -0800
> > > Sowjanya Komatineni <skomatineni@xxxxxxxxxx> wrote:
> > >    
> > >> This patch adds a flag SPI_MASTER_USES_HW_DUMMY_CYCLES for the controllers
> > >> that support transfer of dummy cycles by the hardware directly.    
> > > Hm, not sure this is a good idea. I mean, if we expect regular SPI
> > > devices to use this feature, then why not, but if it's just for
> > > spi-mem, I'd recommend implementing a driver-specific exec_op() instead
> > > of using the default one.    
> > 
> > dummy cycles programming is SPI device specific.
> > 
> > Transfer of dummy bytes by SW or HW controller can be depending on 
> > features supported by controller.
> > 
> > Adding controller driver specific exec_op() Just for skipping dummy 
> > bytes transfer will have so much of redundant code pretty much what all 
> > spi_mem_exec_op does.
> > 
> > So in v1, I handled this in controller driver by skipping SW transfer of 
> > dummy bytes during dummy phase and programming dummy cycles in 
> > controller register to allow HW to transfer.
> > 
> > Based on v1 feedback discussion, added this flag 
> > SPI_MASTER_USES_HW_DUMMY_CYCLES which can be used by controllers 
> > supporting HW dummy bytes transfer and updated spi_mem_exec_op to skip 
> > SW dummy bytes.
> > 
> > This helps other controllers supporting HW transfer of dummy bytes as 
> > well just to set the flag and use dummy cycles directly.  
> 
> Except saying a spi_message has X dummy cycle is not precise enough.
> Where are those dummy cycles in the transfer sequence? spi-mem has well
> defined sequencing (cmd[+addr][+dummy][+data]) so we know exacly where
> dummy cycles are, but trying to retro-fit the dummy-cycle concept in
> the generic spi_message is confusing IMHO. If want to avoid code
> duplication, I'm pretty sure the driver can be reworked so the
> spi_transfer/exec_op() path can share most of the logic (that probably
> implies declaring a tegra_qspi_op).

Something like that might also do the trick:

--->8---

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index ef53290b7d24..8b0476f37fbb 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -353,6 +353,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
                xfers[xferpos].tx_buf = tmpbuf + op->addr.nbytes + 1;
                xfers[xferpos].len = op->dummy.nbytes;
                xfers[xferpos].tx_nbits = op->dummy.buswidth;
+               xfers[xferpos].dummy_data = 1;
                spi_message_add_tail(&xfers[xferpos], &msg);
                xferpos++;
                totalxferlen += op->dummy.nbytes;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 99380c0825db..ecf7989318c5 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -807,6 +807,10 @@ extern void spi_res_release(struct spi_controller *ctlr,
  *      transfer. If 0 the default (from @spi_device) is used.
  * @bits_per_word: select a bits_per_word other than the device default
  *      for this transfer. If 0 the default (from @spi_device) is used.
+ * @dummy_data: set to 1 for a dummy transfer (a transfer whose data is
+ *      ignored). Controllers that are able to issue dummy cycles can ignore
+ *      tx_buf, for those that can't tx_buf will contain dummy bytes. The
+ *      number of  dummy cycles to issue is (len * tx_bits) / 8.
  * @cs_change: affects chipselect after this transfer completes
  * @cs_change_delay: delay between cs deassert and assert when
  *      @cs_change is set and @spi_transfer is not the last in @spi_message
@@ -919,6 +923,7 @@ struct spi_transfer {
        struct sg_table tx_sg;
        struct sg_table rx_sg;
 
+       unsigned        dummy_data:1;
        unsigned        cs_change:1;
        unsigned        tx_nbits:3;
        unsigned        rx_nbits:3;