[PATCH v2] spi: spi-mem: ease checks in dtr_supports_op()

[Pratyush Yadav]

Hi Dhruva :-)

On 25/10/22 11:50AM, Dhruva Gole wrote:
> Remove the extra conditions that cause some cases to fail prematurely
> like if the data number of bytes is odd. The controller can handle odd

First, some background on why I added this in the first place. In 
1S-1S-1S mode, it takes 8 cycles to send a byte. So regardless of how 
many bytes you transmit in a transaction, the number of cycles will 
always be a whole number (in fact, it will be a multiple of 8). 
Similarly, in 2S-2S-2S mode it takes 4 cycles, in 8S-8S-8S 1 cycle.

But in 8D-8D-8D mode, you transmit a byte in just half a cycle. So the 
question becomes whether you can ever transmit an odd number of bytes in 
8D-8D-8D mode. I have not seen any official document/spec explicitly say 
this, but to me having half a cycle in the data phase does not make 
sense. Your transaction should have a whole number of cycles, which 
means you will always transmit an even number of bytes.

This is why I added this check. Transactions of odd length do not make 
sense for 8D-8D-8D mode. I have looked at multiple 8D-8D-8D flashes, and 
all of them forbid 3 byte addressing and odd length data transfers in 8D 
mode.

> number of bytes data read in DTR Mode. Don't fail supports op for this

You should mention which controller. There is more than one out there. 
And not all of them work the same.

I assume you are talking about the Cadence QSPI controller, since that 
is what TI chips use. In that case, I have heard from HW folks that it 
does support odd number of bytes, but when I experimented with it a bit 
I found out that you can program it to write an odd number of bytes in 
Octal DTR mode, but it actually ends up writing an extra byte at the 
end. So it would seem like it does not _actually_ support odd number of 
bytes.

But that is beside the point anyway IMO. It does not matter much what 
one specific controller can do. We should stick to what the protocol 
allows, which does not include odd length transactions.

> condition. This change can also be justified by taking a look at the
> equivalent code in the linux kernel (v6.0.3), in drivers/spi/spi-mem.c,
> where such an even number of data bytes check is absent as well.

Sorry, that is not a good justification. Linux code is not the golden 
source of truth. It can be wrong too. In fact, I'd suggest you add this 
check there as well.

> The presence of this even byte check causes supports op failure even if
> the controller can indeed work in case of odd bytes data reads in
> DTR (Dual Transfer Rate) mode in xSPI.

As I mentioned above, that assertion from TI's HW folks seems incorrect 
to me. Plus, I am yet to see a flash that can do odd byte reads in Octal 
DTR mode. Both the flashes TI uses for their boards, Cypress S28 and 
Micron MT35, do not allow this. Do you have a new flash that does 
support this feature? If yes, can you please give a link to its 
datasheet?

> There have not been any sort of major bugs in the absence of this
> particular supports_op check, so it is safe to discard this
> check from here.
> 
> Signed-off-by: Dhruva Gole <d-gole at ti.com>
> ---
> v1 of the patch had a relatively shorter commit body that did not
> sufficiently describe and justify this patch. link to v1:
> https://lore.kernel.org/u-boot/20221020083424.86848-1-d-gole@ti.com/
> 
> v2 has just updated the commit body while preserving the code changes from
> earlier v1 patch. This tries to address the changes requested from Jagan Teki.
> 
>  drivers/spi/spi-mem.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
> index 8e8995fc537f..eecc13bec90d 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -181,10 +181,6 @@ bool spi_mem_dtr_supports_op(struct spi_slave *slave,
>  	if (op->dummy.nbytes && op->dummy.buswidth == 8 && op->dummy.nbytes % 2)
>  		return false;
>  
> -	if (op->data.dir != SPI_MEM_NO_DATA &&
> -	    op->dummy.buswidth == 8 && op->data.nbytes % 2)

Hmm, this should have been op->data.buswidth instead. That would be a 
welcome fix indeed :-)

You can also put my explanation for why this exists in a comment here so 
other people reading this code can also get to know this.

> -		return false;
> -
>  	return spi_mem_check_buswidth(slave, op);
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op);
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav