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

2014-04-14 Thread Jagan Teki 
On Fri, Apr 11, 2014 at 12:47 AM, Andrew Ruder a...@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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-04-10 Thread 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.ru...@elecsyscorp.com
Cc: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com
Cc: Jagan Teki jagannadh.t...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-04-10 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-04-10 Thread Jagan Teki 
On Thu, Apr 10, 2014 at 9:49 PM, Andrew Ruder
andrew.ru...@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.ru...@elecsyscorp.com
 Cc: Jean-Christophe PLAGNIOL-VILLARD plagn...@jcrosoft.com
 Cc: Jagan Teki jagannadh.t...@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.

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

2014-04-10 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2014-04-10 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot