public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Stephen Warren <swarren@wwwdotorg.org>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 3/9] disk/part: introduce get_device_and_partition
Date: Thu, 23 Aug 2012 16:36:15 -0600	[thread overview]
Message-ID: <5036B05F.1070406@wwwdotorg.org> (raw)
In-Reply-To: <1345757510-6756-4-git-send-email-robherring2@gmail.com>

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

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.

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.

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?

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

  reply	other threads:[~2012-08-23 22:36 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-23 21:31 [U-Boot] [PATCH 0/9] Auto partition selection and fs partition consolidation Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 1/9] combine block device load commands into common function Rob Herring
2012-09-05 23:36   ` Tom Rini
2012-09-05 23:47     ` Rob Herring
2012-09-05 23:50       ` Tom Rini
2012-09-21 14:02   ` [U-Boot] [PATCH v2 " Rob Herring
2012-09-25 23:17     ` Tom Rini
2012-08-23 21:31 ` [U-Boot] [PATCH 2/9] disk/part: check bootable flag for DOS partitions Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 3/9] disk/part: introduce get_device_and_partition Rob Herring
2012-08-23 22:36   ` Stephen Warren [this message]
2012-08-24  1:57     ` Rob Herring
2012-08-24  2:51       ` Stephen Warren
2012-09-05 23:53         ` Tom Rini
2012-09-21 14:08   ` [U-Boot] [PATCH v2 " Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 4/9] ext4: remove init_fs/deinit_fs Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 5/9] cmd_extX: use common get_device_and_partition function Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 6/9] cmd_fat: " Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 7/9] cmd_disk: " Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 8/9] cmd_zfs: " Rob Herring
2012-08-23 21:31 ` [U-Boot] [PATCH 9/9] cmd_reiser: " Rob Herring

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5036B05F.1070406@wwwdotorg.org \
    --to=swarren@wwwdotorg.org \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox