Dear Gabe Black,
In message <1323134730-18471-1-git-send-email-gabeblack at 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 at 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