sdio: enhance IO_RW_EXTENDED support

sdio: enhance IO_RW_EXTENDED support

[David Vrabel]

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

[David Vrabel]

Signed-off-by: David Vrabel <david.vrabel@csr.com>

---
commit 51755c3d59be1ba778bef45888f9f5e341dc4af4
tree c7bbb562b2d801197eefb619a17c94467c1299cd
parent 1cf0b6019aa3916197eecafe058bd2f3d700d24a
author David Vrabel <david.vrabel@csr.com> Mon, 30 Jul 2007 19:20:59 +0100
committer David Vrabel <dvrabel@cantab.net> 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(-)

diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 6589fd6..08f579c 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -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;
 	}
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index b0c6697..8aa4666 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -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);
diff --git a/include/linux/mmc/sdio.h b/include/linux/mmc/sdio.h
index 145ce23..e2bda4b 100644
--- a/include/linux/mmc/sdio.h
+++ b/include/linux/mmc/sdio.h
@@ -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

[Pierre Ossman]

On Tue, 31 Jul 2007 16:36:31 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> Signed-off-by: David Vrabel <david.vrabel@csr.com>
> 
> ---
> commit 51755c3d59be1ba778bef45888f9f5e341dc4af4
> tree c7bbb562b2d801197eefb619a17c94467c1299cd
> parent 1cf0b6019aa3916197eecafe058bd2f3d700d24a
> author David Vrabel <david.vrabel@csr.com> Mon, 30 Jul 2007 19:20:59
> +0100 committer David Vrabel <dvrabel@cantab.net> 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(-)
> 
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index 6589fd6..08f579c 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -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

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

[David Vrabel]

Pierre Ossman wrote:
> On Tue, 31 Jul 2007 16:36:31 +0100
> David Vrabel <david.vrabel@csr.com> wrote:
> 
>> Signed-off-by: David Vrabel <david.vrabel@csr.com>
>>
>> ---
>> commit 51755c3d59be1ba778bef45888f9f5e341dc4af4
>> tree c7bbb562b2d801197eefb619a17c94467c1299cd
>> parent 1cf0b6019aa3916197eecafe058bd2f3d700d24a
>> author David Vrabel <david.vrabel@csr.com> Mon, 30 Jul 2007 19:20:59
>> +0100 committer David Vrabel <dvrabel@cantab.net> 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(-)
>>
>> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
>> index 6589fd6..08f579c 100644
>> --- a/drivers/mmc/core/sdio.c
>> +++ b/drivers/mmc/core/sdio.c
>> @@ -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.

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

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

[Pierre Ossman]

On Mon, 06 Aug 2007 11:14:03 +0100
David Vrabel <david.vrabel@csr.com> 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.

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

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

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
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

sdio: set the functions' block size

[David Vrabel]

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 <david.vrabel@csr.com>

---
commit 6d367fd822cbb2b8089ab7ef83f706f1984ab25b
tree 8c9cc84b4c8c1c8f959abe540aa02f14aa95c51d
parent 51755c3d59be1ba778bef45888f9f5e341dc4af4
author David Vrabel <david.vrabel@csr.com> Mon, 30 Jul 2007 19:58:54 +0100
committer David Vrabel <dvrabel@cantab.net> 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(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index a3ac1aa..f4d40c1 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -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);
 }
 
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index 8aa4666..c16c536 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -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",
diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index cc62ff7..e74e605 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -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
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index e34bc96..bfb0432 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -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

[Pierre Ossman]

On Tue, 31 Jul 2007 16:36:32 +0100
David Vrabel <david.vrabel@csr.com> 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 <david.vrabel@csr.com>
> 

Why a maximum size for the driver?

> diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
> index 8aa4666..c16c536 100644
> --- a/drivers/mmc/core/sdio_cis.c
> +++ b/drivers/mmc/core/sdio_cis.c
> @@ -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",

I guess you aren't working against the latest tree as this code is
already there.

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

> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index cc62ff7..e74e605 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -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);
> +

You also need to check that the host supports that block size.

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

[David Vrabel]

Pierre Ossman wrote:
> On Tue, 31 Jul 2007 16:36:32 +0100
> David Vrabel <david.vrabel@csr.com> 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 <david.vrabel@csr.com>
>>
> 
> 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
-- 
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

[David Vrabel]

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.

Signed-off-by: David Vrabel <david.vrabel@csr.com>

