[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 d-gole@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) - return false; - return spi_mem_check_buswidth(slave, op); } EXPORT_SYMBOL_GPL(spi_mem_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-gole@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 d-gole@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)return false;- return spi_mem_check_buswidth(slave, op); } EXPORT_SYMBOL_GPL(spi_mem_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 d-gole@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
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 Goled-gole@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.
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 Goled-gole@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...
participants (3)
-
Dhruva Gole -
Gole, Dhruva
-
Pratyush Yadav