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.zal...@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.p...@samsung.com> >> Reviewed-by: Lukasz Majewski <l.majew...@samsung.com> >> Cc: Minkyu Kang <mk7.k...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot