From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josh Wu Date: Fri, 13 Jun 2014 11:33:19 +0800 Subject: [U-Boot] [PATCH] fs/fat: add a parameter: allow_whole_dev to fat_register_device() In-Reply-To: <20140612085240.6E953380133@gemini.denx.de> References: <1402552643-13297-1-git-send-email-josh.wu@atmel.com> <20140612062648.E79553803E2@gemini.denx.de> <53995100.9080307@atmel.com> <20140612085240.6E953380133@gemini.denx.de> Message-ID: <539A70FF.4080003@atmel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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