[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers
This mirrors the conventions used in other SPI drivers (kirkwood, davinci, atmel, et al) where the din/dout buffer can be NULL when the received/transmitted data isn't important. This reduces the need for allocating additional buffers when write-only/read-only functionality is needed.
In the din == NULL case, the received data is simply not stored. In the dout == NULL case, zeroes are transmitted.
Signed-off-by: Andrew Ruder andrew.ruder@elecsyscorp.com Cc: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com Cc: Jagan Teki jagannadh.teki@gmail.com --- So going through and cleaning up some of my trees and determining which patches have not been taken. A little more justification for this patch - I've gone through all of the current SPI implementations and have determined what (if any) support the driver has for half duplex operation by giving NULL for one of the two buffers (din/dout). This greatly reduces the need for temporary buffers for writing or reading large amounts of half-duplex data to devices over SPI. Here's the list:
altera_spi.c - SUPPORTS NULL din or dout andes_spi.c - REQUIRES NULL din or dout armada100_spi.c - SUPPORTS NULL din or dout atmel_spi.c - SUPPORTS NULL din or dout bfin_spi6xx.c - SUPPORTS NULL din or dout bfin_spi.c - SUPPORTS NULL din or dout cf_qspi.c - SUPPORTS NULL din or dout cf_spi.c - SUPPORTS NULL din or dout davinci_spi.c - SUPPORTS NULL din or dout exynos_spi.c - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sf - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sl - SUPPORTS NULL din or dout fdt_spi.c-tegra114 - SUPPORTS NULL din or dout fsl_espi.c - SUPPORTS NULL din (NOT dout) ftssp010_spi.c - SUPPORTS NULL din or dout ich.c - SUPPORTS NULL din or dout kirkwood_spi.c - SUPPORTS NULL din or dout mpc52xx_spi.c - NO SUPPORT mpc8xxx_spi.c - NO SUPPORT mxc_spi.c - NO SUPPORT mxs_spi.c - REQUIRES NULL din or dout oc_tiny_spi.c - SUPPORTS NULL din or dout omap3_spi.c - SUPPORTS NULL din or dout sandbox_spi.c - SUPPORTS NULL din or dout sh_qspi.c - SUPPORTS NULL din or dout sh_spi.c - SUPPORTS NULL din or dout soft_spi.c - NO SUPPORT ti_qspi.c - SUPPORTS NULL din or dout xilinx_spi.c - SUPPORTS NULL din or dout zynq_spi.c - SUPPORTS NULL din or dout
Furthermore, this would not affect any board already using temporary buffers and it would continue to work as usual. The *only* obscure corner case that this will not work is if NULL (0x0) is a valid buffer address in your system and you are transferring SPI to/from it. This seems very unlikely and would probably break in other ways from library functions that check their arguments.
drivers/spi/soft_spi.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/soft_spi.c b/drivers/spi/soft_spi.c index 5d22351..5fdd091 100644 --- a/drivers/spi/soft_spi.c +++ b/drivers/spi/soft_spi.c @@ -137,9 +137,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, * Check if it is time to work on a new byte. */ if((j % 8) == 0) { - tmpdout = *txd++; + if (txd) { + tmpdout = *txd++; + } else { + tmpdout = 0; + } if(j != 0) { - *rxd++ = tmpdin; + if (rxd) { + *rxd++ = tmpdin; + } } tmpdin = 0; } @@ -164,9 +170,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, * bits over to left-justify them. Then store the last byte * read in. */ - if((bitlen % 8) != 0) - tmpdin <<= 8 - (bitlen % 8); - *rxd++ = tmpdin; + if (rxd) { + if((bitlen % 8) != 0) + tmpdin <<= 8 - (bitlen % 8); + *rxd++ = tmpdin; + }
if (flags & SPI_XFER_END) spi_cs_deactivate(slave);
I further justify this with a list of drivers that depend on half-duplex SPI operation:
drivers/mmc/mmc_spi.c: spi_xfer(spi, 2 * 8, tok, NULL, 0); drivers/mtd/spi/eeprom_m95xxx.c: if (spi_xfer(slave, 8, buf, NULL, SPI_XFER_BEGIN | SPI_XFER_END)) drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0); drivers/net/e1000_spi.c: return e1000_spi_xfer(hw, 8*sizeof(op), op, NULL, intr); drivers/net/enc28j60.c: spi_xfer(enc->slave, 2 * 8, dout, NULL, drivers/rtc/m41t94.c: ret = spi_xfer(slave, 64, buf, NULL, SPI_XFER_BEGIN | SPI_XFER_END);
And an old email first highlighting this issue in 2010 with the soft_spi driver:
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/75839/match=soft_spi
On Thu, Apr 10, 2014 at 9:49 PM, Andrew Ruder andrew.ruder@elecsyscorp.com wrote:
This mirrors the conventions used in other SPI drivers (kirkwood, davinci, atmel, et al) where the din/dout buffer can be NULL when the received/transmitted data isn't important. This reduces the need for allocating additional buffers when write-only/read-only functionality is needed.
In the din == NULL case, the received data is simply not stored. In the dout == NULL case, zeroes are transmitted.
Signed-off-by: Andrew Ruder andrew.ruder@elecsyscorp.com Cc: Jean-Christophe PLAGNIOL-VILLARD plagnioj@jcrosoft.com Cc: Jagan Teki jagannadh.teki@gmail.com
So going through and cleaning up some of my trees and determining which patches have not been taken. A little more justification for this patch - I've gone through all of the current SPI implementations and have determined what (if any) support the driver has for half duplex operation by giving NULL for one of the two buffers (din/dout). This greatly reduces the need for temporary buffers for writing or reading large amounts of half-duplex data to devices over SPI. Here's the list:
altera_spi.c - SUPPORTS NULL din or dout andes_spi.c - REQUIRES NULL din or dout armada100_spi.c - SUPPORTS NULL din or dout atmel_spi.c - SUPPORTS NULL din or dout bfin_spi6xx.c - SUPPORTS NULL din or dout bfin_spi.c - SUPPORTS NULL din or dout cf_qspi.c - SUPPORTS NULL din or dout cf_spi.c - SUPPORTS NULL din or dout davinci_spi.c - SUPPORTS NULL din or dout exynos_spi.c - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sf - SUPPORTS NULL din or dout fdt_spi.c-tegra20_sl - SUPPORTS NULL din or dout fdt_spi.c-tegra114 - SUPPORTS NULL din or dout fsl_espi.c - SUPPORTS NULL din (NOT dout) ftssp010_spi.c - SUPPORTS NULL din or dout ich.c - SUPPORTS NULL din or dout kirkwood_spi.c - SUPPORTS NULL din or dout mpc52xx_spi.c - NO SUPPORT mpc8xxx_spi.c - NO SUPPORT mxc_spi.c - NO SUPPORT mxs_spi.c - REQUIRES NULL din or dout oc_tiny_spi.c - SUPPORTS NULL din or dout omap3_spi.c - SUPPORTS NULL din or dout sandbox_spi.c - SUPPORTS NULL din or dout sh_qspi.c - SUPPORTS NULL din or dout sh_spi.c - SUPPORTS NULL din or dout soft_spi.c - NO SUPPORT ti_qspi.c - SUPPORTS NULL din or dout xilinx_spi.c - SUPPORTS NULL din or dout zynq_spi.c - SUPPORTS NULL din or dout
Furthermore, this would not affect any board already using temporary buffers and it would continue to work as usual. The *only* obscure corner case that this will not work is if NULL (0x0) is a valid buffer address in your system and you are transferring SPI to/from it. This seems very unlikely and would probably break in other ways from library functions that check their arguments.
At-least from zynq_spi.c case - I wouldn't find that obscure case as you pointed and even with dout NULL which is status poll from (sf_ops.c).
Let me come back again and will show my results for zynq_spi.c
It would be great if you mentioned issue scenario for status poll case drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0);
drivers/spi/soft_spi.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/soft_spi.c b/drivers/spi/soft_spi.c index 5d22351..5fdd091 100644 --- a/drivers/spi/soft_spi.c +++ b/drivers/spi/soft_spi.c @@ -137,9 +137,15 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, * Check if it is time to work on a new byte. */ if((j % 8) == 0) {
tmpdout = *txd++;
if (txd) {tmpdout = *txd++;} else {tmpdout = 0;} if(j != 0) {
*rxd++ = tmpdin;
if (rxd) {*rxd++ = tmpdin;} } tmpdin = 0; }@@ -164,9 +170,11 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, * bits over to left-justify them. Then store the last byte * read in. */
if((bitlen % 8) != 0)tmpdin <<= 8 - (bitlen % 8);*rxd++ = tmpdin;
if (rxd) {if((bitlen % 8) != 0)tmpdin <<= 8 - (bitlen % 8);*rxd++ = tmpdin;} if (flags & SPI_XFER_END) spi_cs_deactivate(slave);-- 1.9.0.rc3.12.gbc97e2d
thanks!
On Fri, Apr 11, 2014 at 12:24:20AM +0530, Jagan Teki wrote:
At-least from zynq_spi.c case - I wouldn't find that obscure case as you pointed and even with dout NULL which is status poll from (sf_ops.c).
zynq_spi is correct, soft_spi is not.
zynq_spi.c: int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { const u8 *tx_buf = dout; u8 *rx_buf = din, buf; [...] if (tx_buf) buf = *tx_buf++; else buf = 0; [...] if (rx_buf) *rx_buf++ = buf; [...]
It would be great if you mentioned issue scenario for status poll case drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0);
That breaks on soft_spi, hence the patch.
Cheers, Andy
On Fri, Apr 11, 2014 at 12:27 AM, Andrew Ruder andy@aeruder.net wrote:
On Fri, Apr 11, 2014 at 12:24:20AM +0530, Jagan Teki wrote:
At-least from zynq_spi.c case - I wouldn't find that obscure case as you pointed and even with dout NULL which is status poll from (sf_ops.c).
zynq_spi is correct, soft_spi is not.
zynq_spi.c: int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { const u8 *tx_buf = dout; u8 *rx_buf = din, buf; [...] if (tx_buf) buf = *tx_buf++; else buf = 0; [...] if (rx_buf) *rx_buf++ = buf; [...]
It would be great if you mentioned issue scenario for status poll case drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0);
That breaks on soft_spi, hence the patch.
OK - means issue only with soft_spi.c is it? Can you share the issue log or typical use case scenario w.r.t soft_spi.c, I need to understand how this got resolved with your change.
I understand you assigned '0' when dout is NULL and you took the buf only when din is !NULL.
thanks!
On Fri, Apr 11, 2014 at 12:33:45AM +0530, Jagan Teki wrote:
It would be great if you mentioned issue scenario for status poll case drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0);
OK - means issue only with soft_spi.c is it?
Yes, and a couple other drivers.
Can you share the issue log or typical use case scenario w.r.t soft_spi.c, I need to understand how this got resolved with your change.
Yes, you actually posted one such case that will not work "correctly" with soft_spi. In the line of code you posted above, soft_spi will actually perform a read from address 0x0. In most cases, the read side isn't a huge deal, but on the write side it can cause all kinds of surprises.
I understand you assigned '0' when dout is NULL and you took the buf only when din is !NULL.
Yes, just handling NULL case to be how most drivers are handling it and how apparently most users of the spi modules (like mtd/spi/sf_ops) are clearly expecting it to work.
- Andy
On Fri, Apr 11, 2014 at 12:47 AM, Andrew Ruder andy@aeruder.net wrote:
On Fri, Apr 11, 2014 at 12:33:45AM +0530, Jagan Teki wrote:
It would be great if you mentioned issue scenario for status poll case drivers/mtd/spi/sf_ops.c: ret = spi_xfer(spi, 8, NULL, &status, 0);
OK - means issue only with soft_spi.c is it?
Yes, and a couple other drivers.
Can you share the issue log or typical use case scenario w.r.t soft_spi.c, I need to understand how this got resolved with your change.
Yes, you actually posted one such case that will not work "correctly" with soft_spi. In the line of code you posted above, soft_spi will actually perform a read from address 0x0. In most cases, the read side isn't a huge deal, but on the write side it can cause all kinds of surprises.
I understand you assigned '0' when dout is NULL and you took the buf only when din is !NULL.
Yes, just handling NULL case to be how most drivers are handling it and how apparently most users of the spi modules (like mtd/spi/sf_ops) are clearly expecting it to work.
I saw some check-patch issues, can you fix - plan to apply.
thanks!
participants (3)
-
Andrew Ruder -
Andrew Ruder -
Jagan Teki