[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Andrew Ruder]

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);
-- 
1.9.0.rc3.12.gbc97e2d

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Andrew Ruder]

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

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Jagan Teki]

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

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Andrew Ruder]

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

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Jagan Teki]

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

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Andrew Ruder]

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

[U-Boot] [PATCH RESEND-WITH-JUSTIFICATION] spi: soft_spi: Support NULL din/dout buffers

[Jagan Teki]

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