2007-08-04 13:30:50

by Pierre Ossman

[permalink] [raw]
Subject: Re: sdio: set the functions' block size

On Tue, 31 Jul 2007 16:36:32 +0100
David Vrabel <[email protected]> wrote:

> Prior to calling the driver's probe(), set the functions' block size
> to the largest that's supported by both the card and the driver.
>
> Signed-off-by: David Vrabel <[email protected]>
>

Why a maximum size for the driver?

> 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


2007-08-06 10:52:21

by David Vrabel

[permalink] [raw]
Subject: Re: sdio: set the functions' block size

Pierre Ossman wrote:
> On Tue, 31 Jul 2007 16:36:32 +0100
> David Vrabel <[email protected]> wrote:
>
>> Prior to calling the driver's probe(), set the functions' block size
>> to the largest that's supported by both the card and the driver.
>>
>> Signed-off-by: David Vrabel <[email protected]>
>>
>
> Why a maximum size for the driver?

For efficient use of the bus it is useful to pad transfers to a multiple
of the block size. I reckoned it was potentially beneficial for drivers
to use smaller blocks so less padding was required. However, after
looking at some figures the per block overhead (28 clocks for a write)
really does limit the transfer sizes where small block sizes is beneficial.

I'll remove this. Drivers can still override the block size by calling
sdio_set_block_size() directly.

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


.