[U-Boot] [PATCH v3 7/9] dfu:mmc: raw data write fix

[Mateusz Zalega <m.zalega@samsung.com>]

On 01/10/14 06:03, Jaehoon Chung wrote:
> This patch should be separated with dfu and mmc.

ACK, because we're going to remove the bl_len assertion, see below.

> On 01/09/2014 11:31 PM, Mateusz Zalega wrote:
>> When user attempted to perform a raw write using DFU (vide
>> dfu_fill_entity_mmc) with MMC interface not initialized before,
>> get_mmc_blk_size() reported invalid (zero) block size - it wasn't
>> possible to write ie. a new u-boot image.
>>
>> This commit fixes that by initializing device in get_mmc_blk_size() when
>> needed.
>>
>> Tested on Samsung Goni.
>>
>> v2 changes:
>> - code cleanup
>> - minor dfu_alt_info format change
>>
>> v3 changes:
>> - moved invalid block length check to mmc core
>> - removed redundant 'has_init' check
>>
>> Change-Id: Icb50bb9f805a9a78848acd19f682fad474cb9082
>> Signed-off-by: Mateusz Zalega <m.zalega@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>> Cc: Minkyu Kang <mk7.kang@samsung.com>
>> ---
>>  drivers/dfu/dfu_mmc.c        | 106 ++++++++++++++++++++++++++-----------------
>>  drivers/mmc/mmc.c            |  13 ++++--
>>  include/configs/am335x_evm.h |   8 ++--
>>  include/configs/trats.h      |   2 +-
>>  include/configs/trats2.h     |   2 +-
>>  include/dfu.h                |   5 --
>>  6 files changed, 80 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
(...)
>> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
>> index e1461a9..f2fa230 100644
>> --- a/drivers/mmc/mmc.c
>> +++ b/drivers/mmc/mmc.c
>> @@ -15,6 +15,7 @@
>>  #include <malloc.h>
>>  #include <linux/list.h>
>>  #include <div64.h>
>> +#include <errno.h>
>>  #include "mmc_private.h"
>>  
>>  /* Set block count limit because of 16 bit register limit on some hardware*/
>> @@ -1266,17 +1267,23 @@ static int mmc_complete_init(struct mmc *mmc)
>>  
>>  int mmc_init(struct mmc *mmc)
>>  {
>> +	if (mmc->has_init)
>> +		return 0;
>> +
> What difference before?

It doesn't have to go through get_timer(). The effect, although
negligible, saves some cycles.

>>  	int err = IN_PROGRESS;
>>  	unsigned start = get_timer(0);
>>  
>> -	if (mmc->has_init)
>> -		return 0;
>>  	if (!mmc->init_in_progress)
>>  		err = mmc_start_init(mmc);
>> -
> Need not to change.
> 
>>  	if (!err || err == IN_PROGRESS)
>>  		err = mmc_complete_init(mmc);
>> +
> Ditto.

NAK, it improves code readability.

>>  	debug("%s: %d, time %lu\n", __func__, err, get_timer(start));
>> +
>> +	if (!mmc->read_bl_len || !mmc->write_bl_len) {
>> +		error("invalid block length\n");
>> +		return -ENODEV;
>> +	}
> I know mmc->read_bl_len and write_bl_len is set at init time.
> Why this condition needs?

I added it as a countermeasure after fixing the bug and mistook its
purpose when writing a late update to this patch, my bad.

Given the circumstances it might be a sound assertion, but we shouldn't
clobber the codebase that we aim to optimize for size, should we.

ACK, will remove.

>>  	return err;
>>  }
>>  
>> diff --git a/include/configs/am335x_evm.h b/include/configs/am335x_evm.h
>> index 8af4d6a..d76962f 100644
>> --- a/include/configs/am335x_evm.h
>> +++ b/include/configs/am335x_evm.h
>> @@ -312,10 +312,10 @@
>>  	"boot part 0 1;" \
>>  	"rootfs part 0 2;" \
>>  	"MLO fat 0 1;" \
>> -	"MLO.raw mmc 100 100;" \
>> -	"u-boot.img.raw mmc 300 400;" \
>> -	"spl-os-args.raw mmc 80 80;" \
>> -	"spl-os-image.raw mmc 900 2000;" \
>> +	"MLO.raw mmc 0x100 0x100;" \
>> +	"u-boot.img.raw mmc 0x300 0x400;" \
>> +	"spl-os-args.raw mmc 0x80 0x80;" \
>> +	"spl-os-image.raw mmc 0x900 0x2000;" \
>>  	"spl-os-args fat 0 1;" \
>>  	"spl-os-image fat 0 1;" \
>>  	"u-boot.img fat 0 1;" \
>> diff --git a/include/configs/trats.h b/include/configs/trats.h
>> index 6cd15c2..ed3b278 100644
>> --- a/include/configs/trats.h
>> +++ b/include/configs/trats.h
>> @@ -140,7 +140,7 @@
>>  	"name="PARTS_UMS",size=-,uuid=${uuid_gpt_"PARTS_UMS"}\0" \
>>  
>>  #define CONFIG_DFU_ALT \
>> -	"u-boot mmc 80 400;" \
>> +	"u-boot raw 0x80 0x400;" \
>>  	"uImage ext4 0 2;" \
>>  	"exynos4210-trats.dtb ext4 0 2;" \
>>  	""PARTS_ROOT" part 0 5\0"
>> diff --git a/include/configs/trats2.h b/include/configs/trats2.h
>> index 5d86a3d..a22be63 100644
>> --- a/include/configs/trats2.h
>> +++ b/include/configs/trats2.h
>> @@ -169,7 +169,7 @@
>>  	"name="PARTS_UMS",size=-,uuid=${uuid_gpt_"PARTS_UMS"}\0" \
>>  
>>  #define CONFIG_DFU_ALT \
>> -	"u-boot mmc 80 800;" \
>> +	"u-boot mmc 0x80 0x800;" \
> 
> u-boot mmc? u-boot raw? what's correct?

raw - ACK

>>  	"uImage ext4 0 2;" \
>>  	"exynos4412-trats2.dtb ext4 0 2;" \
>>  	""PARTS_ROOT" part 0 5\0"
>> diff --git a/include/dfu.h b/include/dfu.h
>> index f973426..f2e83db 100644
>> --- a/include/dfu.h
>> +++ b/include/dfu.h
>> @@ -64,11 +64,6 @@ struct ram_internal_data {
>>  	unsigned int	size;
>>  };
>>  
>> -static inline unsigned int get_mmc_blk_size(int dev)
>> -{
>> -	return find_mmc_device(dev)->read_bl_len;
>> -}
>> -
>>  #define DFU_NAME_SIZE			32
>>  #define DFU_CMD_BUF_SIZE		128
>>  #ifndef CONFIG_SYS_DFU_DATA_BUF_SIZE
>>
> 
> 


-- 
Mateusz Zalega
Samsung R&D Institute Poland