[U-Boot] [PATCH 0/6] Allow sandbox to use config_distro_bootcmd

[U-Boot] [PATCH 0/6] Allow sandbox to use config_distro_bootcmd

[Sjoerd Simons]

Testing whether images will correctly boot with the standard distro bootcmds
can be rather time-consuming as it tends to require flashing the images and
booting on a device. Ditto for testing changes to config_distro_bootcmd.

Adding support for sandbox to run distro bootcmds makes things a lot more
convenient.

Sjoerd Simons (6):
  sandbox: only do sandboxfs for hostfs interface
  sandbox: Add support for bootz
  sandbox: Implement host dev [device]
  config_distro_bootcmd.h: Add shared block definition for the host
    interface
  pxe: Ensure all memory access is to mapped memory
  sandbox: add config_distro_defaults and config_distro_bootcmd

 arch/sandbox/cpu/cpu.c          | 20 +++++++++++
 common/cmd_pxe.c                | 80 +++++++++++++++++++++++------------------
 common/cmd_sandbox.c            | 48 +++++++++++++++++++++++++
 fs/sandbox/sandboxfs.c          |  5 ++-
 include/config_distro_bootcmd.h | 13 +++++++
 include/configs/sandbox.h       | 29 +++++++++++++--
 6 files changed, 157 insertions(+), 38 deletions(-)

-- 
2.1.4

[U-Boot] [PATCH 1/6] sandbox: only do sandboxfs for hostfs interface

[Sjoerd Simons]

Only do sandbox filesystem access when using the hostfs device
interface, rather then falling back to it in all cases. This prevents
confusion situations due to the fallback being taken rather then an
unsupported error being raised.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 fs/sandbox/sandboxfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c
index a920bc0..c6540c6 100644
--- a/fs/sandbox/sandboxfs.c
+++ b/fs/sandbox/sandboxfs.c
@@ -10,7 +10,10 @@
 
 int sandbox_fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
 {
-	return 0;
+	/* Only accept a NULL block_dev_desc_t for the sandbox, which is when
+	 * hostfs interface is used
+	 */
+	return rbdd != NULL;
 }
 
 int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
-- 
2.1.4

[U-Boot] [PATCH 2/6] sandbox: Add support for bootz

[Sjoerd Simons]

Add dummy bootz_setup implementation allowing the u-boot sandbox to run
bootz. This recognizes both ARM and x86 zImages to validate a valid
zImage was loaded.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 1aa397c..c71fb86 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -7,6 +7,7 @@
 #include <dm/root.h>
 #include <os.h>
 #include <asm/state.h>
+#include <asm/io.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -40,6 +41,25 @@ unsigned long __attribute__((no_instrument_function)) timer_get_us(void)
 	return os_get_nsec() / 1000;
 }
 
+int bootz_setup(ulong image, ulong *start, ulong *end)
+{
+  uint8_t *zimage = (uint8_t *)map_sysmem(image, 0);
+  uint8_t arm_magic[] = { 0x18, 0x28, 0x6f, 0x01 };
+
+  if (memcmp(zimage + 0x202, "HdrS", 4) == 0) {
+    printf ("setting up x86 zImage\n");
+  } else if (memcmp (zimage + 0x24, arm_magic, 4) == 0) {
+    printf ("setting up ARM zImage\n");
+  } else {
+    printf ("Unrecognized zImage\n");
+    return 1;
+  }
+
+  *start = 0xdead;
+  *end = 0xbeef;
+  return 0;
+}
+
 int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 {
 	if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
-- 
2.1.4

[U-Boot] [PATCH 3/6] sandbox: Implement host dev [device]

[Sjoerd Simons]

A common pattern to check if a certain device exists (e.g. in
config_distro_bootcmd) is to use: <interface> dev [device]

Implement host dev [device] so this pattern can be used for sandbox host
devices.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
index 4286969..7447d43 100644
--- a/common/cmd_sandbox.c
+++ b/common/cmd_sandbox.c
@@ -10,6 +10,8 @@
 #include <sandboxblockdev.h>
 #include <asm/errno.h>
 
+static int sandbox_curr_device = -1;
+
 static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc,
 			   char * const argv[])
 {
@@ -124,3 +126,49 @@ U_BOOT_CMD(
 	"sb commands use the \"hostfs\" device. The \"host\" device is used\n"
 	"with standard IO commands such as fatls or ext2load"
 );
+
+
+static int do_host_dev(cmd_tbl_t *cmdtp, int flag, int argc,
+		       char * const argv[])
+{
+	int dev;
+	char *ep;
+	block_dev_desc_t *blk_dev;
+	int ret;
+
+	if (argc < 2 || strcmp(argv[1], "dev"))
+		return CMD_RET_USAGE;
+
+	if (argc == 2) {
+		if (sandbox_curr_device < 0) {
+			printf("No current host device\n");
+			return 1;
+		}
+		printf ("Current host device %d\n", sandbox_curr_device);
+		return 0;
+	}
+
+	dev = simple_strtoul(argv[2], &ep, 16);
+	if (*ep) {
+		printf("** Bad device specification %s **\n", argv[2]);
+		return CMD_RET_USAGE;
+	}
+
+	ret = host_get_dev_err(dev, &blk_dev);
+	if (ret) {
+		if (ret == -ENOENT)
+			puts("Not bound to a backing file\n");
+		else if (ret == -ENODEV)
+			puts("Invalid host device number\n");
+
+		return 1;
+	}
+
+	sandbox_curr_device = dev;
+	return 0;
+}
+
+U_BOOT_CMD(
+	host, 3, 2, do_host_dev,
+	"Set or get current host device", "dev [dev] - Set or retrieve the current host device"
+);
-- 
2.1.4

[U-Boot] [PATCH 4/6] config_distro_bootcmd.h: Add shared block definition for the host interface

[Sjoerd Simons]

Define the common shared block environment for the host interface in
preperation for the sandbox build to use config_distro_bootcmd.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 include/config_distro_bootcmd.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 07a0b3b..ff0e3a8 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -32,6 +32,18 @@
 #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
 	#devtypel #instance " "
 
+#ifdef CONFIG_SANDBOX
+#define BOOTENV_SHARED_HOST	BOOTENV_SHARED_BLKDEV(host)
+#define BOOTENV_DEV_HOST	BOOTENV_DEV_BLKDEV
+#define BOOTENV_DEV_NAME_HOST	BOOTENV_DEV_NAME_BLKDEV
+#else
+#define BOOTENV_SHARED_HOST
+#define BOOTENV_DEV_HOST \
+	BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
+#define BOOTENV_DEV_NAME_HOST \
+	BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
+#endif
+
 #ifdef CONFIG_CMD_MMC
 #define BOOTENV_SHARED_MMC	BOOTENV_SHARED_BLKDEV(mmc)
 #define BOOTENV_DEV_MMC		BOOTENV_DEV_BLKDEV
@@ -151,6 +163,7 @@
 #define BOOTENV_DEV(devtypeu, devtypel, instance) \
 	BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
 #define BOOTENV \
+	BOOTENV_SHARED_HOST \
 	BOOTENV_SHARED_MMC \
 	BOOTENV_SHARED_USB \
 	BOOTENV_SHARED_SATA \
-- 
2.1.4

[U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory

[Sjoerd Simons]

Properly map memory through map_sysmem so that pxe can be used from the
sandbox.

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
 1 file changed, 46 insertions(+), 34 deletions(-)

diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
index 7e32c95..ec81e70 100644
--- a/common/cmd_pxe.c
+++ b/common/cmd_pxe.c
@@ -13,6 +13,7 @@
 #include <errno.h>
 #include <linux/list.h>
 #include <fs.h>
+#include <asm/io.h>
 
 #include "menu.h"
 #include "cli.h"
@@ -188,11 +189,11 @@ static int do_get_any(cmd_tbl_t *cmdtp, const char *file_path, char *file_addr)
  *
  * Returns 1 for success, or < 0 on error.
  */
-static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
+static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr)
 {
 	size_t path_len;
 	char relfile[MAX_TFTP_PATH_LEN+1];
-	char addr_buf[10];
+	char addr_buf[18];
 	int err;
 
 	err = get_bootfile_path(file_path, relfile, sizeof(relfile));
@@ -215,7 +216,7 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
 
 	printf("Retrieving file: %s\n", relfile);
 
-	sprintf(addr_buf, "%p", file_addr);
+	sprintf(addr_buf, "%lx", file_addr);
 
 	return do_getfile(cmdtp, relfile, addr_buf);
 }
@@ -227,11 +228,12 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
  *
  * Returns 1 on success, or < 0 for error.
  */
-static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
+static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr)
 {
 	unsigned long config_file_size;
 	char *tftp_filesize;
 	int err;
+	void *buf;
 
 	err = get_relfile(cmdtp, file_path, file_addr);
 
@@ -250,7 +252,9 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr
 	if (strict_strtoul(tftp_filesize, 16, &config_file_size) < 0)
 		return -EINVAL;
 
-	*(char *)(file_addr + config_file_size) = '\0';
+	buf = map_sysmem (file_addr + config_file_size, 1);
+	* (char *)buf = '\0';
+	unmap_sysmem (buf);
 
 	return 1;
 }
