| Message ID | 1402552643-13297-1-git-send-email-josh.wu@atmel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Josh Wu, In message <1402552643-13297-1-git-send-email-josh.wu@atmel.com> you wrote: > For SPL in FAT and envrionment load/save in FAT, to support no partition > table device (whole device is FAT partition). We need specify the partition > number as 0. Sorry, I cannot parse this. What exactly do you mean here? > But in FAT SPL and environment case, when we specify the partition number as > 1, it is reasonable to support the device has no partition table and only a > FAT partition. Why would the expectations in SPL be different from other use cases? > +int fat_register_device(block_dev_desc_t *dev_desc, int part_no, > + bool allow_whole_dev); Please make this an "int" type, and use 0 and 1. Thanks. Best regards, Wolfgang Denk
Dear Wolfgang Thanks for the review. On 6/12/2014 2:26 PM, Wolfgang Denk wrote: > Dear Josh Wu, > > In message <1402552643-13297-1-git-send-email-josh.wu@atmel.com> you wrote: >> For SPL in FAT and envrionment load/save in FAT, to support no partition >> table device (whole device is FAT partition). We need specify the partition >> number as 0. > Sorry, I cannot parse this. What exactly do you mean here? Sorry, Let me try to explain it a little bit: In U-Boot when we access a partition of a device, we use 'ifname dev:part' format. For instance: 'mmc 0:1' means the MMC card's #1 partition of the device #0. But for a case if the mmc device has no partition table (MBR), it only has one FAT partition. To support that case, we need to access by using 'mmc 0:0'. So the problem is: if we specify mmc 0:0 then I cannot access the mmc device if it has a partition table. And if we specify mmc 0:1 then I cannot access if it has no partition table. For the fs layer this case is solved by use 'mmc 0', or 'mmc 0:auto' by commit: 10a37fd7a4 (disk: get_device_and_partition() "auto" partition and cleanup) But for env fat and SPL fat, we don't use the function in above commit as we use a simpler function fat_register_device(). So this patch make this function works too. > >> But in FAT SPL and environment case, when we specify the partition number as >> 1, it is reasonable to support the device has no partition table and only a >> FAT partition. > Why would the expectations in SPL be different from other use cases? For example, when I use SPL binary in mmc card, I want it to load the file: u-boot.img from the first partition. I expect it should work even if the mmc device has no partition table. But when I define CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION as 1. it cannot work when the mmc has no partition table. same thing happens for saving environment to a FAT file in MMC. > >> +int fat_register_device(block_dev_desc_t *dev_desc, int part_no, >> + bool allow_whole_dev); > Please make this an "int" type, and use 0 and 1. Is there any special concern for that? like cause machine compatiable issue? Best Regards, Josh Wu > > Thanks. > > Best regards, > > Wolfgang Denk >
Dear Josh Wu, In message <53995100.9080307@atmel.com> you wrote: > > In U-Boot when we access a partition of a device, we use 'ifname > dev:part' format. > For instance: 'mmc 0:1' means the MMC card's #1 partition of the device #0. Don;t we also support plain "ifname dev", i. e. without partition specification? > But for a case if the mmc device has no partition table (MBR), it only > has one FAT partition. Um... No. Without a partition table, there are no partitins at all, so there can be no FAT partitions. I guess you mean there is a FAT file system on the device? > To support that case, we need to access by using 'mmc 0:0'. I feel that should be "mmc 0". > So the problem is: if we specify mmc 0:0 then I cannot access the mmc > device if it has a partition table. Well, if it _has_ a partition table, then you should select the correct partition number, and not use the (nonexitent) number 0. > And if we specify mmc 0:1 then I cannot access if it has no partition table. Correct. Because no partition 1 exists. > For the fs layer this case is solved by use 'mmc 0', or 'mmc 0:auto' by > commit: > 10a37fd7a4 (disk: get_device_and_partition() "auto" partition and cleanup) > > But for env fat and SPL fat, we don't use the function in above commit > as we use a simpler function fat_register_device(). > So this patch make this function works too. I may be wrong, but I think with your patch we do not implement the same behaviour as for get_device_and_partition() [see the commit message for the aforementioned commit how it is supposed to work]. I think, we should implemnt consistent behaviour here. > > > >> But in FAT SPL and environment case, when we specify the partition number as > >> 1, it is reasonable to support the device has no partition table and only a > >> FAT partition. > > Why would the expectations in SPL be different from other use cases? > > For example, when I use SPL binary in mmc card, I want it to load the > file: u-boot.img from the first partition. > I expect it should work even if the mmc device has no partition table. Please see the description in the commit message how it is supposed to work. Note that it's not just the first partition, but actully the first _bootable_ partition which should get used. > But when I define CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION as 1. it cannot > work when the mmc has no partition table. > > same thing happens for saving environment to a FAT file in MMC. This is even more problematic, as we do not have any definition where the environment would be located. Is it the first partition? Or the first bootable partition? Or a specifically defined one? I think it is dangerous to just "make it work" without clearly specifying first what the expected behaviour should be. > > > >> +int fat_register_device(block_dev_desc_t *dev_desc, int part_no, > >> + bool allow_whole_dev); > > Please make this an "int" type, and use 0 and 1. > > Is there any special concern for that? like cause machine compatiable issue? Boolean values in C are 1 and 0. Hiding these under other names (like "true" and "false") doesn't buy anything. Best regards, Wolfgang Denk
Dear Wolfgang On 6/12/2014 4:52 PM, Wolfgang Denk wrote: > Dear Josh Wu, > > In message <53995100.9080307@atmel.com> you wrote: >> In U-Boot when we access a partition of a device, we use 'ifname >> dev:part' format. >> For instance: 'mmc 0:1' means the MMC card's #1 partition of the device #0. > Don;t we also support plain "ifname dev", i. e. without partition > specification? The problem is we only support "ifname dev" on command line mode or the filesystem call which calls get_device_and_partition(). For environment save/load and SPL load on FAT, which use fat_register_device() instead of calling get_device_and_partition(), we need specify the partition number. It DOESN'T support "ifname dev" without partition number. for example: 1. to enable FAT env save/load, we need define: FAT_ENV_INTERFACE(=mmc), FAT_ENV_DEVICE(=0), FAT_ENV_PART(=1) 2. to enable FAT SPL load, we need fine: CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION(=1) > >> But for a case if the mmc device has no partition table (MBR), it only >> has one FAT partition. > Um... No. Without a partition table, there are no partitins at all, so > there can be no FAT partitions. I guess you mean there is a FAT file > system on the device? yes, if we cannot find a partition table, then the first sector is assumed as FAT file system's first sector. > >> To support that case, we need to access by using 'mmc 0:0'. > I feel that should be "mmc 0". > >> So the problem is: if we specify mmc 0:0 then I cannot access the mmc >> device if it has a partition table. > Well, if it _has_ a partition table, then you should select the > correct partition number, and not use the (nonexitent) number 0. > >> And if we specify mmc 0:1 then I cannot access if it has no partition table. > Correct. Because no partition 1 exists. > >> For the fs layer this case is solved by use 'mmc 0', or 'mmc 0:auto' by >> commit: >> 10a37fd7a4 (disk: get_device_and_partition() "auto" partition and cleanup) >> >> But for env fat and SPL fat, we don't use the function in above commit >> as we use a simpler function fat_register_device(). >> So this patch make this function works too. > I may be wrong, but I think with your patch we do not implement the > same behaviour as for get_device_and_partition() [see the commit > message for the aforementioned commit how it is supposed to work]. > > I think, we should implemnt consistent behaviour here. I agree. I will read the get_device_and_partition() code to understand it. > >>>> But in FAT SPL and environment case, when we specify the partition number as >>>> 1, it is reasonable to support the device has no partition table and only a >>>> FAT partition. >>> Why would the expectations in SPL be different from other use cases? >> For example, when I use SPL binary in mmc card, I want it to load the >> file: u-boot.img from the first partition. >> I expect it should work even if the mmc device has no partition table. > Please see the description in the commit message how it is supposed to > work. Note that it's not just the first partition, but actully the > first _bootable_ partition which should get used. > >> But when I define CONFIG_SYS_MMC_SD_FAT_BOOT_PARTITION as 1. it cannot >> work when the mmc has no partition table. >> >> same thing happens for saving environment to a FAT file in MMC. > This is even more problematic, as we do not have any definition where > the environment would be located. Is it the first partition? Or the > first bootable partition? Or a specifically defined one? > > I think it is dangerous to just "make it work" without clearly > specifying first what the expected behaviour should be. I understand your concern. Thanks a lot for your comments. After the discussing I get a further think of this problem. Let me summary here: The problem I met is FAT env save/load and FAT spl load use fat_register_device() instead of get_device_and_partition(). So that means I must specify a partition number. I think for usually user case, we don't want to specify the partition number. that means: 1. if has partition table, please use partition #1. 2. if no partition table, assume the whole device is a FAT file system. if we agree with above, then the solution should be implement a way to support the case we don't specify a partition number. 1. use get_device_and_partition(). 2. add a unspecify partition number support in fat_register_device() I think #1 is the best way be cause we don't have to implement same things in two place. But I am doubt that the FAT spl can use it. I'll check this. What do think of that? > >>>> +int fat_register_device(block_dev_desc_t *dev_desc, int part_no, >>>> + bool allow_whole_dev); >>> Please make this an "int" type, and use 0 and 1. >> Is there any special concern for that? like cause machine compatiable issue? > Boolean values in C are 1 and 0. Hiding these under other names (like > "true" and "false") doesn't buy anything. Okay. I just think use bool will be more readable. That also can make people less use an integer number, which in some case it's hard to understand it. > > Best regards, > > Wolfgang Denk > Best Regards, Josh Wu
On 06/12/2014 09:33 PM, Josh Wu wrote: > Dear Wolfgang > > On 6/12/2014 4:52 PM, Wolfgang Denk wrote: >> Dear Josh Wu, >> >> In message <53995100.9080307@atmel.com> you wrote: >>> In U-Boot when we access a partition of a device, we use 'ifname >>> dev:part' format. >>> For instance: 'mmc 0:1' means the MMC card's #1 partition of the >>> device #0. >> Don;t we also support plain "ifname dev", i. e. without partition >> specification? > > The problem is we only support "ifname dev" on command line mode or the > filesystem call which calls get_device_and_partition(). Why not just replace the calls to fat_register_device() with calls to get_device_and_partition() (perhaps some wrapper is needed to make the APIs match). That way, someone (U-Boot config file author I suppose) can always specify exactly what they want, without needing any non-deterministic fallbacks.
Dear Josh Wu, In message <539A70FF.4080003@atmel.com> you wrote: > > > Don;t we also support plain "ifname dev", i. e. without partition > > specification? > > The problem is we only support "ifname dev" on command line mode or the > filesystem call which calls get_device_and_partition(). > > For environment save/load and SPL load on FAT, which use > fat_register_device() instead of calling get_device_and_partition(), > we need specify the partition number. It DOESN'T support "ifname dev" > without partition number. OK. so we have indentified the problem. It seems we should fix that? > > I think, we should implemnt consistent behaviour here. > > I agree. I will read the get_device_and_partition() code to understand it. The commit message of the patch you referred to has a nice description of how it is supposed to work. > After the discussing I get a further think of this problem. Let me > summary here: > > The problem I met is FAT env save/load and FAT spl load use > fat_register_device() instead of get_device_and_partition(). > So that means I must specify a partition number. > > I think for usually user case, we don't want to specify the partition > number. that means: > 1. if has partition table, please use partition #1. This is different from the non-SPL case then, which uses the first bootable partition instead, which may or may bnot be # 1. > if we agree with above, then the solution should be implement a way to > support the case we don't specify a partition number. > 1. use get_device_and_partition(). > 2. add a unspecify partition number support in fat_register_device() > > I think #1 is the best way be cause we don't have to implement same > things in two place. But I am doubt that the FAT spl can use it. I'll > check this. Using the same code is also a good way to make sure the behaviour is the same :-) > What do think of that? Sounds good to me. > >>> Please make this an "int" type, and use 0 and 1. > >> Is there any special concern for that? like cause machine compatiable issue? > > Boolean values in C are 1 and 0. Hiding these under other names (like > > "true" and "false") doesn't buy anything. > > Okay. I just think use bool will be more readable. That also can make > people less use an integer number, which in some case it's hard to > understand it. None of the related code uses ant bool types so far, so please do not introduce it now. Thanks. Best regards, Wolfgang Denk
Hi, Stephen On 6/13/2014 12:07 PM, Stephen Warren wrote: > On 06/12/2014 09:33 PM, Josh Wu wrote: >> Dear Wolfgang >> >> On 6/12/2014 4:52 PM, Wolfgang Denk wrote: >>> Dear Josh Wu, >>> >>> In message <53995100.9080307@atmel.com> you wrote: >>>> In U-Boot when we access a partition of a device, we use 'ifname >>>> dev:part' format. >>>> For instance: 'mmc 0:1' means the MMC card's #1 partition of the >>>> device #0. >>> Don;t we also support plain "ifname dev", i. e. without partition >>> specification? >> The problem is we only support "ifname dev" on command line mode or the >> filesystem call which calls get_device_and_partition(). > Why not just replace the calls to fat_register_device() with calls to > get_device_and_partition() (perhaps some wrapper is needed to make the > APIs match). That way, someone (U-Boot config file author I suppose) can > always specify exactly what they want, without needing any > non-deterministic fallbacks. yes, that is the exactly I will do. Thanks. Best Regards, Josh Wu
On Fri, Jun 13, 2014 at 11:33:19AM +0800, Josh Wu wrote: > Dear Wolfgang > > On 6/12/2014 4:52 PM, Wolfgang Denk wrote: > >Dear Josh Wu, > > > >In message <53995100.9080307@atmel.com> you wrote: I will read and think about the rest of this, but: [snip] > >>>>+int fat_register_device(block_dev_desc_t *dev_desc, int part_no, > >>>>+ bool allow_whole_dev); > >>>Please make this an "int" type, and use 0 and 1. > >>Is there any special concern for that? like cause machine compatiable issue? > >Boolean values in C are 1 and 0. Hiding these under other names (like > >"true" and "false") doesn't buy anything. > > Okay. I just think use bool will be more readable. That also can > make people less use an integer number, which in some case it's hard > to understand it. We have bool and make use of bool when things are a boolean, your way here is fine.
On Fri, Jun 13, 2014 at 12:54:17PM +0800, Josh Wu wrote: > Hi, Stephen > > On 6/13/2014 12:07 PM, Stephen Warren wrote: > >On 06/12/2014 09:33 PM, Josh Wu wrote: > >>Dear Wolfgang > >> > >>On 6/12/2014 4:52 PM, Wolfgang Denk wrote: > >>>Dear Josh Wu, > >>> > >>>In message <53995100.9080307@atmel.com> you wrote: > >>>>In U-Boot when we access a partition of a device, we use 'ifname > >>>>dev:part' format. > >>>>For instance: 'mmc 0:1' means the MMC card's #1 partition of the > >>>>device #0. > >>>Don;t we also support plain "ifname dev", i. e. without partition > >>>specification? > >>The problem is we only support "ifname dev" on command line mode or the > >>filesystem call which calls get_device_and_partition(). > >Why not just replace the calls to fat_register_device() with calls to > >get_device_and_partition() (perhaps some wrapper is needed to make the > >APIs match). That way, someone (U-Boot config file author I suppose) can > >always specify exactly what they want, without needing any > >non-deterministic fallbacks. > yes, that is the exactly I will do. Thanks. Yes, I also agree, thanks!
diff --git a/board/cm5200/fwupdate.c b/board/cm5200/fwupdate.c index 06d5023..801d82a 100644 --- a/board/cm5200/fwupdate.c +++ b/board/cm5200/fwupdate.c @@ -120,7 +120,7 @@ static int load_rescue_image(ulong addr) /* Detect partition */ for (partno = -1, i = 0; i < 6; i++) { if (get_partition_info(stor_dev, i, &info) == 0) { - if (fat_register_device(stor_dev, i) == 0) { + if (fat_register_device(stor_dev, i, false) == 0) { /* Check if rescue image is present */ FW_DEBUG("Looking for firmware directory '%s'" " on partition %d\n", fwdir, i); diff --git a/board/esd/common/auto_update.c b/board/esd/common/auto_update.c index 85c3567..d2dd76a 100644 --- a/board/esd/common/auto_update.c +++ b/board/esd/common/auto_update.c @@ -30,7 +30,6 @@ extern int N_AU_IMAGES; #define MAX_LOADSZ 0x1c00000 /* externals */ -extern int fat_register_device(block_dev_desc_t *, int); extern int file_fat_detectfs(void); extern long file_fat_read(const char *, void *, unsigned long); long do_fat_read (const char *filename, void *buffer, @@ -390,7 +389,7 @@ int do_auto_update(void) } } - if (fat_register_device (stor_dev, 1) != 0) { + if (fat_register_device(stor_dev, 1, false) != 0) { debug ("Unable to register ide disk 0:1\n"); return -1; } diff --git a/board/mcc200/auto_update.c b/board/mcc200/auto_update.c index 43173ce..6cc12d5 100644 --- a/board/mcc200/auto_update.c +++ b/board/mcc200/auto_update.c @@ -106,7 +106,7 @@ ulong totsize; #define KEYPAD_MASK_HI ((1<<(KEYPAD_COL-1+(KEYPAD_ROW*3-3)))>>8) /* externals */ -extern int fat_register_device(block_dev_desc_t *, int); +extern int fat_register_device(block_dev_desc_t *, int, bool); extern int file_fat_detectfs(void); extern long file_fat_read(const char *, void *, unsigned long); extern int i2c_read (unsigned char, unsigned int, int , unsigned char* , int); @@ -373,7 +373,7 @@ int do_auto_update(void) res = -1; goto xit; } - if (fat_register_device(stor_dev, 1) != 0) { + if (fat_register_device(stor_dev, 1, false) != 0) { debug ("Unable to use USB %d:%d for fatls\n", au_usb_stor_curr_dev, 1); res = -1; diff --git a/common/env_fat.c b/common/env_fat.c index aad0487..b36e08e 100644 --- a/common/env_fat.c +++ b/common/env_fat.c @@ -67,7 +67,7 @@ int saveenv(void) return 1; } - err = fat_register_device(dev_desc, part); + err = fat_register_device(dev_desc, part, true); if (err) { printf("Failed to register %s%d:%d\n", FAT_ENV_INTERFACE, dev, part); @@ -117,7 +117,7 @@ void env_relocate_spec(void) return; } - err = fat_register_device(dev_desc, part); + err = fat_register_device(dev_desc, part, true); if (err) { printf("Failed to register %s%d:%d\n", FAT_ENV_INTERFACE, dev, part); diff --git a/common/spl/spl_fat.c b/common/spl/spl_fat.c index 56be943..c7a4fd6 100644 --- a/common/spl/spl_fat.c +++ b/common/spl/spl_fat.c @@ -25,7 +25,7 @@ static int spl_register_fat_device(block_dev_desc_t *block_dev, int partition) if (fat_registered) return err; - err = fat_register_device(block_dev, partition); + err = fat_register_device(block_dev, partition, true); if (err) { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT printf("%s: fat register err - %d\n", __func__, err); diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 54f42ea..ad08cde 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -81,7 +81,8 @@ int fat_set_blk_dev(block_dev_desc_t *dev_desc, disk_partition_t *info) return -1; } -int fat_register_device(block_dev_desc_t *dev_desc, int part_no) +int fat_register_device(block_dev_desc_t *dev_desc, int part_no, + bool allow_whole_dev) { disk_partition_t info; @@ -91,9 +92,15 @@ int fat_register_device(block_dev_desc_t *dev_desc, int part_no) /* Read the partition table, if present */ if (get_partition_info(dev_desc, part_no, &info)) { if (part_no != 0) { - printf("** Partition %d not valid on device %d **\n", - part_no, dev_desc->dev); - return -1; + /* + * If part_no = 1, we will support whole partition + * device if allow_whole_dev is true + */ + if (!(allow_whole_dev && part_no == 1)) { + printf("** Partition %d not valid on device %d **\n", + part_no, dev_desc->dev); + return -1; + } } info.start = 0; diff --git a/include/fat.h b/include/fat.h index 63cf787..df15945 100644 --- a/include/fat.h +++ b/include/fat.h @@ -203,7 +203,8 @@ long file_fat_read_at(const char *filename, unsigned long pos, void *buffer, long file_fat_read(const char *filename, void *buffer, unsigned long maxsize); const char *file_getfsname(int idx); int fat_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info); -int fat_register_device(block_dev_desc_t *dev_desc, int part_no); +int fat_register_device(block_dev_desc_t *dev_desc, int part_no, + bool allow_whole_dev); int file_fat_write(const char *filename, void *buffer, unsigned long maxsize); int fat_read_file(const char *filename, void *buf, int offset, int len);
For SPL in FAT and envrionment load/save in FAT, to support no partition table device (whole device is FAT partition). We need specify the partition number as 0. But in FAT SPL and environment case, when we specify the partition number as 1, it is reasonable to support the device has no partition table and only a FAT partition. This patch add a parameter: allow_whole_dev for fat_register_device(). If allow_whole_dev is true, it will enable no partition table device. In FAT SPL and FAT file environment, we call fat_register_device() with allow_whole_dev as true. Other cases we call it with false. Signed-off-by: Josh Wu <josh.wu@atmel.com> --- board/cm5200/fwupdate.c | 2 +- board/esd/common/auto_update.c | 3 +-- board/mcc200/auto_update.c | 4 ++-- common/env_fat.c | 4 ++-- common/spl/spl_fat.c | 2 +- fs/fat/fat.c | 15 +++++++++++---- include/fat.h | 3 ++- 7 files changed, 20 insertions(+), 13 deletions(-)