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

[Pratyush Yadav]

On 07/11/22 06:46PM, Gole, Dhruva wrote:
> 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.

Not always. On writes the extra byte goes to the flash. We do not want 
to write an undefined value there.

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

It usually does but I still think just saying "Linux does not do it" is 
not a strong enough 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.

Reading an extra byte is not so bad, but writing an extra byte is. That 
is where we need to be more careful.

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

I think it should be the opposite. SPI MEM should to sanity check the op 
and only pass an op to the controllers that is actually valid by the 
protocol. The controller can then do extra checks for things it 
specifically does not support.

If we do not do this then each controller driver would have to implement 
this same logic of rejecting these bogus ops and some are bound to get 
it wrong. And this is just the default supports_op hook, if your 
controller can roll its own if absolutely necessary. This is supposed to 
cover the general case.

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

If your controller can do weird stuff then it can roll its own 
supports_op hook. The default should serve the general case.

But I think we should actually talk about the problem you are trying to 
solve here. Do you want to allow userspace (or in U-Boot's case, the sf 
command) to write odd number of bytes? Then in that case I would suggest 
you look at this [0] series I sent some time ago for Linux. That teaches 
SPI NOR to pad reads and writes with extra bytes. Does it solve the 
problem you are looking to solve?

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

SPI MEM does not have enough information to make the right decision. For 
example, what value do you pad a write with? The answer can be different 
depending on if you are writing to a register or the main storage area. 
Do you read an extra byte at the start or at the end? The answer can 
depend on the flash size. Only the upper layer like SPI NOR can know 
this. So I think the logic to pad reads or writes should lie in upper 
layers.


[0] https://patchwork.ozlabs.org/project/linux-mtd/list/?series=246518&state=*

-- 
Regards,
Pratyush Yadav