[U-Boot] [PATCH 5/6] gpt: Support for new "gpt" command

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> New command - "gpt" is now supported. It shows and restores the GPT partition
> table.
> It looks into the "partitions" environment variable for partitions definition.
> It can be enabled at target configuration file with CONFIG_CMD_GPT.

> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

> +int set_gpt_info(block_dev_desc_t *dev_desc)
> +{
> +	char *ps[GPT_PARTS_NUM], *name[GPT_PARTS_NUM];
> +	unsigned int size[GPT_PARTS_NUM];
> +	char *tok, *t, *p, *s, *ss;
> +	int i, ret;
> +
> +	s = getenv("partitions");
> +	if (s == NULL) {
> +		printf("%s: \"partitions\" env variable not defined!\n",
> +		       __func__);
> +		return -1;
> +	}

It'd be nice to be able to pass the partition definition on the
command-line instead of (or perhaps as an alternative to) reading an
environment variable.

Some documentation of the expected format of the partitions variable
would be useful. From the following patch, the format appears to be:

8M(csa-mmc),60M(u-boot),60M(kernel),...

That's not particularly extensible (think about allowing partition type
UUID to or attributes to be specified), and the brackets are a bit
painful. Can we use key/value for defining the values, and have a simple
separate separator between fields and partitions, e.g. something like:

size=8M,name=csa-mmc;size=60M,name=u-boot;size=60M,name=kernel;...

That would allow us to very easily allow new fields to be specified per
partition in the future, e.g.:

uuid=XXXXX,size=512M,name=boot,type=21686148-6449-6E6F-744E-656564454649,attrs=2;uuid=YYYYY,size=7000M,name=root,type=0FC63DAF-8483-4772-8E79-3D69D8477DE4,attrs=0;...

> +U_BOOT_CMD(
> +	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
> +	"GUID Partition Table",
...
> +	"gpt restore - reset GPT partition to defaults\n"
> +	"gpt dev #num - set device number\n"
...
> +static void set_gpt_dev(int dev)
> +{
> +	gpt_dev = dev;
> +}
> +

Hmmm. I think it'd be better to specify the device each time the gpt
command was invoked. That would be simpler to use, more flexible, and
more consistent with how other commands such as "ext2load" operate.

In other words, I would get rid of "gpt dev" completely, and instead of
implementing "gpt restore", implement "gpt restore mmc 0".

I'm not sure "restore" is the correct name, given that the command can
write arbitrary new partition layouts, rather than just restoring some
specific hard-coded table from e.g. a backup image.

I'm not sure that "gpt" is even the best command name; it'd be nice if
this were generic and could be extended to work with e.g. FAT in the
future - something like:

part write usb 1 gpt uuid=XXXXX,size=512M,name=boot,...
part write mmc 0 fat size=512M,attrs=0x80;...

> +U_BOOT_CMD(
> +	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
> +	"GUID Partition Table",
> +	"show - show GPT\n"

s/show/gpt show/

> +static void gpt_show(void)
> +{
> +	struct mmc *mmc = find_mmc_device(gpt_dev);
> +
> +	print_part_efi(&mmc->block_dev);
> +}

Do we really need another way of showing partition tables; "mmc part",
"usb part", ... already exist. I think if we want another way, it'd be
better to add this functionality to my proposed "part" command, i.e.
"part show mmc 0".

> +static int gpt_default(void)
> +{
> +	struct mmc *mmc = find_mmc_device(gpt_dev);
> +
> +	if (mmc == NULL) {
> +		printf("%s: mmc dev %d NOT available\n", __func__, gpt_dev);
> +		return CMD_RET_FAILURE;
> +	}

Why only allow mmc devices; what about USB for example? Other commands
such as ext2load allow arbitrary device types to be used. Rob Herring
recently posted some patches to unify how commands such as ext2load,
ext2ls, fatload, fatls, ... all obtain a device/partition handle from
their command-line - this patch should probably build on top of those
patches.

> +	puts("Using default GPT UUID\n");
> +
> +	return set_gpt_info(&mmc->block_dev);
> +}