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

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

[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.

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

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 drivers/mmc/core/sdio_io.c    |   31 +++++++++++++++++++++++++++++++
 include/linux/mmc/sdio_func.h |    3 +++
 2 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index ff27c8c..3f681be 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -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
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index c6c0cce..8974b19 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -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

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[Grazvydas Ignotas]

Hi Kalle,

On Wed, Apr 28, 2010 at 12:18 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> 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.

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

>
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
>  drivers/mmc/core/sdio_io.c    |   31 +++++++++++++++++++++++++++++++
>  include/linux/mmc/sdio_func.h |    3 +++
>  2 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index ff27c8c..3f681be 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -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
> diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
> index c6c0cce..8974b19 100644
> --- a/include/linux/mmc/sdio_func.h
> +++ b/include/linux/mmc/sdio_func.h
> @@ -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
>
>

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[Andrew Morton]

On Fri, 14 May 2010 14:04:17 +0300
Grazvydas Ignotas <notasas@gmail.com> wrote:

> Hi Kalle,
> 
> On Wed, Apr 28, 2010 at 12:18 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> > 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.
> 
> 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.

> 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 <notasas@gmail.com>
: 
: 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 <notasas@gmail.com>
: Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
: ---
: 
:  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,

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[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.

However, patch author for some reason is using WRITE operation instead of READ.
>>> 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.

Thanks,

Dmitry

On Fri, May 14, 2010 at 3:06 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Fri, 14 May 2010 14:04:17 +0300
> Grazvydas Ignotas <notasas@gmail.com> wrote:
>
>> Hi Kalle,
>>
>> On Wed, Apr 28, 2010 at 12:18 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>> > 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.
>>
>> 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.
>
>> 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 <notasas@gmail.com>
> :
> : 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 <notasas@gmail.com>
> : Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> : ---
> :
> :  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,
>
>
>

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[Grazvydas Ignotas]

On Sat, May 15, 2010 at 3:02 AM, Dmitry Shmidt <dimitrysh@google.com> wrote:
> 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.
>>>> 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?

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[Dmitry Shmidt]

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.

Thanks,

Dmitry

On Sat, May 15, 2010 at 5:16 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Sat, May 15, 2010 at 3:02 AM, Dmitry Shmidt <dimitrysh@google.com> wrote:
>> 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.
>>>>> 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?
>

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[Grazvydas Ignotas]

On Sat, May 15, 2010 at 7:25 PM, Dmitry Shmidt <dimitrysh@google.com> wrote:
> 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.

I found it a bit hard to find a fitting name that would be similar to
other SDIO function names so I'm open to suggestions. Few other names:

sdio_read_after_write_b
sdio_rawb

Maybe our kernel veteran Andrew can suggest us something? :-)

Re: [PATCH] sdio: add new function for RAW (Read after Write) operation

[Kalle Valo]

On 14 May 2010 14:04, Grazvydas Ignotas <notasas@gmail.com> wrote:
> Hi Kalle,

Hi Grazvydas,

> On Wed, Apr 28, 2010 at 12:18 AM, Grazvydas Ignotas <notasas@gmail.com> wrote:
>> 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.
>
> 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.

Yes, this is needed for wl1251 (and I would assume also for wl1271) to
wake up firmware from it's sleep state, also called as the ELP mode. A
TI engineer has confirmed this for me.

Kalle