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

[Gole, Dhruva]

Hey Pratyush,

On 11/7/2022 4:16 AM, Pratyush Yadav wrote:
> 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
Yes, your assumption is correct. I am talking about the CQSPI Controller.
> 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.

I agree, it does not support odd number of byte read writes, what I meant
was it does handle it by as you said padding that extra byte at the end.
This extra byte can always be disregarded at a later stage.

>
> 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.

True, the protocol ideally can not support odd byte 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.

Okay, I was under the impression that U-Boot closely follows linux source and hence thought

this along with the other points that I am making to be a valid justification.

>
>> 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.

No, a flash does not support an odd bytes read (yet to my knowledge).
The controller while finally doing the transaction does infact issue
an even byte read.

> 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?

Again, how to handle an odd byte read should be taken care of by the
controller in my opinion and hence the check feels strict to me to
be in spi-mem. The controller can always read an extra byte (or fail
op if it doesn't).

>
>> 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 :-)
Noted,
>
> 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

In the end, what I feel is that this is a policy decision that which check should be where.

And it is my opinion that the controller drivers should be given the power to decide

what to do with the odd byte transactions (read extra byte or return ERRORs) whatever

they feel is suitable. I don't think spi-mem should care so much since it is still

at a pretty higher level.

Another idea: Don't fail for odd byte reads/writes (in DTR mode) but rather make the data len even

and then disregard the extra byte later on in spi-mem itself. Let me know if this

sounds better to you rather than simply returning false here.

-- 

Regards,
Dhruva Gole<d-gole at ti.com>