sdio: add new function for RAW (Read after Write) operation

SDIO specification allows RAW (Read after Write) operation using IO_RW_DIRECT command (CMD52) by setting the RAW bit. This operation is similar to ordinary read/write commands, except that both write and read are performed using single command/response pair. The Linux SDIO layer already supports this internaly, only external function is missing for drivers to make use, which is added by this patch.

This type of command is required to implement proper power save mode support in wl1251 wifi driver.

Signed-off-by Grazvydas Ignotas <[email protected]>

---
 drivers/mmc/core/sdio_io.c    |   31 +++++++++++++++++++++++++++++++ include/linux/mmc/sdio_func.h |    3 +++

 2 files changed, 34 insertions(+), 0 deletions(-)

drivers/mmc/core/sdio_io.c +31 -0

index ff27c8c..3f681be 100644
@@ -406,6 +406,37 @@ void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)
  EXPORT_SYMBOL_GPL(sdio_writeb);
 
  /**
+ *     sdio_writeb_readb - write and read a byte from SDIO function
+ *     @func: SDIO function to access
+ *     @b: byte to write
+ *     @addr: address to write to
+ *     @err_ret: optional status value from transfer
+ *
+ *     Performs a RAW (Read after Write) operation as defined by SDIO spec -
+ *     single byte is written to address space of a given SDIO function and
+ *     response is read back from the same address, both using single request.
+ *     If there is a problem with the operation, 0xff is returned and
+ *     @err_ret will contain the error code.
+ */
+u8 sdio_writeb_readb(struct sdio_func *func, u8 b,
+       unsigned int addr, int *err_ret)
+{
+       int ret;
+       u8 val;
+
+       BUG_ON(!func);
+
+       ret = mmc_io_rw_direct(func->card, 1, func->num, addr, b, &val);
+       if (err_ret)
+               *err_ret = ret;
+       if (ret)
+               val = 0xff;
+
+       return val;
+}
+EXPORT_SYMBOL_GPL(sdio_writeb_readb);
+
+/**
  *     sdio_memcpy_fromio - read a chunk of memory from a SDIO function
  *     @func: SDIO function to access
  *     @dst: buffer to store the data

include/linux/mmc/sdio_func.h +3 -1

index c6c0cce..8974b19 100644
@@ -145,6 +145,9 @@ extern void sdio_writew(struct sdio_func *func, u16 b,
  extern void sdio_writel(struct sdio_func *func, u32 b,
        unsigned int addr, int *err_ret);
 
+extern u8 sdio_writeb_readb(struct sdio_func *func, u8 b,
+       unsigned int addr, int *err_ret);
+
  extern int sdio_memcpy_toio(struct sdio_func *func, unsigned int addr,
        void *src, int count);
  extern int sdio_writesb(struct sdio_func *func, unsigned int addr,
--

1.6.3.3

Hi Kalle,

Quoted 7 lines · Grazvydas Ignotas

SDIO specification allows RAW (Read after Write) operation using IO_RW_DIRECT command (CMD52) by setting the RAW bit. This operation is similar to ordinary read/write commands, except that both write and read are performed using single command/response pair. The Linux SDIO layer already supports this internaly, only external function is missing for drivers to make use, which is added by this patch.

Quoted 2 lines

This type of command is required to implement proper power save mode support in wl1251 wifi driver.

As wl1251 maintainer, can you confirm this is needed for wl1251 driver to function in SDIO mode? Perhaps this could help convince Andrew to merge this patch.

Without this the chip is having problems leaving ELP mode. Android has similar patch for G1 in it's tree for the same reason:

http://android.git.kernel.org/?p=kernel/common.git;a=commitdiff;h=74a47786f6ecbe6c1cf9fb15efe6a968451deb52

Quoted 2 lines

Quoted 2 lines

ret = mmc_io_rw_direct(func->card, 1, func->num, addr, b, &val); 1 means WRITE

Quoted 10 lines · On Fri, 14 May 2010 14:04:17 +0300 Grazvydas Ignotas <[email protected]> wrote:

Hi Kalle,

Quoted 7 lines · Grazvydas Ignotas

SDIO specification allows RAW (Read after Write) operation using IO_RW_DIRECT command (CMD52) by setting the RAW bit. This operation is similar to ordinary read/write commands, except that both write and read are performed using single command/response pair. The Linux SDIO layer already supports this internaly, only external function is missing for drivers to make use, which is added by this patch.

Quoted 6 lines

Quoted 2 lines

This type of command is required to implement proper power save mode support in wl1251 wifi driver.

As wl1251 maintainer, can you confirm this is needed for wl1251 driver to function in SDIO mode? Perhaps this could help convince Andrew to merge this patch.

I kinda ducked the patch because there are no callers of the function.

Quoted 5 lines

Without this the chip is having problems leaving ELP mode. Android has similar patch for G1 in it's tree for the same reason:

http://android.git.kernel.org/?p=kernel/common.git;a=commitdiff;h=74a47786f6ecbe6c1cf9fb15efe6a968451deb52

Well let's cc Dmitry then.  Dmitry's patch is somewhat different from yours and that needs to be looked at.

Some comments:

: From: Grazvydas Ignotas <[email protected]>
:
: SDIO specification allows RAW (Read after Write) operation using
: IO_RW_DIRECT command (CMD52) by setting the RAW bit.  This operation is
: similar to ordinary read/write commands, except that both write and read
: are performed using single command/response pair.  The Linux SDIO layer
: already supports this internaly, only external function is missing for
: drivers to make use, which is added by this patch.
:
: This type of command is required to implement proper power save mode
: support in wl1251 wifi driver.
:
: Signed-off-by: Grazvydas Ignotas <[email protected]>
: Signed-off-by: Andrew Morton <[email protected]>
: ---
:
:  drivers/mmc/core/sdio_io.c    |   31 +++++++++++++++++++++++++++++++
:  include/linux/mmc/sdio_func.h |    3 +++
:  2 files changed, 34 insertions(+)
:
: diff -puN drivers/mmc/core/sdio_io.c~sdio-add-new-function-for-raw-read-after-write-operation drivers/mmc/core/sdio_io.c : --- a/drivers/mmc/core/sdio_io.c~sdio-add-new-function-for-raw-read-after-write-operation
: +++ a/drivers/mmc/core/sdio_io.c
: @@ -406,6 +406,37 @@ void sdio_writeb(struct sdio_func *func,
:  EXPORT_SYMBOL_GPL(sdio_writeb);
:
:  /**
: + *   sdio_writeb_readb - write and read a byte from SDIO function
: + *   @func: SDIO function to access
: + *   @b: byte to write
: + *   @addr: address to write to
: + *   @err_ret: optional status value from transfer
: + *
: + *   Performs a RAW (Read after Write) operation as defined by SDIO spec -
: + *   single byte is written to address space of a given SDIO function and
: + *   response is read back from the same address, both using single request.
: + *   If there is a problem with the operation, 0xff is returned and
: + *   @err_ret will contain the error code.
: + */
: +u8 sdio_writeb_readb(struct sdio_func *func, u8 b,

"b" is a poor name.  Please choose something meaningful.

: +     unsigned int addr, int *err_ret)
: +{
: +     int ret;
: +     u8 val;
: +
: +     BUG_ON(!func);

This test doesn't gain us much.  If `func' is NULL, we'll reliably oops when dereferencing it, which gives the same info as the BUG_ON().

: +     ret = mmc_io_rw_direct(func->card, 1, func->num, addr, b, &val);
: +     if (err_ret)
: +             *err_ret = ret;
: +     if (ret)
: +             val = 0xff;
: +
: +     return val;
: +}
: +EXPORT_SYMBOL_GPL(sdio_writeb_readb);
: +
: +/**
:   *   sdio_memcpy_fromio - read a chunk of memory from a SDIO function
:   *   @func: SDIO function to access
:   *   @dst: buffer to store the data
: diff -puN include/linux/mmc/sdio_func.h~sdio-add-new-function-for-raw-read-after-write-operation include/linux/mmc/sdio_func.h : --- a/include/linux/mmc/sdio_func.h~sdio-add-new-function-for-raw-read-after-write-operation
: +++ a/include/linux/mmc/sdio_func.h
: @@ -145,6 +145,9 @@ extern void sdio_writew(struct sdio_func
:  extern void sdio_writel(struct sdio_func *func, u32 b,
:       unsigned int addr, int *err_ret);
:
: +extern u8 sdio_writeb_readb(struct sdio_func *func, u8 b,
: +     unsigned int addr, int *err_ret);
: +
:  extern int sdio_memcpy_toio(struct sdio_func *func, unsigned int addr,
:       void *src, int count);
:  extern int sdio_writesb(struct sdio_func *func, unsigned int addr,

Hi Andrew,

wl1251 has some HW problem for ELP register that demands to do something like "write during read" in order
not to loose bits from this register during simple read. Fortunately it was enough just to add low 8 bits previous register value to the cmd.args during read operation
and SD Controller will behave as needed. And it was tested on real wl1251 device that is used in G1 and MyTouch.

However, patch author for some reason is using WRITE operation instead of READ.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

ret = mmc_io_rw_direct(func->card, 1, func->num, addr, b, &val); 1 means WRITE

Maybe in case of wl1251 HW it is not important - to write during read, or to read during write, but the problem in wl1251 was during ELP register READ operation, so I do not understand why author used WRITE flag.

Quoted 10 lines · Dmitry Shmidt

Hi Andrew,

wl1251 has some HW problem for ELP register that demands to do something like "write during read" in order
not to loose bits from this register during simple read. Fortunately it was enough just to add low 8 bits previous register value to the cmd.args during read operation
and SD Controller will behave as needed. And it was tested on real wl1251 device that is used in G1 and MyTouch.

Quoted 8 lines

However, patch author for some reason is using WRITE operation instead of READ.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

ret = mmc_io_rw_direct(func->card, 1, func->num, addr, b, &val); 1 means WRITE

Maybe in case of wl1251 HW it is not important - to write during read, or to read during write, but the problem in wl1251 was during ELP register READ operation, so I do not understand why author used WRITE flag.

Well like you said, 'wl1251 [...] demands to do something like "write during read"', and SDIO spec provides exactly this functionality with the RAW flag (quoting SDIO spec):

"If this bit [RAW] is set to 1 and the R/W flag is set to 1, then the command shall read the value of the register after the write. This is useful to allow writing to a control register and reading the status at the same address."

If you pass WRITE flag and a pointer for read value to mmc_io_rw_direct(), it sets the R/W and RAW bits in the CMD52 request. According to my tests wl1251 actually ignores both R/W and RAW flags, but by the looks of it it is expecting the RAW request. With my patch we can add a function that both conforms the SDIO spec and suits wl1251 hardware.

Does that make sense to you?

Hi Grazvydas,

You are right.
mmc_io_rw_direct() implementation doesn't check R/W flag and returns data if "out" != NULL.
So if SDIO spec "demands" WRITE flag and wl1251 ignores it anyway, then we should use WRITE.
You function name is slightly confusing I would say, but I am fine with the patch.

Hi Kalle,

Quoted 7 lines

SDIO specification allows RAW (Read after Write) operation using IO_RW_DIRECT command (CMD52) by setting the RAW bit. This operation is similar to ordinary read/write commands, except that both write and read are performed using single command/response pair. The Linux SDIO layer already supports this internaly, only external function is missing for drivers to make use, which is added by this patch.

Quoted 2 lines

This type of command is required to implement proper power save mode support in wl1251 wifi driver.

As wl1251 maintainer, can you confirm this is needed for wl1251 driver to function in SDIO mode? Perhaps this could help convince Andrew to merge this patch. Without this the chip is having problems leaving ELP mode.