---
commit 1c701a6566f373ede250b51c99216227fd881535
tree 2bbc5005de65bbdc497c8603e8a68486465065dc
parent 6d367fd822cbb2b8089ab7ef83f706f1984ab25b
author David Vrabel <david.vrabel@csr.com> Tue, 31 Jul 2007 11:08:49 +0100
committer David Vrabel <dvrabel@cantab.net> 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(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index e74e605..8fb1880 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -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);
 }
 
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index a91fe41..72590c4 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -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);
 
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index 1d42e4f..e2e74b0 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -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

[Pierre Ossman]

On Tue, 31 Jul 2007 16:36:33 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> 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.
> 
> Signed-off-by: David Vrabel <david.vrabel@csr.com>
> 

*snip*

> @@ -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;

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.

> @@ -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;
>  

Until this function is made complete, I'd like some kind of test that
blksz <= 512 when blocks == 1.

-- 
     -- 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: enhance IO_RW_EXTENDED support

[Pierre Ossman]

On Tue, 31 Jul 2007 16:36:30 +0100
David Vrabel <david.vrabel@csr.com> wrote:

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

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
-- 
     -- 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: enhance IO_RW_EXTENDED support

[David Vrabel]

Pierre Ossman wrote:
> On Tue, 31 Jul 2007 16:36:30 +0100
> David Vrabel <david.vrabel@csr.com> wrote:
> 
>> 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.
>>
> 
> 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).

> 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
-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

Re: sdio: enhance IO_RW_EXTENDED support

[Pierre Ossman]

On Mon, 06 Aug 2007 11:31:19 +0100
David Vrabel <david.vrabel@csr.com> 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>. :)

> > 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
-- 
     -- 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: enhance IO_RW_EXTENDED support

[David Vrabel]

Pierre Ossman wrote:
> On Mon, 06 Aug 2007 11:31:19 +0100
> David Vrabel <david.vrabel@csr.com> 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.

>>> 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. :)

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
-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

Re: sdio: enhance IO_RW_EXTENDED support

[Pierre Ossman]

On Mon, 06 Aug 2007 16:32:40 +0100
David Vrabel <david.vrabel@csr.com> 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.

> > 
> > 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 */
> 
> 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
-- 
     -- 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: enhance IO_RW_EXTENDED support

[Pierre Ossman]

[-- Attachment #1: Type: text/plain, Size: 241 bytes --]

This is essentially what I mean.

-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org

[-- Attachment #2: sdio-block.patch --]
[-- Type: text/x-patch, Size: 10417 bytes --]

commit 8f9fca61fbacd15d1be4215584ed00aa1119d87f
Author: David Vrabel <david.vrabel@csr.com>
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 ]
    
    Signed-off-by: David Vrabel <david.vrabel@csr.com>
    Signed-off-by: Pierre Ossman <drzeus@drzeus.cx>

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 7f08ba5..e49b381 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -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);
+
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 277870c..14bd244 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -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);
 
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index 1d42e4f..e2e74b0 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -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
 
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index 14d9147..fded343 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -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
 

Re: sdio: enhance IO_RW_EXTENDED support

[David Vrabel]

Pierre Ossman wrote:
> 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.

