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

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