On 08/23/2012 05:36 PM, Stephen Warren wrote:
On 08/23/2012 03:31 PM, Rob Herring wrote:
From: Rob Herring rob.herring@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:
- 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.
- 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.