[U-Boot] [PATCH 3/9] disk/part: introduce get_device_and_partition

[Stephen Warren]

On 08/23/2012 07:57 PM, Rob Herring wrote:
> 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 at 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.

I guess that syntax would be fine if we don't need to distinguish all
the cases. "-" isn't that descriptive though, and I've only seen it mean
"nothing" in U-Boot commands. So, bike-shedding a bit, it doesn't seem
exactly correct. Perhaps just "auto"?

>> 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?

I can imagine a boot.scr that does:

setenv bootargs root=/dev/mmcblk0p${bootpart}

But then, you may as well use the partition UUID feature instead of
that, so that boot.scr doesn't need to know the kernel's device name.

>> 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.

That could work too, although commands using environment variables seems
a little implicit/hidden.