Re: [PATCH v2] spi: spi-mem: ease checks in dtr_supports_op()
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
Re: [PATCH v2] spi: spi-mem: ease checks in dtr_supports_op()
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 --- 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-g...@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
Re: [PATCH v2] spi: spi-mem: ease checks in dtr_supports_op()
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 > --- > 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-g...@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
Re: [PATCH v2] spi: spi-mem: ease checks in dtr_supports_op()
Hi, A gentle reminder to review the following patch: spi: spi-mem: ease checks in dtr_supports_op() https://lore.kernel.org/u-boot/20221025062036.383460-1-d-g...@ti.com/ Kindly merge if it looks okay. On 25/10/22 11:50, 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 number of bytes data read in DTR Mode. Don't fail supports op for this 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. 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. 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 --- 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-g...@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) - return false; - return spi_mem_check_buswidth(slave, op); } EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op); -- Best regards, Dhruva Gole Texas Instruments Incorporated
[PATCH v2] spi: spi-mem: ease checks in dtr_supports_op()
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 number of bytes data read in DTR Mode. Don't fail supports op for this 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. 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. 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 --- 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-g...@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) - return false; - return spi_mem_check_buswidth(slave, op); } EXPORT_SYMBOL_GPL(spi_mem_dtr_supports_op); -- 2.25.1
