[U-Boot] [PATCH v2] Add a CBFS driver and commands to u-boot

[Wolfgang Denk <wd@denx.de>]

Dear Gabe Black,

In message <1323134730-18471-1-git-send-email-gabeblack@chromium.org> you wrote:
> Coreboot uses a very simple "file system" called CBFS to keep track of and
> allow access to multiple "files" in a ROM image. This change adds CBFS
> support and some commands to use it to u-boot. These commands are:
> 
> cbfsinit - Initialize CBFS support and pull all metadata into RAM. The end
> of the ROM is an optional parameter which defaults to the standard
> 0xffffffff and can be used to support multiple CBFSes in a system. The last
> one set up with cbfsinit is the one that will be used.
> 
> cbfsinfo - Print information from the CBFS header.
> 
> cbfsls - Print out the size, type, and name of all the files in the current
> CBFS. Recognized types are translated into symbolic names.
> 
> cbfsload - Load a file from CBFS into memory. Like the similar command for
> fat filesystems, you can optionally provide a maximum size.
> 
> Also, if u-boot needs something out of CBFS very early before the heap is
> configured, it won't be able to use the normal CBFS support which caches
> some information in memory it allocates from the heap. This change adds a
> new cbfs_file_find_uncached function which searchs a CBFS instance without
> touching the heap.
> 
> Support for CBFS is compiled in when the CONFIG_CMD_CBFS option is
> specified.
> 
> Signed-off-by: Gabe Black <gabeblack@chromium.org>
> ---
> Changes in v2:
> Fix checkpatch problems, change around identifiers, and change printf to
> puts where possible.

There is still a checkpatch warning that should be fixed:

WARNING: do not add new typedefs
#853: FILE: include/cbfs.h:61:
+typedef const struct cbfs_cache_node *cbfs_file;


> --- a/Makefile
> +++ b/Makefile
> @@ -234,9 +234,9 @@ ifeq ($(CONFIG_OF_EMBED),y)
>  LIBS += dts/libdts.o
>  endif
>  LIBS += arch/$(ARCH)/lib/lib$(ARCH).o
> -LIBS += fs/cramfs/libcramfs.o fs/fat/libfat.o fs/fdos/libfdos.o fs/jffs2/libjffs2.o \
> -	fs/reiserfs/libreiserfs.o fs/ext2/libext2fs.o fs/yaffs2/libyaffs2.o \
> -	fs/ubifs/libubifs.o
> +LIBS += fs/cramfs/libcramfs.o fs/fat/libfat.o fs/fdos/libfdos.o \
> +	fs/jffs2/libjffs2.o fs/reiserfs/libreiserfs.o fs/ext2/libext2fs.o \
> +	fs/yaffs2/libyaffs2.o fs/ubifs/libubifs.o fs/cbfs/libcbfs.o

Please keep the list sorted. [It may make sense to split it into a
list of entries with one FS per line.]

> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -89,6 +89,7 @@ COBJS-$(CONFIG_ENV_IS_IN_EEPROM) += cmd_eeprom.o
>  COBJS-$(CONFIG_CMD_EEPROM) += cmd_eeprom.o
>  COBJS-$(CONFIG_CMD_ELF) += cmd_elf.o
>  COBJS-$(CONFIG_SYS_HUSH_PARSER) += cmd_exit.o
> +COBJS-$(CONFIG_CMD_CBFS) += cmd_cbfs.o
>  COBJS-$(CONFIG_CMD_EXT2) += cmd_ext2.o
>  COBJS-$(CONFIG_CMD_FAT) += cmd_fat.o
>  COBJS-$(CONFIG_CMD_FDC)$(CONFIG_CMD_FDOS) += cmd_fdc.o

Please keep sorted (by object file names).


> +int do_cbfs_init(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	uintptr_t end_of_rom = 0xffffffff;
> +	char *ep;
> +
> +	if (argc > 2) {
> +		puts("usage: cbfsls [end of rom]>\n");

What is the meaning / intention of that tailing '>' ?

> +	if (file_cbfs_result != CBFS_SUCCESS) {
> +		printf("%s.\n", file_cbfs_error());

Use
		puts(file_cbfs_error());
?

> +int do_cbfs_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
...
> +	if (!file) {
> +		if (file_cbfs_result == CBFS_FILE_NOT_FOUND)
> +			printf("%s: %s\n", file_cbfs_error(), argv[2]);
> +		else
> +			printf("%s.\n", file_cbfs_error());

See above. Please consider changing globally.

> +	printf("reading %s\n", file_cbfs_name(file));
> +
> +	size = file_cbfs_read(file, (void *)offset, count);
> +
> +	printf("\n%ld bytes read\n", size);

There should be no need for the initial newline here.  Please drop it
(fix globally?).

...
> +		if (type_name)
> +			printf("  %16s", type_name);
> +		else
> +			printf("  %16d", type);

This is probably confusing to the user.  How can he know if the file
type has the numerical value of "123" or if the file name is "123" ?

> +		if (filename[0])
> +			printf("  %s\n", filename);
> +		else
> +			printf("  %s\n", "(empty)");

Use puts().

> +	printf("\n%d file(s)\n\n", files);

Please do not print that many empty lines.

Imagine output is going to a QVGA display or similar which shows only
12 text lines or so...

> +int do_cbfs_fsinfo(cmd_tbl_t *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	const struct cbfs_header *header = file_cbfs_get_header();
> +	if (!header) {
> +		printf("%s.\n", file_cbfs_error());
> +		return 1;
> +	}

Please always insert a blank line between declarations and code.
Please fix globally.

> +	puts("\n");

What is this blank line good for?  Drop it!

> +	printf("CBFS version: %#x\n", header->version);
> +	printf("ROM size: %#x\n", header->rom_size);
> +	printf("Boot block size: %#x\n", header->boot_block_size);
> +	printf("CBFS size: %#x\n",
> +		header->rom_size - header->boot_block_size - header->offset);
> +	printf("Alignment: %d\n", header->align);
> +	printf("Offset: %#x\n", header->offset);

How about some vertical alignment of the output?

> +	puts("\n");

Drop that, too.

> +const char *file_cbfs_error(void)
> +{
> +	switch (file_cbfs_result) {
> +	case CBFS_SUCCESS:
> +		return "Success";
> +	case CBFS_NOT_INITIALIZED:
> +		return "CBFS not initialized";
> +	case CBFS_BAD_HEADER:
> +		return "Bad CBFS header";
> +	case CBFS_BAD_FILE:
> +		return "Bad CBFS file";
> +	case CBFS_FILE_NOT_FOUND:
> +		return "File not found";
> +	default:
> +		return "Unknown";

Better make this "Unknown error" or similar, otherwise the user might
not know what "Unknown" means.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A good aphorism is too hard for the tooth of time, and  is  not  worn
away  by  all  the  centuries,  although  it serves as food for every
epoch.                                  - Friedrich Wilhelm Nietzsche
                          _Miscellaneous Maxims and Opinions_ no. 168