From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Thu, 23 Aug 2012 20:51:50 -0600 Subject: [U-Boot] [PATCH 3/9] disk/part: introduce get_device_and_partition In-Reply-To: <5036DFA6.9050901@gmail.com> References: <1345757510-6756-1-git-send-email-robherring2@gmail.com> <1345757510-6756-4-git-send-email-robherring2@gmail.com> <5036B05F.1070406@wwwdotorg.org> <5036DFA6.9050901@gmail.com> Message-ID: <5036EC46.3060306@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> >>> 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.