[U-Boot] [PATCH 0/2] New abstraction layer for handling MMC and FAT commands
New abstraction layer for performing MMC and FAT operations has been added. It is necessary for using MMC and FAT operations from other subsystems without the need to call "sprintf" and "run_command" afterwards.
MMC/FAT function call allows for better type checking during compilation time. For FAT, common code has been encapsulated to a separate function.
Lukasz Majewski (2): mmc: New abstract function (do_mmcops_safe) for MMC subsystem fat: New abstract function (do_fat_safe) for FAT subsystem
common/cmd_fat.c | 89 +++++++++++++++++++++++++++++++----------------------- common/cmd_mmc.c | 69 ++++++++++++++++++++++------------------- common/env_fat.c | 20 +----------- include/fat.h | 7 ++++ include/mmc.h | 9 +++++ 5 files changed, 106 insertions(+), 88 deletions(-)
The new do_mmcops_safe function has been added to the mmc subsystem. It is necessary to decouple the core mmc read/write operation from code responsible for parsing user input.
The do_mmcops_safe function shall be used from other subsystems to read/write data to MMC devices.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com Cc: Andy Fleming afleming@gmail.com --- common/cmd_mmc.c | 69 +++++++++++++++++++++++++++++------------------------- include/mmc.h | 9 +++++++ 2 files changed, 46 insertions(+), 32 deletions(-)
diff --git a/common/cmd_mmc.c b/common/cmd_mmc.c index 750509d..60e6873 100644 --- a/common/cmd_mmc.c +++ b/common/cmd_mmc.c @@ -87,12 +87,6 @@ U_BOOT_CMD( ); #else /* !CONFIG_GENERIC_MMC */
-enum mmc_state { - MMC_INVALID, - MMC_READ, - MMC_WRITE, - MMC_ERASE, -}; static void print_mmcinfo(struct mmc *mmc) { printf("Device: %s\n", mmc->name); @@ -148,7 +142,42 @@ U_BOOT_CMD( "" );
-int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) +u32 do_mmcops_safe(enum mmc_state state, int curr_device, u32 blk, + u32 cnt, void *addr) +{ + struct mmc *mmc = find_mmc_device(curr_device); + u32 n = 0; + + if (!mmc) { + printf("no mmc device at slot %x\n", curr_device); + return 1; + } + + mmc_init(mmc); + + switch (state) { + case MMC_READ: + n = mmc->block_dev.block_read(curr_device, blk, + cnt, addr); + /* flush cache after read */ + flush_cache((ulong)addr, cnt * 512); /* FIXME */ + break; + case MMC_WRITE: + n = mmc->block_dev.block_write(curr_device, blk, + cnt, addr); + break; + case MMC_ERASE: + n = mmc->block_dev.block_erase(curr_device, blk, cnt); + break; + default: + BUG(); + } + + return n; +} + +int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, + char * const argv[]) { enum mmc_state state;
@@ -261,7 +290,6 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) state = MMC_INVALID;
if (state != MMC_INVALID) { - struct mmc *mmc = find_mmc_device(curr_device); int idx = 2; u32 blk, cnt, n; void *addr; @@ -274,33 +302,10 @@ int do_mmcops(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) blk = simple_strtoul(argv[idx], NULL, 16); cnt = simple_strtoul(argv[idx + 1], NULL, 16);
- if (!mmc) { - printf("no mmc device at slot %x\n", curr_device); - return 1; - } - printf("\nMMC %s: dev # %d, block # %d, count %d ... ", argv[1], curr_device, blk, cnt);
- mmc_init(mmc); - - switch (state) { - case MMC_READ: - n = mmc->block_dev.block_read(curr_device, blk, - cnt, addr); - /* flush cache after read */ - flush_cache((ulong)addr, cnt * 512); /* FIXME */ - break; - case MMC_WRITE: - n = mmc->block_dev.block_write(curr_device, blk, - cnt, addr); - break; - case MMC_ERASE: - n = mmc->block_dev.block_erase(curr_device, blk, cnt); - break; - default: - BUG(); - } + n = do_mmcops_safe(state, curr_device, blk, cnt, addr);
printf("%d blocks %s: %s\n", n, argv[1], (n == cnt) ? "OK" : "ERROR"); diff --git a/include/mmc.h b/include/mmc.h index 2305986..5686e1b 100644 --- a/include/mmc.h +++ b/include/mmc.h @@ -199,6 +199,13 @@ #define PART_ACCESS_MASK (0x7) #define PART_SUPPORT (0x1)
+enum mmc_state { + MMC_INVALID, + MMC_READ, + MMC_WRITE, + MMC_ERASE, +}; + struct mmc_cid { unsigned long psn; unsigned short oid; @@ -274,6 +281,8 @@ int board_mmc_getcd(struct mmc *mmc); int mmc_switch_part(int dev_num, unsigned int part_num); int mmc_getcd(struct mmc *mmc);
+u32 do_mmcops_safe(enum mmc_state state, int curr_device, u32 blk, + u32 cnt, void *addr); #ifdef CONFIG_GENERIC_MMC #define mmc_host_is_spi(mmc) ((mmc)->host_caps & MMC_MODE_SPI) struct mmc *mmc_spi_init(uint bus, uint cs, uint speed, uint mode);
The new do_fat_safe function has been added to the FAT handling command. It is necessary to decouple the core fat command read/write operation from code responsible for parsing user input.
The do_fat_safe function shall be used from other subsystems willing to read/write data to FAT file system.
Due to code reorganization a common code has been removed from env_fat.c.
Signed-off-by: Lukasz Majewski l.majewski@samsung.com Signed-off-by: Kyungmin Park kyungmin.park@samsung.com --- common/cmd_fat.c | 89 +++++++++++++++++++++++++++++++----------------------- common/env_fat.c | 20 +----------- include/fat.h | 7 ++++ 3 files changed, 60 insertions(+), 56 deletions(-)
diff --git a/common/cmd_fat.c b/common/cmd_fat.c index 559a16d..a197393 100644 --- a/common/cmd_fat.c +++ b/common/cmd_fat.c @@ -32,6 +32,48 @@ #include <part.h> #include <fat.h>
+long do_fat_safe(enum fat_op op, char *iname, char *file, int dev, int part, + void *addr, unsigned long count) +{ + block_dev_desc_t *dev_desc = NULL; + long size = 0; + + dev_desc = get_dev(iname, dev); + if (dev_desc == NULL) { + puts("\n** Invalid boot device **\n"); + return -1; + } + + if (fat_register_device(dev_desc, part) != 0) { + printf("\n** Unable to use %s %d:%d for fat%s **\n", + iname, dev, part, op == FAT_READ ? "load" : "write"); + return -1; + } + + switch (op) { + case FAT_READ: + size = file_fat_read(file, addr, count); + break; +#ifdef CONFIG_FAT_WRITE + case FAT_WRITE: + size = file_fat_write(file, addr, count); + break; +#endif + default: + puts("Operation not supported!\n"); + return -1; + } + + if (size == -1) { + printf("\n** Unable to %s "%s" from %s %d:%d **\n", + op == FAT_READ ? "read" : "write", + file, iname, dev, part); + return -1; + } + printf("%ld bytes %s\n", size, op == FAT_READ ? "read" : "written"); + + return size; +}
int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { @@ -39,7 +81,6 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) unsigned long offset; unsigned long count; char buf [12]; - block_dev_desc_t *dev_desc=NULL; int dev=0; int part=1; char *ep; @@ -51,11 +92,6 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) }
dev = (int)simple_strtoul(argv[2], &ep, 16); - dev_desc = get_dev(argv[1],dev); - if (dev_desc == NULL) { - puts("\n** Invalid boot device **\n"); - return 1; - } if (*ep) { if (*ep != ':') { puts("\n** Invalid boot device, use `dev[:part]' **\n"); @@ -63,25 +99,17 @@ int do_fat_fsload (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) } part = (int)simple_strtoul(++ep, NULL, 16); } - if (fat_register_device(dev_desc,part)!=0) { - printf("\n** Unable to use %s %d:%d for fatload **\n", - argv[1], dev, part); - return 1; - } offset = simple_strtoul(argv[3], NULL, 16); if (argc == 6) count = simple_strtoul(argv[5], NULL, 16); else count = 0; - size = file_fat_read(argv[4], (unsigned char *)offset, count);
- if(size==-1) { - printf("\n** Unable to read "%s" from %s %d:%d **\n", - argv[4], argv[1], dev, part); - return 1; - } + size = do_fat_safe(FAT_READ, argv[1], argv[4], dev, part, + (void *)offset, count);
- printf("\n%ld bytes read\n", size); + if (size == -1) + return 1;
sprintf(buf, "%lX", size); setenv("filesize", buf); @@ -134,7 +162,7 @@ int do_fat_ls (cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) else ret = file_fat_ls(filename);
- if(ret!=0) + if (ret != 0) printf("No Fat FS detected\n"); return ret; } @@ -189,10 +217,9 @@ U_BOOT_CMD( static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - long size; unsigned long addr; unsigned long count; - block_dev_desc_t *dev_desc = NULL; + int dev = 0; int part = 1; char *ep; @@ -201,11 +228,6 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag, return cmd_usage(cmdtp);
dev = (int)simple_strtoul(argv[2], &ep, 16); - dev_desc = get_dev(argv[1], dev); - if (dev_desc == NULL) { - puts("\n** Invalid boot device **\n"); - return 1; - } if (*ep) { if (*ep != ':') { puts("\n** Invalid boot device, use `dev[:part]' **\n"); @@ -213,22 +235,13 @@ static int do_fat_fswrite(cmd_tbl_t *cmdtp, int flag, } part = (int)simple_strtoul(++ep, NULL, 16); } - if (fat_register_device(dev_desc, part) != 0) { - printf("\n** Unable to use %s %d:%d for fatwrite **\n", - argv[1], dev, part); - return 1; - } + addr = simple_strtoul(argv[3], NULL, 16); count = simple_strtoul(argv[5], NULL, 16);
- size = file_fat_write(argv[4], (void *)addr, count); - if (size == -1) { - printf("\n** Unable to write "%s" from %s %d:%d **\n", - argv[4], argv[1], dev, part); + if (do_fat_safe(FAT_WRITE, argv[1], argv[4], dev, part, + (void *)addr, count) == -1) return 1; - } - - printf("%ld bytes written\n", size);
return 0; } diff --git a/common/env_fat.c b/common/env_fat.c index bad92aa..35d8ac0 100644 --- a/common/env_fat.c +++ b/common/env_fat.c @@ -85,25 +85,9 @@ int saveenv(void) } #endif /* CONFIG_MMC */
- dev_desc = get_dev(FAT_ENV_INTERFACE, dev); - if (dev_desc == NULL) { - printf("Failed to find %s%d\n", - FAT_ENV_INTERFACE, dev); - return 1; - } - if (fat_register_device(dev_desc, part) != 0) { - printf("Failed to register %s%d:%d\n", - FAT_ENV_INTERFACE, dev, part); - return 1; - } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); - if (file_fat_write(FAT_ENV_FILE, (void *)&env_new, sizeof(env_t)) == -1) { - printf("\n** Unable to write "%s" from %s%d:%d **\n", - FAT_ENV_FILE, FAT_ENV_INTERFACE, dev, part); - return 1; - } - + do_fat_safe(FAT_WRITE, FAT_ENV_INTERFACE, FAT_ENV_FILE, dev, part, + (void *)&env_new, sizeof(env_t)); puts("done\n"); return 0; } diff --git a/include/fat.h b/include/fat.h index f1b4a0d..20a0682 100644 --- a/include/fat.h +++ b/include/fat.h @@ -199,6 +199,11 @@ struct filesystem { const char name[12]; };
+enum fat_op { + FAT_READ = 1, + FAT_WRITE +}; + /* FAT tables */ file_detectfs_func file_fat_detectfs; file_ls_func file_fat_ls; @@ -213,4 +218,6 @@ const char *file_getfsname(int idx); int fat_register_device(block_dev_desc_t *dev_desc, int part_no);
int file_fat_write(const char *filename, void *buffer, unsigned long maxsize); +long do_fat_safe(enum fat_op op, char *iname, char *file, int dev, int part, + void *addr, unsigned long count); #endif /* _FAT_H_ */
Dear Lukasz Majewski,
In message 1343378116-5569-1-git-send-email-l.majewski@samsung.com you wrote:
New abstraction layer for performing MMC and FAT operations has been added. It is necessary for using MMC and FAT operations from other subsystems without the need to call "sprintf" and "run_command" afterwards.
MMC/FAT function call allows for better type checking during compilation time. For FAT, common code has been encapsulated to a separate function.
Lukasz Majewski (2): mmc: New abstract function (do_mmcops_safe) for MMC subsystem fat: New abstract function (do_fat_safe) for FAT subsystem
Sorry if I don't understand, but what exactly is special with MMC and FAT here?
Why are the same changes as to the MMC code not also needed for example for the IDE, SATA, SCSI or USB code?
Why are the same changes as for FAT not also needed for the other file systems?
And what exactly are you changing anyway? The commit messages in the patches give neither sufficient information to understand why you make a change at all, nor how the new code is supposed to be used, nor in which way he is better ("more safe" ?) than the existing code?
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
Dear Lukasz Majewski,
In message 1343378116-5569-1-git-send-email-l.majewski@samsung.com you wrote:
New abstraction layer for performing MMC and FAT operations has been added. It is necessary for using MMC and FAT operations from other subsystems without the need to call "sprintf" and "run_command" afterwards.
MMC/FAT function call allows for better type checking during compilation time. For FAT, common code has been encapsulated to a separate function.
Lukasz Majewski (2): mmc: New abstract function (do_mmcops_safe) for MMC subsystem fat: New abstract function (do_fat_safe) for FAT subsystem
Sorry if I don't understand, but what exactly is special with MMC and FAT here?
Those patches are a follow up of a discussion about DFU support in u-boot. "[PATCH 4/7] dfu: MMC specific routines for DFU operation"
http://article.gmane.org/gmane.comp.boot-loaders.u-boot/134401/match=dfu+mmc...
In short - during this discussion it has been suggested, that sprintf() + run_command() call shall be replaced with a "_safe" function call, which decouples "real" e.g. MMC write from parsing user data from prompt.
Why are the same changes as to the MMC code not also needed for example for the IDE, SATA, SCSI or USB code?
I need the FAT and MMC for DFU patches, on which I work now. Those patches (actually v2) have already been posted to ML and are under review.
Why are the same changes as for FAT not also needed for the other file systems?
It is because DFU is not supporting other file systems for now. I only changed the code which I use.
And what exactly are you changing anyway? The commit messages in the patches give neither sufficient information to understand why you make a change at all, nor how the new code is supposed to be used, nor in which way he is better ("more safe" ?) than the existing code?
The problem is as follows: 1. DFU patches requires access to FAT and MMC 2. I'm using sprintf() + run_command() approach to execute e.g. "mmc write 0x44000000 60 100" to store data from DFU in a proper place. 3. The approach presented at 2. was under discussion on ML. Reviewers suggested, that run_command() approach is unsafe and for compilation time type checking I shall write mmc with using special "_safe" function.
For more details, please visit link, which I've posted. In this discussion we are arguing about pros and cons of using sprintf + run_command approach.
Dear Lukasz Majewski,
In message 20120727123832.69195dcd@amdc308.digital.local you wrote:
Sorry if I don't understand, but what exactly is special with MMC and FAT here?
Those patches are a follow up of a discussion about DFU support in u-boot. "[PATCH 4/7] dfu: MMC specific routines for DFU operation"
OK, I see. Guess I will have to start reading these patches then (which I didn't so far).
In short - during this discussion it has been suggested, that sprintf() + run_command() call shall be replaced with a "_safe" function call, which decouples "real" e.g. MMC write from parsing user data from prompt.
...
I need the FAT and MMC for DFU patches, on which I work now. Those patches (actually v2) have already been posted to ML and are under review.
This makes no sense to me. MMC is just one of the storagte devices we support, and especially with the upcoming support of a clean device model it makes no sense to handle it special in any way.
The same holds for FAT - it is just one out of a number of file systems we support, and it makes no sense to add any code to FAT support that makes it incompatible with other file systems.
Any such addition should be implemented in a generic way, that is agnostic of the underlying storage device and the file system used on top of it.
It is OK if you then test with one combination only, but the implementation shall be generic.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
Dear Lukasz Majewski,
In message 20120727123832.69195dcd@amdc308.digital.local you wrote:
Sorry if I don't understand, but what exactly is special with MMC and FAT here?
Those patches are a follow up of a discussion about DFU support in u-boot. "[PATCH 4/7] dfu: MMC specific routines for DFU operation"
OK, I see. Guess I will have to start reading these patches then (which I didn't so far).
In short - during this discussion it has been suggested, that sprintf() + run_command() call shall be replaced with a "_safe" function call, which decouples "real" e.g. MMC write from parsing user data from prompt.
...
I need the FAT and MMC for DFU patches, on which I work now. Those patches (actually v2) have already been posted to ML and are under review.
This makes no sense to me. MMC is just one of the storagte devices we support, and especially with the upcoming support of a clean device model it makes no sense to handle it special in any way.
Sure, but that might still be a release or so away. So let's not hinder people from doing so, rather let's coordinate. Also, I consider there patches helpful, as we can use that abstraction during the DM work.
The same holds for FAT - it is just one out of a number of file systems we support, and it makes no sense to add any code to FAT support that makes it incompatible with other file systems.
It's not incompatible, how?
Any such addition should be implemented in a generic way, that is agnostic of the underlying storage device and the file system used on top of it.
Should, that's the proper word here. It isn't in most of FSs in uboot though and I think this is better than nothing.
It is OK if you then test with one combination only, but the implementation shall be generic.
I still consider it better than nothing -- if we have to pick this up and tear it apart during the DM, it's also OK, because we will easily figure out what parts to put where. So I'm quite fine with these patches.
Best regards,
Wolfgang Denk
Best regards, Marek Vasut
Dear Marek,
In message 201207271330.46967.marex@denx.de you wrote:
This makes no sense to me. MMC is just one of the storagte devices we support, and especially with the upcoming support of a clean device model it makes no sense to handle it special in any way.
Sure, but that might still be a release or so away. So let's not hinder people from doing so, rather let's coordinate. Also, I consider there patches helpful, as we can use that abstraction during the DM work.
Fine.
But why adding special code to only one storage media, and one file system type?
The same holds for FAT - it is just one out of a number of file systems we support, and it makes no sense to add any code to FAT support that makes it incompatible with other file systems.
It's not incompatible, how?
I cannot use - say - a USB storage device instead of MMC.
Why is this so?
I will reply to that in the context of the DFU thread, it makes more sense there.
Any such addition should be implemented in a generic way, that is agnostic of the underlying storage device and the file system used on top of it.
Should, that's the proper word here. It isn't in most of FSs in uboot though and I think this is better than nothing.
Wrong. Adding special code to one file system which makes it "special" compared to other file systems is actually really, really bad.
Best regards,
Wolfgang Denk
Dear Wolfgang Denk,
Dear Marek,
In message 201207271330.46967.marex@denx.de you wrote:
This makes no sense to me. MMC is just one of the storagte devices we support, and especially with the upcoming support of a clean device model it makes no sense to handle it special in any way.
Sure, but that might still be a release or so away. So let's not hinder people from doing so, rather let's coordinate. Also, I consider there patches helpful, as we can use that abstraction during the DM work.
Fine.
But why adding special code to only one storage media, and one file system type?
Well, that's other thing. But I hope Lukasz will eventually extend this work over other media as the DFU gains support for more stuff. Right, Lukasz?
The same holds for FAT - it is just one out of a number of file systems we support, and it makes no sense to add any code to FAT support that makes it incompatible with other file systems.
It's not incompatible, how?
I cannot use - say - a USB storage device instead of MMC.
Not _yet_ .
Why is this so?
Because it's not implemented yet.
I will reply to that in the context of the DFU thread, it makes more sense there.
Any such addition should be implemented in a generic way, that is agnostic of the underlying storage device and the file system used on top of it.
Should, that's the proper word here. It isn't in most of FSs in uboot though and I think this is better than nothing.
Wrong. Adding special code to one file system which makes it "special" compared to other file systems is actually really, really bad.
I hope it'll not only be fat over the time.
Best regards,
Wolfgang Denk
Best regards, Marek Vasut
Dear Marek Vasut,
Dear Wolfgang Denk,
Dear Lukasz Majewski,
In message 20120727123832.69195dcd@amdc308.digital.local you wrote:
Sorry if I don't understand, but what exactly is special with MMC and FAT here?
Those patches are a follow up of a discussion about DFU support in u-boot. "[PATCH 4/7] dfu: MMC specific routines for DFU operation"
OK, I see. Guess I will have to start reading these patches then (which I didn't so far).
In short - during this discussion it has been suggested, that sprintf() + run_command() call shall be replaced with a "_safe" function call, which decouples "real" e.g. MMC write from parsing user data from prompt.
...
I need the FAT and MMC for DFU patches, on which I work now. Those patches (actually v2) have already been posted to ML and are under review.
This makes no sense to me. MMC is just one of the storagte devices we support, and especially with the upcoming support of a clean device model it makes no sense to handle it special in any way.
Sure, but that might still be a release or so away. So let's not hinder people from doing so, rather let's coordinate. Also, I consider there patches helpful, as we can use that abstraction during the DM work.
The same holds for FAT - it is just one out of a number of file systems we support, and it makes no sense to add any code to FAT support that makes it incompatible with other file systems.
It's not incompatible, how?
Any such addition should be implemented in a generic way, that is agnostic of the underlying storage device and the file system used on top of it.
Should, that's the proper word here. It isn't in most of FSs in uboot though and I think this is better than nothing.
It is OK if you then test with one combination only, but the implementation shall be generic.
I still consider it better than nothing -- if we have to pick this up and tear it apart during the DM, it's also OK, because we will easily figure out what parts to put where. So I'm quite fine with these patches.
Nice to hear. Anyway I think, that we shall wait for Wolfgang's opinion.
participants (3)
-
Lukasz Majewski -
Marek Vasut -
Wolfgang Denk