> +/*
> + * 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;

The maximum size for a byte mode transfer is the block size set in the card.

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

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

> +	}

David
-- 
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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-parameterize-sdio-fbr-register-defines.patch --]
[-- Type: text/x-patch, Size: 1767 bytes --]

sdio: parameterize SDIO FBR register defines

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
Index: mmc/drivers/mmc/core/sdio.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio.c	2007-08-06 14:37:30.000000000 +0100
+++ mmc/drivers/mmc/core/sdio.c	2007-08-06 17:30:14.000000000 +0100
@@ -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;
 	}
Index: mmc/drivers/mmc/core/sdio_cis.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_cis.c	2007-08-06 14:37:30.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_cis.c	2007-08-06 17:31:02.000000000 +0100
@@ -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);
Index: mmc/include/linux/mmc/sdio.h
===================================================================
--- mmc.orig/include/linux/mmc/sdio.h	2007-08-06 14:37:35.000000000 +0100
+++ mmc/include/linux/mmc/sdio.h	2007-08-06 17:29:21.000000000 +0100
@@ -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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-set-the-functions-block-size.patch --]
[-- Type: text/x-patch, Size: 3726 bytes --]

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.

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
Index: mmc/drivers/mmc/core/sdio_cis.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_cis.c	2007-08-07 00:38:33.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_cis.c	2007-08-07 00:38:33.000000000 +0100
@@ -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",
Index: mmc/drivers/mmc/core/sdio_io.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_io.c	2007-08-07 00:38:31.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_io.c	2007-08-07 07:17:33.000000000 +0100
@@ -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
Index: mmc/include/linux/mmc/sdio_func.h
===================================================================
--- mmc.orig/include/linux/mmc/sdio_func.h	2007-08-07 00:38:31.000000000 +0100
+++ mmc/include/linux/mmc/sdio_func.h	2007-08-07 07:18:10.000000000 +0100
@@ -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);
 
Index: mmc/drivers/mmc/core/sdio.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio.c	2007-08-07 00:38:33.000000000 +0100
+++ mmc/drivers/mmc/core/sdio.c	2007-08-07 07:17:29.000000000 +0100
@@ -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

[Pierre Ossman]

On Tue, 07 Aug 2007 13:54:32 +0100
David Vrabel <david.vrabel@csr.com> wrote:

>  
> Index: mmc/drivers/mmc/core/sdio.c
> ===================================================================
> --- mmc.orig/drivers/mmc/core/sdio.c	2007-08-07
> 00:38:33.000000000 +0100 +++ mmc/drivers/mmc/core/sdio.c
> 2007-08-07 07:17:29.000000000 +0100 @@ -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;

This is probably better done in the sdio_enable_func().

We also need to check that the card actually supports block io.

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

[David Vrabel]

Pierre Ossman wrote:
> On Tue, 07 Aug 2007 13:54:32 +0100
> David Vrabel <david.vrabel@csr.com> wrote:
> 
>>  
>> Index: mmc/drivers/mmc/core/sdio.c
>> ===================================================================
>> --- mmc.orig/drivers/mmc/core/sdio.c	2007-08-07
>> 00:38:33.000000000 +0100 +++ mmc/drivers/mmc/core/sdio.c
>> 2007-08-07 07:17:29.000000000 +0100 @@ -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;
> 
> 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
-- 
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

[Pierre Ossman]

On Tue, 07 Aug 2007 18:20:26 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> Pierre Ossman wrote:
> > On Tue, 07 Aug 2007 13:54:32 +0100
> > David Vrabel <david.vrabel@csr.com> wrote:
> > 
> >>  
> >> Index: mmc/drivers/mmc/core/sdio.c
> >> ===================================================================
> >> --- mmc.orig/drivers/mmc/core/sdio.c	2007-08-07
> >> 00:38:33.000000000 +0100 +++ mmc/drivers/mmc/core/sdio.c
> >> 2007-08-07 07:17:29.000000000 +0100 @@ -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;
> > 
> > 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?

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

[David Vrabel]

Pierre Ossman wrote:
> 
>> 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
-- 
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

[Pierre Ossman]

On Wed, 08 Aug 2007 10:46:24 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> Pierre Ossman wrote:
> > 
> >> 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. :)

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

[David Vrabel]

Pierre Ossman wrote:
> On Wed, 08 Aug 2007 10:46:24 +0100
> David Vrabel <david.vrabel@csr.com> wrote:
> 
>> Pierre Ossman wrote:
>>>> 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
-- 
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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-extend-sdio-readsb-and-friends-to-handle-any-length-of-buffer.patch --]
[-- Type: text/x-patch, Size: 6864 bytes --]

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.

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
Index: mmc/drivers/mmc/core/sdio_io.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_io.c	2007-08-07 07:27:31.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_io.c	2007-08-07 07:35:02.000000000 +0100
@@ -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);
 }
 
Index: mmc/drivers/mmc/core/sdio_ops.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_ops.c	2007-08-07 07:27:27.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_ops.c	2007-08-07 07:27:31.000000000 +0100
@@ -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);
 
Index: mmc/drivers/mmc/core/sdio_ops.h
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_ops.h	2007-08-07 07:27:27.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_ops.h	2007-08-07 07:27:31.000000000 +0100
@@ -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

[Pierre Ossman]

On Tue, 07 Aug 2007 13:55:12 +0100
David Vrabel <david.vrabel@csr.com> wrote:

> 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.
> 
> Signed-off-by: David Vrabel <david.vrabel@csr.com>
> ---
> Index: mmc/drivers/mmc/core/sdio_io.c
> ===================================================================
> --- mmc.orig/drivers/mmc/core/sdio_io.c	2007-08-07
> 07:27:31.000000000 +0100 +++ mmc/drivers/mmc/core/sdio_io.c
> 2007-08-07 07:35:02.000000000 +0100 @@ -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");
> +	}
> +

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?

>  	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;
> +

We need to check support for block transfers here as well.

Also, the parameter fn is redundant.

> @@ -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
-- 
     -- 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

[Pierre Ossman]

Here'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.

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index ca1d4b2..6ead36a 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -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);
 }
 
diff --git a/drivers/mmc/core/sdio_cis.c b/drivers/mmc/core/sdio_cis.c
index fffbc5a..20fdd1d 100644
--- a/drivers/mmc/core/sdio_cis.c
+++ b/drivers/mmc/core/sdio_cis.c
@@ -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",
diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 7f08ba5..46ada64 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -202,6 +202,115 @@ 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 {
+		blksz = func->max_blksz;
+		if (blksz > host->max_blk_size)
+			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;
+
+	while (remainder >= blksz) {
+		unsigned blocks;
+
+		blocks = remainder / blksz;
+
+		if (blocks > 511)
+			blocks = 511;
+		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;
+	}
+
+	/*
+	 * Because the block size is smaller than both the card
+	 * maximum size and 512, then we know we can fit the remainder
+	 * into a byte transfer.
+	 */
+	if (remainder) {
+		/*
+		 * 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, remainder);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  *	sdio_memcpy_fromio - read a chunk of memory from a SDIO function
  *	@func: SDIO function to access
@@ -209,14 +318,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 +337,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 +354,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 +373,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 +498,30 @@ 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->max_blksz)
+		return -EINVAL;
+	if (size > func->card->host->max_blk_size)
+		return -EINVAL;
+	if (size > 512)
+		return -EINVAL;
+
+	func->forced_blksz = size;
+
+	return 0;
+}
+
+EXPORT_SYMBOL_GPL(sdio_force_block_size);
+
diff --git a/drivers/mmc/core/sdio_ops.c b/drivers/mmc/core/sdio_ops.c
index 277870c..b5bbcfc 100644
--- a/drivers/mmc/core/sdio_ops.c
+++ b/drivers/mmc/core/sdio_ops.c
@@ -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);
 
diff --git a/drivers/mmc/core/sdio_ops.h b/drivers/mmc/core/sdio_ops.h
index 1d42e4f..e2e74b0 100644
--- a/drivers/mmc/core/sdio_ops.h
+++ b/drivers/mmc/core/sdio_ops.h
@@ -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
 
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index 14d9147..8bdbb3b 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -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
 

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

[David Vrabel]

Pierre Ossman wrote:
> +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)
> +			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.
-- 
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

[Pierre Ossman]

On Wed, 08 Aug 2007 11:19:33 +0100
David Vrabel <david.vrabel@csr.com> 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.

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

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

[David Vrabel]

Pierre Ossman wrote:
> On Wed, 08 Aug 2007 11:19:33 +0100
> David Vrabel <david.vrabel@csr.com> 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.

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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-add-fbr-base-macro.patch --]
[-- Type: text/x-patch, Size: 1755 bytes --]

sdio: add SDIO_FBR_BASE(f) macro

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
Index: mmc/drivers/mmc/core/sdio.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio.c	2007-08-06 14:37:30.000000000 +0100
+++ mmc/drivers/mmc/core/sdio.c	2007-08-06 17:30:14.000000000 +0100
@@ -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;
 	}
Index: mmc/drivers/mmc/core/sdio_cis.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_cis.c	2007-08-06 14:37:30.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_cis.c	2007-08-06 17:31:02.000000000 +0100
@@ -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);
Index: mmc/include/linux/mmc/sdio.h
===================================================================
--- mmc.orig/include/linux/mmc/sdio.h	2007-08-06 14:37:35.000000000 +0100
+++ mmc/include/linux/mmc/sdio.h	2007-08-06 17:29:21.000000000 +0100
@@ -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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-set-the-functions-block-size.patch --]
[-- Type: text/x-patch, Size: 4888 bytes --]

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.

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
Index: mmc/drivers/mmc/core/sdio_cis.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_cis.c	2007-08-08 13:29:56.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_cis.c	2007-08-08 13:30:21.000000000 +0100
@@ -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",
Index: mmc/drivers/mmc/core/sdio_io.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_io.c	2007-08-08 13:29:47.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_io.c	2007-08-08 13:34:09.000000000 +0100
@@ -145,6 +145,55 @@
 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 or 0 to use the default.
+ *
+ *	The default block size is the largest supported by both the function
+ *	and the host, with a maximum of 512 to ensure that arbitrarily sized
+ *	data transfer use the optimal (least) number of commands.
+ *
+ *	A driver may call this to override the default block size set by the
+ *	core. This can be used to set a block size greater than the maximum
+ *	that reported by the card; it is the driver's responsibility to ensure
+ *	it uses a value that the card supports.
+ *
+ *	Returns 0 on success, -EINVAL if the host does not support the
+ *	requested block size, or -EIO (etc.) if one of the resultant FBR block
+ *	size register writes failed.
+ *
+ */
+int sdio_set_block_size(struct sdio_func *func, unsigned blksz)
+{
+	int ret;
+
+	if (blksz > func->card->host->max_blk_size)
+		return -EINVAL;
+
+	if (blksz == 0) {
+		blksz = min(min(
+			func->max_blksize,
+			func->card->host->max_blk_size),
+			512u);
+	}
+
+	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
Index: mmc/include/linux/mmc/sdio_func.h
===================================================================
--- mmc.orig/include/linux/mmc/sdio_func.h	2007-08-08 13:29:47.000000000 +0100
+++ mmc/include/linux/mmc/sdio_func.h	2007-08-08 13:30:21.000000000 +0100
@@ -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);
 
Index: mmc/drivers/mmc/core/sdio_bus.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_bus.c	2007-08-08 13:29:47.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_bus.c	2007-08-08 13:30:21.000000000 +0100
@@ -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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-extend-sdio-readsb-and-friends-to-handle-any-length-of-buffer.patch --]
[-- Type: text/x-patch, Size: 7004 bytes --]

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.

