sdio: enhance IO_RW_EXTENDED support
These three patches enhance the support for the SDIO IO_RW_EXTENDED command. The block size of functions is managed and the I/O ops (sdio_readsb() etc) are extended to handle arbitrary lengths of data (by using multiple commands).
I've not yet had a chance to test this stuff as I don't (yet) have the time to write a Bluetooth Type-A driver so these are posted as an example of the sort of API I'd expect.
David
Signature · 2 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ
sdio: set the functions' block size
Prior to calling the driver's probe(), set the functions' block size to the largest that's supported by both the card and the driver.
---
commit 6d367fd822cbb2b8089ab7ef83f706f1984ab25b
tree 8c9cc84b4c8c1c8f959abe540aa02f14aa95c51d
parent 51755c3d59be1ba778bef45888f9f5e341dc4af4
author David Vrabel <[email protected]> Mon, 30 Jul 2007 19:58:54 +0100 committer David Vrabel <[email protected]> Mon, 30 Jul 2007 19:58:54 +0100
drivers/mmc/core/sdio_bus.c | 13 +++++++++++++ drivers/mmc/core/sdio_cis.c | 8 ++++---- drivers/mmc/core/sdio_io.c | 23 +++++++++++++++++++++++ include/linux/mmc/sdio_func.h | 7 +++++++ 4 files changed, 47 insertions(+), 4 deletions(-)
drivers/mmc/core/sdio_bus.c +13 -0
@@ -127,11 +127,24 @@ static int sdio_bus_probe(struct device *dev)struct sdio_driver *drv =to_sdio_driver (dev->driver);struct sdio_func *func =dev_to_sdio_func (dev);const struct sdio_device_id *id; +unsigned short block_size; +int ret; id =sdio_match_device (func, drv);if (!id)return -ENODEV ; +/* +* Set the function's block size to the largest supported by +* both the function and the driver. +*/ + block_size =min (drv->max_block_size, func->max_block_size); +sdio_claim_host (func); + ret =sdio_set_block_size (func, block_size); +sdio_release_host (func); +if (ret) +return ret; +return drv->probe (func, id); }
drivers/mmc/core/sdio_cis.c +4 -4
@@ -93,9 +93,9 @@ static int cistpl_funce(struct sdio_func *func, unsigned fn,if (size <0x04 || buf[0 ] !=0 ) goto bad;/* TPLFE_FN0_BLK_SIZE */ - val = buf[1 ] | (buf[2 ] <<8 ); + func->max_block_size = buf[1 ] | (buf[2 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_FN0_BLK_SIZE = %u\n" , -sdio_func_id (func), val); +sdio_func_id (func), func->max_block_size);/* TPLFE_MAX_TRAN_SPEED */ val = speed_val[(buf[3 ] >>3 ) &15 ] * speed_unit[buf[3 ] &7 ]; printk(KERN_DEBUG "%s: max speed = %u kbps\n",
@@ -126,9 +126,9 @@ static int cistpl_funce(struct sdio_func *func, unsigned fn,printk (KERN_DEBUG "%s: TPLFE_CSA_PROPERTY = 0x%02x\n" ,sdio_func_id (func), val);/* TPLFE_MAX_BLK_SIZE */ - val = buf[12 ] | (buf[13 ] <<8 ); + func->max_block_size = buf[12 ] | (buf[13 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_MAX_BLK_SIZE = %u\n" , -sdio_func_id (func), val); +sdio_func_id (func), func->max_block_size);/* TPLFE_OCR */ val = buf[14 ] | (buf[15 ] <<8 ) | (buf[16 ] <<16 ) | (buf[17 ] <<24 ); printk(KERN_DEBUG "%s: TPLFE_OCR = 0x%08x\n",
drivers/mmc/core/sdio_io.c +23 -0
@@ -148,6 +148,29 @@ err:EXPORT_SYMBOL_GPL (sdio_disable_func);/** +* sdio_set_block_size - set the block size of an SDIO function +* @func: SDIO function to change +* @blksz: new block size +*/ +int sdio_set_block_size (struct sdio_func *func,unsigned short blksz) +{ +int ret; + + ret =mmc_io_rw_direct (func->card,1 ,0 ,SDIO_FBR_BLKSIZE (func->num), + blksz &0xff ,NULL ); +if (ret) +return ret; + ret =mmc_io_rw_direct (func->card,1 ,0 ,SDIO_FBR_BLKSIZE (func->num) +1 , + (blksz >>8 ) &0xff ,NULL ); +if (ret) +return ret; + func->block_size = blksz; +return 0 ; +} + +EXPORT_SYMBOL_GPL (sdio_set_block_size); + +/** * sdio_readb - read a single byte from a SDIO function * @func: SDIO function to access * @addr: address to read
include/linux/mmc/sdio_func.h +7 -0
@@ -43,6 +43,9 @@ struct sdio_func {unsigned short vendor;/* vendor id */ unsigned short device;/* device id */ +unsigned short max_block_size;/* max block size supported */ +unsigned short block_size;/* current block size */ +unsigned int state;/* function state */ #define SDIO_STATE_PRESENT (1<<0)/* present in sysfs */
@@ -68,6 +71,8 @@ struct sdio_driver {int (*probe)(struct sdio_func *,const struct sdio_device_id *);void (*remove)(struct sdio_func *); +unsigned short max_block_size;/* max block size supported by driver */ +struct device_driver drv; };
@@ -107,6 +112,8 @@ extern void sdio_release_host(struct sdio_func *func);extern int sdio_enable_func (struct sdio_func *func);extern int sdio_disable_func (struct sdio_func *func); +extern int sdio_set_block_size (struct sdio_func *func,unsigned short blksz); +extern int sdio_claim_irq (struct sdio_func *func,sdio_irq_handler_t *handler);extern int sdio_release_irq (struct sdio_func *func);
Re: sdio: set the functions' block size
Quoted 5 lines · On Tue, 31 Jul 2007 16:36:32 +0100 David Vrabel <[email protected]> wrote:
Prior to calling the driver's
probe(), set the functions' block size to the largest that's supported by both the card and the driver.Signed-off-by David Vrabel <[email protected]>
Why a maximum size for the driver?
Quoted 6 lines
drivers/mmc/core/sdio_cis.c +0
-0@@ -93,9 +93,9 @@ static int cistpl_funce(struct sdio_func *func, unsigned fn, if (size < 0x04 || buf[0] != 0)
drivers/mmc/core/sdio_cis.c static int cistpl_funce(struct sdio_func *func, · line 101
Quoted 10 lines
drivers/mmc/core/sdio_cis.c +2
-2goto bad;/* TPLFE_FN0_BLK_SIZE */ - val = buf[1 ] | (buf[2 ] <<8 ); + func->max_block_size = buf[1 ] | (buf[2 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_FN0_BLK_SIZE = %u\n" , -sdio_func_id (func), val); +sdio_func_id (func), func->max_block_size);/* TPLFE_MAX_TRAN_SPEED */ val = speed_val[(buf[3 ] >>3 ) &15 ] * speed_unit[buf[3] & 7]; printk(KERN_DEBUG "%s: max speed = %u kbps\n",
I guess you aren't working against the latest tree as this code is already there.
drivers/mmc/core/sdio_cis.c static int cistpl_funce(struct sdio_func *func, · line 134
Quoted 12 lines
drivers/mmc/core/sdio_cis.c +2
-2@@ -126,9 +126,9 @@ static int cistpl_funce(struct sdio_func *func, unsigned fn, printk(KERN_DEBUG "%s: TPLFE_CSA_PROPERTY = 0x%02x\n", sdio_func_id(func), val);/* TPLFE_MAX_BLK_SIZE */ - val = buf[12 ] | (buf[13 ] <<8 ); + func->max_block_size = buf[12 ] | (buf[13 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_MAX_BLK_SIZE = %u\n" , -sdio_func_id (func), val); +sdio_func_id (func), func->max_block_size);/* TPLFE_OCR */ val = buf[14 ] | (buf[15 ] <<8 ) | (buf[16 ] <<16 ) | (buf[17] << 24); printk(KERN_DEBUG "%s: TPLFE_OCR = 0x%08x\n",
Ditto.
Quoted 24 lines
drivers/mmc/core/sdio_io.c +1
-0@@ -148,6 +148,29 @@ err:EXPORT_SYMBOL_GPL (sdio_disable_func);/** +* sdio_set_block_size - set the block size of an SDIO function
+ * @func: SDIO function to change
+ * @blksz: new block size
+ */drivers/mmc/core/sdio_io.c +5
-0+int sdio_set_block_size (struct sdio_func *func,unsigned short blksz) +{ +int ret; + + ret = mmc_io_rw_direct(func->card, 1, 0,
SDIO_FBR_BLKSIZE(func->num),drivers/mmc/core/sdio_io.c +4
-0+ blksz & 0xff, NULL); +if (ret) +return ret; + ret = mmc_io_rw_direct(func->card, 1, 0,
SDIO_FBR_BLKSIZE(func->num)+ 1,
drivers/mmc/core/sdio_io.c err: · line 172
Quoted 9 lines
drivers/mmc/core/sdio_io.c +9
-0+ (blksz >> 8) & 0xff, NULL); +if (ret) +return ret; + func->block_size = blksz; +return 0 ; +} + +EXPORT_SYMBOL_GPL (sdio_set_block_size); +
You also need to check that the host supports that block size.
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: sdio: set the functions' block size
Quoted 10 lines · Pierre Ossman
Quoted 5 lines · On Tue, 31 Jul 2007 16:36:32 +0100 David Vrabel <[email protected]> wrote:
Prior to calling the driver's
probe(), set the functions' block size to the largest that's supported by both the card and the driver.Signed-off-by David Vrabel <[email protected]>Why a maximum size for the driver?
For efficient use of the bus it is useful to pad transfers to a multiple of the block size. I reckoned it was potentially beneficial for drivers to use smaller blocks so less padding was required. However, after looking at some figures the per block overhead (28 clocks for a write) really does limit the transfer sizes where small block sizes is beneficial.
I'll remove this. Drivers can still override the block size by calling sdio_set_block_size() directly.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: extend sdio_readsb() and friends to handle any length of buffer
In reply to David VrabelExtend sdio_readsb(), sdio_writesb(), sdio_memcpy_fromio(), and sdio_memcpy_toio() to handle any length of buffer by splitting the transfer into several IO_RW_EXTENDED commands. Typically, a transfer would be split into a single block mode transfer followed by a byte mode transfer for the remainder.
For this to work the block size must be limited to the maximum size of a byte mode transfer (512 bytes). This limitation could be revisited if there are any cards out there that require a block size > 512.
---
commit 1c701a6566f373ede250b51c99216227fd881535
tree 2bbc5005de65bbdc497c8603e8a68486465065dc
parent 6d367fd822cbb2b8089ab7ef83f706f1984ab25b
author David Vrabel <[email protected]> Tue, 31 Jul 2007 11:08:49 +0100 committer David Vrabel <[email protected]> Tue, 31 Jul 2007 11:44:44 +0100
drivers/mmc/core/sdio_io.c | 63 ++++++++++++++++++++++++++++++++++--------- drivers/mmc/core/sdio_ops.c | 17 +++++++----- drivers/mmc/core/sdio_ops.h | 2 + 3 files changed, 61 insertions(+), 21 deletions(-)
drivers/mmc/core/sdio_io.c +50 -13
@@ -156,6 +156,13 @@ int sdio_set_block_size(struct sdio_func *func, unsigned short blksz) {int ret; +/* The standard permits block sizes up to 2048 bytes, but +* sdio_readsb() etc. can only work with block size <= 512. */ +if (blksz >512 ) { + blksz =512 ; +dev_warn (&func->dev,"block size limited to 512 bytes\n" ); + } + ret =mmc_io_rw_direct (func->card,1 ,0 ,SDIO_FBR_BLKSIZE (func->num), blksz &0xff ,NULL );if (ret)
@@ -228,6 +235,39 @@ void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr,EXPORT_SYMBOL_GPL (sdio_writeb); +/* Split an arbitrarily sized data transfer into several +* IO_RW_EXTENDED commands. */ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned fn,unsigned addr,int incr_addr,u8 *buf,unsigned size) +{ +unsigned remainder = size; +int ret; + +while (remainder > func->block_size) { +unsigned blocks; + + blocks = remainder % func->block_size; +if (blocks >511 ) + blocks =511 ; + size = blocks * func->block_size; + + ret =mmc_io_rw_extended (func->card, write, fn, addr, + incr_addr, buf, blocks, func->block_size); +if (ret) +return ret; + + remainder -= size; + buf += size; +if (incr_addr) + addr += size; + } + +if (remainder) + ret =mmc_io_rw_extended (func->card, write, fn, addr, + incr_addr, buf,1 , remainder); +return ret; +} +/** * sdio_memcpy_fromio - read a chunk of memory from a SDIO function * @func: SDIO function to access
@@ -235,14 +275,13 @@ EXPORT_SYMBOL_GPL(sdio_writeb);* @addr: address to begin reading from * @count: number of bytes to read * -* Reads up to 512 bytes from the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_fromio (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,0 , dst, +return sdio_io_rw_ext_helper (func,0 , func->num, addr,1 , dst, count); }
@@ -255,14 +294,13 @@ EXPORT_SYMBOL_GPL(sdio_memcpy_fromio);* @src: buffer that contains the data to write * @count: number of bytes to write * -* Writes up to 512 bytes to the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Writes to the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_toio (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,0 , src, +return sdio_io_rw_ext_helper (func,1 , func->num, addr,1 , src, count); }
@@ -273,14 +311,13 @@ int sdio_memcpy_toio(struct sdio_func *func, unsigned int addr,* @addr: address of (single byte) FIFO * @count: number of bytes to read * -* Reads up to 512 bytes from the specified FIFO of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the specified FIFO of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_readsb (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,1 , dst, +return sdio_io_rw_ext_helper (func,0 , func->num, addr,0 , dst, count); }
@@ -300,7 +337,7 @@ EXPORT_SYMBOL_GPL(sdio_readsb);int sdio_writesb (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,1 , src, +return sdio_io_rw_ext_helper (func,1 , func->num, addr,0 , src, count); }
drivers/mmc/core/sdio_ops.c +10 -7
@@ -92,7 +92,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn, }int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *buf,unsigned size) +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz) {struct mmc_request mrq;struct mmc_command cmd;
@@ -101,7 +101,6 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,BUG_ON (!card);BUG_ON (fn >7 ); -BUG_ON (size >512 );memset (&mrq,0 ,sizeof (struct mmc_request ));memset (&cmd,0 ,sizeof (struct mmc_command ));
@@ -113,18 +112,22 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, cmd.opcode =SD_IO_RW_EXTENDED ; cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ; + cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; +if (blocks >1 ) { + cmd.arg |=0x08000000 ; + cmd.arg |= blocks; + }else + cmd.arg |= (blksz ==512 ) ?0 : blksz; cmd.flags =MMC_RSP_R5 |MMC_CMD_ADTC ; - data.blksz = size; - data.blocks =1 ; + data.blksz = blksz; + data.blocks = blocks; data.flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; data.sg = &sg; data.sg_len =1 ; -sg_init_one (&sg, buf, size); +sg_init_one (&sg, buf, blksz * blocks);mmc_set_data_timeout (&data, card,0 );
drivers/mmc/core/sdio_ops.h +1 -1
@@ -16,7 +16,7 @@ int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);int mmc_io_rw_direct (struct mmc_card *card,int write,unsigned fn,unsigned addr,u8 in,u8 * out);int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *data,unsigned size); +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz);#endif
Re: sdio: extend sdio_readsb() and friends to handle any length of buffer
Quoted 6 lines · On Tue, 31 Jul 2007 16:36:33 +0100 David Vrabel <[email protected]> wrote:
Extend
sdio_readsb(),sdio_writesb(),sdio_memcpy_fromio(), andsdio_memcpy_toio()to handle any length of buffer by splitting the transfer into severalIO_RW_EXTENDEDcommands. Typically, a transfer would be split into a single block mode transfer followed by a byte mode transfer for the remainder.
Quoted 7 lines
For this to work the block size must be limited to the maximum size of a byte mode transfer (512 bytes). This limitation could be revisited if there are any cards out there that require a block size
Quoted 1 line
512.
Signed-off-by David Vrabel <[email protected]>
*snip*
Quoted 8 lines
drivers/mmc/core/sdio_io.c +4
-0@@ -228,6 +235,39 @@ void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr,EXPORT_SYMBOL_GPL (sdio_writeb ); +/* Split an arbitrarily sized data transfer into several +* IO_RW_EXTENDED commands. */ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, + unsigned fn, unsigned addr, int incr_addr, u8 *buf, unsigned
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, · line 251
Quoted 10 lines
drivers/mmc/core/sdio_io.c +9
-0size) +{ +unsigned remainder = size; +int ret; + +while (remainder > func->block_size) { +unsigned blocks; + + blocks = remainder % func->block_size; +if (blocks >511 ) + blocks =511 ;
You need to check how many blocks the host supports in one go. Also, the total size of the transfer might exceed the host's capabilities.
Quoted 5 lines
drivers/mmc/core/sdio_ops.c +0
-1@@ -113,18 +112,22 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, cmd.opcode = SD_IO_RW_EXTENDED; cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ;
drivers/mmc/core/sdio_ops.c int mmc_io_rw_extended(struct mmc_card *card, · line 123
Quoted 10 lines
drivers/mmc/core/sdio_ops.c +6
-1+ cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; +if (blocks >1 ) { + cmd.arg |=0x08000000 ; + cmd.arg |= blocks; + }else + cmd.arg |= (blksz ==512 ) ?0 : blksz; cmd.flags =MMC_RSP_R5 |MMC_CMD_ADTC ;
Until this function is made complete, I'd like some kind of test that blksz <= 512 when blocks == 1.
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
sdio: parameterize SDIO FBR register defines
In reply to David Vrabel---
commit 51755c3d59be1ba778bef45888f9f5e341dc4af4
tree c7bbb562b2d801197eefb619a17c94467c1299cd
parent 1cf0b6019aa3916197eecafe058bd2f3d700d24a
author David Vrabel <[email protected]> Mon, 30 Jul 2007 19:20:59 +0100 committer David Vrabel <[email protected]> Mon, 30 Jul 2007 19:28:30 +0100
drivers/mmc/core/sdio.c | 4 ++-- drivers/mmc/core/sdio_cis.c | 2 +- include/linux/mmc/sdio.h | 16 +++++++++------- 3 files changed, 12 insertions(+), 10 deletions(-)
drivers/mmc/core/sdio.c +2 -2
@@ -30,7 +30,7 @@ static int sdio_read_fbr(struct sdio_func *func)unsigned char data; ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF ,0 , &data); +SDIO_FBR_STD_IF (func->num),0 , &data);if (ret !=MMC_ERR_NONE ) goto out;
@@ -38,7 +38,7 @@ static int sdio_read_fbr(struct sdio_func *func)if (data ==0x0f ) { ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF_EXT ,0 , &data); +SDIO_FBR_STD_IF_EXT (func->num),0 , &data);if (ret !=MMC_ERR_NONE ) goto out; }
drivers/mmc/core/sdio_cis.c +1 -1
@@ -215,7 +215,7 @@ int sdio_read_cis(struct sdio_func *func, unsigned int fn)for (i =0 ; i <3 ; i++) {unsigned char x; ret =mmc_io_rw_direct (func->card,0 ,0 , - fn *0x100 +SDIO_FBR_CIS + i,0 , &x); +SDIO_FBR_CIS (fn) + i,0 , &x);if (ret !=MMC_ERR_NONE )return -EIO ; ptr |= x << (i *8 );
include/linux/mmc/sdio.h +9 -7
@@ -132,26 +132,28 @@* Function Basic Registers (FBR) */ -#define SDIO_FBR_STD_IF 0x00 +#define __SDIO_FBR (f, r) ((f) * 0x100 + (r)) + +#define SDIO_FBR_STD_IF (f) __SDIO_FBR((f), 0x00)#define SDIO_FBR_SUPPORTS_CSA 0x40/* supports Code Storage Area */ #define SDIO_FBR_ENABLE_CSA 0x80/* enable Code Storage Area */ -#define SDIO_FBR_STD_IF_EXT 0x01 +#define SDIO_FBR_STD_IF_EXT (f) __SDIO_FBR((f), 0x01) -#define SDIO_FBR_POWER 0x02 +#define SDIO_FBR_POWER (f) __SDIO_FBR((f), 0x02)#define SDIO_FBR_POWER_SPS 0x01/* Supports Power Selection */ #define SDIO_FBR_POWER_EPS 0x02/* Enable (low) Power Selection */ -#define SDIO_FBR_CIS 0x09/* CIS pointer (3 bytes) */ +#define SDIO_FBR_CIS (f) __SDIO_FBR((f), 0x09)/* CIS pointer (3 bytes) */ -#define SDIO_FBR_CSA 0x0C/* CSA pointer (3 bytes) */ +#define SDIO_FBR_CSA (f) __SDIO_FBR((f), 0x0C)/* CSA pointer (3 bytes) */ -#define SDIO_FBR_CSA_DATA 0x0F +#define SDIO_FBR_CSA_DATA (f) __SDIO_FBR((f), 0x0F) -#define SDIO_FBR_BLKSIZE 0x10/* block size (2 bytes) */ +#define SDIO_FBR_BLKSIZE (f) __SDIO_FBR((f), 0x10)/* block size (2 bytes) */ #endif
Re: sdio: parameterize SDIO FBR register defines
Quoted 20 lines · On Tue, 31 Jul 2007 16:36:31 +0100 David Vrabel <[email protected]> wrote:
Signed-off-by David Vrabel <[email protected]>---
commit51755c3d59be1ba778bef45888f9f5e341dc4af4
treec7bbb562b2d801197eefb619a17c94467c1299cd
parent1cf0b6019aa3916197eecafe058bd2f3d700d24a
author David Vrabel <[email protected]> Mon, 30 Jul 2007 19:20:59 +0100 committer David Vrabel <[email protected]> Mon, 30 Jul 2007 19:28:30 +0100drivers/mmc/core/sdio.c | 4 ++-- drivers/mmc/core/sdio_cis.c | 2 +- include/linux/mmc/sdio.h | 16 +++++++++------- 3 files changed, 12 insertions(+), 10 deletions(-)drivers/mmc/core/sdio.c +0
-0
drivers/mmc/core/sdio.c static int sdio_read_fbr(struct sdio_func *func) · line 36
Quoted 8 lines
drivers/mmc/core/sdio.c +1
-1@@ -30,7 +30,7 @@ static int sdio_read_fbr(struct sdio_func *func)unsigned char data; ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF ,0 , &data); +SDIO_FBR_STD_IF (func->num),0 , &data);if (ret !=MMC_ERR_NONE ) goto out;
I am a bit sceptical about these macros. Most i/o code in the kernel is in the form of "<base address> + <register offset>". For one thing, that model keeps the register defines clean and means people don't have to go reaching for specs all the time.
Would you be content with replacing "func->num * 0x100" with a macro so that the code becomes something like:
SDIO_FBR_BASE(func->num) + SDIO_FBR_STD_IF
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: sdio: parameterize SDIO FBR register defines
Quoted 25 lines · Pierre Ossman
Quoted 22 lines · On Tue, 31 Jul 2007 16:36:31 +0100 David Vrabel <[email protected]> wrote:
Signed-off-by David Vrabel <[email protected]>---
commit51755c3d59be1ba778bef45888f9f5e341dc4af4
treec7bbb562b2d801197eefb619a17c94467c1299cd
parent1cf0b6019aa3916197eecafe058bd2f3d700d24a
author David Vrabel <[email protected]> Mon, 30 Jul 2007 19:20:59 +0100 committer David Vrabel <[email protected]> Mon, 30 Jul 2007 19:28:30 +0100drivers/mmc/core/sdio.c | 4 ++-- drivers/mmc/core/sdio_cis.c | 2 +- include/linux/mmc/sdio.h | 16 +++++++++------- 3 files changed, 12 insertions(+), 10 deletions(-)drivers/mmc/core/sdio.c +0
-0@@ -30,7 +30,7 @@ static int sdio_read_fbr(struct sdio_func *func)unsigned char data;
Quoted 11 lines
drivers/mmc/core/sdio.c static int sdio_read_fbr(struct sdio_func *func) · line 36
Quoted 6 lines
drivers/mmc/core/sdio.c +1
-1ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF ,0 , &data); +SDIO_FBR_STD_IF (func->num),0 , &data);if (ret !=MMC_ERR_NONE ) goto out;I am a bit sceptical about these macros. Most i/o code in the kernel is in the form of "<base address> + <register offset>". For one thing, that model keeps the register defines clean and means people don't have to go reaching for specs all the time.
I really don't follow you objection to this. If one is maintaining the SDIO core then I would expect some familiarity with the spec and an understanding the FBRs are per-function but contained in the same CCCR/F0 register space.
Also, I would consider the start of the CCCR as the "base address".
Quoted 4 lines
Would you be content with replacing "
func->num*0x100" with a macro so that the code becomes something like:SDIO_FBR_BASE(func->num) + SDIO_FBR_STD_IF
I think this is less readable than SDIO_FBR_STD_IF(func->num).
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Re: sdio: parameterize SDIO FBR register defines
Quoted 6 lines · On Mon, 06 Aug 2007 11:14:03 +0100 David Vrabel <[email protected]> wrote:
I really don't follow you objection to this. If one is maintaining the SDIO core then I would expect some familiarity with the spec and an understanding the FBRs are per-function but contained in the same CCCR/F0 register space.
If we can reduce that barrier, then I think we should. People can't be expected to keep everything fresh in memory all the time. And we won't have a team dedicated to hacking this all the time.
Quoted 2 lines
Also, I would consider the start of the CCCR as the "base address".
In some sense, but there are also several identical FBR chunks on the card. So by most definitions of a base address, the start of each chunk would be it.
Quoted 7 lines
Quoted 4 lines
Would you be content with replacing "
func->num*0x100" with a macro so that the code becomes something like:SDIO_FBR_BASE(func->num) + SDIO_FBR_STD_IFI think this is less readable than
SDIO_FBR_STD_IF(func->num).
It's subjective. But the longer version is more understandable for someone who doesn't have the details of the SDIO protocol fresh in his mind.
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Quoted 9 lines · On Tue, 31 Jul 2007 16:36:30 +0100 David Vrabel <[email protected]> wrote:
These three patches enhance the support for the SDIO
IO_RW_EXTENDEDcommand. The block size of functions is managed and the I/O ops (sdio_readsb()etc) are extended to handle arbitrary lengths of data (by using multiple commands).I've not yet had a chance to test this stuff as I don't (yet) have the time to write a Bluetooth Type-A driver so these are posted as an example of the sort of API I'd expect.
Thanks. These are some nice improvements. I do have one suggestion though:
Could we design it so that sdio_io_rw_ext_helper() sets the block size itself? That way most drivers wouldn't have to care about that detail and the core would be free to choose optimal values.
I suspect that some transactions might require a certain block size. But we could satisfy that by stating that any transfer small enough to fit into one block will not be split up.
(PS. You might want to add [PATCH x/3] to your subjects so that the order of the patches is crystal clear)
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Quoted 13 lines · Pierre Ossman
Quoted 9 lines · On Tue, 31 Jul 2007 16:36:30 +0100 David Vrabel <[email protected]> wrote:
These three patches enhance the support for the SDIO
IO_RW_EXTENDEDcommand. The block size of functions is managed and the I/O ops (sdio_readsb()etc) are extended to handle arbitrary lengths of data (by using multiple commands).I've not yet had a chance to test this stuff as I don't (yet) have the time to write a Bluetooth Type-A driver so these are posted as an example of the sort of API I'd expect.
Quoted 6 lines
Thanks. These are some nice improvements. I do have one suggestion though:
Could we design it so that
sdio_io_rw_ext_helper()sets the block size itself? That way most drivers wouldn't have to care about that detail and the core would be free to choose optimal values.
I would expect the block size to be set once per card, and never be changed and thus it's not logically a per-transfer operation. We certainly wouldn't want to change the block size willy-nilly as it's an expensive operation.
The patch I've presented does put the selection of the block size in the core (bar one thing which I agree should be removed).
Quoted 3 lines
I suspect that some transactions might require a certain block size. But we could satisfy that by stating that any transfer small enough to fit into one block will not be split up.
I consider it unlikely that any card would want to do anything other than always use the largest possible block size.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Quoted 6 lines · On Mon, 06 Aug 2007 11:31:19 +0100 David Vrabel <[email protected]> wrote:
I would expect the block size to be set once per card, and never be changed and thus it's not logically a per-transfer operation. We certainly wouldn't want to change the block size willy-nilly as it's an expensive operation.
Indeed. It would of course be optimized so that it doesn't change the size needlessly.
The beauty is that drivers wouldn't have to care. Things just work<tm>. :)
Quoted 7 lines
Quoted 3 lines
I suspect that some transactions might require a certain block size. But we could satisfy that by stating that any transfer small enough to fit into one block will not be split up.
I consider it unlikely that any card would want to do anything other than always use the largest possible block size.
I have a counter example. I have here a Marvell wifi card which needs a firmware upload. And it seems to be rather picky about parameters during that upload.
I'm still experimenting with a clean way to do things for this card. I'll get back to you. :)
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Quoted 11 lines · Pierre Ossman
Quoted 5 lines · On Mon, 06 Aug 2007 11:31:19 +0100 David Vrabel <[email protected]> wrote:
I would expect the block size to be set once per card, and never be changed and thus it's not logically a per-transfer operation. We certainly wouldn't want to change the block size willy-nilly as it's an expensive operation.
Indeed. It would of course be optimized so that it doesn't change the size needlessly.
Drivers may care about the block size though so you can't have it changing behind their backs. e.g., they may need to pad data to a multiple of the block size.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
I suspect that some transactions might require a certain block size. But we could satisfy that by stating that any transfer small enough to fit into one block will not be split up.
I consider it unlikely that any card would want to do anything other than always use the largest possible block size.
Quoted 6 lines
I have a counter example. I have here a Marvell wifi card which needs a firmware upload. And it seems to be rather picky about parameters during that upload.
I'm still experimenting with a clean way to do things for this card. I'll get back to you. :)
sdio_set_block_size(func, 64); /* ew, this card is fussy */
download_firmware();
sdio_set_block_size(func, func->max_blksize); /* Ahhh, back to the card's default */
If a card is fussy about the block size I don't think there's anything cleaner than specifying directly in the driver.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Quoted 5 lines · On Mon, 06 Aug 2007 16:32:40 +0100 David Vrabel <[email protected]> wrote:
Drivers may care about the block size though so you can't have it changing behind their backs. e.g., they may need to pad data to a multiple of the block size.
Well, we have to assume that they aren't just padding to pass the time.
I can see a couple of reasons:
1. They are padding because it made their code easier by allowing just one transfer.
This is what I believe is the common case, and one that will go away if we allow the core free access to the block size. All the complexity is in the core and the drivers don't even have to bother with setting a block size.
2. They must write an exact number of bytes to the card before it activates.
As far as the core is concerned, padding and "real" data are the same thing, so this poses no restriction on how the core can fiddle with block sizes and transfers.
3. The entire transfer must be in one transaction.
Now this is a problem as the core might prefer to do two transfers (probably one block and one byte).
As I believe this will be a rare case, we should try to make sure we can handle this in a way that can keep less fussy transactions simple. So I propose changing your sdio_set_block_size() API to sdio_force_block_size().
That way the driver can lock the block size when it has particular needs, yet keep it under the (assumingly more optimal) control of the core at other times.
One could have a calling convention such as specifying a block size of zero to turn off the forced block size.
One could also use this as a less complex way of informing the drivers of host restrictions. If the block size specified isn't possible, we can return an error code stating such. Without that, every driver that needs this would need to duplicate code that computes possible block size and would make our life a pain when we want to add new host restrictions.
Quoted 8 lines
Quoted 5 lines
I have a counter example. I have here a Marvell wifi card which needs a firmware upload. And it seems to be rather picky about parameters during that upload.
sdio_set_block_size(func, 64); /* ew, this card is fussy */
Quoted 8 lines
download_firmware();
sdio_set_block_size(func, func->max_blksize); /* Ahhh, back to the card's default */If a card is fussy about the block size I don't think there's anything cleaner than specifying directly in the driver.
Well, the driver has to specify the information somehow. As to how, there are a number of options. My proposed sdio_force_block_size() will work here as well.
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
This is essentially what I mean.
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
commit 8f9fca61fbacd15d1be4215584ed00aa1119d87f
Author: David Vrabel <[email protected]> Date: Mon Aug 6 22:00:47 2007 +0200
sdio: extend sdio_readsb() and friends to handle any length of buffer
Extend sdio_readsb(), sdio_writesb(), sdio_memcpy_fromio(), and sdio_memcpy_toio() to handle any length of buffer by splitting the transfer into several IO_RW_EXTENDED commands. Typically, a transfer would be split into a single block mode transfer followed by a byte mode transfer for the remainder.
[ Automatic block size selection logic by Pierre Ossman ]
drivers/mmc/core/sdio_io.c +163 -16
@@ -95,6 +95,9 @@ int sdio_enable_func(struct sdio_func *func) goto err; } + func->cur_blksz =0 ; + func->forced_blksz =0 ; +pr_debug ("SDIO: Enabled device %s\n" ,sdio_func_id (func));return 0 ;
@@ -202,6 +205,129 @@ void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr,EXPORT_SYMBOL_GPL (sdio_writeb); +/* +* Internal function. Sets the block size of the card. +*/ +static int sdio_set_block_size (struct sdio_func *func,unsigned short blksz) +{ +int ret; + +if (blksz == func->cur_blksz) +return 0 ; + +/* +* We start by clearing the current block size as a failure +* will leave the size on the card in an unknown state. +*/ + func->cur_blksz =0 ; + + ret =mmc_io_rw_direct (func->card,1 ,0 , + func->num *0x100 +SDIO_FBR_BLKSIZE , + blksz &0xff ,NULL ); +if (ret) +return ret; + + ret =mmc_io_rw_direct (func->card,1 ,0 , + func->num *0x100 +SDIO_FBR_BLKSIZE +1 , + (blksz >>8 ) &0xff ,NULL ); +if (ret) +return ret; + + func->cur_blksz = blksz; + +return 0 ; +} + +/* +* Internal function. Splits an arbitrarily sized data transfer +* into several IO_RW_EXTENDED commands. +*/ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned addr,int incr_addr,u8 *buf,unsigned size) +{ +unsigned remainder = size; +int ret; +unsigned short blksz; +struct mmc_host *host = func->card->host; + +if (func->forced_blksz) + blksz = func->forced_blksz; +else { +if (size <=512 ) + goto byte_remainder; + + blksz = size; +if (size >0xffff ) + blksz =0xffff ; + +if (blksz > func->blksize) + blksz = func->blksize; +if (blksz > host->max_blk_size) + blksz = host->max_blk_size; + +/* avoid changing blksize needlessly */ +if (func->cur_blksz && ((blksz % func->cur_blksz) ==0 )) + blksz = func->cur_blksz; + } + + ret =sdio_set_block_size (func, blksz); +if (ret) +return ret; + +while (remainder >= blksz) { +unsigned blocks; + + blocks = remainder / blksz; + +if (blocks >512 ) + blocks =512 ; +if (blocks > host->max_blk_count) + blocks = host->max_blk_count; +if (blocks * blksz > host->max_req_size) + blocks = host->max_req_size / blksz; +if (blocks * blksz > host->max_seg_size) + blocks = host->max_seg_size / blksz; + + size = blocks * blksz; + + ret =mmc_io_rw_extended (func->card, write, func->num, + addr, incr_addr, buf, blocks, blksz); +if (ret) +return ret; + + remainder -= size; + buf += size; +if (incr_addr) + addr += size; + } + +byte_remainder: +while (remainder) { + size = remainder; + +if (size >512 ) + size =512 ; + +/* +* We don't check host requirements as a block of 512 +* bytes is the bare minimum support a host must +* provide. +*/ + + ret =mmc_io_rw_extended (func->card, write, func->num, + addr, incr_addr, buf,1 , size); +if (ret) +return ret; + + remainder -= size; + buf += size; +if (incr_addr) + addr += size; + } + +return 0 ; +} +/** * sdio_memcpy_fromio - read a chunk of memory from a SDIO function * @func: SDIO function to access
@@ -209,14 +335,13 @@ EXPORT_SYMBOL_GPL(sdio_writeb);* @addr: address to begin reading from * @count: number of bytes to read * -* Reads up to 512 bytes from the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_fromio (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,0 , dst, +return sdio_io_rw_ext_helper (func,0 , addr,1 , dst, count); }
@@ -229,14 +354,13 @@ EXPORT_SYMBOL_GPL(sdio_memcpy_fromio);* @src: buffer that contains the data to write * @count: number of bytes to write * -* Writes up to 512 bytes to the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Writes to the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_toio (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,0 , src, +return sdio_io_rw_ext_helper (func,1 , addr,1 , src, count); }
@@ -247,14 +371,13 @@ int sdio_memcpy_toio(struct sdio_func *func, unsigned int addr,* @addr: address of (single byte) FIFO * @count: number of bytes to read * -* Reads up to 512 bytes from the specified FIFO of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the specified FIFO of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_readsb (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,1 , dst, +return sdio_io_rw_ext_helper (func,0 , addr,0 , dst, count); }
@@ -267,14 +390,13 @@ EXPORT_SYMBOL_GPL(sdio_readsb);* @src: buffer that contains the data to write * @count: number of bytes to write * -* Writes up to 512 bytes to the specified FIFO of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Writes to the specified FIFO of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_writesb (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,1 , src, +return sdio_io_rw_ext_helper (func,1 , addr,0 , src, count); }
@@ -393,3 +515,28 @@ void sdio_writel(struct sdio_func *func, unsigned long b, unsigned int addr,EXPORT_SYMBOL_GPL (sdio_writel); +/** +* sdio_force_block_size - lock a certain block size +* @func: SDIO function to set block size for +* @size: block size in bytes, or 0 to remove any lock +* +* Locks the used block size for all multi-block sdio functions +* in order to satisfy transactions with precise requirements. +* This lock can be removed by specifying 0 (zero) as the block +* size. Returns failure if the selected block size isn't +* supported. +*/ +int sdio_force_block_size (struct sdio_func *func,unsigned short size) +{ +if (size > func->blksize) +return -EINVAL ; +if (size > func->card->host->max_blk_size) +return -EINVAL ; + + func->forced_blksz = size; + +return 0 ; +} + +EXPORT_SYMBOL_GPL (sdio_force_block_size); +
drivers/mmc/core/sdio_ops.c +14 -7
@@ -88,7 +88,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn, }int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *buf,unsigned size) +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz) {struct mmc_request mrq;struct mmc_command cmd;
@@ -97,7 +97,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,BUG_ON (!card);BUG_ON (fn >7 ); -BUG_ON (size >512 ); +BUG_ON (blocks >512 );memset (&mrq,0 ,sizeof (struct mmc_request ));memset (&cmd,0 ,sizeof (struct mmc_command ));
@@ -107,20 +107,27 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, mrq.data = &data; cmd.opcode =SD_IO_RW_EXTENDED ; + cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ; + cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; + +if ((blocks >1 ) || (blksz >512 )) { + cmd.arg |=0x08000000 ; + cmd.arg |= (blocks ==512 ) ?0 : blocks; + }else + cmd.arg |= (blksz ==512 ) ?0 : blksz; + cmd.flags =MMC_RSP_R5 |MMC_CMD_ADTC ; - data.blksz = size; - data.blocks =1 ; + data.blksz = blksz; + data.blocks = blocks; data.flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; data.sg = &sg; data.sg_len =1 ; -sg_init_one (&sg, buf, size); +sg_init_one (&sg, buf, blksz * blocks);mmc_set_data_timeout (&data, card,0 );
drivers/mmc/core/sdio_ops.h +1 -1
@@ -16,7 +16,7 @@ int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);int mmc_io_rw_direct (struct mmc_card *card,int write,unsigned fn,unsigned addr,u8 in,u8 * out);int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *data,unsigned size); +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz);#endif
include/linux/mmc/sdio_func.h +4 -0
@@ -44,6 +44,8 @@ struct sdio_func {unsigned short device;/* device id */ unsigned short blksize;/* maximum block size */ +unsigned short cur_blksz;/* current block size */ +unsigned short forced_blksz;/* driver forced block size */ unsigned int state;/* function state */ #define SDIO_STATE_PRESENT (1<<0)/* present in sysfs */
@@ -136,5 +138,7 @@ extern int sdio_memcpy_toio(struct sdio_func *func, unsigned int addr,extern int sdio_writesb (struct sdio_func *func,unsigned int addr,void *src,int count); +extern int sdio_force_block_size (struct sdio_func *func,unsigned short size); +#endif
Quoted 1 line · Pierre Ossman
This is essentially what I mean.
It does not behave very well, see the specific comments at the end.
When considering optimizing SDIO it is important to always keep in mind that a command is very expensive. They're some 100-150 clocks long and there's also overheads in interrupt handling/scheduling, this can add to up to 10s of us. Therefore, when selecting an optimum block size one should aim to reduce the number of commands before attempting to maximize the block size.
There are two cases to consider.
1. Card's max block_size <= 512.
In this case the optimum block size is /always/ the card's maximum. This means we can do all transfers in two commands (and if the driver is aware of the block size it may choose to pad transfers so only one command is done).
2. Card's max block size > 512.
We can't handle arbitrary sized transfers with a block size > 512 (since 512 is maximum length of a byte mode transfer), so if we wish to use a block size of 1024 (say) some transfers will require three commands (a block mode, and two byte mode transfers). For a block size of 2048 (the maximum possible) some transfer may require 5 commands!
We could attempt to set the block size as follows:
transfer size % 1024 == 0 => block size = 1024
transfer size % 2048 == 0 => block size = 2048
otherwise => block size = 512
However, without knowing what sizes of transfers the driver is going to use we cannot intelligently know when it's best to switch the block size. A block size change is expensive (two commands).
The change in block size from 512 to 2048 improves performance (for an individual command) by 2.0% [1]. I don't believe this performance benefit is worth the possible overhead of frequent block size changes nor the cost of potentially more than 2 commands per transfer.
[1] The real performance benefit will be much less due to per command overhead and other non IO_RW_EXTENDED traffic on the bus.
In conclusion, the optimum block size is based solely on the card and host capabilities and should be limited to at most 512. Hence the block size can (and should) only be set once during card initialization.
This has the added benefit of making the code simpler and hence easier to understand and maintain.
Quoted 7 lines
drivers/mmc/core/sdio_io.c +7
-0+/* +* Internal function. Splits an arbitrarily sized data transfer +* into several IO_RW_EXTENDED commands. +*/ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned addr,int incr_addr,u8 *buf,unsigned size) +{
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr, · line 257
Quoted 10 lines
drivers/mmc/core/sdio_io.c +10
-0+unsigned remainder = size; +int ret; +unsigned short blksz; +struct mmc_host *host = func->card->host; + +if (func->forced_blksz) + blksz = func->forced_blksz; +else { +if (size <=512 ) + goto byte_remainder;
The maximum size for a byte mode transfer is the block size set in the card.
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr, · line 261
Quoted 4 lines
drivers/mmc/core/sdio_io.c +4
-0+ + blksz = size; +if (size >0xffff ) + blksz =0xffff ;
The largest possible block size is 2048. I'd also not be keen on using a block size which isn't a power of 2 -- hardware is unlikely to have been exercised with unusual block sizes.
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr, · line 270
Quoted 8 lines
drivers/mmc/core/sdio_io.c +8
-0+if (blksz > func->blksize) + blksz = func->blksize; +if (blksz > host->max_blk_size) + blksz = host->max_blk_size; + +/* avoid changing blksize needlessly */ +if (func->cur_blksz && ((blksz % func->cur_blksz) ==0 )) + blksz = func->cur_blksz;
If func->blksize == 1024 and we perform transfers in the 512 to 1024 range, this would set the block size on every transfer.
Review
Quoted 1 line
(quoted diff) +1
-0+ }
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
PATCH 1/4 sdio: parameterize SDIO FBR register defines
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: parameterize SDIO FBR register defines
---
drivers/mmc/core/sdio.c +2 -2
@@ -30,7 +30,7 @@unsigned char data; ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF ,0 , &data); +SDIO_FBR_BASE (func->num) +SDIO_FBR_STD_IF ,0 , &data);if (ret) goto out;
@@ -38,7 +38,7 @@if (data ==0x0f ) { ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF_EXT ,0 , &data); +SDIO_FBR_BASE (func->num) +SDIO_FBR_STD_IF_EXT ,0 , &data);if (ret) goto out; }
drivers/mmc/core/sdio_cis.c +1 -1
@@ -250,7 +250,7 @@ fn =0 ; ret =mmc_io_rw_direct (card,0 ,0 , - fn *0x100 +SDIO_FBR_CIS + i,0 , &x); +SDIO_FBR_BASE (fn) +SDIO_FBR_CIS + i,0 , &x);if (ret)return ret; ptr |= x << (i *8 );
include/linux/mmc/sdio.h +2 -0
@@ -132,6 +132,8 @@* Function Basic Registers (FBR) */ +#define SDIO_FBR_BASE (f) ((f) * 0x100)/* base of function f's FBRs */ +#define SDIO_FBR_STD_IF 0x00#define SDIO_FBR_SUPPORTS_CSA 0x40/* supports Code Storage Area */
PATCH 2/4 sdio: set the functions' block size
In reply to David VrabelSignature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: set the functions' block size
During function initialization, set the functions' block size to the largest that's supported by both the card and the host.
---
drivers/mmc/core/sdio_cis.c +2 -2
@@ -145,9 +145,9 @@printk (KERN_DEBUG "%s: TPLFE_CSA_PROPERTY = 0x%02x\n" ,sdio_func_id (func), val);/* TPLFE_MAX_BLK_SIZE */ - func->blksize = buf[12 ] | (buf[13 ] <<8 ); + func->max_blksize = buf[12 ] | (buf[13 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_MAX_BLK_SIZE = %u\n" , -sdio_func_id (func), (unsigned )func->blksize); +sdio_func_id (func), (unsigned )func->max_blksize);/* TPLFE_OCR */ val = buf[14 ] | (buf[15 ] <<8 ) | (buf[16 ] <<16 ) | (buf[17 ] <<24 ); printk(KERN_DEBUG "%s: TPLFE_OCR = 0x%08x\n",
drivers/mmc/core/sdio_io.c +25 -0
@@ -145,6 +145,31 @@EXPORT_SYMBOL_GPL (sdio_disable_func);/** +* sdio_set_block_size - set the block size of an SDIO function +* @func: SDIO function to change +* @blksz: new block size +*/ +int sdio_set_block_size (struct sdio_func *func,unsigned blksz) +{ +int ret; + + ret =mmc_io_rw_direct (func->card,1 ,0 , +SDIO_FBR_BASE (func->num) +SDIO_FBR_BLKSIZE , + blksz &0xff ,NULL ); +if (ret) +return ret; + ret =mmc_io_rw_direct (func->card,1 ,0 , +SDIO_FBR_BASE (func->num) +SDIO_FBR_BLKSIZE +1 , + (blksz >>8 ) &0xff ,NULL ); +if (ret) +return ret; + func->cur_blksize = blksz; +return 0 ; +} + +EXPORT_SYMBOL_GPL (sdio_set_block_size); + +/** * sdio_readb - read a single byte from a SDIO function * @func: SDIO function to access * @addr: address to read
include/linux/mmc/sdio_func.h +4 -1
@@ -43,7 +43,8 @@unsigned short vendor;/* vendor id */ unsigned short device;/* device id */ -unsigned short blksize;/* maximum block size */ +unsigned max_blksize;/* maximum block size */ +unsigned cur_blksize;/* current block size */ unsigned int state;/* function state */ #define SDIO_STATE_PRESENT (1<<0)/* present in sysfs */
@@ -109,6 +110,8 @@extern int sdio_enable_func (struct sdio_func *func);extern int sdio_disable_func (struct sdio_func *func); +extern int sdio_set_block_size (struct sdio_func *func,unsigned blksz); +extern int sdio_claim_irq (struct sdio_func *func,sdio_irq_handler_t *handler);extern int sdio_release_irq (struct sdio_func *func);
drivers/mmc/core/sdio.c +10 -0
@@ -53,6 +53,7 @@ {int ret;struct sdio_func *func; +unsigned block_size;BUG_ON (fn >SDIO_MAX_FUNCS );
@@ -70,6 +71,15 @@if (ret) goto fail; +/* +* Set the function's block size to the largest supported by +* both the function and the host. +*/ + block_size =min (func->max_blksize, func->card->host->max_blk_size); + ret =sdio_set_block_size (func, block_size); +if (ret) + goto fail; + card->sdio_func[fn -1 ] = func;return 0 ;
Re: [patch 2/4] sdio: set the functions' block size
Quoted 29 lines · On Tue, 07 Aug 2007 13:54:32 +0100 David Vrabel <[email protected]> wrote:
Index:
mmc/drivers/mmc/core/sdio.c
===================================================================
---mmc.orig/drivers/mmc/core/sdio.c2007-08-07
00:38:33.000000000 +0100 +++mmc/drivers/mmc/core/sdio.c2007-08-07 07:17:29.000000000 +0100 @@ -53,6 +53,7 @@ {drivers/mmc/core/sdio.c +10
-0int ret;struct sdio_func *func; +unsigned block_size;BUG_ON (fn >SDIO_MAX_FUNCS );@@ -70,6 +71,15 @@if (ret) goto fail; +/* +* Set the function's block size to the largest supported by +* both the function and the host. +*/ + block_size =min (func->max_blksize, func->card->host->max_blk_size); + ret =sdio_set_block_size (func, block_size); +if (ret) + goto fail; + card->sdio_func[fn -1 ] = func;
Quoted 1 line
return 0;
This is probably better done in the sdio_enable_func().
We also need to check that the card actually supports block io.
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 2/4] sdio: set the functions' block size
Quoted 32 lines · Pierre Ossman
Quoted 29 lines · On Tue, 07 Aug 2007 13:54:32 +0100 David Vrabel <[email protected]> wrote:
Index:
mmc/drivers/mmc/core/sdio.c
===================================================================
---mmc.orig/drivers/mmc/core/sdio.c2007-08-07
00:38:33.000000000 +0100 +++mmc/drivers/mmc/core/sdio.c2007-08-07 07:17:29.000000000 +0100 @@ -53,6 +53,7 @@ {drivers/mmc/core/sdio.c +10
-0int ret;struct sdio_func *func; +unsigned block_size;BUG_ON (fn >SDIO_MAX_FUNCS );@@ -70,6 +71,15 @@if (ret) goto fail; +/* +* Set the function's block size to the largest supported by +* both the function and the host. +*/ + block_size =min (func->max_blksize, func->card->host->max_blk_size); + ret =sdio_set_block_size (func, block_size); +if (ret) + goto fail; + card->sdio_func[fn -1 ] = func;
Quoted 3 lines
Quoted 1 line
return 0;
This is probably better done in the
sdio_enable_func().
I don't think so. A driver might enable/disable a function multiple times but there's no need to set the block size every time.
It may be best to move setting the block size back to before the probe so a driver can be sure the block size is something sensible. Consider a failed probe that called sdio_set_block_size() -- this will currently mess up drivers probed later.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Re: [patch 2/4] sdio: set the functions' block size
Quoted 34 lines · On Tue, 07 Aug 2007 18:20:26 +0100 David Vrabel <[email protected]> wrote:
Quoted 33 lines · Pierre Ossman
Quoted 30 lines · On Tue, 07 Aug 2007 13:54:32 +0100 David Vrabel <[email protected]> wrote:
Index:
mmc/drivers/mmc/core/sdio.c
===================================================================
---mmc.orig/drivers/mmc/core/sdio.c2007-08-07
00:38:33.000000000 +0100 +++mmc/drivers/mmc/core/sdio.c2007-08-07 07:17:29.000000000 +0100 @@ -53,6 +53,7 @@ {drivers/mmc/core/sdio.c +3
-0int ret;struct sdio_func *func; +unsigned block_size;BUG_ON (fn >SDIO_MAX_FUNCS );@@ -70,6 +71,15 @@if (ret) goto fail; +/* +* Set the function's block size to the largest supported by
drivers/mmc/core/sdio.c +3
-0+* both the function and the host. +*/ + block_size = min(func->max_blksize,
func->card->host->max_blk_size);drivers/mmc/core/sdio.c +4
-0+ ret =sdio_set_block_size (func, block_size); +if (ret) + goto fail; + card->sdio_func[fn -1 ] = func;
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
return 0;
This is probably better done in the
sdio_enable_func().I don't think so. A driver might enable/disable a function multiple times but there's no need to set the block size every time.
Why would it want to do that?
Anyway, as long as cur_blksz is preserved, sdio_set_block_size() can easily filter out redundant calls. No need to compromise in the rest of the code for that.
In the patch I sent, the block size is set the first time is needed. Wouldn't that avoid all problems?
Quoted 5 lines
It may be best to move setting the block size back to before the probe so a driver can be sure the block size is something sensible. Consider a failed probe that called
sdio_set_block_size()-- this will currently mess up drivers probed later.
Right, or remove the lock in the variant I proposed.
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 2/4] sdio: set the functions' block size
Quoted 5 lines · Pierre Ossman
Quoted 3 lines
A driver might enable/disable a function multiple times but there's no need to set the block size every time.
Why would it want to do that?
A function enable/disable cycle should act as a per-function reset.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Re: [patch 2/4] sdio: set the functions' block size
Quoted 9 lines · On Wed, 08 Aug 2007 10:46:24 +0100 David Vrabel <[email protected]> wrote:
Quoted 5 lines · Pierre Ossman
Quoted 3 lines
A driver might enable/disable a function multiple times but there's no need to set the block size every time.
Why would it want to do that?
A function enable/disable cycle should act as a per-function reset.
It could be argued that it would reset the other per-function information as well. :)
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 2/4] sdio: set the functions' block size
Quoted 4 lines · Pierre Ossman
Quoted 1 line · On Wed, 08 Aug 2007 10:46:24 +0100 David Vrabel <[email protected]> wrote:
Pierre Ossman wrote:
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
A driver might enable/disable a function multiple times but there's no need to set the block size every time.
Why would it want to do that?
A function enable/disable cycle should act as a per-function reset.
It could be argued that it would reset the other per-function information as well. :)
I do not think that that's what the SDIO specification requires, nor is it what hardware I am familiar with does.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
PATCH 3/4 sdio: extend sdio_readsb() and friends to handle any length of buffer
In reply to David VrabelSignature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: extend sdio_readsb() and friends to handle any length of buffer
Extend sdio_readsb(), sdio_writesb(), sdio_memcpy_fromio(), and sdio_memcpy_toio() to handle any length of buffer by splitting the transfer into several IO_RW_EXTENDED commands. Typically, a transfer would be split into a single block mode transfer followed by a byte mode transfer for the remainder.
For this to work the block size must be limited to the maximum size of a byte mode transfer (512 bytes). This limitation could be revisited if there are any cards out there that require a block size > 512.
host->max_seg_size <= host->max_req_size so there's no need to check both when determining the maximum data size for a single command.
---
drivers/mmc/core/sdio_io.c +59 -13
@@ -153,6 +153,13 @@ {int ret; +/* The standard permits block sizes up to 2048 bytes, but +* sdio_readsb() etc. can only work with block size <= 512. */ +if (blksz >512 ) { + blksz =512 ; +dev_warn (&func->dev,"block size limited to 512 bytes\n" ); + } + ret =mmc_io_rw_direct (func->card,1 ,0 ,SDIO_FBR_BASE (func->num) +SDIO_FBR_BLKSIZE , blksz &0xff ,NULL );
@@ -227,6 +234,48 @@EXPORT_SYMBOL_GPL (sdio_writeb); +/* Split an arbitrarily sized data transfer into several +* IO_RW_EXTENDED commands. */ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned fn,unsigned addr,int incr_addr,u8 *buf,unsigned size) +{ +unsigned remainder = size; +unsigned max_blocks; +int ret; + +/* Blocks per command is limited by host count, host transfer size +(we only use a single sg entry) and the maximum for IO_RW_EXTENDED +of 511 blocks. */ + max_blocks =min (min ( + func->card->host->max_blk_count, + func->card->host->max_seg_size / func->cur_blksize), +511u ); + +while (remainder > func->cur_blksize) { +unsigned blocks; + + blocks = remainder % func->cur_blksize; +if (blocks > max_blocks) + blocks = max_blocks; + size = blocks * func->cur_blksize; + + ret =mmc_io_rw_extended (func->card, write, fn, addr, + incr_addr, buf, blocks, func->cur_blksize); +if (ret) +return ret; + + remainder -= size; + buf += size; +if (incr_addr) + addr += size; + } + +if (remainder) + ret =mmc_io_rw_extended (func->card, write, fn, addr, + incr_addr, buf,1 , remainder); +return ret; +} +/** * sdio_memcpy_fromio - read a chunk of memory from a SDIO function * @func: SDIO function to access
@@ -234,14 +283,13 @@* @addr: address to begin reading from * @count: number of bytes to read * -* Reads up to 512 bytes from the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_fromio (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,0 , dst, +return sdio_io_rw_ext_helper (func,0 , func->num, addr,1 , dst, count); }
@@ -254,14 +302,13 @@* @src: buffer that contains the data to write * @count: number of bytes to write * -* Writes up to 512 bytes to the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Writes to the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_toio (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,0 , src, +return sdio_io_rw_ext_helper (func,1 , func->num, addr,1 , src, count); }
@@ -272,14 +319,13 @@* @addr: address of (single byte) FIFO * @count: number of bytes to read * -* Reads up to 512 bytes from the specified FIFO of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the specified FIFO of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_readsb (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,1 , dst, +return sdio_io_rw_ext_helper (func,0 , func->num, addr,0 , dst, count); }
@@ -299,7 +345,7 @@int sdio_writesb (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,1 , src, +return sdio_io_rw_ext_helper (func,1 , func->num, addr,0 , src, count); }
drivers/mmc/core/sdio_ops.c +11 -7
@@ -88,7 +88,7 @@ }int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *buf,unsigned size) +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz) {struct mmc_request mrq;struct mmc_command cmd;
@@ -97,7 +97,7 @@BUG_ON (!card);BUG_ON (fn >7 ); -BUG_ON (size >512 ); +BUG_ON (blocks ==1 && blksz >512 );memset (&mrq,0 ,sizeof (struct mmc_request ));memset (&cmd,0 ,sizeof (struct mmc_command ));
@@ -109,18 +109,22 @@ cmd.opcode =SD_IO_RW_EXTENDED ; cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ; + cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; +if (blocks >1 ) { + cmd.arg |=0x08000000 ; + cmd.arg |= blocks; + }else + cmd.arg |= (blksz ==512 ) ?0 : blksz; cmd.flags =MMC_RSP_R5 |MMC_CMD_ADTC ; - data.blksz = size; - data.blocks =1 ; + data.blksz = blksz; + data.blocks = blocks; data.flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; data.sg = &sg; data.sg_len =1 ; -sg_init_one (&sg, buf, size); +sg_init_one (&sg, buf, blksz * blocks);mmc_set_data_timeout (&data, card,0 );
drivers/mmc/core/sdio_ops.h +1 -1
@@ -16,7 +16,7 @@int mmc_io_rw_direct (struct mmc_card *card,int write,unsigned fn,unsigned addr,u8 in,u8 * out);int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *data,unsigned size); +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz);#endif
Re: [patch 3/4] sdio: extend sdio_readsb() and friends to handle any length of buffer
Quoted 33 lines · On Tue, 07 Aug 2007 13:55:12 +0100 David Vrabel <[email protected]> wrote:
sdio: extend
sdio_readsb()and friends to handle any length of bufferExtend
sdio_readsb(),sdio_writesb(),sdio_memcpy_fromio(), andsdio_memcpy_toio()to handle any length of buffer by splitting the transfer into severalIO_RW_EXTENDEDcommands. Typically, a transfer would be split into a single block mode transfer followed by a byte mode transfer for the remainder.For this to work the block size must be limited to the maximum size of a byte mode transfer (512 bytes). This limitation could be revisited if there are any cards out there that require a block size
Quoted 1 line
512.
host->max_seg_size<=host->max_req_sizeso there's no need to check both when determining the maximum data size for a single command.Signed-off-by David Vrabel <[email protected]>---
Index:mmc/drivers/mmc/core/sdio_io.c
===================================================================
---mmc.orig/drivers/mmc/core/sdio_io.c2007-08-07
07:27:31.000000000 +0100 +++mmc/drivers/mmc/core/sdio_io.c2007-08-07 07:35:02.000000000 +0100 @@ -153,6 +153,13 @@ {drivers/mmc/core/sdio_io.c +2
-0int ret; +/* The standard permits block sizes up to 2048 bytes, but +* sdio_readsb() etc. can only work with block size <= 512. */
drivers/mmc/core/sdio_io.c +3
-0+if (blksz >512 ) { + blksz =512 ; + dev_warn(&func->dev, "block size limited to 512bytes\n");
Review
Quoted 2 lines
(quoted diff) +2
-0+ } +
I'm not sure about not returning an error here. What if a driver calls it and relies on being able to transfer >512 blocks?
Quoted 6 lines
drivers/mmc/core/sdio_io.c +0
-0ret =mmc_io_rw_direct (func->card,1 ,0 ,SDIO_FBR_BASE (func->num) +SDIO_FBR_BLKSIZE , blksz &0xff ,NULL );@@ -227,6 +234,48 @@EXPORT_SYMBOL_GPL (sdio_writeb);
drivers/mmc/core/sdio_io.c line 245
Quoted 10 lines
drivers/mmc/core/sdio_io.c +8
-0+/* Split an arbitrarily sized data transfer into several +* IO_RW_EXTENDED commands. */ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned fn,unsigned addr,int incr_addr,u8 *buf,unsigned size) +{ +unsigned remainder = size; +unsigned max_blocks; +int ret; +
We need to check support for block transfers here as well.
Also, the parameter fn is redundant.
drivers/mmc/core/sdio_ops.c line 114
Quoted 9 lines
drivers/mmc/core/sdio_ops.c +2
-2@@ -109,18 +109,22 @@ cmd.opcode =SD_IO_RW_EXTENDED ; cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ; + cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; +if (blocks >1 ) {
|| blksz > 512
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 3/4] sdio: extend sdio_readsb() and friends to handle any length of buffer
In reply to David VrabelHere's my proposed compromise.
If a driver uses sdio_force_block_size() extensively, it will be like your original version with sdio_set_block_size(). If it doesn't, however, then that is a way to indicate that the driver has no specific requirements (meaning we are free to change things in the future).
You'll also notice that calling sdio_force_block_size() is free. The actual change comes when it is first used, meaning that drivers don't have to be as careful when calling it.
The reset of custom/forced block size is moved to the probe function as you suggested.
drivers/mmc/core/sdio_bus.c +2 -0
@@ -133,6 +133,8 @@ static int sdio_bus_probe(struct device *dev)if (!id)return -ENODEV ; + func->forced_blksz =0 ; +return drv->probe (func, id); }
drivers/mmc/core/sdio_cis.c +2 -2
@@ -145,9 +145,9 @@ static int cistpl_funce_func(struct sdio_func *func,printk (KERN_DEBUG "%s: TPLFE_CSA_PROPERTY = 0x%02x\n" ,sdio_func_id (func), val);/* TPLFE_MAX_BLK_SIZE */ - func->blksize = buf[12 ] | (buf[13 ] <<8 ); + func->max_blksz = buf[12 ] | (buf[13 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_MAX_BLK_SIZE = %u\n" , -sdio_func_id (func), (unsigned )func->blksize); +sdio_func_id (func), (unsigned )func->max_blksz);/* TPLFE_OCR */ val = buf[14 ] | (buf[15 ] <<8 ) | (buf[16 ] <<16 ) | (buf[17 ] <<24 ); printk(KERN_DEBUG "%s: TPLFE_OCR = 0x%08x\n",
drivers/mmc/core/sdio_io.c +148 -16
213 lines not on this page · raw message
drivers/mmc/core/sdio_ops.c +14 -7
@@ -88,7 +88,7 @@ int mmc_io_rw_direct(struct mmc_card *card, int write, unsigned fn, }int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *buf,unsigned size) +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz) {struct mmc_request mrq;struct mmc_command cmd;
@@ -97,7 +97,7 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn,BUG_ON (!card);BUG_ON (fn >7 ); -BUG_ON (size >512 ); +BUG_ON (blocks >511 );memset (&mrq,0 ,sizeof (struct mmc_request ));memset (&cmd,0 ,sizeof (struct mmc_command ));
@@ -107,20 +107,27 @@ int mmc_io_rw_extended(struct mmc_card *card, int write, unsigned fn, mrq.data = &data; cmd.opcode =SD_IO_RW_EXTENDED ; + cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ; + cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; + +if ((blocks >1 ) || (blksz >512 )) { + cmd.arg |=0x08000000 ; + cmd.arg |= blocks; + }else + cmd.arg |= (blksz ==512 ) ?0 : blksz; + cmd.flags =MMC_RSP_R5 |MMC_CMD_ADTC ; - data.blksz = size; - data.blocks =1 ; + data.blksz = blksz; + data.blocks = blocks; data.flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; data.sg = &sg; data.sg_len =1 ; -sg_init_one (&sg, buf, size); +sg_init_one (&sg, buf, blksz * blocks);mmc_set_data_timeout (&data, card,0 );
drivers/mmc/core/sdio_ops.h +1 -1
@@ -16,7 +16,7 @@ int mmc_send_io_op_cond(struct mmc_host *host, u32 ocr, u32 *rocr);int mmc_io_rw_direct (struct mmc_card *card,int write,unsigned fn,unsigned addr,u8 in,u8 * out);int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *data,unsigned size); +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz);#endif
include/linux/mmc/sdio_func.h +5 -1
@@ -43,7 +43,9 @@ struct sdio_func {unsigned short vendor;/* vendor id */ unsigned short device;/* device id */ -unsigned short blksize;/* maximum block size */ +unsigned short max_blksz;/* maximum block size */ +unsigned short cur_blksz;/* current block size */ +unsigned short forced_blksz;/* driver forced block size */ unsigned int state;/* function state */ #define SDIO_STATE_PRESENT (1<<0)/* present in sysfs */
@@ -136,5 +138,7 @@ extern int sdio_memcpy_toio(struct sdio_func *func, unsigned int addr,extern int sdio_writesb (struct sdio_func *func,unsigned int addr,void *src,int count); +extern int sdio_force_block_size (struct sdio_func *func,unsigned short size); +#endif
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 3/4] sdio: extend sdio_readsb() and friends to handle any length of buffer
Quoted 13 lines · Pierre Ossman
drivers/mmc/core/sdio_io.c +13
-0+static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned addr,int incr_addr,u8 *buf,unsigned size) +{ +unsigned remainder = size; +int ret; +unsigned short blksz; +struct mmc_host *host = func->card->host; + +if (func->forced_blksz) + blksz = func->forced_blksz; +else { + blksz = func->max_blksz; +if (blksz > host->max_blk_size)
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr, · line 264
Quoted 10 lines
drivers/mmc/core/sdio_io.c +10
-0+ blksz = host->max_blk_size; +if (blksz >512 ) + blksz =512 ; + } + +WARN_ON (blksz >512 ); + + ret =sdio_set_block_size (func, blksz); +if (ret) +return ret;
We need to know the block size in use /before/ the start of the transfer as we would like drivers to be able to perform transfers with single commands as this can result in significantly better performance[1]. The drivers for our (CSR's) WiFi chips should do this. This isn't some (as you suggested in a previous post) 'rare' requirement.
Therefore, we should not change the block size here.
We can also support block sizes > 512 and have this function do the correct thing (as you'll see when I post my final patches).
David
[1] Consider a chip with a block size of 64 that regularly does short transfers of between 64 - 128 bytes. Without padding, this would require two commands per transfer instead of just one, cutting performance by 50%! This scenario could be a WiFi chip doing VoIP.
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Re: [patch 3/4] sdio: extend sdio_readsb() and friends to handle any length of buffer
Quoted 8 lines · On Wed, 08 Aug 2007 11:19:33 +0100 David Vrabel <[email protected]> wrote:
We need to know the block size in use /before/ the start of the transfer as we would like drivers to be able to perform transfers with single commands as this can result in significantly better performance[1]. The drivers for our (CSR's) WiFi chips should do this. This isn't some (as you suggested in a previous post) 'rare' requirement.
Well, there are more ways that can be achieved.
First, the driver could lock down the block size using sdio_force_block_size(). Then it knows what it gets.
Second, we could try to make it possible for the driver to indicate "feel free to pad this transfer". Then we could also remove the need for drivers to mess with buffers and keep such stuff in the core. We could even magically remove a memcpy() by setting up two sg entries, one for the data and one for the padding.
Quoted 5 lines
[1] Consider a chip with a block size of 64 that regularly does short transfers of between 64 - 128 bytes. Without padding, this would require two commands per transfer instead of just one, cutting performance by 50%! This scenario could be a WiFi chip doing VoIP.
Provided block size < 128. Otherwise it'll jump straight to the byte transfer.
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 3/4] sdio: extend sdio_readsb() and friends to handle any length of buffer
Quoted 13 lines · Pierre Ossman
Quoted 7 lines · On Wed, 08 Aug 2007 11:19:33 +0100 David Vrabel <[email protected]> wrote:
We need to know the block size in use /before/ the start of the transfer as we would like drivers to be able to perform transfers with single commands as this can result in significantly better performance[1]. The drivers for our (CSR's) WiFi chips should do this. This isn't some (as you suggested in a previous post) 'rare' requirement.
Well, there are more ways that can be achieved.
Quoted 8 lines
First, the driver could lock down the block size using
sdio_force_block_size(). Then it knows what it gets.Second, we could try to make it possible for the driver to indicate "feel free to pad this transfer". Then we could also remove the need for drivers to mess with buffers and keep such stuff in the core. We could even magically remove a
memcpy()by setting up two sg entries, one for the data and one for the padding.
Setting the block size in io_rw_ext_helper() has several drawbacks, namely:
1. Reduces the flexibility of drivers to manage what commands are performed.
2. A performance penalty on the first transfer.
3. Greater code complexity.
4. Non-intuitive location for card initialization code.
Sure we could just through hoops and add (much) extra complexity to the core, to improve item 1 but 2, 3 and 4 are insoluble. Given that setting block size before the first command has /zero/ benefits[1], why bother?
Your insistence on this stupid idea baffles me, particularly in the light of your other useful comments.
I would also like to advise that until a larger number of function drivers become available that the core is kept as simple and as flexible as possible. Without knowing how different cards operate it is difficult to know what's common behaviour and what's card specific.
My latest (and hopefully final) patch set follows.
David
[1] dynamic block sizing was (potentially) a useful benefit but I have comprehensibly shown it's not beneficial. The idea that is somehow necessary to permit the core to change it's block size selection algorithm is bogus.
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
PATCH 1/3 sdio: add SDIO_FBR_BASE(f) macro
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: add SDIO_FBR_BASE(f) macro
---
drivers/mmc/core/sdio.c +2 -2
@@ -30,7 +30,7 @@unsigned char data; ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF ,0 , &data); +SDIO_FBR_BASE (func->num) +SDIO_FBR_STD_IF ,0 , &data);if (ret) goto out;
@@ -38,7 +38,7 @@if (data ==0x0f ) { ret =mmc_io_rw_direct (func->card,0 ,0 , - func->num *0x100 +SDIO_FBR_STD_IF_EXT ,0 , &data); +SDIO_FBR_BASE (func->num) +SDIO_FBR_STD_IF_EXT ,0 , &data);if (ret) goto out; }
drivers/mmc/core/sdio_cis.c +1 -1
@@ -250,7 +250,7 @@ fn =0 ; ret =mmc_io_rw_direct (card,0 ,0 , - fn *0x100 +SDIO_FBR_CIS + i,0 , &x); +SDIO_FBR_BASE (fn) +SDIO_FBR_CIS + i,0 , &x);if (ret)return ret; ptr |= x << (i *8 );
include/linux/mmc/sdio.h +2 -0
@@ -132,6 +132,8 @@* Function Basic Registers (FBR) */ +#define SDIO_FBR_BASE (f) ((f) * 0x100)/* base of function f's FBRs */ +#define SDIO_FBR_STD_IF 0x00#define SDIO_FBR_SUPPORTS_CSA 0x40/* supports Code Storage Area */
PATCH 2/3 sdio: set the functions' block size
In reply to David VrabelSignature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: set the functions' block size
Before a driver is probed, set the function's block size to the default so the driver is sure the block size is something sensible and it needn't explicitly set it.
The default block size is the largest that's supported by both the card and the host, with a maximum of 512 to ensure aribitrarily sized transfer use the optimal (least) number of commands.
See http://lkml.org/lkml/2007/8/7/150 for reasons for the block size choice.
---
drivers/mmc/core/sdio_cis.c +2 -2
@@ -145,9 +145,9 @@printk (KERN_DEBUG "%s: TPLFE_CSA_PROPERTY = 0x%02x\n" ,sdio_func_id (func), val);/* TPLFE_MAX_BLK_SIZE */ - func->blksize = buf[12 ] | (buf[13 ] <<8 ); + func->max_blksize = buf[12 ] | (buf[13 ] <<8 );printk (KERN_DEBUG "%s: TPLFE_MAX_BLK_SIZE = %u\n" , -sdio_func_id (func), (unsigned )func->blksize); +sdio_func_id (func), (unsigned )func->max_blksize);/* TPLFE_OCR */ val = buf[14 ] | (buf[15 ] <<8 ) | (buf[16 ] <<16 ) | (buf[17 ] <<24 ); printk(KERN_DEBUG "%s: TPLFE_OCR = 0x%08x\n",
drivers/mmc/core/sdio_io.c +49 -0
55 lines not on this page · raw message
include/linux/mmc/sdio_func.h +4 -1
@@ -43,7 +43,8 @@unsigned short vendor;/* vendor id */ unsigned short device;/* device id */ -unsigned short blksize;/* maximum block size */ +unsigned max_blksize;/* maximum block size */ +unsigned cur_blksize;/* current block size */ unsigned int state;/* function state */ #define SDIO_STATE_PRESENT (1<<0)/* present in sysfs */
@@ -109,6 +110,8 @@extern int sdio_enable_func (struct sdio_func *func);extern int sdio_disable_func (struct sdio_func *func); +extern int sdio_set_block_size (struct sdio_func *func,unsigned blksz); +extern int sdio_claim_irq (struct sdio_func *func,sdio_irq_handler_t *handler);extern int sdio_release_irq (struct sdio_func *func);
drivers/mmc/core/sdio_bus.c +7 -0
@@ -128,11 +128,18 @@struct sdio_driver *drv =to_sdio_driver (dev->driver);struct sdio_func *func =dev_to_sdio_func (dev);const struct sdio_device_id *id; +int ret; id =sdio_match_device (func, drv);if (!id)return -ENODEV ; +/* Set the default block size so the driver is sure it's something +* sensible. */ + ret =sdio_set_block_size (func,0 ); +if (ret) +return ret; +return drv->probe (func, id); }
PATCH 3/3 sdio: extend sdio_readsb() and friends to handle any length of buffer
In reply to David VrabelSignature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: extend sdio_readsb() and friends to handle any length of buffer
Extend sdio_readsb(), sdio_writesb(), sdio_memcpy_fromio(), and sdio_memcpy_toio() to handle any length of buffer by splitting the transfer into several IO_RW_EXTENDED commands. Typically, a transfer would be split into a single block mode transfer followed by a byte mode transfer for the remainder but we also handle lack of block mode support and the block size being greater than 512 (the maximum byte mode transfer size).
host->max_seg_size <= host->max_req_size so there's no need to check both when determining the maximum data size for a single command.
---
drivers/mmc/core/sdio_io.c +73 -17
@@ -193,6 +193,69 @@EXPORT_SYMBOL_GPL (sdio_set_block_size); +/* Split an arbitrarily sized data transfer into several +* IO_RW_EXTENDED commands. */ +static int sdio_io_rw_ext_helper (struct sdio_func *func,int write, +unsigned addr,int incr_addr,u8 *buf,unsigned size) +{ +unsigned remainder = size; +unsigned max_blocks; +int ret; + +/* Do the bulk of the transfer using block mode (if supported). */ +if (func->card->cccr.multi_block) { +unsigned max_blocks; + +/* Blocks per command is limited by host count, host transfer +* size (we only use a single sg entry) and the maximum for +* IO_RW_EXTENDED of 511 blocks. */ + max_blocks =min (min ( + func->card->host->max_blk_count, + func->card->host->max_seg_size / func->cur_blksize), +511u ); + +while (remainder > func->cur_blksize) { +unsigned blocks; + + blocks = remainder % func->cur_blksize; +if (blocks > max_blocks) + blocks = max_blocks; + size = blocks * func->cur_blksize; + + ret =mmc_io_rw_extended (func->card, write, + func->num, addr, incr_addr, buf, + blocks, func->cur_blksize); +if (ret) +return ret; + + remainder -= size; + buf += size; +if (incr_addr) + addr += size; + } + } + +/* Write the remainder using byte mode. */ +while (remainder >0 ) { + size = remainder; +if (size > func->cur_blksize) + size = func->cur_blksize; +if (size >512 ) + size =512 ;/* maximum size for byte mode */ + + ret =mmc_io_rw_extended (func->card, write, func->num, addr, + incr_addr, buf,1 , size); +if (ret) +return ret; + + remainder -= size; + buf += size; +if (incr_addr) + addr += size; + } +return 0 ; +} +/** * sdio_readb - read a single byte from a SDIO function * @func: SDIO function to access
@@ -258,15 +321,13 @@* @addr: address to begin reading from * @count: number of bytes to read * -* Reads up to 512 bytes from the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_fromio (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,0 , dst, - count); +return sdio_io_rw_ext_helper (func,0 , addr,1 , dst, count); }EXPORT_SYMBOL_GPL (sdio_memcpy_fromio);
@@ -278,15 +339,13 @@* @src: buffer that contains the data to write * @count: number of bytes to write * -* Writes up to 512 bytes to the address space of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Writes to the address space of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_memcpy_toio (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,0 , src, - count); +return sdio_io_rw_ext_helper (func,1 , addr,1 , src, count); }/**
@@ -296,15 +355,13 @@* @addr: address of (single byte) FIFO * @count: number of bytes to read * -* Reads up to 512 bytes from the specified FIFO of a given SDIO -* function. Return value indicates if the transfer succeeded or -* not. +* Reads from the specified FIFO of a given SDIO function. Return +* value indicates if the transfer succeeded or not. */ int sdio_readsb (struct sdio_func *func,void *dst,unsigned int addr,int count) { -return mmc_io_rw_extended (func->card,0 , func->num, addr,1 , dst, - count); +return sdio_io_rw_ext_helper (func,0 , addr,0 , dst, count); }EXPORT_SYMBOL_GPL (sdio_readsb);
@@ -323,8 +380,7 @@int sdio_writesb (struct sdio_func *func,unsigned int addr,void *src,int count) { -return mmc_io_rw_extended (func->card,1 , func->num, addr,1 , src, - count); +return sdio_io_rw_ext_helper (func,1 , addr,0 , src, count); }EXPORT_SYMBOL_GPL (sdio_writesb);
drivers/mmc/core/sdio_ops.c +10 -7
@@ -88,7 +88,7 @@ }int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *buf,unsigned size) +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz) {struct mmc_request mrq;struct mmc_command cmd;
@@ -97,7 +97,7 @@BUG_ON (!card);BUG_ON (fn >7 ); -BUG_ON (size >512 ); +BUG_ON (blocks ==1 && blksz >512 );memset (&mrq,0 ,sizeof (struct mmc_request ));memset (&cmd,0 ,sizeof (struct mmc_command ));
@@ -109,18 +109,21 @@ cmd.opcode =SD_IO_RW_EXTENDED ; cmd.arg = write ?0x80000000 :0x00000000 ; cmd.arg |= fn <<28 ; - cmd.arg |= bang ?0x00000000 :0x04000000 ; + cmd.arg |= incr_addr ?0x04000000 :0x00000000 ; cmd.arg |= addr <<9 ; - cmd.arg |= (size ==512 ) ?0 : size; +if (blocks ==1 && blksz <=512 ) + cmd.arg |= (blksz ==512 ) ?0 : blksz;/* byte mode */ +else + cmd.arg |=0x08000000 | blocks;/* block mode */ cmd.flags =MMC_RSP_R5 |MMC_CMD_ADTC ; - data.blksz = size; - data.blocks =1 ; + data.blksz = blksz; + data.blocks = blocks; data.flags = write ?MMC_DATA_WRITE :MMC_DATA_READ ; data.sg = &sg; data.sg_len =1 ; -sg_init_one (&sg, buf, size); +sg_init_one (&sg, buf, blksz * blocks);mmc_set_data_timeout (&data, card,0 );
drivers/mmc/core/sdio_ops.h +1 -1
@@ -16,7 +16,7 @@int mmc_io_rw_direct (struct mmc_card *card,int write,unsigned fn,unsigned addr,u8 in,u8 * out);int mmc_io_rw_extended (struct mmc_card *card,int write,unsigned fn, -unsigned addr,int bang,u8 *data,unsigned size); +unsigned addr,int incr_addr,u8 *buf,unsigned blocks,unsigned blksz);#endif
Re: [patch 3/4] sdio: extend sdio_readsb() and friends to handle any length of buffer
In reply to David VrabelQuoted 9 lines · On Wed, 08 Aug 2007 14:22:20 +0100 David Vrabel <[email protected]> wrote:
Setting the block size in
io_rw_ext_helper()has several drawbacks, namely:1. Reduces the flexibility of drivers to manage what commands are performed. 2. A performance penalty on the first transfer.
3. Greater code complexity.
4. Non-intuitive location for card initialization code.
Quoted 4 lines
Sure we could just through hoops and add (much) extra complexity to the core, to improve item 1 but 2, 3 and 4 are insoluble. Given that setting block size before the first command has /zero/ benefits[1], why bother?
3 and 4 are subjective, and for 2, the performance penalty has to come some place. The upside, as explained, is that drivers aren't penalized for setting the block size and then failing to use it. Something that gives us the flexibility to write cleaner code.
As for benefits, there are other issues. We might want to deal with things like alignment problems in the future.
Quoted 9 lines
Your insistence on this stupid idea baffles me, particularly in the light of your other useful comments.
I would also like to advise that until a larger number of function drivers become available that the core is kept as simple and as flexible as possible. Without knowing how different cards operate it is difficult to know what's common behaviour and what's card specific.
Agreed, we're doing a bit more guessing than one would like. But I don't agree in keeping the core simple and flexible. Rather the opposite. Any guarantee that we give drivers now and need to remove in the future needs to be tested with all drivers that rely on it (a difficult task as it often takes some effort to find people with hardware and the ability to test patches).
As such, the guarantees should be kept at a minimum. Code is easily changed, API is not.
IMO, the best way to achieve this is with an API that describes _what_ the drivers want to do, not _how_. Hence my insistence on allowing drivers to specify the difference between "I want this data transferred" and "I want this data transferred with an exact block size of 32 bytes".
Quoted 2 lines
My latest (and hopefully final) patch set follows.
These looks good. The only part I'm really attached to is the blksz == 0 case, which you have.
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
PATCH 4/4 sdio: disable CD resistor
In reply to David VrabelSignature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
sdio: disable CD resistor
Disable the card detect resistor to ensure all data lines are equally loaded. Not doing this can have a negative impact on buses with marginal signal quality.
---
drivers/mmc/core/sdio.c +27 -8
@@ -148,10 +148,25 @@return ret; } +static int sdio_modify_cccr (struct mmc_card *card,unsigned reg, +u8 val,u8 mask) +{ +u8 old; +int ret; + + ret =mmc_io_rw_direct (card,0 ,0 , reg,0 , &old); +if (ret) +return ret; + + val = (old & ~mask) | val; + + ret =mmc_io_rw_direct (card,1 ,0 , reg, val,NULL ); +return ret; +} +static int sdio_enable_wide (struct mmc_card *card) {int ret; -u8 ctrl;if (!(card->host->caps &MMC_CAP_4_BIT_DATA ))return 0 ;
@@ -159,13 +174,8 @@if (card->cccr.low_speed && !card->cccr.wide_bus)return 0 ; - ret =mmc_io_rw_direct (card,0 ,0 ,SDIO_CCCR_IF ,0 , &ctrl); -if (ret) -return ret; - - ctrl |=SDIO_BUS_WIDTH_4BIT ; - - ret =mmc_io_rw_direct (card,1 ,0 ,SDIO_CCCR_IF , ctrl,NULL ); + ret =sdio_modify_cccr (card,SDIO_CCCR_IF ,SDIO_BUS_WIDTH_4BIT , +SDIO_BUS_WIDTH_MASK );if (ret)return ret;
@@ -334,6 +344,15 @@mmc_set_clock (host, card->cis.max_dtr);/* +* Disable Card Detect resistor on DAT3 so all data lines are +* loaded the same. +*/ + err =sdio_modify_cccr (card,SDIO_CCCR_IF ,SDIO_BUS_CD_DISABLE , +SDIO_BUS_CD_DISABLE ); +if (err) + goto remove; + +/* * Switch to wider bus (if supported). */ err =sdio_enable_wide (card);
include/linux/mmc/sdio.h +1 -0
@@ -94,6 +94,7 @@#define SDIO_BUS_WIDTH_1BIT 0x00#define SDIO_BUS_WIDTH_4BIT 0x02 +#define SDIO_BUS_WIDTH_MASK 0x03#define SDIO_BUS_CD_DISABLE 0x80/* disable pull-up on DAT3 (pin 1) */
Re: [patch 4/4] sdio: disable CD resistor
Quoted 7 lines · On Tue, 07 Aug 2007 13:55:59 +0100 David Vrabel <[email protected]> wrote:
sdio: disable CD resistor
Disable the card detect resistor to ensure all data lines are equally loaded. Not doing this can have a negative impact on buses with marginal signal quality.
Signed-off-by David Vrabel <[email protected]>
I'm not sure about this. I've seen several hosts which lack a mechanical detect switch.
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Re: [patch 4/4] sdio: disable CD resistor
Quoted 5 lines · Pierre Ossman
Quoted 2 lines · On Tue, 07 Aug 2007 13:55:59 +0100 David Vrabel <[email protected]> wrote:
sdio: disable CD resistor
Quoted 8 lines
Quoted 5 lines
Disable the card detect resistor to ensure all data lines are equally loaded. Not doing this can have a negative impact on buses with marginal signal quality.
Signed-off-by David Vrabel <[email protected]>I'm not sure about this. I've seen several hosts which lack a mechanical detect switch.
If host is correctly designed it will have identical pull-ups on all the DAT lines, and therefore this internal pull-up should be disconnected.
However, given that disabling the pull-up may cause 4 bit transfers to fail on hardware that omits a host-side pull-up on DAT3 I agree that the resistor should remain enabled.
Is there an example driver for a host that uses pin 1/DAT3 sensing for card detection? I'm curious about how removal detections work.
App Note on card detection goes into the use of this resistor in more detail but I don't believe this document is publicly available :(.
David
Signature · 5 lines
David Vrabel, Software Engineer, Drivers group Tel: +44 (0)1223 692562 CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ .
Re: [patch 4/4] sdio: disable CD resistor
Quoted 4 lines · On Tue, 07 Aug 2007 15:46:36 +0100 David Vrabel <[email protected]> wrote:
Is there an example driver for a host that uses pin 1/DAT3 sensing for card detection? I'm curious about how removal detections work.
Both wbsd and sdhci have experienced hardware with this. In sdhci this is hidden completely in hardware, but wbsd has to do some funky logic to handle the problem.
Quoted 3 lines
App Note on card detection goes into the use of this resistor in more detail but I don't believe this document is publicly available :(.
SD is a wonderful world. :/
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org
Quoted 6 lines · On Tue, 07 Aug 2007 13:51:39 +0100 David Vrabel <[email protected]> wrote:
In conclusion, the optimum block size is based solely on the card and host capabilities and should be limited to at most 512. Hence the block size can (and should) only be set once during card initialization.
Well, I can hardly argue with that thorough analysis. :)
I still like the idea of having the control in the core as much as possible though. It allows people to experiment and figure out new and clever ways of solving this in a way that can benefit all drivers.
So could we do most of what you propose, where we set the block size to maximum, yet <= 512. Drivers can tweak this further by calling sdio_force_block_size() (as need e.g. by my marvell card), and keep the convention of 0 meaning "back to core default"? That way we can modify the meaning of "core default" if more clever ways are available.
Quoted 6 lines
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr, · line 257
Quoted 2 lines
drivers/mmc/core/sdio_io.c +2
-0+if (size <=512 ) + goto byte_remainder;The maximum size for a byte mode transfer is the block size set in the card.
Right, sorry. It was late when I was hacking on this.
Quoted 8 lines
drivers/mmc/core/sdio_io.c void sdio_writeb(struct sdio_func *func, unsigned char b, unsigned int addr, · line 270
Quoted 4 lines
(quoted diff) +2
-0+ /* avoid changing blksize needlessly */ + if (func->cur_blksz && ((blksz % func->cur_blksz)== 0))
drivers/mmc/core/sdio_io.c +1
-0+ blksz = func->cur_blksz;If
func->blksize== 1024 and we perform transfers in the 512 to 1024 range, this would set the block size on every transfer.
Quite right. I had no doubts that thing had room for improvement. But I agree that we might as well select a fixed block size and stick with it for the general case.
Rgds
Signature · 5 lines
-- Pierre Ossman Linux kernel, MMC maintainer http://www.kernel.org PulseAudio, core developer http://pulseaudio.org rdesktop, core developer http://www.rdesktop.org