[U-Boot,V2,3/3] fs: add partition switch libary, implement ls and fsload commands

From: Stephen Warren <swarren@nvidia.com>

Implement "ls" and "fsload" commands that act like {fat,ext2}{ls,load},
and transparently handle either file-system. This scheme could easily be
extended to other filesystem types; I only didn't do it for zfs because
I don't have any filesystems of that type to test with.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2:
* Build cmd_fs.c when CONFIG_CMD_FS_GENERIC not always.
* Use new CONFIG_FS_{FAT,EXT4} rather than CONFIG_CMD_{FAT,EXT2}.
* Implemented fs_close() and call it from the tail of fs_ls() and fs_read().
  This ensures that any other usage of e.g. the ext4 library between fs_*()
  calls, then the two usages won't interfere.
* Re-organized fs.c to reduce the number of ifdef blocks.
* Added argc checking to do_ls().
---
 Makefile        |    3 +-
 common/Makefile |    1 +
 common/cmd_fs.c |   89 ++++++++++++++++++++++
 fs/Makefile     |   47 +++++++++++
 fs/fs.c         |  227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/fs.h    |   49 ++++++++++++
 6 files changed, 415 insertions(+), 1 deletions(-)
 create mode 100644 common/cmd_fs.c
 create mode 100644 fs/Makefile
 create mode 100644 fs/fs.c
 create mode 100644 include/fs.h

Hi Stephen,

