| Message ID | 20211217161654.367782-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | External ECC engines & Macronix support | expand |
On 17/12/21 05:16PM, Miquel Raynal wrote: > It seems that the number of command bytes must be "2" only when the > command itself is sent in DTR mode. The current logic checks if the > number of command bytes is "2" when any of the cycles is a DTR cycle. It > is likely that so far no device was actually mixing DTR/non-DTR cycles > in the same operation, explaining why this was left undetected until > now. This was intentional. spi_mem_dtr_supports_op() must only be called when the operation is DTR in all phases so I did not add any sanity checks if someone was using it for non-DTR ops. In fact, I added on to this function in [0] to check nbytes for other phases as well. The patch fell off my radar unfortunately, and it didn't get merged. I would like to keep this as it is since we have no user of mixed DTR/non-DTR modes yet. But if you really want to support it, please apply my patch first to make sure we check for every phase, not just command. [0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@ti.com/
Hi Pratyush, p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:09:19 +0530: > On 17/12/21 05:16PM, Miquel Raynal wrote: > > It seems that the number of command bytes must be "2" only when the > > command itself is sent in DTR mode. The current logic checks if the > > number of command bytes is "2" when any of the cycles is a DTR cycle. It > > is likely that so far no device was actually mixing DTR/non-DTR cycles > > in the same operation, explaining why this was left undetected until > > now. > > This was intentional. spi_mem_dtr_supports_op() must only be called when > the operation is DTR in all phases so I did not add any sanity checks if > someone was using it for non-DTR ops. Maybe that was the original intention but since then the Macronix driver has been merged and supports (at lest does not reject) these modes. > In fact, I added on to this > function in [0] to check nbytes for other phases as well. The patch fell > off my radar unfortunately, and it didn't get merged. > > I would like to keep this as it is since we have no user of mixed > DTR/non-DTR modes yet. I don't know if the Macronix driver really supports it or if it is the driver that is doing the wrong checks but in appearance such mixed mode could be used. > But if you really want to support it, please > apply my patch first to make sure we check for every phase, not just > command. > > [0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@ti.com/ Nice, I might take that as well indeed in order to make the checks a little bit more robust. Thanks, Miquèl
On 21/12/21 10:50AM, Miquel Raynal wrote: > Hi Pratyush, > > p.yadav@ti.com wrote on Tue, 21 Dec 2021 00:09:19 +0530: > > > On 17/12/21 05:16PM, Miquel Raynal wrote: > > > It seems that the number of command bytes must be "2" only when the > > > command itself is sent in DTR mode. The current logic checks if the > > > number of command bytes is "2" when any of the cycles is a DTR cycle. It > > > is likely that so far no device was actually mixing DTR/non-DTR cycles > > > in the same operation, explaining why this was left undetected until > > > now. > > > > This was intentional. spi_mem_dtr_supports_op() must only be called when > > the operation is DTR in all phases so I did not add any sanity checks if > > someone was using it for non-DTR ops. > > Maybe that was the original intention but since then the Macronix > driver has been merged and supports (at lest does not reject) these > modes. But I don't see any code that actually creates mixed DTR ops. SPI NOR does not do it for sure, and SPI NAND does not have DTR support yet. Those are the only two users of SPI MEM I can see. So we are solving at a problem that doesn't exist yet. Which is fine of course, I just want to point it out. > > > In fact, I added on to this > > function in [0] to check nbytes for other phases as well. The patch fell > > off my radar unfortunately, and it didn't get merged. > > > > I would like to keep this as it is since we have no user of mixed > > DTR/non-DTR modes yet. > > I don't know if the Macronix driver really supports it or if it is the > driver that is doing the wrong checks but in appearance such mixed mode > could be used. > > > But if you really want to support it, please > > apply my patch first to make sure we check for every phase, not just > > command. > > > > [0] https://lore.kernel.org/all/20210531181757.19458-5-p.yadav@ti.com/ > > Nice, I might take that as well indeed in order to make the checks a > little bit more robust. Thanks.
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 37f4443ce9a0..c4da0c9b05e9 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -163,7 +163,7 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem, bool spi_mem_dtr_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) { - if (op->cmd.nbytes != 2) + if (op->cmd.dtr && op->cmd.nbytes != 2) return false; return spi_mem_check_buswidth(mem, op);
It seems that the number of command bytes must be "2" only when the command itself is sent in DTR mode. The current logic checks if the number of command bytes is "2" when any of the cycles is a DTR cycle. It is likely that so far no device was actually mixing DTR/non-DTR cycles in the same operation, explaining why this was left undetected until now. Fixes: 539cf68cd51b ("spi: spi-mem: add spi_mem_dtr_supports_op()") Suggested-by: Boris Brezillon <boris.brezillon@collabora.com> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/spi/spi-mem.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)