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