[U-Boot] [PATCH] mmc: seperate block number into small parts for multi-write cmd

[Reinhard Meyer]

Dear Lei Wen,

> Ok, I am also fine with not include the 512KiB restriction.
> So we comes to a conclusion that we still use v1 patch, but cut the
> 512KiB limitation?

Considering the comments that were given to that one, yes.

> As mmc host limitation, the max number of block in one go
> should be limited to 65535, and the max buffer size should
> not excceed 512k bytes.

Better reads somehow like this:
Since some hardware has a 16 bit block counter, split the multiple block
write into a maximum of (2**16 - 1) blocks to be written at one go.

> 
> Signed-off-by: Lei Wen <leiwen at marvell.com>
> ---
>  drivers/mmc/mmc.c |   56 +++++++++++++++++++++++++++++++++-------------------
>  1 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 94d1ac2..ea398a5 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -77,29 +77,16 @@ struct mmc *find_mmc_device(int dev_num)
>  	return NULL;
>  }
>  
> -static ulong
> -mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
> +int mmc_write_block(struct mmc *mmc, ulong *src, ulong start, uint blocknum)
should be:
static ulong mmc_write_blocks(struct_mmc *mmc, ulong start, lbaint_t blkcnt, const void*src)
(keep parameter types and attributes (const) intact and similar to mmc_bwrite)

>  {
>  	struct mmc_cmd cmd;
>  	struct mmc_data data;
> -	int err;
> -	int stoperr = 0;
> -	struct mmc *mmc = find_mmc_device(dev_num);
> -	int blklen;
> -
> -	if (!mmc)
> -		return -1;
> -
> +	int blklen, err;
>  	blklen = mmc->write_bl_len;
>  
> -	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
> -
> -	if (err) {
> -		printf("set write bl len failed\n\r");
> -		return err;
> -	}
> -
> -	if (blkcnt > 1)
> +	BUG_ON(blklen * blocknum > 524288);
> +	BUG_ON(blocknum > 65535);

remove the BUG_ONs

> +	if (blocknum > 1)
>  		cmd.cmdidx = MMC_CMD_WRITE_MULTIPLE_BLOCK;
>  	else
>  		cmd.cmdidx = MMC_CMD_WRITE_SINGLE_BLOCK;
> @@ -113,7 +100,7 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  	cmd.flags = 0;
>  
>  	data.src = src;
> -	data.blocks = blkcnt;
> +	data.blocks = blocknum;

that change not necessary when above parameters are kept.

>  	data.blocksize = blklen;
>  	data.flags = MMC_DATA_WRITE;
>  
> @@ -124,14 +111,41 @@ mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
>  		return err;
>  	}
>  
> -	if (blkcnt > 1) {
> +	if (blocknum > 1) {

same here.

>  		cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
>  		cmd.cmdarg = 0;
>  		cmd.resp_type = MMC_RSP_R1b;
>  		cmd.flags = 0;
> -		stoperr = mmc_send_cmd(mmc, &cmd, NULL);
> +		mmc_send_cmd(mmc, &cmd, NULL);

should be not keep checking the result for errors?

>  	}
>  
> +	return blocknum;
> +}
> +



> +static ulong
> +mmc_bwrite(int dev_num, ulong start, lbaint_t blkcnt, const void*src)
> +{
> +	int err;
> +	struct mmc *mmc = find_mmc_device(dev_num);
> +	unsigned int max;

keep related variables the same type: lbainit_t

> +	lbaint_t tmp = blkcnt, cur;

tmp is better named: blocks_todo
put cur into the loop

> +
> +	if (!mmc)
> +		return -1;
> +
> +	err = mmc_set_blocklen(mmc, mmc->write_bl_len);
> +	if (err) {
> +		printf("set write bl len failed\n\r");
> +		return err;
> +	}
> +
> +	max = 524288 / mmc->write_bl_len;
remove
> +	do {
> +		cur = (tmp > max) ? max : tmp;
	lbaint_t chunk = blocks_todo;
	if (chunk > 65535)
		chunk = 65535;
> +		tmp -= cur;
do that at end of loop
> +		if(mmc_write_block(mmc, src, start, cur) != cur)
		if (mmc_write_blocks(mmc, start, chunk, src) != chunk)
> +			return -1;

I think src, start should be adjusted, too...
	src += chunk * mmc->write_bl_len;
	start += chunk;
	blocks_todo -= chunk;
> +	} while (blocks_todo > 0);
>  	return blkcnt;
>  }
>  

could the functions be reordered such that diff does not mess up so much?
It would be easier to read if the really new function would not be diffed
with the existing function and the modified existing function not seen as
a new one.

I had similar issues with one of my patches and reordered the functions so
diff did "see" it the way it should be... even if that requires a forward
declaration.

Best Regards
Reinhard