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@xxxxxxxxxxxxxxxxxxxx> wrote: > On Fri, 14 May 2010 14:04:17 +0300 > Grazvydas Ignotas <notasas@xxxxxxxxx> wrote: > >> Hi Kalle, >> >> On Wed, Apr 28, 2010 at 12:18 AM, Grazvydas Ignotas <notasas@xxxxxxxxx> 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@xxxxxxxxx> > : > : 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@xxxxxxxxx> > : Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> > : --- > : > : 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, > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