@@ -266,7 +270,7 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr
  *
  * Returns 1 on success or < 0 on error.
  */
-static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_addr_r)
+static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, unsigned long pxefile_addr_r)
 {
 	size_t base_len = strlen(PXELINUX_DIR);
 	char path[MAX_TFTP_PATH_LEN+1];
@@ -287,7 +291,7 @@ static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_a
  *
  * Returns 1 on success or < 0 on error.
  */
-static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
+static int pxe_uuid_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r)
 {
 	char *uuid_str;
 
@@ -305,7 +309,7 @@ static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
  *
  * Returns 1 on success or < 0 on error.
  */
-static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
+static int pxe_mac_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r)
 {
 	char mac_str[21];
 	int err;
@@ -325,7 +329,7 @@ static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
  *
  * Returns 1 on success or < 0 on error.
  */
-static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
+static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r)
 {
 	char ip_addr[9];
 	int mask_pos, err;
@@ -384,9 +388,9 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	 * Keep trying paths until we successfully get a file we're looking
 	 * for.
 	 */
-	if (pxe_uuid_path(cmdtp, (void *)pxefile_addr_r) > 0 ||
-	    pxe_mac_path(cmdtp, (void *)pxefile_addr_r) > 0 ||
-	    pxe_ipaddr_paths(cmdtp, (void *)pxefile_addr_r) > 0) {
+	if (pxe_uuid_path(cmdtp, pxefile_addr_r) > 0 ||
+	    pxe_mac_path(cmdtp, pxefile_addr_r) > 0 ||
+	    pxe_ipaddr_paths(cmdtp, pxefile_addr_r) > 0) {
 		printf("Config file found\n");
 
 		return 0;
@@ -394,7 +398,7 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	while (pxe_default_paths[i]) {
 		if (get_pxelinux_path(cmdtp, pxe_default_paths[i],
-				      (void *)pxefile_addr_r) > 0) {
+				      pxefile_addr_r) > 0) {
 			printf("Config file found\n");
 			return 0;
 		}
@@ -427,7 +431,7 @@ static int get_relfile_envaddr(cmd_tbl_t *cmdtp, const char *file_path, const ch
 	if (strict_strtoul(envaddr, 16, &file_addr) < 0)
 		return -EINVAL;
 
-	return get_relfile(cmdtp, file_path, (void *)file_addr);
+	return get_relfile(cmdtp, file_path, file_addr);
 }
 
 /*
@@ -1054,7 +1058,7 @@ static int parse_integer(char **c, int *dst)
 	return 1;
 }
 
-static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level);
+static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b, struct pxe_menu *cfg, int nest_level);
 
 /*
  * Parse an include statement, and retrieve and parse the file it mentions.
@@ -1064,12 +1068,13 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
  * include, nest_level has already been incremented and doesn't need to be
  * incremented here.
  */
-static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base,
+static int handle_include(cmd_tbl_t *cmdtp, char **c, unsigned long base,
 				struct pxe_menu *cfg, int nest_level)
 {
 	char *include_path;
 	char *s = *c;
 	int err;
+	char *buf;
 
 	err = parse_sliteral(c, &include_path);
 
@@ -1086,20 +1091,22 @@ static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base,
 		return err;
 	}
 
-	return parse_pxefile_top(cmdtp, base, cfg, nest_level);
+	buf = map_sysmem (base, 0);
+	return parse_pxefile_top(cmdtp, buf, base, cfg, nest_level);
 }
 
 /*
  * Parse lines that begin with 'menu'.
  *
- * b and nest are provided to handle the 'menu include' case.
- *
- * b should be the address where the file currently being parsed is stored.
+ * base and nest are provided to handle the 'menu include' case.
  *
- * nest_level should be 1 when parsing the top level pxe file, 2 when parsing
- * a file it includes, 3 when parsing a file included by that file, and so on.
+ * base should point to a location where it's safe to store the file, and
+ * nest_level should indicate how many nested includes have occurred. For this
+ * include, nest_level has already been incremented and doesn't need to be
+ * incremented here.
  */
-static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, int nest_level)
+static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg,
+				unsigned long base, int nest_level)
 {
 	struct token t;
 	char *s = *c;
@@ -1114,7 +1121,7 @@ static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b,
 		break;
 
 	case T_INCLUDE:
-		err = handle_include(cmdtp, c, b + strlen(b) + 1, cfg,
+		err = handle_include(cmdtp, c, base, cfg,
 						nest_level + 1);
 		break;
 
@@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
  *
  * Returns 1 on success, < 0 on error.
  */
-static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level)
+static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
+				struct pxe_menu *cfg, int nest_level)
 {
 	struct token t;
-	char *s, *b, *label_name;
+	char *s, *label_name, *base;
 	int err;
 
-	b = p;
+	base = p;
 
 	if (nest_level > MAX_NEST_LEVEL) {
 		printf("Maximum nesting (%d) exceeded\n", MAX_NEST_LEVEL);
@@ -1303,7 +1311,9 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
 		switch (t.type) {
 		case T_MENU:
 			cfg->prompt = 1;
-			err = parse_menu(cmdtp, &p, cfg, b, nest_level);
+			err = parse_menu(cmdtp, &p, cfg,
+						b + ALIGN(strlen(base), 4),
+						nest_level);
 			break;
 
 		case T_TIMEOUT:
@@ -1328,7 +1338,7 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
 			break;
 
 		case T_INCLUDE:
-			err = handle_include(cmdtp, &p, b + ALIGN(strlen(b), 4), cfg,
+			err = handle_include(cmdtp, &p, b + ALIGN(strlen(base), 4), cfg,
 							nest_level + 1);
 			break;
 
@@ -1385,9 +1395,10 @@ static void destroy_pxe_menu(struct pxe_menu *cfg)
  * files it includes). The resulting pxe_menu struct can be free()'d by using
  * the destroy_pxe_menu() function.
  */
-static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
+static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, unsigned long menucfg)
 {
 	struct pxe_menu *cfg;
+	char *buf;
 
 	cfg = malloc(sizeof(struct pxe_menu));
 
@@ -1398,7 +1409,8 @@ static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
 
 	INIT_LIST_HEAD(&cfg->labels);
 
-	if (parse_pxefile_top(cmdtp, menucfg, cfg, 1) < 0) {
+	buf = map_sysmem (menucfg, 0);
+	if (parse_pxefile_top(cmdtp, buf, menucfg, cfg, 1) < 0) {
 		destroy_pxe_menu(cfg);
 		return NULL;
 	}
@@ -1556,7 +1568,7 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return 1;
 	}
 
-	cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
+	cfg = parse_pxefile(cmdtp, pxefile_addr_r);
 
 	if (cfg == NULL) {
 		printf("Error parsing config file\n");
@@ -1663,12 +1675,12 @@ static int do_sysboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return 1;
 	}
 
-	if (get_pxe_file(cmdtp, filename, (void *)pxefile_addr_r) < 0) {
+	if (get_pxe_file(cmdtp, filename, pxefile_addr_r) < 0) {
 		printf("Error reading config file\n");
 		return 1;
 	}
 
-	cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
+	cfg = parse_pxefile(cmdtp, pxefile_addr_r);
 
 	if (cfg == NULL) {
 		printf("Error parsing config file\n");
-- 
2.1.4

[U-Boot] [PATCH 6/6] sandbox: add config_distro_defaults and config_distro_bootcmd

[Sjoerd Simons]

Make the sandbox setup more generic/examplary by including
config_distro_defaults.h and config_distro_bootcmd.h.

Among other things this makes it easy to test whether images will boot
though with the standard distro bootcmds by running e.g:
  u-boot -c 'sb bind 0 myimage.img ; boot'

Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
---
 include/configs/sandbox.h | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 5c11650..742bda6 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -75,7 +75,6 @@
 #define CONFIG_CMDLINE_EDITING
 #define CONFIG_COMMAND_HISTORY
 #define CONFIG_AUTO_COMPLETE
-#define CONFIG_BOOTDELAY	3
 
 #define CONFIG_ENV_SIZE		8192
 #define CONFIG_ENV_IS_NOWHERE
@@ -125,10 +124,21 @@
 
 /* include default commands */
 #include <config_cmd_default.h>
+#include <config_distro_defaults.h>
+
+#define BOOT_TARGET_DEVICES(func) \
+  func(HOST, host, 1) \
+  func(HOST, host, 0)
+
+#include <config_distro_bootcmd.h>
 
 /* We don't have networking support yet */
 #undef CONFIG_CMD_NET
 #undef CONFIG_CMD_NFS
+#undef CONFIG_CMD_PING
+
+/* Can't boot elf images */
+#undef CONFIG_CMD_ELF
 
 #define CONFIG_CMD_HASH
 #define CONFIG_HASH_VERIFY
@@ -169,16 +179,29 @@
 #define CONFIG_CROS_EC_KEYB
 #define CONFIG_KEYBOARD
 
-#define CONFIG_EXTRA_ENV_SETTINGS	"stdin=serial,cros-ec-keyb\0" \
+#define EXTRA_ENV_SETTINGS		"stdin=serial,cros-ec-keyb\0" \
 					"stdout=serial,lcd\0" \
 					"stderr=serial,lcd\0"
 #else
 
-#define CONFIG_EXTRA_ENV_SETTINGS	"stdin=serial\0" \
+#define EXTRA_ENV_SETTINGS		"stdin=serial\0" \
 					"stdout=serial,lcd\0" \
 					"stderr=serial,lcd\0"
 #endif
 
+#define MEM_LAYOUT_ENV_SETTINGS \
+	"bootm_size=0x10000000\0" \
+	"kernel_addr_r=0x1000000\0" \
+	"fdt_addr_r=0xc00000\0" \
+	"ramdisk_addr_r=0x2000000\0" \
+	"scriptaddr=0x1000\0" \
+	"pxefile_addr_r=0x2000\0"
+
+#define CONFIG_EXTRA_ENV_SETTINGS \
+	BOOTENV \
+	MEM_LAYOUT_ENV_SETTINGS \
+	EXTRA_ENV_SETTINGS
+
 #define CONFIG_GZIP_COMPRESSED
 #define CONFIG_BZIP2
 #define CONFIG_LZO
-- 
2.1.4

[U-Boot] [PATCH 1/6] sandbox: only do sandboxfs for hostfs interface

[Simon Glass]

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Only do sandbox filesystem access when using the hostfs device
> interface, rather then falling back to it in all cases. This prevents
> confusion situations due to the fallback being taken rather then an
> unsupported error being raised.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Reviewed-by: Simon Glass <sjg@chromium.org>

See nit below.

> ---
>  fs/sandbox/sandboxfs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c
> index a920bc0..c6540c6 100644
> --- a/fs/sandbox/sandboxfs.c
> +++ b/fs/sandbox/sandboxfs.c
> @@ -10,7 +10,10 @@
>
>  int sandbox_fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
>  {
> -       return 0;
> +       /* Only accept a NULL block_dev_desc_t for the sandbox, which is when

/*
 * Only accept

> +        * hostfs interface is used
> +        */
> +       return rbdd != NULL;
>  }
>
>  int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 2/6] sandbox: Add support for bootz

[Simon Glass]

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Add dummy bootz_setup implementation allowing the u-boot sandbox to run
> bootz. This recognizes both ARM and x86 zImages to validate a valid
> zImage was loaded.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>  arch/sandbox/cpu/cpu.c | 20 ++++++++++++++++++++

Can you please split the do_bootm_linux() code out into
arch/sandbox/lib/bootm, and add your patch there?

>  1 file changed, 20 insertions(+)
>
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index 1aa397c..c71fb86 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -7,6 +7,7 @@
>  #include <dm/root.h>
>  #include <os.h>
>  #include <asm/state.h>
> +#include <asm/io.h>
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -40,6 +41,25 @@ unsigned long __attribute__((no_instrument_function)) timer_get_us(void)
>         return os_get_nsec() / 1000;
>  }
>
> +int bootz_setup(ulong image, ulong *start, ulong *end)
> +{
> +  uint8_t *zimage = (uint8_t *)map_sysmem(image, 0);

Do you need the cast?

> +  uint8_t arm_magic[] = { 0x18, 0x28, 0x6f, 0x01 };
> +
> +  if (memcmp(zimage + 0x202, "HdrS", 4) == 0) {
> +    printf ("setting up x86 zImage\n");
> +  } else if (memcmp (zimage + 0x24, arm_magic, 4) == 0) {
> +    printf ("setting up ARM zImage\n");
> +  } else {

The indentation and code style looks wrong here. Did you run patman?

> +    printf ("Unrecognized zImage\n");
> +    return 1;
> +  }
> +
> +  *start = 0xdead;
> +  *end = 0xbeef;
> +  return 0;
> +}
> +
>  int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>  {
>         if (flag & (BOOTM_STATE_OS_GO | BOOTM_STATE_OS_FAKE_GO)) {
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 3/6] sandbox: Implement host dev [device]

[Simon Glass]

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> A common pattern to check if a certain device exists (e.g. in
> config_distro_bootcmd) is to use: <interface> dev [device]
>
> Implement host dev [device] so this pattern can be used for sandbox host
> devices.

I don't see where this actually affects anything? In other words, does
it really use the device you select, or just ignore you?

> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>  common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
> index 4286969..7447d43 100644
> --- a/common/cmd_sandbox.c
> +++ b/common/cmd_sandbox.c
> @@ -10,6 +10,8 @@
>  #include <sandboxblockdev.h>
>  #include <asm/errno.h>
>
> +static int sandbox_curr_device = -1;
> +
>  static int do_sandbox_load(cmd_tbl_t *cmdtp, int flag, int argc,
>                            char * const argv[])
>  {
> @@ -124,3 +126,49 @@ U_BOOT_CMD(
>         "sb commands use the \"hostfs\" device. The \"host\" device is used\n"
>         "with standard IO commands such as fatls or ext2load"
>  );
> +
> +
> +static int do_host_dev(cmd_tbl_t *cmdtp, int flag, int argc,
> +                      char * const argv[])
> +{
> +       int dev;
> +       char *ep;
> +       block_dev_desc_t *blk_dev;
> +       int ret;
> +
> +       if (argc < 2 || strcmp(argv[1], "dev"))
> +               return CMD_RET_USAGE;
> +
> +       if (argc == 2) {
> +               if (sandbox_curr_device < 0) {
> +                       printf("No current host device\n");
> +                       return 1;
> +               }
> +               printf ("Current host device %d\n", sandbox_curr_device);
> +               return 0;
> +       }
> +
> +       dev = simple_strtoul(argv[2], &ep, 16);
> +       if (*ep) {
> +               printf("** Bad device specification %s **\n", argv[2]);
> +               return CMD_RET_USAGE;
> +       }
> +
> +       ret = host_get_dev_err(dev, &blk_dev);
> +       if (ret) {
> +               if (ret == -ENOENT)
> +                       puts("Not bound to a backing file\n");

Just use printf(), we should avoid puts() now.

> +               else if (ret == -ENODEV)
> +                       puts("Invalid host device number\n");
> +
> +               return 1;
> +       }
> +
> +       sandbox_curr_device = dev;
> +       return 0;
> +}
> +
> +U_BOOT_CMD(
> +       host, 3, 2, do_host_dev,
> +       "Set or get current host device", "dev [dev] - Set or retrieve the current host device"

Can we make this command part of the 'sb' command? Like 'sh host ...'.

> +);
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 4/6] config_distro_bootcmd.h: Add shared block definition for the host interface

[Simon Glass]

+Stephen who knows more about this stuff

On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Define the common shared block environment for the host interface in
> preperation for the sandbox build to use config_distro_bootcmd.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>  include/config_distro_bootcmd.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 07a0b3b..ff0e3a8 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -32,6 +32,18 @@
>  #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
>         #devtypel #instance " "
>
> +#ifdef CONFIG_SANDBOX
> +#define BOOTENV_SHARED_HOST    BOOTENV_SHARED_BLKDEV(host)
> +#define BOOTENV_DEV_HOST       BOOTENV_DEV_BLKDEV
> +#define BOOTENV_DEV_NAME_HOST  BOOTENV_DEV_NAME_BLKDEV
> +#else
> +#define BOOTENV_SHARED_HOST
> +#define BOOTENV_DEV_HOST \
> +       BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
> +#define BOOTENV_DEV_NAME_HOST \

Can we use upper case in #defines?

> +       BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
> +#endif
> +
>  #ifdef CONFIG_CMD_MMC
>  #define BOOTENV_SHARED_MMC     BOOTENV_SHARED_BLKDEV(mmc)
>  #define BOOTENV_DEV_MMC                BOOTENV_DEV_BLKDEV
> @@ -151,6 +163,7 @@
>  #define BOOTENV_DEV(devtypeu, devtypel, instance) \
>         BOOTENV_DEV_##devtypeu(devtypeu, devtypel, instance)
>  #define BOOTENV \
> +       BOOTENV_SHARED_HOST \
>         BOOTENV_SHARED_MMC \
>         BOOTENV_SHARED_USB \
>         BOOTENV_SHARED_SATA \
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory

[Simon Glass]

Hi Sjoerd,

On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Properly map memory through map_sysmem so that pxe can be used from the
> sandbox.
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>

Please run your patches through patman as you seem to have style
violations. Also can you add some notes about how you have tested this
on real hardware?

> ---
>  common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
>  1 file changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
> index 7e32c95..ec81e70 100644
> --- a/common/cmd_pxe.c
> +++ b/common/cmd_pxe.c
> @@ -13,6 +13,7 @@
>  #include <errno.h>
>  #include <linux/list.h>
>  #include <fs.h>
> +#include <asm/io.h>
>
>  #include "menu.h"
>  #include "cli.h"
> @@ -188,11 +189,11 @@ static int do_get_any(cmd_tbl_t *cmdtp, const char *file_path, char *file_addr)
>   *
>   * Returns 1 for success, or < 0 on error.
>   */
> -static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
> +static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr)
>  {
>         size_t path_len;
>         char relfile[MAX_TFTP_PATH_LEN+1];
> -       char addr_buf[10];
> +       char addr_buf[18];
>         int err;
>
>         err = get_bootfile_path(file_path, relfile, sizeof(relfile));
> @@ -215,7 +216,7 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
>
>         printf("Retrieving file: %s\n", relfile);
>
> -       sprintf(addr_buf, "%p", file_addr);
> +       sprintf(addr_buf, "%lx", file_addr);
>
>         return do_getfile(cmdtp, relfile, addr_buf);
>  }
> @@ -227,11 +228,12 @@ static int get_relfile(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
>   *
>   * Returns 1 on success, or < 0 for error.
>   */
> -static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr)
> +static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, unsigned long file_addr)
>  {
>         unsigned long config_file_size;
>         char *tftp_filesize;
>         int err;
> +       void *buf;

Can we make this char * please to remove the cast below?

>
>         err = get_relfile(cmdtp, file_path, file_addr);
>
> @@ -250,7 +252,9 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr
>         if (strict_strtoul(tftp_filesize, 16, &config_file_size) < 0)
>                 return -EINVAL;
>
> -       *(char *)(file_addr + config_file_size) = '\0';
> +       buf = map_sysmem (file_addr + config_file_size, 1);
> +       * (char *)buf = '\0';
> +       unmap_sysmem (buf);
>
>         return 1;
>  }
> @@ -266,7 +270,7 @@ static int get_pxe_file(cmd_tbl_t *cmdtp, const char *file_path, void *file_addr
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_addr_r)
> +static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, unsigned long pxefile_addr_r)
>  {
>         size_t base_len = strlen(PXELINUX_DIR);
>         char path[MAX_TFTP_PATH_LEN+1];
> @@ -287,7 +291,7 @@ static int get_pxelinux_path(cmd_tbl_t *cmdtp, const char *file, void *pxefile_a
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
> +static int pxe_uuid_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r)
>  {
>         char *uuid_str;
>
> @@ -305,7 +309,7 @@ static int pxe_uuid_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
> +static int pxe_mac_path(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r)
>  {
>         char mac_str[21];
>         int err;
> @@ -325,7 +329,7 @@ static int pxe_mac_path(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
>   *
>   * Returns 1 on success or < 0 on error.
>   */
> -static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, void *pxefile_addr_r)
> +static int pxe_ipaddr_paths(cmd_tbl_t *cmdtp, unsigned long pxefile_addr_r)
>  {
>         char ip_addr[9];
>         int mask_pos, err;
> @@ -384,9 +388,9 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>          * Keep trying paths until we successfully get a file we're looking
>          * for.
>          */
> -       if (pxe_uuid_path(cmdtp, (void *)pxefile_addr_r) > 0 ||
> -           pxe_mac_path(cmdtp, (void *)pxefile_addr_r) > 0 ||
> -           pxe_ipaddr_paths(cmdtp, (void *)pxefile_addr_r) > 0) {
> +       if (pxe_uuid_path(cmdtp, pxefile_addr_r) > 0 ||
> +           pxe_mac_path(cmdtp, pxefile_addr_r) > 0 ||
> +           pxe_ipaddr_paths(cmdtp, pxefile_addr_r) > 0) {
>                 printf("Config file found\n");
>
>                 return 0;
> @@ -394,7 +398,7 @@ do_pxe_get(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
>         while (pxe_default_paths[i]) {
>                 if (get_pxelinux_path(cmdtp, pxe_default_paths[i],
> -                                     (void *)pxefile_addr_r) > 0) {
> +                                     pxefile_addr_r) > 0) {
>                         printf("Config file found\n");
>                         return 0;
>                 }
> @@ -427,7 +431,7 @@ static int get_relfile_envaddr(cmd_tbl_t *cmdtp, const char *file_path, const ch
>         if (strict_strtoul(envaddr, 16, &file_addr) < 0)
>                 return -EINVAL;
>
> -       return get_relfile(cmdtp, file_path, (void *)file_addr);
> +       return get_relfile(cmdtp, file_path, file_addr);
>  }
>
>  /*
> @@ -1054,7 +1058,7 @@ static int parse_integer(char **c, int *dst)
>         return 1;
>  }
>
> -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level);
> +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b, struct pxe_menu *cfg, int nest_level);
>
>  /*
>   * Parse an include statement, and retrieve and parse the file it mentions.
> @@ -1064,12 +1068,13 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
>   * include, nest_level has already been incremented and doesn't need to be
>   * incremented here.
>   */
> -static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base,
> +static int handle_include(cmd_tbl_t *cmdtp, char **c, unsigned long base,
>                                 struct pxe_menu *cfg, int nest_level)
>  {
>         char *include_path;
>         char *s = *c;
>         int err;
> +       char *buf;
>
>         err = parse_sliteral(c, &include_path);
>
> @@ -1086,20 +1091,22 @@ static int handle_include(cmd_tbl_t *cmdtp, char **c, char *base,
>                 return err;
>         }
>
> -       return parse_pxefile_top(cmdtp, base, cfg, nest_level);
> +       buf = map_sysmem (base, 0);
> +       return parse_pxefile_top(cmdtp, buf, base, cfg, nest_level);

You should call unmap_sysmem() before the return.

>  }
>
>  /*
>   * Parse lines that begin with 'menu'.
>   *
> - * b and nest are provided to handle the 'menu include' case.
> - *
> - * b should be the address where the file currently being parsed is stored.
> + * base and nest are provided to handle the 'menu include' case.
>   *
> - * nest_level should be 1 when parsing the top level pxe file, 2 when parsing
> - * a file it includes, 3 when parsing a file included by that file, and so on.
> + * base should point to a location where it's safe to store the file, and
> + * nest_level should indicate how many nested includes have occurred. For this
> + * include, nest_level has already been incremented and doesn't need to be
> + * incremented here.
>   */
> -static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b, int nest_level)
> +static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg,
> +                               unsigned long base, int nest_level)
>  {
>         struct token t;
>         char *s = *c;
> @@ -1114,7 +1121,7 @@ static int parse_menu(cmd_tbl_t *cmdtp, char **c, struct pxe_menu *cfg, char *b,
>                 break;
>
>         case T_INCLUDE:
> -               err = handle_include(cmdtp, c, b + strlen(b) + 1, cfg,
> +               err = handle_include(cmdtp, c, base, cfg,
>                                                 nest_level + 1);
>                 break;
>
> @@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
>   *
>   * Returns 1 on success, < 0 on error.
>   */
> -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level)
> +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
> +                               struct pxe_menu *cfg, int nest_level)

s/b/base/ or something more meaningful (fix above/below also)

>  {
>         struct token t;
> -       char *s, *b, *label_name;
> +       char *s, *label_name, *base;
>         int err;
>
> -       b = p;
> +       base = p;

This worries me - assigning a pointer to a ulong.

>
>         if (nest_level > MAX_NEST_LEVEL) {
>                 printf("Maximum nesting (%d) exceeded\n", MAX_NEST_LEVEL);
> @@ -1303,7 +1311,9 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
>                 switch (t.type) {
>                 case T_MENU:
>                         cfg->prompt = 1;
> -                       err = parse_menu(cmdtp, &p, cfg, b, nest_level);
> +                       err = parse_menu(cmdtp, &p, cfg,
> +                                               b + ALIGN(strlen(base), 4),
> +                                               nest_level);
>                         break;
>
>                 case T_TIMEOUT:
> @@ -1328,7 +1338,7 @@ static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, in
>                         break;
>
>                 case T_INCLUDE:
> -                       err = handle_include(cmdtp, &p, b + ALIGN(strlen(b), 4), cfg,
> +                       err = handle_include(cmdtp, &p, b + ALIGN(strlen(base), 4), cfg,
>                                                         nest_level + 1);
>                         break;
>
> @@ -1385,9 +1395,10 @@ static void destroy_pxe_menu(struct pxe_menu *cfg)
>   * files it includes). The resulting pxe_menu struct can be free()'d by using
>   * the destroy_pxe_menu() function.
>   */
> -static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
> +static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, unsigned long menucfg)
>  {
>         struct pxe_menu *cfg;
> +       char *buf;
>
>         cfg = malloc(sizeof(struct pxe_menu));
>
> @@ -1398,7 +1409,8 @@ static struct pxe_menu *parse_pxefile(cmd_tbl_t *cmdtp, char *menucfg)
>
>         INIT_LIST_HEAD(&cfg->labels);
>
> -       if (parse_pxefile_top(cmdtp, menucfg, cfg, 1) < 0) {
> +       buf = map_sysmem (menucfg, 0);
> +       if (parse_pxefile_top(cmdtp, buf, menucfg, cfg, 1) < 0) {
>                 destroy_pxe_menu(cfg);
>                 return NULL;

Need unmap_sysmem() after.

>         }
> @@ -1556,7 +1568,7 @@ do_pxe_boot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                 return 1;
>         }
>
> -       cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
> +       cfg = parse_pxefile(cmdtp, pxefile_addr_r);
>
>         if (cfg == NULL) {
>                 printf("Error parsing config file\n");
> @@ -1663,12 +1675,12 @@ static int do_sysboot(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>                 return 1;
>         }
>
> -       if (get_pxe_file(cmdtp, filename, (void *)pxefile_addr_r) < 0) {
> +       if (get_pxe_file(cmdtp, filename, pxefile_addr_r) < 0) {
>                 printf("Error reading config file\n");
>                 return 1;
>         }
>
> -       cfg = parse_pxefile(cmdtp, (char *)(pxefile_addr_r));
> +       cfg = parse_pxefile(cmdtp, pxefile_addr_r);
>
>         if (cfg == NULL) {
>                 printf("Error parsing config file\n");
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 6/6] sandbox: add config_distro_defaults and config_distro_bootcmd

[Simon Glass]

On 19 February 2015 at 15:41, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> Make the sandbox setup more generic/examplary by including
> config_distro_defaults.h and config_distro_bootcmd.h.
>
> Among other things this makes it easy to test whether images will boot
> though with the standard distro bootcmds by running e.g:
>   u-boot -c 'sb bind 0 myimage.img ; boot'
>
> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> ---
>  include/configs/sandbox.h | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)

Reviewed by: Simon Glass <sjg@chromium.org>

>
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 5c11650..742bda6 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -75,7 +75,6 @@
>  #define CONFIG_CMDLINE_EDITING
>  #define CONFIG_COMMAND_HISTORY
>  #define CONFIG_AUTO_COMPLETE
> -#define CONFIG_BOOTDELAY       3
>
>  #define CONFIG_ENV_SIZE                8192
>  #define CONFIG_ENV_IS_NOWHERE
> @@ -125,10 +124,21 @@
>
>  /* include default commands */
>  #include <config_cmd_default.h>
> +#include <config_distro_defaults.h>
> +
> +#define BOOT_TARGET_DEVICES(func) \
> +  func(HOST, host, 1) \
> +  func(HOST, host, 0)

It looks like you support two host devices. Is that somewhat arbitrary?

> +
> +#include <config_distro_bootcmd.h>
>
>  /* We don't have networking support yet */
>  #undef CONFIG_CMD_NET
>  #undef CONFIG_CMD_NFS
> +#undef CONFIG_CMD_PING
> +
> +/* Can't boot elf images */
> +#undef CONFIG_CMD_ELF
>
>  #define CONFIG_CMD_HASH
>  #define CONFIG_HASH_VERIFY
> @@ -169,16 +179,29 @@
>  #define CONFIG_CROS_EC_KEYB
>  #define CONFIG_KEYBOARD
>
> -#define CONFIG_EXTRA_ENV_SETTINGS      "stdin=serial,cros-ec-keyb\0" \
> +#define EXTRA_ENV_SETTINGS             "stdin=serial,cros-ec-keyb\0" \
>                                         "stdout=serial,lcd\0" \
>                                         "stderr=serial,lcd\0"
>  #else
>
> -#define CONFIG_EXTRA_ENV_SETTINGS      "stdin=serial\0" \
> +#define EXTRA_ENV_SETTINGS             "stdin=serial\0" \
>                                         "stdout=serial,lcd\0" \
>                                         "stderr=serial,lcd\0"
>  #endif
>
> +#define MEM_LAYOUT_ENV_SETTINGS \
> +       "bootm_size=0x10000000\0" \
> +       "kernel_addr_r=0x1000000\0" \
> +       "fdt_addr_r=0xc00000\0" \
> +       "ramdisk_addr_r=0x2000000\0" \
> +       "scriptaddr=0x1000\0" \
> +       "pxefile_addr_r=0x2000\0"
> +
> +#define CONFIG_EXTRA_ENV_SETTINGS \
> +       BOOTENV \
> +       MEM_LAYOUT_ENV_SETTINGS \
> +       EXTRA_ENV_SETTINGS
> +
>  #define CONFIG_GZIP_COMPRESSED
>  #define CONFIG_BZIP2
>  #define CONFIG_LZO
> --
> 2.1.4
>

Regards,
Simon

[U-Boot] [PATCH 3/6] sandbox: Implement host dev [device]

[Sjoerd Simons]

On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
> Hi Sjoerd,
> 
> On 19 February 2015 at 15:41, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > A common pattern to check if a certain device exists (e.g. in
> > config_distro_bootcmd) is to use: <interface> dev [device]
> >
> > Implement host dev [device] so this pattern can be used for sandbox host
> > devices.
> 
> I don't see where this actually affects anything? In other words, does
> it really use the device you select, or just ignore you?

It gets ignored, it's only real usage is to detect whether a device
exists. 

To step back the reason I implemented it here is just to make it simpler
to integrate with the command boot commands as it's common blockdevice
macro is:

#define BOOTENV_SHARED_BLKDEV_BODY(devtypel) \
	"if " #devtypel " dev ${devnum}; then " \
		"setenv devtype " #devtypel "; " \
		"run scan_dev_for_boot_part; " \
	"fi\0"

Which follows exactly that pattern. 

> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> >  common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> >
> > diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
> > index 4286969..7447d43 100644
> > --- a/common/cmd_sandbox.c
> > +++ b/common/cmd_sandbox.c
> > @@ -10,6 +10,8 @@
> >  #include <sandboxblockdev.h>

> > +       ret = host_get_dev_err(dev, &blk_dev);
> > +       if (ret) {
> > +               if (ret == -ENOENT)
> > +                       puts("Not bound to a backing file\n");
> 
> Just use printf(), we should avoid puts() now.

ok

> > +               else if (ret == -ENODEV)
> > +                       puts("Invalid host device number\n");
> > +
> > +               return 1;
> > +       }
> > +
> > +       sandbox_curr_device = dev;
> > +       return 0;
> > +}
> > +
> > +U_BOOT_CMD(
> > +       host, 3, 2, do_host_dev,
> > +       "Set or get current host device", "dev [dev] - Set or retrieve the current host device"
> 
> Can we make this command part of the 'sb' command? Like 'sh host ...'.

In principle, sure, however that breaks the consistency with other
commands which all use <interface> <interface-commands>, where
<interface> matches the names used in the fs and partition commands.
Which is exactly the consistency _distro_bootcmd takes advantage of in
the macro i mentioned above.

My thinking was that the sb set of commands are used to manage/setup the
sandbox interface (e.g. setting up the host device bindings) while in
this case the host commands are for the more standard interaction with
host interface devices.


Another way of addressing this and essentially side-stepping this
discussion would be to add a a new generic dev command to allow testing
device existance e.g.
  dev exists host 0
  dev exists mmc 0

And switch _distro_bootcmd to that instead of it relying on all block
interfaces having a dev subcommand with the same semantics.

Opinions ? :)
-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150228/b86906a7/attachment.bin>

[U-Boot] [PATCH 4/6] config_distro_bootcmd.h: Add shared block definition for the host interface

[Sjoerd Simons]

On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
> +Stephen who knows more about this stuff
> 
> On 19 February 2015 at 15:41, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > Define the common shared block environment for the host interface in
> > preperation for the sandbox build to use config_distro_bootcmd.
> >
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> > ---
> >  include/config_distro_bootcmd.h | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> > index 07a0b3b..ff0e3a8 100644
> > --- a/include/config_distro_bootcmd.h
> > +++ b/include/config_distro_bootcmd.h
> > @@ -32,6 +32,18 @@
> >  #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
> >         #devtypel #instance " "
> >
> > +#ifdef CONFIG_SANDBOX
> > +#define BOOTENV_SHARED_HOST    BOOTENV_SHARED_BLKDEV(host)
> > +#define BOOTENV_DEV_HOST       BOOTENV_DEV_BLKDEV
> > +#define BOOTENV_DEV_NAME_HOST  BOOTENV_DEV_NAME_BLKDEV
> > +#else
> > +#define BOOTENV_SHARED_HOST
> > +#define BOOTENV_DEV_HOST \
> > +       BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
> > +#define BOOTENV_DEV_NAME_HOST \
> 
> Can we use upper case in #defines?

The reason for this is that it is consistent with all other block device
blocks in that file e.g:

#define BOOTENV_DEV_SATA \
   BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA

So i'd prefer to to keep it that way. Btw, note that this is used for
compile time error reporting rather then something you'd refer to in
code.

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150228/f4595f00/attachment.bin>

[U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory

[Sjoerd Simons]

On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
> Hi Sjoerd,
> 
> On 19 February 2015 at 15:41, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
> > Properly map memory through map_sysmem so that pxe can be used from the
> > sandbox.
> >
> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
> 
> Please run your patches through patman as you seem to have style
> violations. Also can you add some notes about how you have tested this
> on real hardware?

Will do for the next round together with addressing your comments. One
confused me tough, see below.

> > ---
> >  common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
> >  1 file changed, 46 insertions(+), 34 deletions(-)

> > @@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
> >   *
> >   * Returns 1 on success, < 0 on error.
> >   */
> > -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level)
> > +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
> > +                               struct pxe_menu *cfg, int nest_level)
> 
> s/b/base/ or something more meaningful (fix above/below also)
> >  {
> >         struct token t;
> > -       char *s, *b, *label_name;
> > +       char *s, *label_name, *base;
> >         int err;
> >
> > -       b = p;
> > +       base = p;
> 
> This worries me - assigning a pointer to a ulong.


base is a pointer here though. So this comment confuses me.

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150228/76d88f81/attachment.bin>

[U-Boot] [PATCH 4/6] config_distro_bootcmd.h: Add shared block definition for the host interface

[Simon Glass]

+Stephen for real this time

Hi Sjoerd,

On 28 February 2015 at 07:05, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>> +Stephen who knows more about this stuff
>>
>> On 19 February 2015 at 15:41, Sjoerd Simons
>> <sjoerd.simons@collabora.co.uk> wrote:
>> > Define the common shared block environment for the host interface in
>> > preperation for the sandbox build to use config_distro_bootcmd.
>> >
>> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>> > ---
>> >  include/config_distro_bootcmd.h | 13 +++++++++++++
>> >  1 file changed, 13 insertions(+)
>> >
>> > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>> > index 07a0b3b..ff0e3a8 100644
>> > --- a/include/config_distro_bootcmd.h
>> > +++ b/include/config_distro_bootcmd.h
>> > @@ -32,6 +32,18 @@
>> >  #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
>> >         #devtypel #instance " "
>> >
>> > +#ifdef CONFIG_SANDBOX
>> > +#define BOOTENV_SHARED_HOST    BOOTENV_SHARED_BLKDEV(host)
>> > +#define BOOTENV_DEV_HOST       BOOTENV_DEV_BLKDEV
>> > +#define BOOTENV_DEV_NAME_HOST  BOOTENV_DEV_NAME_BLKDEV
>> > +#else
>> > +#define BOOTENV_SHARED_HOST
>> > +#define BOOTENV_DEV_HOST \
>> > +       BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
>> > +#define BOOTENV_DEV_NAME_HOST \
>>
>> Can we use upper case in #defines?
>
> The reason for this is that it is consistent with all other block device
> blocks in that file e.g:
>
> #define BOOTENV_DEV_SATA \
>    BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
>
> So i'd prefer to to keep it that way. Btw, note that this is used for
> compile time error reporting rather then something you'd refer to in
> code.

OK I see.

Regards,
Simon

[U-Boot] [PATCH 5/6] pxe: Ensure all memory access is to mapped memory

[Simon Glass]

Hi Sjoerd,

On 28 February 2015 at 07:12, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>> Hi Sjoerd,
>>
>> On 19 February 2015 at 15:41, Sjoerd Simons
>> <sjoerd.simons@collabora.co.uk> wrote:
>> > Properly map memory through map_sysmem so that pxe can be used from the
>> > sandbox.
>> >
>> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>
>> Please run your patches through patman as you seem to have style
>> violations. Also can you add some notes about how you have tested this
>> on real hardware?
>
> Will do for the next round together with addressing your comments. One
> confused me tough, see below.
>
>> > ---
>> >  common/cmd_pxe.c | 80 ++++++++++++++++++++++++++++++++------------------------
>> >  1 file changed, 46 insertions(+), 34 deletions(-)
>
>> > @@ -1281,13 +1288,14 @@ static int parse_label(char **c, struct pxe_menu *cfg)
>> >   *
>> >   * Returns 1 on success, < 0 on error.
>> >   */
>> > -static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, struct pxe_menu *cfg, int nest_level)
>> > +static int parse_pxefile_top(cmd_tbl_t *cmdtp, char *p, unsigned long b,
>> > +                               struct pxe_menu *cfg, int nest_level)
>>
>> s/b/base/ or something more meaningful (fix above/below also)
>> >  {
>> >         struct token t;
>> > -       char *s, *b, *label_name;
>> > +       char *s, *label_name, *base;
>> >         int err;
>> >
>> > -       b = p;
>> > +       base = p;
>>
>> This worries me - assigning a pointer to a ulong.
>
>
> base is a pointer here though. So this comment confuses me.

Actually it confused me. I don't think it's good to use base as a
pointer or a ulong in the same file. Maybe use base for the ulong and
base_ptr for the pointer. Or base_addr and base. But code is harder to
read if different functions in the file have different conventions.

Regards,
Simon

[U-Boot] [PATCH 4/6] config_distro_bootcmd.h: Add shared block definition for the host interface

[Stephen Warren]

On 02/28/2015 07:14 AM, Simon Glass wrote:
> +Stephen for real this time
>
> Hi Sjoerd,
>
> On 28 February 2015 at 07:05, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
>> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>>> +Stephen who knows more about this stuff
>>>
>>> On 19 February 2015 at 15:41, Sjoerd Simons
>>> <sjoerd.simons@collabora.co.uk> wrote:
>>>> Define the common shared block environment for the host interface in
>>>> preperation for the sandbox build to use config_distro_bootcmd.
>>>>
>>>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>> ---
>>>>   include/config_distro_bootcmd.h | 13 +++++++++++++
>>>>   1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>>> index 07a0b3b..ff0e3a8 100644
>>>> --- a/include/config_distro_bootcmd.h
>>>> +++ b/include/config_distro_bootcmd.h
>>>> @@ -32,6 +32,18 @@
>>>>   #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
>>>>          #devtypel #instance " "
>>>>
>>>> +#ifdef CONFIG_SANDBOX
>>>> +#define BOOTENV_SHARED_HOST    BOOTENV_SHARED_BLKDEV(host)
>>>> +#define BOOTENV_DEV_HOST       BOOTENV_DEV_BLKDEV
>>>> +#define BOOTENV_DEV_NAME_HOST  BOOTENV_DEV_NAME_BLKDEV
>>>> +#else
>>>> +#define BOOTENV_SHARED_HOST
>>>> +#define BOOTENV_DEV_HOST \
>>>> +       BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
>>>> +#define BOOTENV_DEV_NAME_HOST \
>>>
>>> Can we use upper case in #defines?
>>
>> The reason for this is that it is consistent with all other block device
>> blocks in that file e.g:
>>
>> #define BOOTENV_DEV_SATA \
>>     BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
>>
>> So i'd prefer to to keep it that way. Btw, note that this is used for
>> compile time error reporting rather then something you'd refer to in
>> code.
>
> OK I see.

Yes, in this case that mixed-case variable name is essentially an error 
message to the user. The mixed case hopefully makes it more legible 
since only other variable names are in upper case, and the rest of the 
warning text is lower case. We can't use e.g. #error here, since this 
error can only be detected in the middle of a macro expansion rather 
than via some top-level singleton #if/#ifdef check.

[U-Boot] [PATCH 4/6] config_distro_bootcmd.h: Add shared block definition for the host interface

[Simon Glass]

Hi,

On 2 March 2015 at 09:54, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/28/2015 07:14 AM, Simon Glass wrote:
>>
>> +Stephen for real this time
>>
>> Hi Sjoerd,
>>
>> On 28 February 2015 at 07:05, Sjoerd Simons
>> <sjoerd.simons@collabora.co.uk> wrote:
>>>
>>> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>>>>
>>>> +Stephen who knows more about this stuff
>>>>
>>>> On 19 February 2015 at 15:41, Sjoerd Simons
>>>> <sjoerd.simons@collabora.co.uk> wrote:
>>>>>
>>>>> Define the common shared block environment for the host interface in
>>>>> preperation for the sandbox build to use config_distro_bootcmd.
>>>>>
>>>>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>>> ---
>>>>>   include/config_distro_bootcmd.h | 13 +++++++++++++
>>>>>   1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/include/config_distro_bootcmd.h
>>>>> b/include/config_distro_bootcmd.h
>>>>> index 07a0b3b..ff0e3a8 100644
>>>>> --- a/include/config_distro_bootcmd.h
>>>>> +++ b/include/config_distro_bootcmd.h
>>>>> @@ -32,6 +32,18 @@
>>>>>   #define BOOTENV_DEV_NAME_BLKDEV(devtypeu, devtypel, instance) \
>>>>>          #devtypel #instance " "
>>>>>
>>>>> +#ifdef CONFIG_SANDBOX
>>>>> +#define BOOTENV_SHARED_HOST    BOOTENV_SHARED_BLKDEV(host)
>>>>> +#define BOOTENV_DEV_HOST       BOOTENV_DEV_BLKDEV
>>>>> +#define BOOTENV_DEV_NAME_HOST  BOOTENV_DEV_NAME_BLKDEV
>>>>> +#else
>>>>> +#define BOOTENV_SHARED_HOST
>>>>> +#define BOOTENV_DEV_HOST \
>>>>> +       BOOT_TARGET_DEVICES_references_HOST_without_CONFIG_SANDBOX
>>>>> +#define BOOTENV_DEV_NAME_HOST \
>>>>
>>>>
>>>> Can we use upper case in #defines?
>>>
>>>
>>> The reason for this is that it is consistent with all other block device
>>> blocks in that file e.g:
>>>
>>> #define BOOTENV_DEV_SATA \
>>>     BOOT_TARGET_DEVICES_references_SATA_without_CONFIG_CMD_SATA
>>>
>>> So i'd prefer to to keep it that way. Btw, note that this is used for
>>> compile time error reporting rather then something you'd refer to in
>>> code.
>>
>>
>> OK I see.
>
>
> Yes, in this case that mixed-case variable name is essentially an error
> message to the user. The mixed case hopefully makes it more legible since
> only other variable names are in upper case, and the rest of the warning
> text is lower case. We can't use e.g. #error here, since this error can only
> be detected in the middle of a macro expansion rather than via some
> top-level singleton #if/#ifdef check.

Makes sense. Maybe we could add a comment to this effect?

Regards,
Simon

[U-Boot] [PATCH 3/6] sandbox: Implement host dev [device]

[Simon Glass]

HI Sjoerd,

On 28 February 2015 at 07:00, Sjoerd Simons
<sjoerd.simons@collabora.co.uk> wrote:
> On Fri, 2015-02-20 at 12:23 -0700, Simon Glass wrote:
>> Hi Sjoerd,
>>
>> On 19 February 2015 at 15:41, Sjoerd Simons
>> <sjoerd.simons@collabora.co.uk> wrote:
>> > A common pattern to check if a certain device exists (e.g. in
>> > config_distro_bootcmd) is to use: <interface> dev [device]
>> >
>> > Implement host dev [device] so this pattern can be used for sandbox host
>> > devices.
>>
>> I don't see where this actually affects anything? In other words, does
>> it really use the device you select, or just ignore you?
>
> It gets ignored, it's only real usage is to detect whether a device
> exists.
>
> To step back the reason I implemented it here is just to make it simpler
> to integrate with the command boot commands as it's common blockdevice
> macro is:
>
> #define BOOTENV_SHARED_BLKDEV_BODY(devtypel) \
>         "if " #devtypel " dev ${devnum}; then " \
>                 "setenv devtype " #devtypel "; " \
>                 "run scan_dev_for_boot_part; " \
>         "fi\0"
>
> Which follows exactly that pattern.

I see.

>
>> > Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>> > ---
>> >  common/cmd_sandbox.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 48 insertions(+)
>> >
>> > diff --git a/common/cmd_sandbox.c b/common/cmd_sandbox.c
>> > index 4286969..7447d43 100644
>> > --- a/common/cmd_sandbox.c
>> > +++ b/common/cmd_sandbox.c
>> > @@ -10,6 +10,8 @@
>> >  #include <sandboxblockdev.h>
>
>> > +       ret = host_get_dev_err(dev, &blk_dev);
>> > +       if (ret) {
>> > +               if (ret == -ENOENT)
>> > +                       puts("Not bound to a backing file\n");
>>
>> Just use printf(), we should avoid puts() now.
>
> ok
>
>> > +               else if (ret == -ENODEV)
>> > +                       puts("Invalid host device number\n");
>> > +
>> > +               return 1;
>> > +       }
>> > +
>> > +       sandbox_curr_device = dev;
>> > +       return 0;
>> > +}
>> > +
>> > +U_BOOT_CMD(
>> > +       host, 3, 2, do_host_dev,
>> > +       "Set or get current host device", "dev [dev] - Set or retrieve the current host device"
>>
>> Can we make this command part of the 'sb' command? Like 'sh host ...'.
>
> In principle, sure, however that breaks the consistency with other
> commands which all use <interface> <interface-commands>, where
> <interface> matches the names used in the fs and partition commands.
> Which is exactly the consistency _distro_bootcmd takes advantage of in
> the macro i mentioned above.
>
> My thinking was that the sb set of commands are used to manage/setup the
> sandbox interface (e.g. setting up the host device bindings) while in
> this case the host commands are for the more standard interaction with
> host interface devices.
>
>
> Another way of addressing this and essentially side-stepping this
> discussion would be to add a a new generic dev command to allow testing
> device existance e.g.
>   dev exists host 0
>   dev exists mmc 0
>
> And switch _distro_bootcmd to that instead of it relying on all block
> interfaces having a dev subcommand with the same semantics.
>
> Opinions ? :)

How about we change the existing 'sb' command to 'host'? Arguably 'sb'
is a bit of a silly name. We could maintain an alias for a while.

Regards,
Simon

[U-Boot] [PATCH 3/6] sandbox: Implement host dev [device]

[Sjoerd Simons]

On Mon, 2015-03-02 at 14:58 -0700, Simon Glass wrote:
> HI Sjoerd,
> 

> How about we change the existing 'sb' command to 'host'? Arguably 'sb'
> is a bit of a silly name. We could maintain an alias for a while.

I'm fine with that. I'll prepare an update patchset doing the alias and
addressing your other comments pn this series. 

Thanks for the feedback!

-- 
Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150303/f3ea0749/attachment.bin>

[U-Boot] [PATCH 1/6] sandbox: only do sandboxfs for hostfs interface

[Simon Glass]

Hi Sjoerd,

On 20 February 2015 at 12:22, Simon Glass <sjg@chromium.org> wrote:
> Hi Sjoerd,
>
> On 19 February 2015 at 15:41, Sjoerd Simons
> <sjoerd.simons@collabora.co.uk> wrote:
>> Only do sandbox filesystem access when using the hostfs device
>> interface, rather then falling back to it in all cases. This prevents
>> confusion situations due to the fallback being taken rather then an
>> unsupported error being raised.
>>
>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> See nit below.
>
>> ---
>>  fs/sandbox/sandboxfs.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/sandbox/sandboxfs.c b/fs/sandbox/sandboxfs.c
>> index a920bc0..c6540c6 100644
>> --- a/fs/sandbox/sandboxfs.c
>> +++ b/fs/sandbox/sandboxfs.c
>> @@ -10,7 +10,10 @@
>>
>>  int sandbox_fs_set_blk_dev(block_dev_desc_t *rbdd, disk_partition_t *info)
>>  {
>> -       return 0;
>> +       /* Only accept a NULL block_dev_desc_t for the sandbox, which is when
>
> /*
>  * Only accept
>
>> +        * hostfs interface is used
>> +        */
>> +       return rbdd != NULL;
>>  }
>>
>>  int sandbox_fs_read_at(const char *filename, loff_t pos, void *buffer,
>> --
>> 2.1.4

Can you please respin this series? I would like to apply it to a -next tree.

Regards,
Simon