On Thursday, October 11, 2012 8:59:29 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Implement "ls" and "fsload" commands that act like
> {fat,ext2}{ls,load},
> and transparently handle either file-system. This scheme could easily
> be
> extended to other filesystem types; I only didn't do it for zfs
> because
> I don't have any filesystems of that type to test with.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v2:
> * Build cmd_fs.c when CONFIG_CMD_FS_GENERIC not always.
> * Use new CONFIG_FS_{FAT,EXT4} rather than CONFIG_CMD_{FAT,EXT2}.
> * Implemented fs_close() and call it from the tail of fs_ls() and
> fs_read().
>   This ensures that any other usage of e.g. the ext4 library between
>   fs_*()
>   calls, then the two usages won't interfere.
> * Re-organized fs.c to reduce the number of ifdef blocks.
> * Added argc checking to do_ls().
> ---
>  Makefile        |    3 +-
>  common/Makefile |    1 +
>  common/cmd_fs.c |   89 ++++++++++++++++++++++
>  fs/Makefile     |   47 +++++++++++
>  fs/fs.c         |  227
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fs.h    |   49 ++++++++++++
>  6 files changed, 415 insertions(+), 1 deletions(-)
>  create mode 100644 common/cmd_fs.c
>  create mode 100644 fs/Makefile
>  create mode 100644 fs/fs.c
>  create mode 100644 include/fs.h
> 
> diff --git a/Makefile b/Makefile
> index d7e2c2f..0b50eb7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -242,7 +242,8 @@ LIBS-y += drivers/net/npe/libnpe.o
>  endif
>  LIBS-$(CONFIG_OF_EMBED) += dts/libdts.o
>  LIBS-y += arch/$(ARCH)/lib/lib$(ARCH).o
> -LIBS-y += fs/cramfs/libcramfs.o \
> +LIBS-y += fs/libfs.o \
> +	fs/cramfs/libcramfs.o \
>  	fs/ext4/libext4fs.o \
>  	fs/fat/libfat.o \
>  	fs/fdos/libfdos.o \
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..c6c31f7 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -103,6 +103,7 @@ COBJS-$(CONFIG_CMD_FLASH) += cmd_flash.o
>  ifdef CONFIG_FPGA
>  COBJS-$(CONFIG_CMD_FPGA) += cmd_fpga.o
>  endif
> +COBJS-$(CONFIG_CMD_FS_GENERIC) += cmd_fs.o
>  COBJS-$(CONFIG_CMD_GPIO) += cmd_gpio.o
>  COBJS-$(CONFIG_CMD_I2C) += cmd_i2c.o
>  COBJS-$(CONFIG_CMD_IDE) += cmd_ide.o
> diff --git a/common/cmd_fs.c b/common/cmd_fs.c
> new file mode 100644
> index 0000000..9df0a66
> --- /dev/null
> +++ b/common/cmd_fs.c
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Inspired by cmd_ext_common.c, cmd_fat.c.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <fs.h>
> +
> +int do_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char * const
> argv[])
> +{
> +	unsigned long addr;
> +	unsigned long bytes;
> +	unsigned long pos;
> +	int len_read;
> +	char buf[12];
> +
> +	if (argc < 5)
> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], argv[2]))
> +		return 1;
> +
> +	addr = simple_strtoul(argv[3], NULL, 0);
> +	if (argc >= 6)
> +		bytes = simple_strtoul(argv[5], NULL, 0);
> +	else
> +		bytes = 0;
> +	if (argc >= 7)
> +		pos = simple_strtoul(argv[6], NULL, 0);
> +	else
> +		pos = 0;
> +
> +	len_read = fs_read(argv[4], addr, pos, bytes);
> +	if (len_read <= 0)
> +		return 1;
> +
> +	sprintf(buf, "0x%x", (unsigned int)len_read);
> +	setenv("filesize", buf);
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	fsload,	7,	0,	do_fsload,
> +	"load binary file from a filesystem",
> +	"<interface> [<dev[:part]>] <addr> <filename> [bytes [pos]]\n"
> +	"    - Load binary file 'filename' from partition 'part' on
> device\n"
> +	"       type 'interface' instance 'dev' to address 'addr' in
> memory.\n"
> +	"      'bytes' gives the size to load in bytes.\n"
> +	"      If 'bytes' is 0 or omitted, the file is read until the
> end.\n"
> +	"      'pos' gives the file byte position to start reading from.\n"
> +	"      If 'pos' is 0 or omitted, the file is read from the start."
> +);
> +
> +int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], (argc >= 3) ? argv[2] : NULL))
> +		return 1;
> +
> +	if (fs_ls(argc == 4 ? argv[3] : "/"))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	ls,	4,	1,	do_ls,
> +	"list files in a directory (default /)",
> +	"<interface> [<dev[:part]>] [directory]\n"
> +	"    - List files in directory 'directory' of partition 'part'
> on\n"
> +	"      device type 'interface' instance 'dev'."
> +);
> diff --git a/fs/Makefile b/fs/Makefile
> new file mode 100644
> index 0000000..d0ab3ae
> --- /dev/null
> +++ b/fs/Makefile
> @@ -0,0 +1,47 @@
> +#
> +# (C) Copyright 2000-2006
> +# Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> +# Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB	= $(obj)libfs.o
> +
> +COBJS-y				+= fs.o
> +
> +COBJS	:= $(COBJS-y)
> +SRCS	:= $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS))
> +
> +all:	$(LIB)
> +
> +$(LIB):	$(obj).depend $(OBJS)
> +	$(call cmd_link_o_target, $(OBJS))
> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/fs/fs.c b/fs/fs.c
> new file mode 100644
> index 0000000..18cfaff
> --- /dev/null
> +++ b/fs/fs.c
> @@ -0,0 +1,227 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <part.h>
> +#include <ext4fs.h>
> +#include <fat.h>
> +#include <fs.h>
> +
> +#define FS_TYPE_INVALID	0
> +#define FS_TYPE_FAT	1
> +#define FS_TYPE_EXT	2
> +
> +static block_dev_desc_t *fs_dev_desc;
> +static disk_partition_t fs_partition;
> +static int fs_type = FS_TYPE_INVALID;
> +
> +static inline int fs_ls_unsupported(const char *dirname)
> +{
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +static inline int fs_read_unsupported(const char *filename, ulong
> addr,
> +				      int offset, int len)
> +{
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +#ifdef CONFIG_FS_FAT
> +static int fs_probe_fat(void)
> +{
> +	return fat_set_blk_dev(fs_dev_desc, &fs_partition);
> +}
> +
> +static void fs_close_fat(void)
> +{
> +}
> +
> +#define fs_ls_fat file_fat_ls
> +
> +static int fs_read_fat(const char *filename, ulong addr, int offset,
> int len)
> +{
> +	int len_read;
> +
> +	len_read = file_fat_read_at(filename, offset,
> +				    (unsigned char *)addr, len);
> +	if (len_read == -1) {
> +		printf("** Unable to read file %s **\n", filename);
> +		return -1;
> +	}
> +
> +	return len_read;
> +}
> +#else
> +static inline int fs_probe_fat(void)
> +{
> +	return -1;
> +}
> +
> +static inline void fs_close_fat(void)
> +{
> +}
> +
> +#define fs_ls_fat fs_ls_unsupported
> +#define fs_read_fat fs_read_unsupported
> +#endif
> +
> +#ifdef CONFIG_FS_EXT4
> +static int fs_probe_ext(void)
> +{
> +	ext4fs_set_blk_dev(fs_dev_desc, &fs_partition);
> +
> +	if (!ext4fs_mount(fs_partition.size)) {
> +		ext4fs_close();
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fs_close_ext(void)
> +{
> +	ext4fs_close();
> +}
> +
> +#define fs_ls_ext ext4fs_ls
> +
> +static int fs_read_ext(const char *filename, ulong addr, int offset,
> int len)
> +{
> +	int file_len;
> +	int len_read;
> +
> +	if (offset != 0) {
> +		printf("** Cannot support non-zero offset **\n");
> +		return -1;
> +	}
> +
> +	file_len = ext4fs_open(filename);
> +	if (file_len < 0) {
> +		printf("** File not found %s **\n", filename);
> +		ext4fs_close();
> +		return -1;
> +	}
> +
> +	if (len == 0)
> +		len = file_len;
> +
> +	len_read = ext4fs_read((char *)addr, len);
> +	ext4fs_close();
> +
> +	if (len_read != len) {
> +		printf("** Unable to read file %s **\n", filename);
> +		return -1;
> +	}
> +
> +	return len_read;
> +}
> +#else
> +static inline int fs_probe_ext(void)
> +{
> +	return -1;
> +}
> +
> +static inline void fs_close_ext(void)
> +{
> +}
> +
> +#define fs_ls_ext fs_ls_unsupported
> +#define fs_read_ext fs_read_unsupported
> +#endif
> +
> +int fs_set_blk_dev(const char *ifname, const char *dev_part_str)
> +{
> +	int part;
> +
> +	part = get_device_and_partition(ifname, dev_part_str, &fs_dev_desc,
> +					&fs_partition, 1);
> +	if (part < 0)
> +		return -1;
> +
> +	if (!fs_probe_fat()) {
> +		fs_type = FS_TYPE_FAT;
> +		return 0;
> +	}
> +
> +	if (!fs_probe_ext()) {
> +		fs_type = FS_TYPE_EXT;
> +		return 0;
> +	}
> +
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +static void fs_close(void)
> +{
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		fs_close_fat();
> +		break;
> +	case FS_TYPE_EXT:
> +		fs_close_ext();
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	fs_type = FS_TYPE_INVALID;
> +}
> +
> +int fs_ls(const char *dirname)
> +{
> +	int ret;
> +
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		ret = fs_ls_fat(dirname);
> +		break;
> +	case FS_TYPE_EXT:
> +		ret = fs_ls_ext(dirname);
> +		break;
> +	default:
> +		ret = fs_ls_unsupported(dirname);
> +		break;
> +	}
> +
> +	fs_close();
> +
> +	return ret;
> +}
> +
> +int fs_read(const char *filename, ulong addr, int offset, int len)
> +{
> +	int ret;
> +
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		ret = fs_read_fat(filename, addr, offset, len);
> +		break;
> +	case FS_TYPE_EXT:
> +		ret = fs_read_ext(filename, addr, offset, len);
> +		break;
> +	default:
> +		ret = fs_read_unsupported(filename, addr, offset, len);
> +		break;
> +	}
> +
> +	fs_close();
> +
> +	return ret;
> +}
> diff --git a/include/fs.h b/include/fs.h
> new file mode 100644
> index 0000000..a0fda28
> --- /dev/null
> +++ b/include/fs.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see
> <http://www.gnu.org/licenses/>.
> + */
> +#ifndef _FS_H
> +#define _FS_H
> +
> +/*
> + * Tell the fs layer which block device an partition to use for
> future
> + * commands. This also internally identifies the filesystem that is
> present
> + * within the partition.
> + *
> + * Returns 0 on success.
> + * Returns non-zero if there is an error accessing the disk or
> partition, or
> + * no known filesystem type could be recognized on it.
> + */
> +int fs_set_blk_dev(const char *ifname, const char *dev_part_str);
> +
> +/*
> + * Print the list of files on the partition previously set by
> fs_set_blk_dev(),
> + * in directory "dirname".
> + *
> + * Returns 0 on success. Returns non-zero on error.
> + */
> +int fs_ls(const char *dirname);
> +
> +/*
> + * Read file "filename" from the partition previously set by
> fs_set_blk_dev(),
> + * to address "addr", starting at byte offset "offset", and reading
> "len"
> + * bytes. "offset" may be 0 to read from the start of the file.
> "len" may be
> + * 0 to read the entire file. Note that not all filesystem types
> support
> + * either/both offset!=0 or len!=0.
> + *
> + * Returns number of bytes read on success. Returns <= 0 on error.
> + */
> +int fs_read(const char *filename, ulong addr, int offset, int len);
> +
> +#endif /* _FS_H */

I'm fine with this new version of the series:
Reviewed-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>

Best regards,
Benoît

On 10/11/2012 01:59 PM, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Implement "ls" and "fsload" commands that act like {fat,ext2}{ls,load},
> and transparently handle either file-system. This scheme could easily be
> extended to other filesystem types; I only didn't do it for zfs because
> I don't have any filesystems of that type to test with.
> 

This is exactly what I want to get to. However, I think the first step
should be to unify the filesystem api similar to the block device api.
This would avoid the wrapper here and yet another copy of nearly
identical code. Then we can unify the implementations of *ls and *load.

Rob

> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v2:
> * Build cmd_fs.c when CONFIG_CMD_FS_GENERIC not always.
> * Use new CONFIG_FS_{FAT,EXT4} rather than CONFIG_CMD_{FAT,EXT2}.
> * Implemented fs_close() and call it from the tail of fs_ls() and fs_read().
>   This ensures that any other usage of e.g. the ext4 library between fs_*()
>   calls, then the two usages won't interfere.
> * Re-organized fs.c to reduce the number of ifdef blocks.
> * Added argc checking to do_ls().
> ---
>  Makefile        |    3 +-
>  common/Makefile |    1 +
>  common/cmd_fs.c |   89 ++++++++++++++++++++++
>  fs/Makefile     |   47 +++++++++++
>  fs/fs.c         |  227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/fs.h    |   49 ++++++++++++
>  6 files changed, 415 insertions(+), 1 deletions(-)
>  create mode 100644 common/cmd_fs.c
>  create mode 100644 fs/Makefile
>  create mode 100644 fs/fs.c
>  create mode 100644 include/fs.h
> 
> diff --git a/Makefile b/Makefile
> index d7e2c2f..0b50eb7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -242,7 +242,8 @@ LIBS-y += drivers/net/npe/libnpe.o
>  endif
>  LIBS-$(CONFIG_OF_EMBED) += dts/libdts.o
>  LIBS-y += arch/$(ARCH)/lib/lib$(ARCH).o
> -LIBS-y += fs/cramfs/libcramfs.o \
> +LIBS-y += fs/libfs.o \
> +	fs/cramfs/libcramfs.o \
>  	fs/ext4/libext4fs.o \
>  	fs/fat/libfat.o \
>  	fs/fdos/libfdos.o \
> diff --git a/common/Makefile b/common/Makefile
> index 125b2be..c6c31f7 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -103,6 +103,7 @@ COBJS-$(CONFIG_CMD_FLASH) += cmd_flash.o
>  ifdef CONFIG_FPGA
>  COBJS-$(CONFIG_CMD_FPGA) += cmd_fpga.o
>  endif
> +COBJS-$(CONFIG_CMD_FS_GENERIC) += cmd_fs.o
>  COBJS-$(CONFIG_CMD_GPIO) += cmd_gpio.o
>  COBJS-$(CONFIG_CMD_I2C) += cmd_i2c.o
>  COBJS-$(CONFIG_CMD_IDE) += cmd_ide.o
> diff --git a/common/cmd_fs.c b/common/cmd_fs.c
> new file mode 100644
> index 0000000..9df0a66
> --- /dev/null
> +++ b/common/cmd_fs.c
> @@ -0,0 +1,89 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * Inspired by cmd_ext_common.c, cmd_fat.c.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <fs.h>
> +
> +int do_fsload(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	unsigned long addr;
> +	unsigned long bytes;
> +	unsigned long pos;
> +	int len_read;
> +	char buf[12];
> +
> +	if (argc < 5)
> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], argv[2]))
> +		return 1;
> +
> +	addr = simple_strtoul(argv[3], NULL, 0);
> +	if (argc >= 6)
> +		bytes = simple_strtoul(argv[5], NULL, 0);
> +	else
> +		bytes = 0;
> +	if (argc >= 7)
> +		pos = simple_strtoul(argv[6], NULL, 0);
> +	else
> +		pos = 0;
> +
> +	len_read = fs_read(argv[4], addr, pos, bytes);
> +	if (len_read <= 0)
> +		return 1;
> +
> +	sprintf(buf, "0x%x", (unsigned int)len_read);
> +	setenv("filesize", buf);
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	fsload,	7,	0,	do_fsload,
> +	"load binary file from a filesystem",
> +	"<interface> [<dev[:part]>] <addr> <filename> [bytes [pos]]\n"
> +	"    - Load binary file 'filename' from partition 'part' on device\n"
> +	"       type 'interface' instance 'dev' to address 'addr' in memory.\n"
> +	"      'bytes' gives the size to load in bytes.\n"
> +	"      If 'bytes' is 0 or omitted, the file is read until the end.\n"
> +	"      'pos' gives the file byte position to start reading from.\n"
> +	"      If 'pos' is 0 or omitted, the file is read from the start."
> +);
> +
> +int do_ls(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	if (fs_set_blk_dev(argv[1], (argc >= 3) ? argv[2] : NULL))
> +		return 1;
> +
> +	if (fs_ls(argc == 4 ? argv[3] : "/"))
> +		return 1;
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(
> +	ls,	4,	1,	do_ls,
> +	"list files in a directory (default /)",
> +	"<interface> [<dev[:part]>] [directory]\n"
> +	"    - List files in directory 'directory' of partition 'part' on\n"
> +	"      device type 'interface' instance 'dev'."
> +);
> diff --git a/fs/Makefile b/fs/Makefile
> new file mode 100644
> index 0000000..d0ab3ae
> --- /dev/null
> +++ b/fs/Makefile
> @@ -0,0 +1,47 @@
> +#
> +# (C) Copyright 2000-2006
> +# Wolfgang Denk, DENX Software Engineering, wd@denx.de.
> +# Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> +#
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB	= $(obj)libfs.o
> +
> +COBJS-y				+= fs.o
> +
> +COBJS	:= $(COBJS-y)
> +SRCS	:= $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS))
> +
> +all:	$(LIB)
> +
> +$(LIB):	$(obj).depend $(OBJS)
> +	$(call cmd_link_o_target, $(OBJS))
> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/fs/fs.c b/fs/fs.c
> new file mode 100644
> index 0000000..18cfaff
> --- /dev/null
> +++ b/fs/fs.c
> @@ -0,0 +1,227 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <config.h>
> +#include <common.h>
> +#include <part.h>
> +#include <ext4fs.h>
> +#include <fat.h>
> +#include <fs.h>
> +
> +#define FS_TYPE_INVALID	0
> +#define FS_TYPE_FAT	1
> +#define FS_TYPE_EXT	2
> +
> +static block_dev_desc_t *fs_dev_desc;
> +static disk_partition_t fs_partition;
> +static int fs_type = FS_TYPE_INVALID;
> +
> +static inline int fs_ls_unsupported(const char *dirname)
> +{
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +static inline int fs_read_unsupported(const char *filename, ulong addr,
> +				      int offset, int len)
> +{
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +#ifdef CONFIG_FS_FAT
> +static int fs_probe_fat(void)
> +{
> +	return fat_set_blk_dev(fs_dev_desc, &fs_partition);
> +}
> +
> +static void fs_close_fat(void)
> +{
> +}
> +
> +#define fs_ls_fat file_fat_ls
> +
> +static int fs_read_fat(const char *filename, ulong addr, int offset, int len)
> +{
> +	int len_read;
> +
> +	len_read = file_fat_read_at(filename, offset,
> +				    (unsigned char *)addr, len);
> +	if (len_read == -1) {
> +		printf("** Unable to read file %s **\n", filename);
> +		return -1;
> +	}
> +
> +	return len_read;
> +}
> +#else
> +static inline int fs_probe_fat(void)
> +{
> +	return -1;
> +}
> +
> +static inline void fs_close_fat(void)
> +{
> +}
> +
> +#define fs_ls_fat fs_ls_unsupported
> +#define fs_read_fat fs_read_unsupported
> +#endif
> +
> +#ifdef CONFIG_FS_EXT4
> +static int fs_probe_ext(void)
> +{
> +	ext4fs_set_blk_dev(fs_dev_desc, &fs_partition);
> +
> +	if (!ext4fs_mount(fs_partition.size)) {
> +		ext4fs_close();
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void fs_close_ext(void)
> +{
> +	ext4fs_close();
> +}
> +
> +#define fs_ls_ext ext4fs_ls
> +
> +static int fs_read_ext(const char *filename, ulong addr, int offset, int len)
> +{
> +	int file_len;
> +	int len_read;
> +
> +	if (offset != 0) {
> +		printf("** Cannot support non-zero offset **\n");
> +		return -1;
> +	}
> +
> +	file_len = ext4fs_open(filename);
> +	if (file_len < 0) {
> +		printf("** File not found %s **\n", filename);
> +		ext4fs_close();
> +		return -1;
> +	}
> +
> +	if (len == 0)
> +		len = file_len;
> +
> +	len_read = ext4fs_read((char *)addr, len);
> +	ext4fs_close();
> +
> +	if (len_read != len) {
> +		printf("** Unable to read file %s **\n", filename);
> +		return -1;
> +	}
> +
> +	return len_read;
> +}
> +#else
> +static inline int fs_probe_ext(void)
> +{
> +	return -1;
> +}
> +
> +static inline void fs_close_ext(void)
> +{
> +}
> +
> +#define fs_ls_ext fs_ls_unsupported
> +#define fs_read_ext fs_read_unsupported
> +#endif
> +
> +int fs_set_blk_dev(const char *ifname, const char *dev_part_str)
> +{
> +	int part;
> +
> +	part = get_device_and_partition(ifname, dev_part_str, &fs_dev_desc,
> +					&fs_partition, 1);
> +	if (part < 0)
> +		return -1;
> +
> +	if (!fs_probe_fat()) {
> +		fs_type = FS_TYPE_FAT;
> +		return 0;
> +	}
> +
> +	if (!fs_probe_ext()) {
> +		fs_type = FS_TYPE_EXT;
> +		return 0;
> +	}
> +
> +	printf("** Unrecognized filesystem type **\n");
> +	return -1;
> +}
> +
> +static void fs_close(void)
> +{
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		fs_close_fat();
> +		break;
> +	case FS_TYPE_EXT:
> +		fs_close_ext();
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	fs_type = FS_TYPE_INVALID;
> +}
> +
> +int fs_ls(const char *dirname)
> +{
> +	int ret;
> +
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		ret = fs_ls_fat(dirname);
> +		break;
> +	case FS_TYPE_EXT:
> +		ret = fs_ls_ext(dirname);
> +		break;
> +	default:
> +		ret = fs_ls_unsupported(dirname);
> +		break;
> +	}
> +
> +	fs_close();
> +
> +	return ret;
> +}
> +
> +int fs_read(const char *filename, ulong addr, int offset, int len)
> +{
> +	int ret;
> +
> +	switch (fs_type) {
> +	case FS_TYPE_FAT:
> +		ret = fs_read_fat(filename, addr, offset, len);
> +		break;
> +	case FS_TYPE_EXT:
> +		ret = fs_read_ext(filename, addr, offset, len);
> +		break;
> +	default:
> +		ret = fs_read_unsupported(filename, addr, offset, len);
> +		break;
> +	}
> +
> +	fs_close();
> +
> +	return ret;
> +}
> diff --git a/include/fs.h b/include/fs.h
> new file mode 100644
> index 0000000..a0fda28
> --- /dev/null
> +++ b/include/fs.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef _FS_H
> +#define _FS_H
> +
> +/*
> + * Tell the fs layer which block device an partition to use for future
> + * commands. This also internally identifies the filesystem that is present
> + * within the partition.
> + *
> + * Returns 0 on success.
> + * Returns non-zero if there is an error accessing the disk or partition, or
> + * no known filesystem type could be recognized on it.
> + */
> +int fs_set_blk_dev(const char *ifname, const char *dev_part_str);
> +
> +/*
> + * Print the list of files on the partition previously set by fs_set_blk_dev(),
> + * in directory "dirname".
> + *
> + * Returns 0 on success. Returns non-zero on error.
> + */
> +int fs_ls(const char *dirname);
> +
> +/*
> + * Read file "filename" from the partition previously set by fs_set_blk_dev(),
> + * to address "addr", starting at byte offset "offset", and reading "len"
> + * bytes. "offset" may be 0 to read from the start of the file. "len" may be
> + * 0 to read the entire file. Note that not all filesystem types support
> + * either/both offset!=0 or len!=0.
> + *
> + * Returns number of bytes read on success. Returns <= 0 on error.
> + */
> +int fs_read(const char *filename, ulong addr, int offset, int len);
> +
> +#endif /* _FS_H */
>

On 10/15/2012 10:33 AM, Rob Herring wrote:
> On 10/11/2012 01:59 PM, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>>
>> Implement "ls" and "fsload" commands that act like {fat,ext2}{ls,load},
>> and transparently handle either file-system. This scheme could easily be
>> extended to other filesystem types; I only didn't do it for zfs because
>> I don't have any filesystems of that type to test with.
>>
> 
> This is exactly what I want to get to.

That's good.

> However, I think the first step
> should be to unify the filesystem api similar to the block device api.
> This would avoid the wrapper here and yet another copy of nearly
> identical code. Then we can unify the implementations of *ls and *load.

I think we will always need some form of wrapper.

At the very least, we will need fs_set_blk_dev() (or a function that
does the same thing under a different name) in order to probe which type
of filesystem is present, just like we have get_partition_info() for
partitions, which scans for all known forms of partition table.

The only way to avoid wrappers for the other functions (ls, load) would
be if fs_set_blk_dev() were to set up some global variable pointing at
the filesystem implementation struct. If it did that, client code could
call directly into the filesystem rather than calling into the wrapper
functions to indirect to the correct filesystem. This might be a
reasonable design, although even if that is the long-term plan, I don't
think it precludes using the wrapper approach first, then refactoring
the wrapper away as functionality (e.g. fs_{probe,close}_{fat,ext4})
moves into the filesystem implementations; this seems like a good first
step on the way.

On Mon, Oct 15, 2012 at 10:47:49AM -0600, Stephen Warren wrote:
> On 10/15/2012 10:33 AM, Rob Herring wrote:
> > On 10/11/2012 01:59 PM, Stephen Warren wrote:
> >> From: Stephen Warren <swarren@nvidia.com>
> >>
> >> Implement "ls" and "fsload" commands that act like {fat,ext2}{ls,load},
> >> and transparently handle either file-system. This scheme could easily be
> >> extended to other filesystem types; I only didn't do it for zfs because
> >> I don't have any filesystems of that type to test with.
> >>
> > 
> > This is exactly what I want to get to.
> 
> That's good.
> 
> > However, I think the first step
> > should be to unify the filesystem api similar to the block device api.
> > This would avoid the wrapper here and yet another copy of nearly
> > identical code. Then we can unify the implementations of *ls and *load.
> 
> I think we will always need some form of wrapper.
> 
> At the very least, we will need fs_set_blk_dev() (or a function that
> does the same thing under a different name) in order to probe which type
> of filesystem is present, just like we have get_partition_info() for
> partitions, which scans for all known forms of partition table.
> 
> The only way to avoid wrappers for the other functions (ls, load) would
> be if fs_set_blk_dev() were to set up some global variable pointing at
> the filesystem implementation struct. If it did that, client code could
> call directly into the filesystem rather than calling into the wrapper
> functions to indirect to the correct filesystem. This might be a
> reasonable design, although even if that is the long-term plan, I don't
> think it precludes using the wrapper approach first, then refactoring
> the wrapper away as functionality (e.g. fs_{probe,close}_{fat,ext4})
> moves into the filesystem implementations; this seems like a good first
> step on the way.

Baring further discussion, I intend to grab this really soon, as it
sounds like it's a functional starting point, however we wish to make
this happen.  Or am I not following?  Thanks!

On 10/18/2012 06:01 PM, Tom Rini wrote:
> On Mon, Oct 15, 2012 at 10:47:49AM -0600, Stephen Warren wrote:
>> On 10/15/2012 10:33 AM, Rob Herring wrote:
>>> On 10/11/2012 01:59 PM, Stephen Warren wrote:
>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>
>>>> Implement "ls" and "fsload" commands that act like {fat,ext2}{ls,load},
>>>> and transparently handle either file-system. This scheme could easily be
>>>> extended to other filesystem types; I only didn't do it for zfs because
>>>> I don't have any filesystems of that type to test with.
>>>>
>>>
>>> This is exactly what I want to get to.
>>
>> That's good.
>>
>>> However, I think the first step
>>> should be to unify the filesystem api similar to the block device api.
>>> This would avoid the wrapper here and yet another copy of nearly
>>> identical code. Then we can unify the implementations of *ls and *load.
>>
>> I think we will always need some form of wrapper.
>>
>> At the very least, we will need fs_set_blk_dev() (or a function that
>> does the same thing under a different name) in order to probe which type
>> of filesystem is present, just like we have get_partition_info() for
>> partitions, which scans for all known forms of partition table.
>>
>> The only way to avoid wrappers for the other functions (ls, load) would
>> be if fs_set_blk_dev() were to set up some global variable pointing at
>> the filesystem implementation struct. If it did that, client code could
>> call directly into the filesystem rather than calling into the wrapper
>> functions to indirect to the correct filesystem. This might be a
>> reasonable design, although even if that is the long-term plan, I don't
>> think it precludes using the wrapper approach first, then refactoring
>> the wrapper away as functionality (e.g. fs_{probe,close}_{fat,ext4})
>> moves into the filesystem implementations; this seems like a good first
>> step on the way.
> 
> Baring further discussion, I intend to grab this really soon, as it
> sounds like it's a functional starting point, however we wish to make
> this happen.  Or am I not following?  Thanks!

It's your call. I'd rather see clean-up first and features second, but
that's just me. Either way works. The amount of duplication in u-boot
just annoys me. Hopefully the DM work will fix some of it.

Rob

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/18/12 16:12, Rob Herring wrote:
> On 10/18/2012 06:01 PM, Tom Rini wrote:
>> On Mon, Oct 15, 2012 at 10:47:49AM -0600, Stephen Warren wrote:
>>> On 10/15/2012 10:33 AM, Rob Herring wrote:
>>>> On 10/11/2012 01:59 PM, Stephen Warren wrote:
>>>>> From: Stephen Warren <swarren@nvidia.com>
>>>>> 
>>>>> Implement "ls" and "fsload" commands that act like
>>>>> {fat,ext2}{ls,load}, and transparently handle either
>>>>> file-system. This scheme could easily be extended to other
>>>>> filesystem types; I only didn't do it for zfs because I
>>>>> don't have any filesystems of that type to test with.
>>>>> 
>>>> 
>>>> This is exactly what I want to get to.
>>> 
>>> That's good.
>>> 
>>>> However, I think the first step should be to unify the
>>>> filesystem api similar to the block device api. This would
>>>> avoid the wrapper here and yet another copy of nearly 
>>>> identical code. Then we can unify the implementations of *ls
>>>> and *load.
>>> 
>>> I think we will always need some form of wrapper.
>>> 
>>> At the very least, we will need fs_set_blk_dev() (or a function
>>> that does the same thing under a different name) in order to
>>> probe which type of filesystem is present, just like we have
>>> get_partition_info() for partitions, which scans for all known
>>> forms of partition table.
>>> 
>>> The only way to avoid wrappers for the other functions (ls,
>>> load) would be if fs_set_blk_dev() were to set up some global
>>> variable pointing at the filesystem implementation struct. If
>>> it did that, client code could call directly into the
>>> filesystem rather than calling into the wrapper functions to
>>> indirect to the correct filesystem. This might be a reasonable
>>> design, although even if that is the long-term plan, I don't 
>>> think it precludes using the wrapper approach first, then
>>> refactoring the wrapper away as functionality (e.g.
>>> fs_{probe,close}_{fat,ext4}) moves into the filesystem
>>> implementations; this seems like a good first step on the way.
>> 
>> Baring further discussion, I intend to grab this really soon, as
>> it sounds like it's a functional starting point, however we wish
>> to make this happen.  Or am I not following?  Thanks!
> 
> It's your call. I'd rather see clean-up first and features second,
> but that's just me. Either way works. The amount of duplication in
> u-boot just annoys me. Hopefully the DM work will fix some of it.

I too would like to see more clean-up, and I do expect progress down
that line, in this area, from Stephen as part of this being accepted.
 Unless the two of you would like to start collaborating on a slightly
different path?

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQgI9vAAoJENk4IS6UOR1WLWgP/2MTdElBAG+EYGY8aoLuT0yA
HSLRjz63xdM2Y2sQnI5OTyKQjuAGPjpGlZg9UHkrbBiyGixdcZwjZ+zZlVsjUIRC
eL9Zd5SByyZkYdw+d5BHFt2R6nTS7PTLGxKcfGnz03NKgY8+jP3gGjkY8xx6Egv8
z6SOxSPXAM16A2YZBkR+1JIR58ZQ4rBVTl1pOuFoFugC11ALcpCYqDfkCc/iOcSj
IPdD9/hfU75NBTHuLRyeBjO0s2Z5OTrPR/SNyQhCxouLxiIRyBvACIbKxBHJByl8
8e8W5k+zBw5+FvA+ioh7JCKFNzgwDf7rVc4hIX98lrfvB+bUUJ8AJrLdDVhybf84
BdDSpJ8Pq0ShQZXw3DbylnHhG8H3JuoH0ZfqHE8ws0IfdQQ8QhEosiV1h8x9sc0u
zBLPY9ZkF75HKoHi0B0Aq+atzSUOy2I5PpsaeaX1Lyi4tLhsupE5nXuS1Pv8Jp+3
zwPoGRVZavpEfhvcrNNCTdfWDaTuhzOEM/PvGHrJQimpSw7YC03617muMjwVNk4B
uJVY9k5iuNCpQOiAxm7/XV0+L9U2FFpj733f6w85p0bCLRnj2JTIyLy6bGNFJlP7
S3BLnPaHgbH90dRJRnvnnIT31Qove/9/2DxKIE1DYWmfULPNPTVkscUigA5Bw/16
BYU/4XZShmDnzPHJBTef
=OMSB
-----END PGP SIGNATURE-----

On 10/18/2012 05:23 PM, Tom Rini wrote:
> On 10/18/12 16:12, Rob Herring wrote:
>> On 10/18/2012 06:01 PM, Tom Rini wrote:
...
>>>>> On 10/11/2012 01:59 PM, Stephen Warren wrote:
>>>>>> Implement "ls" and "fsload" commands that act like 
>>>>>> {fat,ext2}{ls,load}, and transparently handle either 
>>>>>> file-system. This scheme could easily be extended to
>>>>>> other filesystem types; I only didn't do it for zfs
>>>>>> because I don't have any filesystems of that type to test
>>>>>> with.
...
>>> Baring further discussion, I intend to grab this really soon,
>>> as it sounds like it's a functional starting point, however we
>>> wish to make this happen.  Or am I not following?  Thanks!
> 
>> It's your call. I'd rather see clean-up first and features
>> second, but that's just me. Either way works. The amount of
>> duplication in u-boot just annoys me. Hopefully the DM work will
>> fix some of it.
> 
> I too would like to see more clean-up,

Which clean-up exactly?

The only duplication I see here is that ext2load/fatload could be
modified to simply call into do_fsload. That'd be pretty simple, I
think, assuming the behaviour change was OK (e.g. fatload would
suddenly support either FAT or ext2*), and that cmd_fs.c and fs.c
would both always be pulled in.

Re: refactoring of the interface to the filesystem code: I'm curious
what the DM-related plans are for filesystems. It seems that any such
refactoring would be part of that work. Unfortunately I haven't been
paying any attention to who might be proposing doing what and when
there. Would it be appropriate to defer any fs-related API changes
until any DM+fs rework went it to avoid conflicts or duplicate work?

On 10/19/2012 11:56 AM, Stephen Warren wrote:
> On 10/18/2012 05:23 PM, Tom Rini wrote:
>> On 10/18/12 16:12, Rob Herring wrote:
>>> On 10/18/2012 06:01 PM, Tom Rini wrote:
> ...
>>>>>> On 10/11/2012 01:59 PM, Stephen Warren wrote:
>>>>>>> Implement "ls" and "fsload" commands that act like 
>>>>>>> {fat,ext2}{ls,load}, and transparently handle either 
>>>>>>> file-system. This scheme could easily be extended to
>>>>>>> other filesystem types; I only didn't do it for zfs
>>>>>>> because I don't have any filesystems of that type to test
>>>>>>> with.
> ...
>>>> Baring further discussion, I intend to grab this really soon,
>>>> as it sounds like it's a functional starting point, however we
>>>> wish to make this happen.  Or am I not following?  Thanks!
>>
>>> It's your call. I'd rather see clean-up first and features
>>> second, but that's just me. Either way works. The amount of
>>> duplication in u-boot just annoys me. Hopefully the DM work will
>>> fix some of it.
>>
>> I too would like to see more clean-up,
> 
> Which clean-up exactly?
> 
> The only duplication I see here is that ext2load/fatload could be
> modified to simply call into do_fsload. That'd be pretty simple, I
> think, assuming the behaviour change was OK (e.g. fatload would
> suddenly support either FAT or ext2*), and that cmd_fs.c and fs.c
> would both always be pulled in.

Can't you make do_fsload support either specifying the fs for legacy use
or detecting it on the new commands?

> Re: refactoring of the interface to the filesystem code: I'm curious
> what the DM-related plans are for filesystems. It seems that any such
> refactoring would be part of that work. Unfortunately I haven't been
> paying any attention to who might be proposing doing what and when
> there. Would it be appropriate to defer any fs-related API changes
> until any DM+fs rework went it to avoid conflicts or duplicate work?

I've had the same questions as well...

Rob

On 10/19/2012 01:18 PM, Rob Herring wrote:
> On 10/19/2012 11:56 AM, Stephen Warren wrote:
>> On 10/18/2012 05:23 PM, Tom Rini wrote:
>>> On 10/18/12 16:12, Rob Herring wrote:
>>>> On 10/18/2012 06:01 PM, Tom Rini wrote:
>> ...
>>>>>>> On 10/11/2012 01:59 PM, Stephen Warren wrote:
>>>>>>>> Implement "ls" and "fsload" commands that act like 
>>>>>>>> {fat,ext2}{ls,load}, and transparently handle either 
>>>>>>>> file-system. This scheme could easily be extended to
>>>>>>>> other filesystem types; I only didn't do it for zfs
>>>>>>>> because I don't have any filesystems of that type to test
>>>>>>>> with.
>> ...
>>>>> Baring further discussion, I intend to grab this really soon,
>>>>> as it sounds like it's a functional starting point, however we
>>>>> wish to make this happen.  Or am I not following?  Thanks!
>>>
>>>> It's your call. I'd rather see clean-up first and features
>>>> second, but that's just me. Either way works. The amount of
>>>> duplication in u-boot just annoys me. Hopefully the DM work will
>>>> fix some of it.
>>>
>>> I too would like to see more clean-up,
>>
>> Which clean-up exactly?
>>
>> The only duplication I see here is that ext2load/fatload could be
>> modified to simply call into do_fsload. That'd be pretty simple, I
>> think, assuming the behaviour change was OK (e.g. fatload would
>> suddenly support either FAT or ext2*), and that cmd_fs.c and fs.c
>> would both always be pulled in.
> 
> Can't you make do_fsload support either specifying the fs for legacy use
> or detecting it on the new commands?

Yes, I suppose I could:

* Add a bit-mask of legal filesystems as a parameter to fs_set_blk_dev().

* Move the body of do_fsload() into some common called by do_fsload(),
do_ext2load(), do_fatload(), each passing in the appropriate bit-mask,
which gets passed down to fs_set_blk_dev().

That'd be easy, and probably entail only extremely minimal code-size
increases for an ext2-only or FAT-only build; just a few bytes for a few
more function calls. Sound like a plan?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/19/12 12:26, Stephen Warren wrote:

[snip]
> Yes, I suppose I could:
> 
> * Add a bit-mask of legal filesystems as a parameter to 
> fs_set_blk_dev().
> 
> * Move the body of do_fsload() into some common called by 
> do_fsload(), do_ext2load(), do_fatload(), each passing in the 
> appropriate bit-mask, which gets passed down to fs_set_blk_dev().
> 
> That'd be easy, and probably entail only extremely minimal 
> code-size increases for an ext2-only or FAT-only build; just a few 
> bytes for a few more function calls. Sound like a plan?

Sounds good to me.  I bet you'll find some other clean-ups while
you're in there.  Marek, any comments from the DM perspective here?

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://www.enigmail.net/

iQIcBAEBAgAGBQJQgbODAAoJENk4IS6UOR1Wk1gP/0yNXaGlxt2LPDcJ5ux7Vc1W
Y0m5gnOzcF3gehW7EArvy+pIhJMD0Gu4XVdrY0A7GKdJiYs2Nv56vJpkYMu8MfU+
trMquPFqGeAe5Uq2LRVmEKIb3+YdbEv7xuiMThwt8zpEtbQ6ujZheeYHPDoAZwdL
jI4geIsOHibo24+Encux8AlcDJuP0PSfsrY4bvBMlriuAx/D7yCeaVJcbtb9mSQM
S/PYkTIHOfUzrntK4nrjoi4YHW39wUQv+5B5wUWEHeDnfhkba/uD2HsoG57RuiT+
cc7jMokhtz1vIzZHs2glfQI3UHEcoMo4NY6sznghCkHtNRb0cKlo+YUWoarjPpjV
z0aygHKz5ovC9SguzNmGSiXkz/GFI5Pwi2vneaaVp2eRn6hsPkQadQbiNFhjXKo/
L7Olu8iiAOqvkVTcYZ68PmEp75kX+WQ/o59lrFvRl8Ny3ir55r7oBAsrTyFgOmmi
I4q/VablIKl9VtvD27lzbAB2tHfggJOrO0XU2iv4mZxDkY/N/qvtfLpEmzZXbv7r
VnK2MUKGR2t7hue64llrNIffivBonrseoyUtbSMummzANYN/NJDIg87K8DYlLFvz
c6FiDvtbEpUxVTzIkALb5MVfOWaKbFqnq6mffwG9Y41X0ZfpRpV54TPA/wTo8CyQ
GrEJAzv8QBlWrsXjNqEm
=NCgN
-----END PGP SIGNATURE-----

Dear Tom Rini,

> On 10/19/12 12:26, Stephen Warren wrote:
> 
> [snip]
> 
> > Yes, I suppose I could:
> > 
> > * Add a bit-mask of legal filesystems as a parameter to
> > fs_set_blk_dev().
> > 
> > * Move the body of do_fsload() into some common called by
> > do_fsload(), do_ext2load(), do_fatload(), each passing in the
> > appropriate bit-mask, which gets passed down to fs_set_blk_dev().
> > 
> > That'd be easy, and probably entail only extremely minimal
> > code-size increases for an ext2-only or FAT-only build; just a few
> > bytes for a few more function calls. Sound like a plan?
> 
> Sounds good to me.  I bet you'll find some other clean-ups while
> you're in there.  Marek, any comments from the DM perspective here?

CCing pavel, block goo is on his slate.

Best regards,
Marek Vasut