Signed-off-by: David Vrabel <david.vrabel@csr.com>
---
Index: mmc/drivers/mmc/core/sdio_io.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_io.c	2007-08-08 13:34:09.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_io.c	2007-08-08 13:34:17.000000000 +0100
@@ -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);
Index: mmc/drivers/mmc/core/sdio_ops.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_ops.c	2007-08-08 13:29:47.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_ops.c	2007-08-08 13:34:17.000000000 +0100
@@ -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);
 
Index: mmc/drivers/mmc/core/sdio_ops.h
===================================================================
--- mmc.orig/drivers/mmc/core/sdio_ops.h	2007-08-08 13:29:47.000000000 +0100
+++ mmc/drivers/mmc/core/sdio_ops.h	2007-08-08 13:34:17.000000000 +0100
@@ -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

[Pierre Ossman]

On Wed, 08 Aug 2007 14:22:20 +0100
David Vrabel <david.vrabel@csr.com> 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.
> 
> 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.

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

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

[David Vrabel]

[-- Attachment #1: Type: text/plain, Size: 152 bytes --]

-- 
David Vrabel, Software Engineer, Drivers group  Tel: +44 (0)1223 692562
CSR plc, Churchill House, Cambridge Business Park, Cowley Road, CB4 0WZ


.

[-- Attachment #2: sdio-disable-cd-resistor.patch --]
[-- Type: text/x-patch, Size: 2174 bytes --]

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 <david.vrabel@csr.com
---
Index: mmc/drivers/mmc/core/sdio.c
===================================================================
--- mmc.orig/drivers/mmc/core/sdio.c	2007-08-07 07:27:31.000000000 +0100
+++ mmc/drivers/mmc/core/sdio.c	2007-08-07 07:30:58.000000000 +0100
@@ -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);
Index: mmc/include/linux/mmc/sdio.h
===================================================================
--- mmc.orig/include/linux/mmc/sdio.h	2007-08-07 07:27:30.000000000 +0100
+++ mmc/include/linux/mmc/sdio.h	2007-08-07 07:27:31.000000000 +0100
@@ -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

[Pierre Ossman]

On Tue, 07 Aug 2007 13:55:59 +0100
David Vrabel <david.vrabel@csr.com> 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 <david.vrabel@csr.com>

I'm not sure about this. I've seen several hosts which lack a
mechanical detect switch.

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

[David Vrabel]

Pierre Ossman wrote:
> On Tue, 07 Aug 2007 13:55:59 +0100
> David Vrabel <david.vrabel@csr.com> 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 <david.vrabel@csr.com>
> 
> 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
-- 
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

[Pierre Ossman]

On Tue, 07 Aug 2007 15:46:36 +0100
David Vrabel <david.vrabel@csr.com> 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.

> 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. :/

-- 
     -- 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: enhance IO_RW_EXTENDED support

[Pierre Ossman]

On Tue, 07 Aug 2007 13:51:39 +0100
David Vrabel <david.vrabel@csr.com> 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.

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

> > +		/* 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.
> 

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
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  PulseAudio, core developer          http://pulseaudio.org
  rdesktop, core developer          http://www.rdesktop.org