[U-Boot] [PATCH 3/7] mmc: Enhance erase handling procedure

[Pantelis Antoniou]

Hi Haijun,

On Nov 5, 2013, at 8:23 AM, Haijun Zhang wrote:

> Erass sequence:
> 1. check if erase command is support by card. If not return.
> 2. Check the erase range to see if it was aligned. The min erase size
> should be one erase group. SD card it was one block(512), mmc card
> it should be one erase group.
> 3. If not, aligned the erase rang according to the erase group size.
> 4. Send erase command to erase card once one erase group.
> 5. If it was SD card, erase with arg 0x00000000 should be send.
>   else if support secure feature, erase with arg 0x80000000(Spec eMMC 4.41).
>   else erase with arg 0x00000000.(Trim and discard is ingnored here)
> 6. Check card status after erase.
> 

Can I get a few lines about what this patch implements before explaining
the algorithm?

Something like

	"This patch enhances the currently implemented erase procedure in u-boot,
	which has the following problems/missing features..."

> Signed-off-by: Haijun Zhang <haijun.zhang at freescale.com>
> ---
> drivers/mmc/mmc_write.c | 70 ++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 60 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mmc/mmc_write.c b/drivers/mmc/mmc_write.c
> index aa2fdef..f2e9baf 100644
> --- a/drivers/mmc/mmc_write.c
> +++ b/drivers/mmc/mmc_write.c
> @@ -17,6 +17,10 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> 	struct mmc_cmd cmd;
> 	ulong end;
> 	int err, start_cmd, end_cmd;
> +	uint arg = 0;
> +
> +	if (!(mmc->cmdclass & CCC_ERASE))
> +		return NOT_SUPPORT;
> 
> 	if (mmc->high_capacity) {
> 		end = start + blkcnt - 1;
> @@ -48,8 +52,17 @@ static ulong mmc_erase_t(struct mmc *mmc, ulong start, lbaint_t blkcnt)
> 	if (err)
> 		goto err_out;
> 
> +	arg = MMC_ERASE_ARG;
> +
> +	if (mmc->sec_feature_support & EXT_CSD_SEC_ER_EN)
> +		arg = MMC_SECURE_ERASE_ARG;
> +
> +	/* SD card only support %MMC_ERASE_ARG */
> +	if (IS_SD(mmc))
> +		arg = MMC_ERASE_ARG;
> +

Shouldn't that be:

	/* SD card only support %MMC_ERASE_ARG */
	if (!IS_SD(mmc) && 
		(mmc->sec_feature_support & EXT_CSD_SEC_ER_EN))
		arg = MMC_SECURE_ERASE_ARG;
	else
		arg = MMC_ERASE_ARG;

> 	cmd.cmdidx = MMC_CMD_ERASE;
> -	cmd.cmdarg = SECURE_ERASE;
> +	cmd.cmdarg = arg;
> 	cmd.resp_type = MMC_RSP_R1b;
> 
> 	err = mmc_send_cmd(mmc, &cmd, NULL);
> @@ -69,24 +82,61 @@ unsigned long mmc_berase(int dev_num, lbaint_t start, lbaint_t blkcnt)
> 	struct mmc *mmc = find_mmc_device(dev_num);
> 	lbaint_t blk = 0, blk_r = 0;
> 	int timeout = 1000;
> +	int res;
> +	bool align = false;
> 
> 	if (!mmc)
> 		return -1;
> 
> +	if (!(mmc->cmdclass & CCC_ERASE)) {
> +		printf("\nErase operation is not support by card\n");
> +		return NOT_SUPPORT;
> +	}
> +
> 	if ((start % mmc->erase_grp_size) || (blkcnt % mmc->erase_grp_size))
> +		align = true;
> +

^ The use of align here is confusing.

AFAIKT the meaning is reversed.

align actually means not_aligned.
 
> +	res = start % mmc->erase_grp_size;
> +	if (res) {
> +		res = mmc->erase_grp_size - res;
> +		start += res;
> +		if (blkcnt > res)
> +			blkcnt -= res;
> +		else {
> +			printf("\nErase size smaller than Erase group "
> +				"size [0x%x] is not support by the device.\n",
> +				mmc->erase_grp_size);
> +			return NOT_SUPPORT;
> +		}
> +	}
> +
> +	res = blkcnt % mmc->erase_grp_size;
> +	if (res)
> +		blkcnt -= res;
> +
> +	if (!blkcnt) {
> +		printf("\nErase size smaller than Erase group size [0x%x] "
> +			"is not support by the device.\n",
> +			mmc->erase_grp_size);
> +		return NOT_SUPPORT;
> +	}
> +
> +	if (align)
> 		printf("\n\nCaution! Your devices Erase group is 0x%x\n"
> -		       "The erase range would be change to "
> -		       "0x" LBAF "~0x" LBAF "\n\n",
> -		       mmc->erase_grp_size, start & ~(mmc->erase_grp_size - 1),
> -		       ((start + blkcnt + mmc->erase_grp_size)
> -		       & ~(mmc->erase_grp_size - 1)) - 1);
> +			"The erase range would be change to "
> +			"0x" LBAF "~0x" LBAF "\n\n",
> +			mmc->erase_grp_size, start, start + blkcnt);
> +
> 
> 	while (blk < blkcnt) {
> -		blk_r = ((blkcnt - blk) > mmc->erase_grp_size) ?
> -			mmc->erase_grp_size : (blkcnt - blk);
> +		if ((blkcnt - blk) >= mmc->erase_grp_size)
> +			blk_r = mmc->erase_grp_size;
> 		err = mmc_erase_t(mmc, start + blk, blk_r);
> -		if (err)
> -			break;
> +		if (err) {
> +			printf("\nErase range from 0x" LBAF "~0x" LBAF
> +				" Failed\n", start + blk, blkcnt);
> +			return COMM_ERR;
> +		}
> 
> 		blk += blk_r;
> 
> -- 
> 1.8.4
> 
> 

Regards

-- Pantelis