On 08/23/2012 05:36 PM, Stephen Warren wrote:
> On 08/23/2012 03:31 PM, Rob Herring wrote:
>> From: Rob Herring <rob.herr...@calxeda.com>
>>
>> All block device related commands (scsiboot, fatload, ext2ls, etc.) have
>> simliar duplicated device and partition parsing and selection code. This
>> adds a common function to replace various implementations.
>>
>> The new function has some enhancements over current versions. If no device
>> or partition is specified on the command line, the bootdevice env variable
>> will be used (scsiboot does this). If the partition is not specified and
>> the device has partitions, then the first bootable partition will be used.
>> If a bootable partition is not found, the first valid partition is used.
>> The ret value is not needed since part will be zero when no partition is
>> found.
>
> Two thoughts on this patch:
>
> First, if I write "mmc 0" right now, command will always attempt to
> access precisely partion 1, whereas after this patch, they will search
> for the first bootable, or valid, partition. This is a change in
> behavior. It's a pretty reasonable change, but I wonder if it might
> cause problems somewhere.
>
> Instead, perhaps this new feature should be explicitly requested,
> supporting the following device/partition specifications:
>
> # existing:
> dev 0:0 # whole device
> dev 0:n # n >= 1: explicit partition
> dev 0 # partition 1
> # new:
> dev 0:valid # first valid partition
> dev 0:bootable # first bootable partition
> dev 0:default # first bootable partition if there is one,
> # else first valid
I'm not sure we need to distinguish valid vs. bootable. Returning the
first valid partition was really just to maintain somewhat backwards
compatible behavior.
Perhaps just "0:-" would be sufficient.
>
> That would allow scripts to be very explicit about whether they wanted
> this new functionality.
>
> Second, if I run a slew of ext2load commands:
>
> ext2load mmc 0:bootable ${scriptaddr} boot.scr
> source ${scriptaddr}
> # script does:
> ext2load mmc 0:bootable ${kernel_addr} zImage
> ext2load mmc 0:bootable ${initrd_addr} initrd.bin
> ext2load mmc 0:bootable ${fdt_addr} foo.dtb
>
> Then there are two disadvantages:
>
> 1) I believe the partition table is read and decoded and search for
> every one of those ext2load commands. Slightly inefficient.
It was already multiple times per command with the command function
calling get_partition_info and then the filesystem code calling it again
internally as well. Now it is only 1 time at least. I would think the
1st partition being bootable is the common case.
> 2) There's no permanent record of the partition number, so this couldn't
> be e.g. used to construct a kernel command-line etc.
You mean to setup rootfs? I don't think we want u-boot to do that. Or
what would be the use?
> Instead, I wonder if get_device_and_partition() should just support the
> existing 3 device specification options, and we introduce a new command
> to determine which partition to boot from, e.g.:
>
> # writes result to "bootpart" variable
> # or get-default or get-first-valid
> part get-first-bootable mmc 0 bootpart
>
> ext2load mmc 0:${bootpart} ${scriptaddr} boot.scr
> source ${scriptaddr}
> # script does:
> ext2load mmc 0:${bootpart} ${kernel_addr} zImage
> ext2load mmc 0:${bootpart} ${initrd_addr} initrd.bin
> ext2load mmc 0:${bootpart} ${fdt_addr} foo.dtb
>
> That solves those issues. Does anyone have any comment on the two
> approaches?
I'm really open to either way.
Another option would be for the first command run to set bootpart and
then re-use that value on subsequent commands.
Rob
>
> (although perhaps e.g. ext2load always re-reads the partition table
> anyway, so perhaps that advantage is moot?)
>
> Aside from that, this series looks conceptually reasonable at a quick
> glance. I'd be happy to provide an equivalent to patch 2 for GPT/EFI
> partition tables.
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
