[U-Boot] [PATCH v4 1/8] sandbox: fdt: Add support for CONFIG_OF_CONTROL

[U-Boot] [PATCH v4 1/8] sandbox: fdt: Add support for CONFIG_OF_CONTROL

[Simon Glass]

This adds support for a controlling fdt, mirroring the ARM implementation.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Use #if defined()..#elif defined, instead of #ifdef..#elif defined

 arch/sandbox/include/asm/global_data.h |    1 +
 arch/sandbox/lib/board.c               |    8 ++++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/arch/sandbox/include/asm/global_data.h b/arch/sandbox/include/asm/global_data.h
index 8d47191..01a7063 100644
--- a/arch/sandbox/include/asm/global_data.h
+++ b/arch/sandbox/include/asm/global_data.h
@@ -45,6 +45,7 @@ typedef	struct global_data {
 	unsigned long	fb_base;	/* base address of frame buffer */
 	u8		*ram_buf;	/* emulated RAM buffer */
 	phys_size_t	ram_size;	/* RAM size */
+	const void	*fdt_blob;	/* Our device tree, NULL if none */
 	void		**jt;		/* jump table */
 	char		env_buf[32];	/* buffer for getenv() before reloc. */
 } gd_t;
diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c
index b7997e9..6d464d6 100644
--- a/arch/sandbox/lib/board.c
+++ b/arch/sandbox/lib/board.c
@@ -156,6 +156,14 @@ void board_init_f(ulong bootflag)
 
 	memset((void *)gd, 0, sizeof(gd_t));
 
+#if defined(CONFIG_OF_EMBED)
+	/* Get a pointer to the FDT */
+	gd->fdt_blob = _binary_dt_dtb_start;
+#elif defined(CONFIG_OF_SEPARATE)
+	/* FDT is at end of image */
+	gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
+#endif
+
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		if ((*init_fnc_ptr)() != 0)
 			hang();
-- 
1.7.7.3

[U-Boot] [PATCH v4 2/8] sandbox: config: Enable fdt and snprintf() options

[Simon Glass]

Enable fdt code and safe snprintf() options for sandbox.

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

 include/configs/sandbox.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 10565e6..6790216 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -28,6 +28,12 @@
 /* Number of bits in a C 'long' on this architecture */
 #define CONFIG_SANDBOX_BITS_PER_LONG	64
 
+#define CONFIG_OF_CONTROL
+#define CONFIG_OF_LIBFDT
+#define CONFIG_LMB
+
+#define CONFIG_SYS_VSNPRINTF
+
 /*
  * Size of malloc() pool, although we don't actually use this yet.
  */
-- 
1.7.7.3

[U-Boot] [PATCH v4 2/8] sandbox: config: Enable fdt and snprintf() options

[Mike Frysinger]

On Wednesday 15 February 2012 18:51:12 Simon Glass wrote:
> Enable fdt code and safe snprintf() options for sandbox.

merged into my sandbox branch
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120216/f2ea2db3/attachment.pgp>

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Simon Glass]

This provides a way of simulating GPIOs by setting values which are seen
by the normal gpio_get/set_value() calls.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Fix gpio_direction_output() to actually set the value
- Use generic GPIO command and interface

Changes in v3:
- Change GPIO numbers to unsigned as per new asm-generic/gpio.h
- Implement GPIO interface more fully (reservation, gpio state)
- Use __func__ when function names are required

Changes in v4:
- Add comments to clearly explain the purpose of the sandbox GPIO interface

 arch/sandbox/include/asm/gpio.h |   81 ++++++++++++++++
 drivers/gpio/Makefile           |    1 +
 drivers/gpio/sandbox.c          |  194 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 276 insertions(+), 0 deletions(-)
 create mode 100644 arch/sandbox/include/asm/gpio.h
 create mode 100644 drivers/gpio/sandbox.c

diff --git a/arch/sandbox/include/asm/gpio.h b/arch/sandbox/include/asm/gpio.h
new file mode 100644
index 0000000..0500c53
--- /dev/null
+++ b/arch/sandbox/include/asm/gpio.h
@@ -0,0 +1,81 @@
+/*
+ * This is the interface to the sandbox GPIO driver for test code which
+ * wants to change the GPIO values reported to U-Boot.
+ *
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * 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
+ */
+
+#ifndef __ASM_SANDBOX_GPIO_H
+#define __ASM_SANDBOX_GPIO_H
+
+/*
+ * We use the generic interface, and add a back-channel.
+ *
+ * The back-channel functions are declared in this file. They should not be used
+ * except in test code.
+ *
+ * Test code can, for example, call sandbox_gpio_set_value() to set the value of
+ * a simulated GPIO. From then on, normal code in U-Boot will see this new
+ * value when it calls gpio_get_value().
+ *
+ * NOTE: DO NOT use the functions in this file except in test code!
+ */
+#include <asm-generic/gpio.h>
+
+/**
+ * Return the simulated value of a GPIO (used only in sandbox test code)
+ *
+ * @param gp	GPIO number
+ * @return -1 on error, 0 if GPIO is low, >0 if high
+ */
+int sandbox_gpio_get_value(unsigned gp);
+
+/**
+ * Set the simulated value of a GPIO (used only in sandbox test code)
+ *
+ * @param gp	GPIO number
+ * @param value	value to set (0 for low, non-zero for high)
+ * @return -1 on error, 0 if ok
+ */
+int sandbox_gpio_set_value(unsigned gp, int value);
+
+/**
+ * Return the simulated direction of a GPIO (used only in sandbox test code)
+ *
+ * @param gp	GPIO number
+ * @return -1 on error, 0 if GPIO is input, >0 if output
+ */
+int sandbox_gpio_get_direction(unsigned gp);
+
+/**
+ * Set the simulated direction of a GPIO (used only in sandbox test code)
+ *
+ * @param gp	GPIO number
+ * @param output 0 to set as input, 1 to set as output
+ * @return -1 on error, 0 if ok
+ */
+int sandbox_gpio_set_direction(unsigned gp, int output);
+
+/* Display information about each GPIO */
+void gpio_info(void);
+
+#define gpio_status()	gpio_info()
+
+#endif
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4375a55..fb3b09a 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -34,6 +34,7 @@ COBJS-$(CONFIG_MXS_GPIO)	+= mxs_gpio.o
 COBJS-$(CONFIG_PCA953X)		+= pca953x.o
 COBJS-$(CONFIG_PCA9698)		+= pca9698.o
 COBJS-$(CONFIG_S5P)		+= s5p_gpio.o
+COBJS-$(CONFIG_SANDBOX_GPIO)	+= sandbox.o
 COBJS-$(CONFIG_TEGRA2_GPIO)	+= tegra2_gpio.o
 COBJS-$(CONFIG_DA8XX_GPIO)	+= da8xx_gpio.o
 COBJS-$(CONFIG_ALTERA_PIO)	+= altera_pio.o
diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
new file mode 100644
index 0000000..17e601d
--- /dev/null
+++ b/drivers/gpio/sandbox.c
@@ -0,0 +1,194 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * 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 <common.h>
+#include <asm/io.h>
+#include <asm/bitops.h>
+#include <asm-generic/gpio.h>
+#include <asm/gpio.h>
+
+/* Flags for each GPIO */
+#define GPIOF_OUTPUT	(1 << 0)	/* Currently set as an output */
+#define GPIOF_HIGH	(1 << 1)	/* Currently set high */
+#define GPIOF_RESERVED	(1 << 2)	/* Is in use / requested */
+
+struct gpio_state {
+	const char *label;	/* label given by requester */
+	u8 flags;		/* flags (GPIOF_...) */
+};
+
+/*
+ * State of GPIOs
+ * TODO: Put this into sandbox state
+ */
+static struct gpio_state state[CONFIG_SANDBOX_GPIO_COUNT];
+
+/* Access routines for GPIO state */
+static u8 *get_gpio(unsigned gp)
+{
+	assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
+	return &state[gp].flags;
+}
+
+static int get_gpio_flag(unsigned gp, int flag)
+{
+	return (*get_gpio(gp) & flag) != 0;
+}
+
+static void set_gpio_flag(unsigned gp, int flag, int value)
+{
+	u8 *gpio = get_gpio(gp);
+
+	if (value)
+		*gpio |= flag;
+	else
+		*gpio &= ~flag;
+}
+
+int sandbox_gpio_get_value(unsigned gp)
+{
+	if (get_gpio_flag(gp, GPIOF_OUTPUT))
+		printf("sandbox_gpio: get_value on output GPIO %d\n", gp);
+	return *get_gpio(gp) & GPIOF_HIGH;
+}
+
+int sandbox_gpio_set_value(unsigned gp, int value)
+{
+	set_gpio_flag(gp, GPIOF_HIGH, value);
+	return 0;
+}
+
+int sandbox_gpio_get_direction(unsigned gp)
+{
+	return get_gpio_flag(gp, GPIOF_OUTPUT);
+}
+
+int sandbox_gpio_set_direction(unsigned gp, int output)
+{
+	set_gpio_flag(gp, GPIOF_OUTPUT, output);
+	return 0;
+}
+
+static int check_reserved(unsigned gpio, const char *op_name)
+{
+	if (!get_gpio_flag(gpio, GPIOF_RESERVED)) {
+		printf("sandbox_gpio: '%s' on unreserved GPIO\n", op_name);
+		return -1;
+	}
+
+	return 0;
+}
+
+/* These functions implement the public interface within U-Boot */
+
+/* set GPIO port 'gp' as an input */
+int gpio_direction_input(unsigned gp)
+{
+	debug("%s: gp = %d\n", __func__, gp);
+	if (check_reserved(gp, __func__))
+		return -1;
+	set_gpio_flag(gp, GPIOF_OUTPUT, 0);
+
+	return 0;
+}
+
+/* set GPIO port 'gp' as an output, with polarity 'value' */
+int gpio_direction_output(unsigned gp, int value)
+{
+	debug("%s: gp = %d, value = %d\n", __func__, gp, value);
+	if (check_reserved(gp, __func__))
+		return -1;
+	set_gpio_flag(gp, GPIOF_OUTPUT, 1);
+	set_gpio_flag(gp, GPIOF_HIGH, value);
+
+	return 0;
+}
+
+/* read GPIO IN value of port 'gp' */
+int gpio_get_value(unsigned gp)
+{
+	debug("%s: gp = %d\n", __func__, gp);
+	if (check_reserved(gp, __func__))
+		return -1;
+	if (get_gpio_flag(gp, GPIOF_OUTPUT))
+		printf("sandbox_gpio: get_value on output GPIO %d\n", gp);
+
+	return get_gpio_flag(gp, GPIOF_HIGH);
+}
+
+/* write GPIO OUT value to port 'gp' */
+int gpio_set_value(unsigned gp, int value)
+{
+	debug("%s: gp = %d, value = %d\n", __func__, gp, value);
+	if (check_reserved(gp, __func__))
+		return -1;
+	if (get_gpio_flag(gp, GPIOF_OUTPUT)) {
+		set_gpio_flag(gp, GPIOF_HIGH, value);
+	} else {
+		printf("sandbox_gpio: set_value on input GPIO %d\n", gp);
+		return -1;
+	}
+
+	return 0;
+}
+
+int gpio_request(unsigned gp, const char *label)
+{
+	debug("%s: gp = %d, label= %s\n", __func__, gp, label);
+	if (get_gpio_flag(gp, GPIOF_RESERVED)) {
+		printf("sandbox_gpio: request on reserved GPIO\n");
+		return -1;
+	}
+	set_gpio_flag(gp, GPIOF_RESERVED, 1);
+	state[gp].label = label;
+
+	return 0;
+}
+
+int gpio_free(unsigned gp)
+{
+	debug("%s: gp = %d\n", __func__, gp);
+	if (check_reserved(gp, __func__))
+		return -1;
+	set_gpio_flag(gp, GPIOF_RESERVED, 0);
+	state[gp].label = NULL;
+
+	return 0;
+}
+
+/* Display GPIO information */
+void gpio_info(void)
+{
+	unsigned gpio;
+
+	printf("Sandbox GPIOs\n");
+
+	for (gpio = 0; gpio < CONFIG_SANDBOX_GPIO_COUNT; ++gpio) {
+		const char *label = state[gpio].label;
+
+		printf("%4d: %s: %d [%c] %s\n",
+			gpio,
+			get_gpio_flag(gpio, GPIOF_OUTPUT) ? "out" : " in",
+			get_gpio_flag(gpio, GPIOF_HIGH),
+			get_gpio_flag(gpio, GPIOF_RESERVED) ? 'x' : ' ',
+			label ? label : "");
+	}
+}
-- 
1.7.7.3

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Mike Frysinger]

here's the incremental/full diff against your v4 showing the direction i think
things should end up at ... incremental inline below while full is attached.
-mike

diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
index 17e601d..aa2aa4c 100644
--- a/drivers/gpio/sandbox.c
+++ b/drivers/gpio/sandbox.c
@@ -20,9 +20,6 @@
  */
 
 #include <common.h>
-#include <asm/io.h>
-#include <asm/bitops.h>
-#include <asm-generic/gpio.h>
 #include <asm/gpio.h>
 
 /* Flags for each GPIO */
@@ -42,38 +39,59 @@ struct gpio_state {
 static struct gpio_state state[CONFIG_SANDBOX_GPIO_COUNT];
 
 /* Access routines for GPIO state */
-static u8 *get_gpio(unsigned gp)
+static u8 *get_gpio_flags(unsigned gp)
 {
-	assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
+	if (gp >= ARRAY_SIZE(state)) {
+		static u8 invalid_flags;
+		printf("sandbox_gpio: error: invalid gpio %u\n", gp);
+		return &invalid_flags;
+	}
+
 	return &state[gp].flags;
 }
 
 static int get_gpio_flag(unsigned gp, int flag)
 {
-	return (*get_gpio(gp) & flag) != 0;
+	return (*get_gpio_flags(gp) & flag) != 0;
 }
 
-static void set_gpio_flag(unsigned gp, int flag, int value)
+static int set_gpio_flag(unsigned gp, int flag, int value)
 {
-	u8 *gpio = get_gpio(gp);
+	u8 *gpio = get_gpio_flags(gp);
 
 	if (value)
 		*gpio |= flag;
 	else
 		*gpio &= ~flag;
+
+	return 0;
 }
 
+static int check_reserved(unsigned gpio, const char *func)
+{
+	if (!get_gpio_flag(gpio, GPIOF_RESERVED)) {
+		printf("sandbox_gpio: %s: error: gpio %u not reserved\n",
+			func, gpio);
+		return -1;
+	}
+
+	return 0;
+}
+
+/*
+ * Back-channel sandbox-internal-only access to GPIO state
+ */
+
 int sandbox_gpio_get_value(unsigned gp)
 {
 	if (get_gpio_flag(gp, GPIOF_OUTPUT))
-		printf("sandbox_gpio: get_value on output GPIO %d\n", gp);
-	return *get_gpio(gp) & GPIOF_HIGH;
+		debug("sandbox_gpio: get_value on output gpio %u\n", gp);
+	return get_gpio_flag(gp, GPIOF_HIGH);
 }
 
 int sandbox_gpio_set_value(unsigned gp, int value)
 {
-	set_gpio_flag(gp, GPIOF_HIGH, value);
-	return 0;
+	return set_gpio_flag(gp, GPIOF_HIGH, value);
 }
 
 int sandbox_gpio_get_direction(unsigned gp)
@@ -83,95 +101,90 @@ int sandbox_gpio_get_direction(unsigned gp)
 
 int sandbox_gpio_set_direction(unsigned gp, int output)
 {
-	set_gpio_flag(gp, GPIOF_OUTPUT, output);
-	return 0;
+	return set_gpio_flag(gp, GPIOF_OUTPUT, output);
 }
 
-static int check_reserved(unsigned gpio, const char *op_name)
-{
-	if (!get_gpio_flag(gpio, GPIOF_RESERVED)) {
-		printf("sandbox_gpio: '%s' on unreserved GPIO\n", op_name);
-		return -1;
-	}
-
-	return 0;
-}
-
-/* These functions implement the public interface within U-Boot */
+/*
+ * These functions implement the public interface within U-Boot
+ */
 
 /* set GPIO port 'gp' as an input */
 int gpio_direction_input(unsigned gp)
 {
-	debug("%s: gp = %d\n", __func__, gp);
+	debug("%s: gp:%u\n", __func__, gp);
+
 	if (check_reserved(gp, __func__))
 		return -1;
-	set_gpio_flag(gp, GPIOF_OUTPUT, 0);
 
-	return 0;
+	return sandbox_gpio_set_direction(gp, 0);
 }
 
 /* set GPIO port 'gp' as an output, with polarity 'value' */
 int gpio_direction_output(unsigned gp, int value)
 {
-	debug("%s: gp = %d, value = %d\n", __func__, gp, value);
+	debug("%s: gp:%u, value = %d\n", __func__, gp, value);
+
 	if (check_reserved(gp, __func__))
 		return -1;
-	set_gpio_flag(gp, GPIOF_OUTPUT, 1);
-	set_gpio_flag(gp, GPIOF_HIGH, value);
 
-	return 0;
+	return sandbox_gpio_set_direction(gp, 1) |
+		sandbox_gpio_set_value(gp, value);
 }
 
 /* read GPIO IN value of port 'gp' */
 int gpio_get_value(unsigned gp)
 {
-	debug("%s: gp = %d\n", __func__, gp);
+	debug("%s: gp:%u\n", __func__, gp);
+
 	if (check_reserved(gp, __func__))
 		return -1;
-	if (get_gpio_flag(gp, GPIOF_OUTPUT))
-		printf("sandbox_gpio: get_value on output GPIO %d\n", gp);
 
-	return get_gpio_flag(gp, GPIOF_HIGH);
+	return sandbox_gpio_get_value(gp);
 }
 
 /* write GPIO OUT value to port 'gp' */
 int gpio_set_value(unsigned gp, int value)
 {
-	debug("%s: gp = %d, value = %d\n", __func__, gp, value);
+	debug("%s: gp:%u, value = %d\n", __func__, gp, value);
+
 	if (check_reserved(gp, __func__))
 		return -1;
-	if (get_gpio_flag(gp, GPIOF_OUTPUT)) {
-		set_gpio_flag(gp, GPIOF_HIGH, value);
-	} else {
-		printf("sandbox_gpio: set_value on input GPIO %d\n", gp);
+
+	if (!sandbox_gpio_get_direction(gp)) {
+		printf("sandbox_gpio: error: set_value on input gpio %u\n", gp);
 		return -1;
 	}
 
-	return 0;
+	return sandbox_gpio_set_value(gp, value);
 }
 
 int gpio_request(unsigned gp, const char *label)
 {
-	debug("%s: gp = %d, label= %s\n", __func__, gp, label);
+	debug("%s: gp:%u, label:%s\n", __func__, gp, label);
+
+	if (gp >= ARRAY_SIZE(state)) {
+		printf("sandbox_gpio: error: invalid gpio %u\n", gp);
+		return -1;
+	}
+
 	if (get_gpio_flag(gp, GPIOF_RESERVED)) {
-		printf("sandbox_gpio: request on reserved GPIO\n");
+		printf("sandbox_gpio: error: gpio %u already reserved\n", gp);
 		return -1;
 	}
-	set_gpio_flag(gp, GPIOF_RESERVED, 1);
-	state[gp].label = label;
 
-	return 0;
+	state[gp].label = label;
+	return set_gpio_flag(gp, GPIOF_RESERVED, 1);
 }
 
 int gpio_free(unsigned gp)
 {
-	debug("%s: gp = %d\n", __func__, gp);
+	debug("%s: gp:%u\n", __func__, gp);
+
 	if (check_reserved(gp, __func__))
 		return -1;
-	set_gpio_flag(gp, GPIOF_RESERVED, 0);
-	state[gp].label = NULL;
 
-	return 0;
+	state[gp].label = NULL;
+	return set_gpio_flag(gp, GPIOF_RESERVED, 0);
 }
 
 /* Display GPIO information */
@@ -179,15 +192,15 @@ void gpio_info(void)
 {
 	unsigned gpio;
 
-	printf("Sandbox GPIOs\n");
+	puts("Sandbox GPIOs\n");
 
-	for (gpio = 0; gpio < CONFIG_SANDBOX_GPIO_COUNT; ++gpio) {
+	for (gpio = 0; gpio < ARRAY_SIZE(state); ++gpio) {
 		const char *label = state[gpio].label;
 
 		printf("%4d: %s: %d [%c] %s\n",
 			gpio,
-			get_gpio_flag(gpio, GPIOF_OUTPUT) ? "out" : " in",
-			get_gpio_flag(gpio, GPIOF_HIGH),
+			sandbox_gpio_get_direction(gpio) ? "out" : " in",
+			sandbox_gpio_get_value(gpio),
 			get_gpio_flag(gpio, GPIOF_RESERVED) ? 'x' : ' ',
 			label ? label : "");
 	}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sandbox-gpio.patch
Type: text/x-patch
Size: 9199 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120221/62740b64/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120221/62740b64/attachment.pgp>

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Simon Glass]

Hi Mike,

On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> here's the incremental/full diff against your v4 showing the direction i think
> things should end up at ... incremental inline below while full is attached.
> -mike

Hmmm I'm fine with the get_gpio_flags() rename, printf() changes, but
please see below.

>
> diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
> index 17e601d..aa2aa4c 100644
> --- a/drivers/gpio/sandbox.c
> +++ b/drivers/gpio/sandbox.c
> @@ -20,9 +20,6 @@
> ?*/
>
> ?#include <common.h>
> -#include <asm/io.h>
> -#include <asm/bitops.h>
> -#include <asm-generic/gpio.h>
> ?#include <asm/gpio.h>
>
> ?/* Flags for each GPIO */
> @@ -42,38 +39,59 @@ struct gpio_state {
> ?static struct gpio_state state[CONFIG_SANDBOX_GPIO_COUNT];
>
> ?/* Access routines for GPIO state */
> -static u8 *get_gpio(unsigned gp)
> +static u8 *get_gpio_flags(unsigned gp)
> ?{
> - ? ? ? assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
> + ? ? ? if (gp >= ARRAY_SIZE(state)) {
> + ? ? ? ? ? ? ? static u8 invalid_flags;
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: invalid gpio %u\n", gp);
> + ? ? ? ? ? ? ? return &invalid_flags;
> + ? ? ? }
> +

I think we want to die / fail the test here, but since we don't have
any tests I suppose this is ok for now. I like assert() because it
halts.

> ? ? ? ?return &state[gp].flags;
> ?}
>
> ?static int get_gpio_flag(unsigned gp, int flag)
> ?{
> - ? ? ? return (*get_gpio(gp) & flag) != 0;
> + ? ? ? return (*get_gpio_flags(gp) & flag) != 0;
> ?}
>
> -static void set_gpio_flag(unsigned gp, int flag, int value)
> +static int set_gpio_flag(unsigned gp, int flag, int value)
> ?{
> - ? ? ? u8 *gpio = get_gpio(gp);
> + ? ? ? u8 *gpio = get_gpio_flags(gp);
>
> ? ? ? ?if (value)
> ? ? ? ? ? ? ? ?*gpio |= flag;
> ? ? ? ?else
> ? ? ? ? ? ? ? ?*gpio &= ~flag;
> +
> + ? ? ? return 0;
> ?}
>
> +static int check_reserved(unsigned gpio, const char *func)
> +{
> + ? ? ? if (!get_gpio_flag(gpio, GPIOF_RESERVED)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: %s: error: gpio %u not reserved\n",
> + ? ? ? ? ? ? ? ? ? ? ? func, gpio);
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
> +
> + ? ? ? return 0;
> +}
> +
> +/*
> + * Back-channel sandbox-internal-only access to GPIO state
> + */
> +
> ?int sandbox_gpio_get_value(unsigned gp)
> ?{
> ? ? ? ?if (get_gpio_flag(gp, GPIOF_OUTPUT))
> - ? ? ? ? ? ? ? printf("sandbox_gpio: get_value on output GPIO %d\n", gp);
> - ? ? ? return *get_gpio(gp) & GPIOF_HIGH;
> + ? ? ? ? ? ? ? debug("sandbox_gpio: get_value on output gpio %u\n", gp);
> + ? ? ? return get_gpio_flag(gp, GPIOF_HIGH);
> ?}
>
> ?int sandbox_gpio_set_value(unsigned gp, int value)
> ?{
> - ? ? ? set_gpio_flag(gp, GPIOF_HIGH, value);
> - ? ? ? return 0;
> + ? ? ? return set_gpio_flag(gp, GPIOF_HIGH, value);
> ?}
>
> ?int sandbox_gpio_get_direction(unsigned gp)
> @@ -83,95 +101,90 @@ int sandbox_gpio_get_direction(unsigned gp)
>
> ?int sandbox_gpio_set_direction(unsigned gp, int output)
> ?{
> - ? ? ? set_gpio_flag(gp, GPIOF_OUTPUT, output);
> - ? ? ? return 0;
> + ? ? ? return set_gpio_flag(gp, GPIOF_OUTPUT, output);
> ?}
>
> -static int check_reserved(unsigned gpio, const char *op_name)
> -{
> - ? ? ? if (!get_gpio_flag(gpio, GPIOF_RESERVED)) {
> - ? ? ? ? ? ? ? printf("sandbox_gpio: '%s' on unreserved GPIO\n", op_name);
> - ? ? ? ? ? ? ? return -1;
> - ? ? ? }
> -
> - ? ? ? return 0;
> -}
> -
> -/* These functions implement the public interface within U-Boot */
> +/*
> + * These functions implement the public interface within U-Boot
> + */
>
> ?/* set GPIO port 'gp' as an input */
> ?int gpio_direction_input(unsigned gp)
> ?{
> - ? ? ? debug("%s: gp = %d\n", __func__, gp);
> + ? ? ? debug("%s: gp:%u\n", __func__, gp);
> +
> ? ? ? ?if (check_reserved(gp, __func__))
> ? ? ? ? ? ? ? ?return -1;
> - ? ? ? set_gpio_flag(gp, GPIOF_OUTPUT, 0);
>
> - ? ? ? return 0;
> + ? ? ? return sandbox_gpio_set_direction(gp, 0);

Ick, we shouldn't call that function here - it is in the test code. Same below.

The idea is that this state has two completely separate sides to it -
by calling the 'test' functions from the 'U-Boot' functions I think
you are going to confuse people a lot.

> ?}
>
> ?/* set GPIO port 'gp' as an output, with polarity 'value' */
> ?int gpio_direction_output(unsigned gp, int value)
> ?{
> - ? ? ? debug("%s: gp = %d, value = %d\n", __func__, gp, value);
> + ? ? ? debug("%s: gp:%u, value = %d\n", __func__, gp, value);
> +
> ? ? ? ?if (check_reserved(gp, __func__))
> ? ? ? ? ? ? ? ?return -1;
> - ? ? ? set_gpio_flag(gp, GPIOF_OUTPUT, 1);
> - ? ? ? set_gpio_flag(gp, GPIOF_HIGH, value);
>
> - ? ? ? return 0;
> + ? ? ? return sandbox_gpio_set_direction(gp, 1) |
> + ? ? ? ? ? ? ? sandbox_gpio_set_value(gp, value);
> ?}
>
> ?/* read GPIO IN value of port 'gp' */
> ?int gpio_get_value(unsigned gp)
> ?{
> - ? ? ? debug("%s: gp = %d\n", __func__, gp);
> + ? ? ? debug("%s: gp:%u\n", __func__, gp);
> +
> ? ? ? ?if (check_reserved(gp, __func__))
> ? ? ? ? ? ? ? ?return -1;
> - ? ? ? if (get_gpio_flag(gp, GPIOF_OUTPUT))
> - ? ? ? ? ? ? ? printf("sandbox_gpio: get_value on output GPIO %d\n", gp);
>
> - ? ? ? return get_gpio_flag(gp, GPIOF_HIGH);
> + ? ? ? return sandbox_gpio_get_value(gp);
> ?}
>
> ?/* write GPIO OUT value to port 'gp' */
> ?int gpio_set_value(unsigned gp, int value)
> ?{
> - ? ? ? debug("%s: gp = %d, value = %d\n", __func__, gp, value);
> + ? ? ? debug("%s: gp:%u, value = %d\n", __func__, gp, value);
> +
> ? ? ? ?if (check_reserved(gp, __func__))
> ? ? ? ? ? ? ? ?return -1;
> - ? ? ? if (get_gpio_flag(gp, GPIOF_OUTPUT)) {
> - ? ? ? ? ? ? ? set_gpio_flag(gp, GPIOF_HIGH, value);
> - ? ? ? } else {
> - ? ? ? ? ? ? ? printf("sandbox_gpio: set_value on input GPIO %d\n", gp);
> +
> + ? ? ? if (!sandbox_gpio_get_direction(gp)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: set_value on input gpio %u\n", gp);
> ? ? ? ? ? ? ? ?return -1;
> ? ? ? ?}
>
> - ? ? ? return 0;
> + ? ? ? return sandbox_gpio_set_value(gp, value);
> ?}
>
> ?int gpio_request(unsigned gp, const char *label)
> ?{
> - ? ? ? debug("%s: gp = %d, label= %s\n", __func__, gp, label);
> + ? ? ? debug("%s: gp:%u, label:%s\n", __func__, gp, label);
> +
> + ? ? ? if (gp >= ARRAY_SIZE(state)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: invalid gpio %u\n", gp);
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
> +
> ? ? ? ?if (get_gpio_flag(gp, GPIOF_RESERVED)) {
> - ? ? ? ? ? ? ? printf("sandbox_gpio: request on reserved GPIO\n");
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: gpio %u already reserved\n", gp);
> ? ? ? ? ? ? ? ?return -1;
> ? ? ? ?}
> - ? ? ? set_gpio_flag(gp, GPIOF_RESERVED, 1);
> - ? ? ? state[gp].label = label;
>
> - ? ? ? return 0;
> + ? ? ? state[gp].label = label;
> + ? ? ? return set_gpio_flag(gp, GPIOF_RESERVED, 1);
> ?}
>
> ?int gpio_free(unsigned gp)
> ?{
> - ? ? ? debug("%s: gp = %d\n", __func__, gp);
> + ? ? ? debug("%s: gp:%u\n", __func__, gp);
> +
> ? ? ? ?if (check_reserved(gp, __func__))
> ? ? ? ? ? ? ? ?return -1;
> - ? ? ? set_gpio_flag(gp, GPIOF_RESERVED, 0);
> - ? ? ? state[gp].label = NULL;
>
> - ? ? ? return 0;
> + ? ? ? state[gp].label = NULL;
> + ? ? ? return set_gpio_flag(gp, GPIOF_RESERVED, 0);
> ?}
>
> ?/* Display GPIO information */
> @@ -179,15 +192,15 @@ void gpio_info(void)
> ?{
> ? ? ? ?unsigned gpio;
>
> - ? ? ? printf("Sandbox GPIOs\n");
> + ? ? ? puts("Sandbox GPIOs\n");
>
> - ? ? ? for (gpio = 0; gpio < CONFIG_SANDBOX_GPIO_COUNT; ++gpio) {
> + ? ? ? for (gpio = 0; gpio < ARRAY_SIZE(state); ++gpio) {
> ? ? ? ? ? ? ? ?const char *label = state[gpio].label;
>
> ? ? ? ? ? ? ? ?printf("%4d: %s: %d [%c] %s\n",
> ? ? ? ? ? ? ? ? ? ? ? ?gpio,
> - ? ? ? ? ? ? ? ? ? ? ? get_gpio_flag(gpio, GPIOF_OUTPUT) ? "out" : " in",
> - ? ? ? ? ? ? ? ? ? ? ? get_gpio_flag(gpio, GPIOF_HIGH),
> + ? ? ? ? ? ? ? ? ? ? ? sandbox_gpio_get_direction(gpio) ? "out" : " in",
> + ? ? ? ? ? ? ? ? ? ? ? sandbox_gpio_get_value(gpio),
> ? ? ? ? ? ? ? ? ? ? ? ?get_gpio_flag(gpio, GPIOF_RESERVED) ? 'x' : ' ',
> ? ? ? ? ? ? ? ? ? ? ? ?label ? label : "");
> ? ? ? ?}

Regards,
Simon

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Mike Frysinger]

On Tuesday 21 February 2012 01:27:31 Simon Glass wrote:
> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote:
> > --- a/drivers/gpio/sandbox.c
> > +++ b/drivers/gpio/sandbox.c
> > 
> >  /* Access routines for GPIO state */
> > -static u8 *get_gpio(unsigned gp)
> > +static u8 *get_gpio_flags(unsigned gp)
> >  {
> > -       assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
> > +       if (gp >= ARRAY_SIZE(state)) {
> > +               static u8 invalid_flags;
> > +               printf("sandbox_gpio: error: invalid gpio %u\n", gp);
> > +               return &invalid_flags;
> > +       }
> > +
> 
> I think we want to die / fail the test here, but since we don't have
> any tests I suppose this is ok for now. I like assert() because it
> halts.

the problem is that assert() is disabled by default, so by default, we get 
memory corruption :).  i tend to agree with your previous statements (in 
another thread) that the sandbox should, by default, do arg checking since the 
sandbox env is expected to be tested/developed under.

extending that logic, i think it makes more sense to get output that includes 
errors but "works" so people can play around more on the command line without 
interrupting things.  after all, i'd rather see an error message if i was in 
the middle of typing "gpio ..." on the command line but fat fingered the gpio 
number and typed "gpio set 199" instead of "gpio set 19".

> >  /* set GPIO port 'gp' as an input */
> >  int gpio_direction_input(unsigned gp)
> >  {
> > -       debug("%s: gp = %d\n", __func__, gp);
> > +       debug("%s: gp:%u\n", __func__, gp);
> > +
> >        if (check_reserved(gp, __func__))
> >                return -1;
> > -       set_gpio_flag(gp, GPIOF_OUTPUT, 0);
> > 
> > -       return 0;
> > +       return sandbox_gpio_set_direction(gp, 0);
> 
> Ick, we shouldn't call that function here - it is in the test code. Same
> below.
> 
> The idea is that this state has two completely separate sides to it -
> by calling the 'test' functions from the 'U-Boot' functions I think
> you are going to confuse people a lot.

the way i see it is we have the pin state ("state"), we have direct accessor 
functions with no error checking so other things can directly manipulate that 
state (sandbox_gpio_xxx), and we have the generic gpio api (gpio_xxx).  i 
don't think both API's should get to directly manipulate the state ... it's 
more logical (to me) that the generic gpio api be built off the hardware state 
api rather than grubbin' around directly.

the only place that gets confusing is when we have one structure that ends up 
storing the hardware state (pin direction/levels) along side the generic gpio 
state (pin reservation and friendly label names).  although, thinking a little 
more, we should be able to split that out easily enough -- have an array of 
labels and if a gpio's label is NULL, we know the pin is not reserved.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120221/109902bf/attachment.pgp>

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Simon Glass]

Hi Mike,

On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 21 February 2012 01:27:31 Simon Glass wrote:
>> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote:
>> > --- a/drivers/gpio/sandbox.c
>> > +++ b/drivers/gpio/sandbox.c
>> >
>> > ?/* Access routines for GPIO state */
>> > -static u8 *get_gpio(unsigned gp)
>> > +static u8 *get_gpio_flags(unsigned gp)
>> > ?{
>> > - ? ? ? assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
>> > + ? ? ? if (gp >= ARRAY_SIZE(state)) {
>> > + ? ? ? ? ? ? ? static u8 invalid_flags;
>> > + ? ? ? ? ? ? ? printf("sandbox_gpio: error: invalid gpio %u\n", gp);
>> > + ? ? ? ? ? ? ? return &invalid_flags;
>> > + ? ? ? }
>> > +
>>
>> I think we want to die / fail the test here, but since we don't have
>> any tests I suppose this is ok for now. I like assert() because it
>> halts.
>
> the problem is that assert() is disabled by default, so by default, we get
> memory corruption :). ?i tend to agree with your previous statements (in
> another thread) that the sandbox should, by default, do arg checking since the
> sandbox env is expected to be tested/developed under.
>
> extending that logic, i think it makes more sense to get output that includes
> errors but "works" so people can play around more on the command line without
> interrupting things. ?after all, i'd rather see an error message if i was in
> the middle of typing "gpio ..." on the command line but fat fingered the gpio
> number and typed "gpio set 199" instead of "gpio set 19".

I think the opposite though - it makes more sense to me that the test
fails and reports failure, than continues with bogus results.

How about you leave the assert in as well - then I will be happy enough.

Later we could change assert to always bail on sandbox, or make
sandbox always build with DEBUG (although we would need to introduce a
way of skipping the output).

>
>> > ?/* set GPIO port 'gp' as an input */
>> > ?int gpio_direction_input(unsigned gp)
>> > ?{
>> > - ? ? ? debug("%s: gp = %d\n", __func__, gp);
>> > + ? ? ? debug("%s: gp:%u\n", __func__, gp);
>> > +
>> > ? ? ? ?if (check_reserved(gp, __func__))
>> > ? ? ? ? ? ? ? ?return -1;
>> > - ? ? ? set_gpio_flag(gp, GPIOF_OUTPUT, 0);
>> >
>> > - ? ? ? return 0;
>> > + ? ? ? return sandbox_gpio_set_direction(gp, 0);
>>
>> Ick, we shouldn't call that function here - it is in the test code. Same
>> below.
>>
>> The idea is that this state has two completely separate sides to it -
>> by calling the 'test' functions from the 'U-Boot' functions I think
>> you are going to confuse people a lot.
>
> the way i see it is we have the pin state ("state"), we have direct accessor
> functions with no error checking so other things can directly manipulate that
> state (sandbox_gpio_xxx), and we have the generic gpio api (gpio_xxx). ?i
> don't think both API's should get to directly manipulate the state ... it's
> more logical (to me) that the generic gpio api be built off the hardware state
> api rather than grubbin' around directly.
>
> the only place that gets confusing is when we have one structure that ends up
> storing the hardware state (pin direction/levels) along side the generic gpio
> state (pin reservation and friendly label names). ?although, thinking a little
> more, we should be able to split that out easily enough -- have an array of
> labels and if a gpio's label is NULL, we know the pin is not reserved.

What I find confusing is that you implement the external API using the
test API - I mean confusing for people reading the code. It would be
better (if you want functions to access all the state) if there were
an internal access functions which the two sets use. I was trying to
keep them as separate as possible.

Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO
and doesn't stop.

IMO

- the test API should fault an invalid access and stop
- the external API should assert() and continue.

I agree that assert() is currently skipped and will not cause a test
to fail, but of course we can address that if you like.

Anyway, this GPIO implementation is better than what we currently
have...so if you really think this is the right thing to do, then
let's go with it and address it later when we have some tests.

> -mike

Regards,
Simon

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Mike Frysinger]

On Tuesday 21 February 2012 16:55:39 Simon Glass wrote:
> On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger wrote:
> > On Tuesday 21 February 2012 01:27:31 Simon Glass wrote:
> >> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote:
> >> > --- a/drivers/gpio/sandbox.c
> >> > +++ b/drivers/gpio/sandbox.c
> >> > 
> >> >  /* Access routines for GPIO state */
> >> > -static u8 *get_gpio(unsigned gp)
> >> > +static u8 *get_gpio_flags(unsigned gp)
> >> >  {
> >> > -       assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
> >> > +       if (gp >= ARRAY_SIZE(state)) {
> >> > +               static u8 invalid_flags;
> >> > +               printf("sandbox_gpio: error: invalid gpio %u\n", gp);
> >> > +               return &invalid_flags;
> >> > +       }
> >> > +
> >> 
> >> I think we want to die / fail the test here, but since we don't have
> >> any tests I suppose this is ok for now. I like assert() because it
> >> halts.
> > 
> > the problem is that assert() is disabled by default, so by default, we
> > get memory corruption :).  i tend to agree with your previous statements
> > (in another thread) that the sandbox should, by default, do arg checking
> > since the sandbox env is expected to be tested/developed under.
> > 
> > extending that logic, i think it makes more sense to get output that
> > includes errors but "works" so people can play around more on the
> > command line without interrupting things.  after all, i'd rather see an
> > error message if i was in the middle of typing "gpio ..." on the command
> > line but fat fingered the gpio number and typed "gpio set 199" instead
> > of "gpio set 19".
> 
> I think the opposite though - it makes more sense to me that the test
> fails and reports failure, than continues with bogus results.

any test framework worth using will catch the error message that is displayed, 
so i don't think that's a big deal

> How about you leave the assert in as well - then I will be happy enough.

i'm OK with that

> Later we could change assert to always bail on sandbox, or make
> sandbox always build with DEBUG (although we would need to introduce a
> way of skipping the output).

we'd have to split the knobs so we could do assert() and not debug()

> >> >  /* set GPIO port 'gp' as an input */
> >> >  int gpio_direction_input(unsigned gp)
> >> >  {
> >> > -       debug("%s: gp = %d\n", __func__, gp);
> >> > +       debug("%s: gp:%u\n", __func__, gp);
> >> > +
> >> >        if (check_reserved(gp, __func__))
> >> >                return -1;
> >> > -       set_gpio_flag(gp, GPIOF_OUTPUT, 0);
> >> > 
> >> > -       return 0;
> >> > +       return sandbox_gpio_set_direction(gp, 0);
> >> 
> >> Ick, we shouldn't call that function here - it is in the test code. Same
> >> below.
> >> 
> >> The idea is that this state has two completely separate sides to it -
> >> by calling the 'test' functions from the 'U-Boot' functions I think
> >> you are going to confuse people a lot.
> > 
> > the way i see it is we have the pin state ("state"), we have direct
> > accessor functions with no error checking so other things can directly
> > manipulate that state (sandbox_gpio_xxx), and we have the generic gpio
> > api (gpio_xxx).  i don't think both API's should get to directly
> > manipulate the state ... it's more logical (to me) that the generic gpio
> > api be built off the hardware state api rather than grubbin' around
> > directly.
> > 
> > the only place that gets confusing is when we have one structure that
> > ends up storing the hardware state (pin direction/levels) along side the
> > generic gpio state (pin reservation and friendly label names).
> >  although, thinking a little more, we should be able to split that out
> > easily enough -- have an array of labels and if a gpio's label is NULL,
> > we know the pin is not reserved.
> 
> What I find confusing is that you implement the external API using the
> test API - I mean confusing for people reading the code. It would be
> better (if you want functions to access all the state) if there were
> an internal access functions which the two sets use. I was trying to
> keep them as separate as possible.

i thought when we discussed this before that sandbox_gpio_xxx isn't a test 
api.  it *could* be used to seed the initial test state, or to fake out a 
simple device, but it's more than that.  if i was writing a proper simulation 
of a device connected over gpio lines, that device would need direct access to 
the pin state and thus would utilize sandbox_gpio_xxx.  i wouldn't label this 
simulated device as "test" code.

so once i have it in my mind that that we've got hardware state, and 
sandbox_gpio_xxx is how you access that state, the gpio_xxx api fits neatly on 
top of that.  it's no different from having memory mapped registers that 
represent a block of gpio's -- in one case i'm doing readl(foo) and in the 
other i'm doing sandbox_gpio_xxx().

if i wanted to push the envelope, i'd even move the sandbox_gpio_xxx logic to 
a dedicated file in arch/sandbox/cpu/gpio.c.  then in order to access the 
"hardware", you'd have to call the sandbox_gpio_xxx funcs.

> Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO
> and doesn't stop.

well, it issues an error message for the developer to see, but there's no 
arbitrary memory access going on.

> - the test API should fault an invalid access and stop
> - the external API should assert() and continue.

"assert() and continue" doesn't make sense ... an assert(), by definition, will 
halt termination when things fail
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120221/c0a0ce77/attachment.pgp>

[U-Boot] [PATCH v4 3/8] sandbox: gpio: Add basic driver for simulating GPIOs

[Simon Glass]

Hi Mike,

On Tue, Feb 21, 2012 at 2:13 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Tuesday 21 February 2012 16:55:39 Simon Glass wrote:
>> On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger wrote:
>> > On Tuesday 21 February 2012 01:27:31 Simon Glass wrote:
>> >> On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote:
>> >> > --- a/drivers/gpio/sandbox.c
>> >> > +++ b/drivers/gpio/sandbox.c
>> >> >
>> >> > ?/* Access routines for GPIO state */
>> >> > -static u8 *get_gpio(unsigned gp)
>> >> > +static u8 *get_gpio_flags(unsigned gp)
>> >> > ?{
>> >> > - ? ? ? assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
>> >> > + ? ? ? if (gp >= ARRAY_SIZE(state)) {
>> >> > + ? ? ? ? ? ? ? static u8 invalid_flags;
>> >> > + ? ? ? ? ? ? ? printf("sandbox_gpio: error: invalid gpio %u\n", gp);
>> >> > + ? ? ? ? ? ? ? return &invalid_flags;
>> >> > + ? ? ? }
>> >> > +
>> >>
>> >> I think we want to die / fail the test here, but since we don't have
>> >> any tests I suppose this is ok for now. I like assert() because it
>> >> halts.
>> >
>> > the problem is that assert() is disabled by default, so by default, we
>> > get memory corruption :). ?i tend to agree with your previous statements
>> > (in another thread) that the sandbox should, by default, do arg checking
>> > since the sandbox env is expected to be tested/developed under.
>> >
>> > extending that logic, i think it makes more sense to get output that
>> > includes errors but "works" so people can play around more on the
>> > command line without interrupting things. ?after all, i'd rather see an
>> > error message if i was in the middle of typing "gpio ..." on the command
>> > line but fat fingered the gpio number and typed "gpio set 199" instead
>> > of "gpio set 19".
>>
>> I think the opposite though - it makes more sense to me that the test
>> fails and reports failure, than continues with bogus results.
>
> any test framework worth using will catch the error message that is displayed,
> so i don't think that's a big deal
>
>> How about you leave the assert in as well - then I will be happy enough.
>
> i'm OK with that

OK

>
>> Later we could change assert to always bail on sandbox, or make
>> sandbox always build with DEBUG (although we would need to introduce a
>> way of skipping the output).
>
> we'd have to split the knobs so we could do assert() and not debug()

Yes. Later, maybe.

>
>> >> > ?/* set GPIO port 'gp' as an input */
>> >> > ?int gpio_direction_input(unsigned gp)
>> >> > ?{
>> >> > - ? ? ? debug("%s: gp = %d\n", __func__, gp);
>> >> > + ? ? ? debug("%s: gp:%u\n", __func__, gp);
>> >> > +
>> >> > ? ? ? ?if (check_reserved(gp, __func__))
>> >> > ? ? ? ? ? ? ? ?return -1;
>> >> > - ? ? ? set_gpio_flag(gp, GPIOF_OUTPUT, 0);
>> >> >
>> >> > - ? ? ? return 0;
>> >> > + ? ? ? return sandbox_gpio_set_direction(gp, 0);
>> >>
>> >> Ick, we shouldn't call that function here - it is in the test code. Same
>> >> below.
>> >>
>> >> The idea is that this state has two completely separate sides to it -
>> >> by calling the 'test' functions from the 'U-Boot' functions I think
>> >> you are going to confuse people a lot.
>> >
>> > the way i see it is we have the pin state ("state"), we have direct
>> > accessor functions with no error checking so other things can directly
>> > manipulate that state (sandbox_gpio_xxx), and we have the generic gpio
>> > api (gpio_xxx). ?i don't think both API's should get to directly
>> > manipulate the state ... it's more logical (to me) that the generic gpio
>> > api be built off the hardware state api rather than grubbin' around
>> > directly.
>> >
>> > the only place that gets confusing is when we have one structure that
>> > ends up storing the hardware state (pin direction/levels) along side the
>> > generic gpio state (pin reservation and friendly label names).
>> > ?although, thinking a little more, we should be able to split that out
>> > easily enough -- have an array of labels and if a gpio's label is NULL,
>> > we know the pin is not reserved.
>>
>> What I find confusing is that you implement the external API using the
>> test API - I mean confusing for people reading the code. It would be
>> better (if you want functions to access all the state) if there were
>> an internal access functions which the two sets use. I was trying to
>> keep them as separate as possible.
>
> i thought when we discussed this before that sandbox_gpio_xxx isn't a test
> api. ?it *could* be used to seed the initial test state, or to fake out a
> simple device, but it's more than that. ?if i was writing a proper simulation
> of a device connected over gpio lines, that device would need direct access to
> the pin state and thus would utilize sandbox_gpio_xxx. ?i wouldn't label this
> simulated device as "test" code.
>
> so once i have it in my mind that that we've got hardware state, and
> sandbox_gpio_xxx is how you access that state, the gpio_xxx api fits neatly on
> top of that. ?it's no different from having memory mapped registers that
> represent a block of gpio's -- in one case i'm doing readl(foo) and in the
> other i'm doing sandbox_gpio_xxx().
>
> if i wanted to push the envelope, i'd even move the sandbox_gpio_xxx logic to
> a dedicated file in arch/sandbox/cpu/gpio.c. ?then in order to access the
> "hardware", you'd have to call the sandbox_gpio_xxx funcs.

Well yes GPIO state could go into state.h one day and be accessed from
there as I think we previously discussed when I was motivating
state.h. Your change makes more sense in that case than it does now. I
didn't do that because we don't yet have a way to load/save state so
the need for centralising it isn't there (yet).

Let's go with your approach until we get to that point.
>
>> Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO
>> and doesn't stop.
>
> well, it issues an error message for the developer to see, but there's no
> arbitrary memory access going on.

If you add the assert() then I'm happy with this.

>
>> - the test API should fault an invalid access and stop
>> - the external API should assert() and continue.
>
> "assert() and continue" doesn't make sense ... an assert(), by definition, will
> halt termination when things fail

You mention above that assert() is a nop when DEBUG is not defined -
that's what I meant by that comment. In other words the dev chooses
whether to abort or not, but it is definitely flagged as an error.

> -mike

Regards,
Simon

[U-Boot] [PATCH v5] sandbox: gpio: Add basic driver for simulating GPIOs

[Mike Frysinger]

let's get your ack-back on this and i'll merge it into my branch.
only diff from last code is restored assert() to get_gpio_flags().
-mike

[U-Boot] [PATCH v5] sandbox: gpio: Add basic driver for simulating GPIOs

[Simon Glass]

On Tue, Feb 21, 2012 at 9:08 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> let's get your ack-back on this and i'll merge it into my branch.
> only diff from last code is restored assert() to get_gpio_flags().
> -mike
>
> From f67fd9b82ec180d8a227bf036a45eb31e713a5a9 Mon Sep 17 00:00:00 2001
> From: Simon Glass <sjg@chromium.org>
> Date: Wed, 15 Feb 2012 15:51:13 -0800
> Subject: [PATCH v5] sandbox: gpio: Add basic driver for simulating GPIOs
>
> This provides a way of simulating GPIOs by setting values which are seen
> by the normal gpio_get/set_value() calls.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

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

> ---
> v5
> ? ? ? ?- combine simon & my work
>
> ?arch/sandbox/include/asm/gpio.h | ? 81 +++++++++++++++
> ?drivers/gpio/Makefile ? ? ? ? ? | ? ?1 +
> ?drivers/gpio/sandbox.c ? ? ? ? ?| ?209 +++++++++++++++++++++++++++++++++++++++
> ?3 files changed, 291 insertions(+), 0 deletions(-)
> ?create mode 100644 arch/sandbox/include/asm/gpio.h
> ?create mode 100644 drivers/gpio/sandbox.c
>
> diff --git a/arch/sandbox/include/asm/gpio.h b/arch/sandbox/include/asm/gpio.h
> new file mode 100644
> index 0000000..0500c53
> --- /dev/null
> +++ b/arch/sandbox/include/asm/gpio.h
> @@ -0,0 +1,81 @@
> +/*
> + * This is the interface to the sandbox GPIO driver for test code which
> + * wants to change the GPIO values reported to U-Boot.
> + *
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * 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
> + */
> +
> +#ifndef __ASM_SANDBOX_GPIO_H
> +#define __ASM_SANDBOX_GPIO_H
> +
> +/*
> + * We use the generic interface, and add a back-channel.
> + *
> + * The back-channel functions are declared in this file. They should not be used
> + * except in test code.
> + *
> + * Test code can, for example, call sandbox_gpio_set_value() to set the value of
> + * a simulated GPIO. From then on, normal code in U-Boot will see this new
> + * value when it calls gpio_get_value().
> + *
> + * NOTE: DO NOT use the functions in this file except in test code!
> + */
> +#include <asm-generic/gpio.h>
> +
> +/**
> + * Return the simulated value of a GPIO (used only in sandbox test code)
> + *
> + * @param gp ? GPIO number
> + * @return -1 on error, 0 if GPIO is low, >0 if high
> + */
> +int sandbox_gpio_get_value(unsigned gp);
> +
> +/**
> + * Set the simulated value of a GPIO (used only in sandbox test code)
> + *
> + * @param gp ? GPIO number
> + * @param value ? ? ? ?value to set (0 for low, non-zero for high)
> + * @return -1 on error, 0 if ok
> + */
> +int sandbox_gpio_set_value(unsigned gp, int value);
> +
> +/**
> + * Return the simulated direction of a GPIO (used only in sandbox test code)
> + *
> + * @param gp ? GPIO number
> + * @return -1 on error, 0 if GPIO is input, >0 if output
> + */
> +int sandbox_gpio_get_direction(unsigned gp);
> +
> +/**
> + * Set the simulated direction of a GPIO (used only in sandbox test code)
> + *
> + * @param gp ? GPIO number
> + * @param output 0 to set as input, 1 to set as output
> + * @return -1 on error, 0 if ok
> + */
> +int sandbox_gpio_set_direction(unsigned gp, int output);
> +
> +/* Display information about each GPIO */
> +void gpio_info(void);
> +
> +#define gpio_status() ?gpio_info()
> +
> +#endif
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 4375a55..fb3b09a 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -34,6 +34,7 @@ COBJS-$(CONFIG_MXS_GPIO) ? ? ?+= mxs_gpio.o
> ?COBJS-$(CONFIG_PCA953X) ? ? ? ? ? ? ? ?+= pca953x.o
> ?COBJS-$(CONFIG_PCA9698) ? ? ? ? ? ? ? ?+= pca9698.o
> ?COBJS-$(CONFIG_S5P) ? ? ? ? ? ?+= s5p_gpio.o
> +COBJS-$(CONFIG_SANDBOX_GPIO) ? += sandbox.o
> ?COBJS-$(CONFIG_TEGRA2_GPIO) ? ?+= tegra2_gpio.o
> ?COBJS-$(CONFIG_DA8XX_GPIO) ? ? += da8xx_gpio.o
> ?COBJS-$(CONFIG_ALTERA_PIO) ? ? += altera_pio.o
> diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
> new file mode 100644
> index 0000000..19d2db0
> --- /dev/null
> +++ b/drivers/gpio/sandbox.c
> @@ -0,0 +1,209 @@
> +/*
> + * Copyright (c) 2011 The Chromium OS Authors.
> + * 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 <common.h>
> +#include <asm/gpio.h>
> +
> +/* Flags for each GPIO */
> +#define GPIOF_OUTPUT ? (1 << 0) ? ? ? ?/* Currently set as an output */
> +#define GPIOF_HIGH ? ? (1 << 1) ? ? ? ?/* Currently set high */
> +#define GPIOF_RESERVED (1 << 2) ? ? ? ?/* Is in use / requested */
> +
> +struct gpio_state {
> + ? ? ? const char *label; ? ? ?/* label given by requester */
> + ? ? ? u8 flags; ? ? ? ? ? ? ? /* flags (GPIOF_...) */
> +};
> +
> +/*
> + * State of GPIOs
> + * TODO: Put this into sandbox state
> + */
> +static struct gpio_state state[CONFIG_SANDBOX_GPIO_COUNT];
> +
> +/* Access routines for GPIO state */
> +static u8 *get_gpio_flags(unsigned gp)
> +{
> + ? ? ? /* assert()'s could be disabled, so make sure we handle that */
> + ? ? ? assert(gp < ARRAY_SIZE(state));
> + ? ? ? if (gp >= ARRAY_SIZE(state)) {
> + ? ? ? ? ? ? ? static u8 invalid_flags;
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: invalid gpio %u\n", gp);
> + ? ? ? ? ? ? ? return &invalid_flags;
> + ? ? ? }
> +
> + ? ? ? return &state[gp].flags;
> +}
> +
> +static int get_gpio_flag(unsigned gp, int flag)
> +{
> + ? ? ? return (*get_gpio_flags(gp) & flag) != 0;
> +}
> +
> +static int set_gpio_flag(unsigned gp, int flag, int value)
> +{
> + ? ? ? u8 *gpio = get_gpio_flags(gp);
> +
> + ? ? ? if (value)
> + ? ? ? ? ? ? ? *gpio |= flag;
> + ? ? ? else
> + ? ? ? ? ? ? ? *gpio &= ~flag;
> +
> + ? ? ? return 0;
> +}
> +
> +static int check_reserved(unsigned gpio, const char *func)
> +{
> + ? ? ? if (!get_gpio_flag(gpio, GPIOF_RESERVED)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: %s: error: gpio %u not reserved\n",
> + ? ? ? ? ? ? ? ? ? ? ? func, gpio);
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
> +
> + ? ? ? return 0;
> +}
> +
> +/*
> + * Back-channel sandbox-internal-only access to GPIO state
> + */
> +
> +int sandbox_gpio_get_value(unsigned gp)
> +{
> + ? ? ? if (get_gpio_flag(gp, GPIOF_OUTPUT))
> + ? ? ? ? ? ? ? debug("sandbox_gpio: get_value on output gpio %u\n", gp);
> + ? ? ? return get_gpio_flag(gp, GPIOF_HIGH);
> +}
> +
> +int sandbox_gpio_set_value(unsigned gp, int value)
> +{
> + ? ? ? return set_gpio_flag(gp, GPIOF_HIGH, value);
> +}
> +
> +int sandbox_gpio_get_direction(unsigned gp)
> +{
> + ? ? ? return get_gpio_flag(gp, GPIOF_OUTPUT);
> +}
> +
> +int sandbox_gpio_set_direction(unsigned gp, int output)
> +{
> + ? ? ? return set_gpio_flag(gp, GPIOF_OUTPUT, output);
> +}
> +
> +/*
> + * These functions implement the public interface within U-Boot
> + */
> +
> +/* set GPIO port 'gp' as an input */
> +int gpio_direction_input(unsigned gp)
> +{
> + ? ? ? debug("%s: gp:%u\n", __func__, gp);
> +
> + ? ? ? if (check_reserved(gp, __func__))
> + ? ? ? ? ? ? ? return -1;
> +
> + ? ? ? return sandbox_gpio_set_direction(gp, 0);
> +}
> +
> +/* set GPIO port 'gp' as an output, with polarity 'value' */
> +int gpio_direction_output(unsigned gp, int value)
> +{
> + ? ? ? debug("%s: gp:%u, value = %d\n", __func__, gp, value);
> +
> + ? ? ? if (check_reserved(gp, __func__))
> + ? ? ? ? ? ? ? return -1;
> +
> + ? ? ? return sandbox_gpio_set_direction(gp, 1) |
> + ? ? ? ? ? ? ? sandbox_gpio_set_value(gp, value);
> +}
> +
> +/* read GPIO IN value of port 'gp' */
> +int gpio_get_value(unsigned gp)
> +{
> + ? ? ? debug("%s: gp:%u\n", __func__, gp);
> +
> + ? ? ? if (check_reserved(gp, __func__))
> + ? ? ? ? ? ? ? return -1;
> +
> + ? ? ? return sandbox_gpio_get_value(gp);
> +}
> +
> +/* write GPIO OUT value to port 'gp' */
> +int gpio_set_value(unsigned gp, int value)
> +{
> + ? ? ? debug("%s: gp:%u, value = %d\n", __func__, gp, value);
> +
> + ? ? ? if (check_reserved(gp, __func__))
> + ? ? ? ? ? ? ? return -1;
> +
> + ? ? ? if (!sandbox_gpio_get_direction(gp)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: set_value on input gpio %u\n", gp);
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
> +
> + ? ? ? return sandbox_gpio_set_value(gp, value);
> +}
> +
> +int gpio_request(unsigned gp, const char *label)
> +{
> + ? ? ? debug("%s: gp:%u, label:%s\n", __func__, gp, label);
> +
> + ? ? ? if (gp >= ARRAY_SIZE(state)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: invalid gpio %u\n", gp);
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
> +
> + ? ? ? if (get_gpio_flag(gp, GPIOF_RESERVED)) {
> + ? ? ? ? ? ? ? printf("sandbox_gpio: error: gpio %u already reserved\n", gp);
> + ? ? ? ? ? ? ? return -1;
> + ? ? ? }
> +
> + ? ? ? state[gp].label = label;
> + ? ? ? return set_gpio_flag(gp, GPIOF_RESERVED, 1);
> +}
> +
> +int gpio_free(unsigned gp)
> +{
> + ? ? ? debug("%s: gp:%u\n", __func__, gp);
> +
> + ? ? ? if (check_reserved(gp, __func__))
> + ? ? ? ? ? ? ? return -1;
> +
> + ? ? ? state[gp].label = NULL;
> + ? ? ? return set_gpio_flag(gp, GPIOF_RESERVED, 0);
> +}
> +
> +/* Display GPIO information */
> +void gpio_info(void)
> +{
> + ? ? ? unsigned gpio;
> +
> + ? ? ? puts("Sandbox GPIOs\n");
> +
> + ? ? ? for (gpio = 0; gpio < ARRAY_SIZE(state); ++gpio) {
> + ? ? ? ? ? ? ? const char *label = state[gpio].label;
> +
> + ? ? ? ? ? ? ? printf("%4d: %s: %d [%c] %s\n",
> + ? ? ? ? ? ? ? ? ? ? ? gpio,
> + ? ? ? ? ? ? ? ? ? ? ? sandbox_gpio_get_direction(gpio) ? "out" : " in",
> + ? ? ? ? ? ? ? ? ? ? ? sandbox_gpio_get_value(gpio),
> + ? ? ? ? ? ? ? ? ? ? ? get_gpio_flag(gpio, GPIOF_RESERVED) ? 'x' : ' ',
> + ? ? ? ? ? ? ? ? ? ? ? label ? label : "");
> + ? ? ? }
> +}
> --
> 1.7.8.4

[U-Boot] [PATCH v5] sandbox: gpio: Add basic driver for simulating GPIOs

[Mike Frysinger]

On Wednesday 22 February 2012 00:45:52 Simon Glass wrote:
> On Tue, Feb 21, 2012 at 9:08 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> > let's get your ack-back on this and i'll merge it into my branch.
> > only diff from last code is restored assert() to get_gpio_flags().
> > -mike
> > 
> > From f67fd9b82ec180d8a227bf036a45eb31e713a5a9 Mon Sep 17 00:00:00 2001
> > From: Simon Glass <sjg@chromium.org>
> > Date: Wed, 15 Feb 2012 15:51:13 -0800
> > Subject: [PATCH v5] sandbox: gpio: Add basic driver for simulating GPIOs
> > 
> > This provides a way of simulating GPIOs by setting values which are seen
> > by the normal gpio_get/set_value() calls.
> > 
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> Acked-by: Simon Glass <sjg@chromium.org>

ok, on to command line processing ;)
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120222/795289f0/attachment.pgp>

[U-Boot] [PATCH v4 4/8] sandbox: Enable GPIO driver

[Simon Glass]

Enable the new GPIO driver for sandbox.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Reduce number of GPIOs from 224 to 20

 include/configs/sandbox.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
index 6790216..a58a34e 100644
--- a/include/configs/sandbox.h
+++ b/include/configs/sandbox.h
@@ -34,6 +34,10 @@
 
 #define CONFIG_SYS_VSNPRINTF
 
+#define CONFIG_CMD_GPIO
+#define CONFIG_SANDBOX_GPIO
+#define CONFIG_SANDBOX_GPIO_COUNT	20
+
 /*
  * Size of malloc() pool, although we don't actually use this yet.
  */
-- 
1.7.7.3

[U-Boot] [PATCH v4 5/8] sandbox: Add concept of sandbox state

[Simon Glass]

The state exists through the life of U-Boot. It can be adjusted by command
line options and perhaps later through a config file. It is available
to U-Boot through state_...() calls (within sandbox code).

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v4:
- Add multiple #include protection to state.h
- Make the state variable static
- Remove unused state_is_processor_reset() function

 arch/sandbox/cpu/Makefile                     |    2 +-
 arch/sandbox/cpu/start.c                      |   10 ++++
 arch/sandbox/cpu/state.c                      |   52 ++++++++++++++++++++++
 arch/sandbox/include/asm/arch-sandbox/state.h |   57 +++++++++++++++++++++++++
 4 files changed, 120 insertions(+), 1 deletions(-)
 create mode 100644 arch/sandbox/cpu/state.c
 create mode 100644 arch/sandbox/include/asm/arch-sandbox/state.h

diff --git a/arch/sandbox/cpu/Makefile b/arch/sandbox/cpu/Makefile
index 2ae0f71..6fd09ff 100644
--- a/arch/sandbox/cpu/Makefile
+++ b/arch/sandbox/cpu/Makefile
@@ -27,7 +27,7 @@ include $(TOPDIR)/config.mk
 
 LIB	= $(obj)lib$(CPU).o
 
-COBJS	:= cpu.o start.o os.o
+COBJS	:= cpu.o os.o start.o state.o
 
 SRCS	:= $(COBJS:.o=.c)
 OBJS	:= $(addprefix $(obj),$(COBJS))
diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index a429e29..b3442e8 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -20,9 +20,19 @@
  */
 
 #include <common.h>
+#include <asm/arch/state.h>
 
 int main(int argc, char *argv[])
 {
+	struct sandbox_state *state = NULL;
+	int err;
+
+	err = state_init();
+	if (!err) {
+		state = state_get_current();
+		assert(state);
+	}
+
 	/*
 	 * Do pre- and post-relocation init, then start up U-Boot. This will
 	 * never return.
diff --git a/arch/sandbox/cpu/state.c b/arch/sandbox/cpu/state.c
new file mode 100644
index 0000000..20cd689
--- /dev/null
+++ b/arch/sandbox/cpu/state.c
@@ -0,0 +1,52 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * 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 <common.h>
+#include <asm/arch/gpio.h>
+#include <asm/arch/state.h>
+
+/* Main state record for the sandbox */
+static struct sandbox_state main_state;
+static struct sandbox_state *state;	/* Pointer to current state record */
+
+void state_record_exit(enum exit_type_id exit_type)
+{
+	state->exit_type = exit_type;
+}
+
+struct sandbox_state *state_get_current(void)
+{
+	assert(state);
+	return state;
+}
+
+int state_init(void)
+{
+	state = &main_state;
+
+	/*
+	 * Example of how to use GPIOs:
+	 *
+	 * sandbox_gpio_set_direction(170, 0);
+	 * sandbox_gpio_set_value(170, 0);
+	 */
+	return 0;
+}
diff --git a/arch/sandbox/include/asm/arch-sandbox/state.h b/arch/sandbox/include/asm/arch-sandbox/state.h
new file mode 100644
index 0000000..ba000cd
--- /dev/null
+++ b/arch/sandbox/include/asm/arch-sandbox/state.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * 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
+ */
+
+#ifndef __SANDBOX_STATE_H
+#define __SANDBOX_STATE_H
+
+/* How we exited U-Boot */
+enum exit_type_id {
+	STATE_EXIT_NORMAL,
+	STATE_EXIT_COLD_REBOOT,
+	STATE_EXIT_POWER_OFF,
+};
+
+/* The complete state of the test system */
+struct sandbox_state {
+	const char *cmd;		/* Command to execute */
+	enum exit_type_id exit_type;	/* How we exited U-Boot */
+};
+
+/**
+ * Record the exit type to be reported by the test program.
+ *
+ * @param exit_type	Exit type to record
+ */
+void state_record_exit(enum exit_type_id exit_type);
+
+/**
+ * Gets a pointer to the current state.
+ *
+ * @return pointer to state
+ */
+struct sandbox_state *state_get_current(void);
+
+/**
+ * Initialize the test system state
+ */
+int state_init(void);
+
+#endif
-- 
1.7.7.3

[U-Boot] [PATCH v4 6/8] sandbox: Allow processing instead of or before main loop

[Simon Glass]

In order to pass command line arguments to sandbox we need to be able
to act on them. So take control back at the end of board_init_r() from
where we can call the main loop or do something else.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Call cpu_main_loop() from board_init_r()

 arch/sandbox/cpu/start.c                  |    8 ++++++++
 arch/sandbox/include/asm/u-boot-sandbox.h |    3 +++
 arch/sandbox/lib/board.c                  |    7 ++++---
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index b3442e8..d7402be 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -22,6 +22,14 @@
 #include <common.h>
 #include <asm/arch/state.h>
 
+#include <os.h>
+
+void start_main_loop(void)
+{
+	for (;;)
+		main_loop();
+}
+
 int main(int argc, char *argv[])
 {
 	struct sandbox_state *state = NULL;
diff --git a/arch/sandbox/include/asm/u-boot-sandbox.h b/arch/sandbox/include/asm/u-boot-sandbox.h
index 236b4ee..3743289 100644
--- a/arch/sandbox/include/asm/u-boot-sandbox.h
+++ b/arch/sandbox/include/asm/u-boot-sandbox.h
@@ -35,4 +35,7 @@
 int board_init(void);
 int dram_init(void);
 
+/* start.c */
+void start_main_loop(void);
+
 #endif	/* _U_BOOT_SANDBOX_H_ */
diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c
index 6d464d6..331ce16 100644
--- a/arch/sandbox/lib/board.c
+++ b/arch/sandbox/lib/board.c
@@ -270,11 +270,12 @@ void board_init_r(gd_t *id, ulong dest_addr)
 #endif
 
 	/*
-	 * For now, run the main loop. Later we might let this be done
-	 * in the main program.
+	 * This function can't return (to match other archs) so call back
+	 * into start.c so that sandbox can do something other than the main
+	 * loop if it likes
 	 */
 	while (1)
-		main_loop();
+		start_main_loop();
 
 	/* NOTREACHED - no way out of command loop except booting */
 }
-- 
1.7.7.3

[U-Boot] [PATCH v4 7/8] sandbox: Add flags for open() call

[Simon Glass]

This provides a way for callers to create files for writing. We define
flags which mirror the POSIX values.

Another approach would be to translate the flags at runtime. Perhaps we can
leave to whoever wants to port this to another OS?

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Change open() flags enum to #define

Changes in v4:
- Change space to tab in os.h
- Remove unneeded declaration of struct sandbox_state in os.h

 arch/sandbox/cpu/os.c |    2 +-
 include/os.h          |   16 ++++++++++------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 093e7dc..9f93f83 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -47,7 +47,7 @@ ssize_t os_write(int fd, const void *buf, size_t count)
 
 int os_open(const char *pathname, int flags)
 {
-	return open(pathname, flags);
+	return open(pathname, flags, 0777);
 }
 
 int os_close(int fd)
diff --git a/include/os.h b/include/os.h
index f3af4f0..136b6c4 100644
--- a/include/os.h
+++ b/include/os.h
@@ -1,4 +1,9 @@
 /*
+ * Operating System Interface
+ *
+ * This provides access to useful OS routines for the sandbox architecture.
+ * They are kept in a separate file so we can include system headers.
+ *
  * Copyright (c) 2011 The Chromium OS Authors.
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -19,12 +24,6 @@
  * MA 02111-1307 USA
  */
 
-/*
- * Operating System Interface
- *
- * This provides access to useful OS routines from the sandbox architecture
- */
-
 /**
  * Access to the OS read() system call
  *
@@ -45,6 +44,11 @@ ssize_t os_read(int fd, void *buf, size_t count);
  */
 ssize_t os_write(int fd, const void *buf, size_t count);
 
+#define OS_O_RDONLY	0
+#define OS_O_WRONLY	1
+#define OS_O_RDWR	2
+#define OS_O_CREAT	0100
+
 /**
  * Access to the OS open() system call
  *
-- 
1.7.7.3

[U-Boot] [PATCH v4 7/8] sandbox: Add flags for open() call

[Mike Frysinger]

On Wednesday 15 February 2012 18:51:17 Simon Glass wrote:
> This provides a way for callers to create files for writing. We define
> flags which mirror the POSIX values.
> 
> Another approach would be to translate the flags at runtime. Perhaps we can
> leave to whoever wants to port this to another OS?

as i mentioned, this isn't a linux-vs-non-linux issue.  even linux ports 
themselves disagree on the open() flags.

> +#define OS_O_RDONLY	0
> +#define OS_O_WRONLY	1
> +#define OS_O_RDWR	2

these are "fine" for linux as every port i see uses these values

> +#define OS_O_CREAT	0100

as soon as you get beyond the extreme basics, you start seeing bitfield drift.  
not all linux arches define O_CREAT to 0100.  so i'd like to see logic similar 
to what i did for lseek().  i.e. make you bite the bullet :P.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120216/a2a481bd/attachment.pgp>

[U-Boot] [PATCH v4 7/8] sandbox: Add flags for open() call

[Simon Glass]

Hi Mike,

On Wed, Feb 15, 2012 at 10:09 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 15 February 2012 18:51:17 Simon Glass wrote:
>> This provides a way for callers to create files for writing. We define
>> flags which mirror the POSIX values.
>>
>> Another approach would be to translate the flags at runtime. Perhaps we can
>> leave to whoever wants to port this to another OS?
>
> as i mentioned, this isn't a linux-vs-non-linux issue. ?even linux ports
> themselves disagree on the open() flags.

Gosh.

>
>> +#define OS_O_RDONLY ?0
>> +#define OS_O_WRONLY ?1
>> +#define OS_O_RDWR ? ?2
>
> these are "fine" for linux as every port i see uses these values
>
>> +#define OS_O_CREAT ? 0100
>
> as soon as you get beyond the extreme basics, you start seeing bitfield drift.
> not all linux arches define O_CREAT to 0100. ?so i'd like to see logic similar
> to what i did for lseek(). ?i.e. make you bite the bullet :P.

Chomp. I will send a new patch. Sandbox does at least have few
time/code size constraints.

> -mike

Regards,
Simon

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Simon Glass]

This adds simple command-line parsing to sandbox. The idea is that it
sets up the state with options provided, and this state can then be
queried later, as needed.

For now we just allow it to run a command.

Passing a command to U-Boot on stdin is not as convenient IMO.

The parsing code is in os.c since it gives us easy access to getopt() and
friends. We could create a separate parse.c file if this grows.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Clean up, added a few fdt and config patches
- Rebase on master

Changes in v3:
- Only output usage on stderr if we get an error
- Tidy code style around getopt()

Changes in v4:
- Store command-line arguments in the state structure

 arch/sandbox/cpu/os.c                         |   45 +++++++++++++++++++++++++
 arch/sandbox/cpu/start.c                      |   23 +++++++++++-
 arch/sandbox/include/asm/arch-sandbox/state.h |    3 ++
 include/os.h                                  |   22 ++++++++++++
 4 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 9f93f83..b37d13f 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -21,6 +21,8 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <getopt.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <termios.h>
 #include <time.h>
@@ -30,6 +32,7 @@
 #include <sys/time.h>
 #include <sys/types.h>
 #include <linux/types.h>
+#include <asm/arch/state.h>
 
 #include <os.h>
 
@@ -122,3 +125,45 @@ u64 os_get_nsec(void)
 	return tv.tv_sec * 1000000000ULL + tv.tv_usec * 1000;
 #endif
 }
+
+static const char options[] = "c:h";
+
+static const struct option long_options[] = {
+	{"command", required_argument, NULL, 'c'},
+	{"help", no_argument, NULL, 'h'},
+	{NULL, 0, NULL, 0}
+};
+
+void os_usage(int err)
+{
+	if (err < 0)
+		fprintf(stderr, "Try `--help' for more information.\n");
+	fprintf(err < 0 ? stderr : stdout, "u-boot, "
+		"a command line test interface to U-Boot\n\n"
+		"usage:\tu-boot [-ch]\n"
+		"Options:\n"
+		"\t-h\tDisplay help\n"
+		"\t-c <command>\tExecute U-Boot command\n");
+	exit(1);
+}
+
+int os_parse_args(struct sandbox_state *state, int argc, char *argv[])
+{
+	int c;
+
+	state->argc = argc;
+	state->argv = argv;
+	while ((c = getopt_long(argc, argv, options, long_options,
+				NULL)) != EOF) {
+		switch (c) {
+		case 'c':
+			state->cmd = optarg;
+			break;
+		case 'h':
+			return 1;
+		default: /* '?' */
+			return -1;
+		}
+	}
+	return 0;
+}
diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index d7402be..fa7a907 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -26,8 +26,26 @@
 
 void start_main_loop(void)
 {
-	for (;;)
-		main_loop();
+	struct sandbox_state *state = state_get_current();
+
+	if (state->err)
+		os_usage(state->err);	/* does not return */
+
+	/* Execute command if required */
+	if (state->cmd) {
+		/* TODO: redo this when cmd tidy-up series lands */
+#ifdef CONFIG_SYS_HUSH_PARSER
+		run_command(state->cmd, 0);
+#else
+		parse_string_outer(state->cmd, FLAG_PARSE_SEMICOLON |
+				    FLAG_EXIT_FROM_LOOP);
+#endif
+	} else {
+		for (;;)
+			main_loop();
+	}
+	printf("U-Boot exited with state %d\n", state->exit_type);
+	os_exit(state->exit_type);
 }
 
 int main(int argc, char *argv[])
@@ -39,6 +57,7 @@ int main(int argc, char *argv[])
 	if (!err) {
 		state = state_get_current();
 		assert(state);
+		state->err = os_parse_args(state, argc, argv);
 	}
 
 	/*
diff --git a/arch/sandbox/include/asm/arch-sandbox/state.h b/arch/sandbox/include/asm/arch-sandbox/state.h
index ba000cd..783ea6e 100644
--- a/arch/sandbox/include/asm/arch-sandbox/state.h
+++ b/arch/sandbox/include/asm/arch-sandbox/state.h
@@ -33,6 +33,9 @@ enum exit_type_id {
 struct sandbox_state {
 	const char *cmd;		/* Command to execute */
 	enum exit_type_id exit_type;	/* How we exited U-Boot */
+	int err;			/* Error to report from parsing */
+	int argc;			/* Program arguments */
+	char **argv;
 };
 
 /**
diff --git a/include/os.h b/include/os.h
index 136b6c4..df07d19 100644
--- a/include/os.h
+++ b/include/os.h
@@ -24,6 +24,8 @@
  * MA 02111-1307 USA
  */
 
+struct sandbox_state;
+
 /**
  * Access to the OS read() system call
  *
@@ -102,3 +104,23 @@ void os_usleep(unsigned long usec);
  * \return A monotonic increasing time scaled in nano seconds
  */
 u64 os_get_nsec(void);
+
+/**
+ * Parse arguments and update sandbox state.
+ *
+ * @param state		Sandbox state to update
+ * @param argc		Argument count
+ * @param argv		Argument vector
+ * @return 0 if ok, and program should continue;
+ *	1 if ok, but program should stop;
+ *	-1 on error: program should terminate.
+ */
+int os_parse_args(struct sandbox_state *state, int argc, char *argv[]);
+
+/**
+ * Display a usage message on stderr and exit
+ *
+ * @param err		Error code (-ve means the user entered an invalid
+ *			option)
+ */
+void os_usage(int err);
-- 
1.7.7.3

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Mike Frysinger]

On Wednesday 15 February 2012 18:51:18 Simon Glass wrote:
> --- a/arch/sandbox/cpu/os.c
> +++ b/arch/sandbox/cpu/os.c
> 
> +#include <stdio.h>
> 
> +void os_usage(int err)
> +{
> +	if (err < 0)
> +		fprintf(stderr, "Try `--help' for more information.\n");
> +	fprintf(err < 0 ? stderr : stdout, "u-boot, "
> +		"a command line test interface to U-Boot\n\n"
> +		"usage:\tu-boot [-ch]\n"
> +		"Options:\n"
> +		"\t-h\tDisplay help\n"
> +		"\t-c <command>\tExecute U-Boot command\n");

this actually doesn't work.  we're using the stdio from u-boot itself, so we 
can't use stdio.h from glibc.  if it works for you, i'm pretty sure it's 
purely an accident (perhaps related to the fortification change i just posted).
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120226/ed8476c4/attachment.pgp>

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Simon Glass]

Hi Mike,

On Sun, Feb 26, 2012 at 1:04 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Wednesday 15 February 2012 18:51:18 Simon Glass wrote:
>> --- a/arch/sandbox/cpu/os.c
>> +++ b/arch/sandbox/cpu/os.c
>>
>> +#include <stdio.h>
>>
>> +void os_usage(int err)
>> +{
>> + ? ? if (err < 0)
>> + ? ? ? ? ? ? fprintf(stderr, "Try `--help' for more information.\n");
>> + ? ? fprintf(err < 0 ? stderr : stdout, "u-boot, "
>> + ? ? ? ? ? ? "a command line test interface to U-Boot\n\n"
>> + ? ? ? ? ? ? "usage:\tu-boot [-ch]\n"
>> + ? ? ? ? ? ? "Options:\n"
>> + ? ? ? ? ? ? "\t-h\tDisplay help\n"
>> + ? ? ? ? ? ? "\t-c <command>\tExecute U-Boot command\n");
>
> this actually doesn't work. ?we're using the stdio from u-boot itself, so we
> can't use stdio.h from glibc. ?if it works for you, i'm pretty sure it's
> purely an accident (perhaps related to the fortification change i just posted).

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Mike Frysinger]

On Sunday 26 February 2012 21:50:32 Simon Glass wrote:
> Given your efforts on the cmdline parsing I'm beginning to think we
> should perhaps add os_printf() and os_printf_stderr() and provide an
> explicit interface. It might only be useful prior to main(), then
> again I'm not so sure.

i've been pondering this.  on one hand, we want to parse flags as soon as 
possible, but on the other, we want to be able to not have to worry about what 
state the system is in when parsing things.  maybe we specially mark the few 
flags that we need very early on and parse those, and then parse the rest once 
the system is mostly functional ?  i can really only think of one or two flags 
that we *might* need very early -- namely, ones that'd select a config or fdt.  
on the other hand, are there things that we'd want to change that'd affect 
everything from console_init_f() and earlier ?

wrt os_printf/etc..., the only way we could do that is if we dlopen-ed glibc 
and called the symbol directly.  u-boot provides printf() so we can't have an 
os_printf() in os.c call printf() in glibc simply because we included stdio.h 
from glibc.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120226/08fca64c/attachment.pgp>

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Simon Glass]

Hi Mike,

On Sun, Feb 26, 2012 at 8:08 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sunday 26 February 2012 21:50:32 Simon Glass wrote:
>> Given your efforts on the cmdline parsing I'm beginning to think we
>> should perhaps add os_printf() and os_printf_stderr() and provide an
>> explicit interface. It might only be useful prior to main(), then
>> again I'm not so sure.
>
> i've been pondering this. ?on one hand, we want to parse flags as soon as
> possible, but on the other, we want to be able to not have to worry about what
> state the system is in when parsing things. ?maybe we specially mark the few
> flags that we need very early on and parse those, and then parse the rest once
> the system is mostly functional ? ?i can really only think of one or two flags
> that we *might* need very early -- namely, ones that'd select a config or fdt.
> on the other hand, are there things that we'd want to change that'd affect
> everything from console_init_f() and earlier ?

IMO flags should purely update the initial state and therefore we
don't need the system to be function to parse them. Once the system is
functional, the various bits that are interested can go and look at
the state to get the info they want. IOW I don't think we need to
delay parsing until the system is functional - we just need to take
'action' then.

Anyway once we have a lot of test subsystems we are going to want to
have options in a config file.

>
> wrt os_printf/etc..., the only way we could do that is if we dlopen-ed glibc
> and called the symbol directly. ?u-boot provides printf() so we can't have an
> os_printf() in os.c call printf() in glibc simply because we included stdio.h
> from glibc.

Yes that reminds me why I didn't do that last time. I was wondering
about using a link flag to rename the library version of the call but
it seemed a bit ugly.

Regards,
Simon

> -mike

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Mike Frysinger]

On Sunday 26 February 2012 23:33:25 Simon Glass wrote:
> On Sun, Feb 26, 2012 at 8:08 PM, Mike Frysinger wrote:
> > On Sunday 26 February 2012 21:50:32 Simon Glass wrote:
> >> Given your efforts on the cmdline parsing I'm beginning to think we
> >> should perhaps add os_printf() and os_printf_stderr() and provide an
> >> explicit interface. It might only be useful prior to main(), then
> >> again I'm not so sure.
> > 
> > i've been pondering this.  on one hand, we want to parse flags as soon as
> > possible, but on the other, we want to be able to not have to worry about
> > what state the system is in when parsing things.  maybe we specially
> > mark the few flags that we need very early on and parse those, and then
> > parse the rest once the system is mostly functional ?  i can really only
> > think of one or two flags that we *might* need very early -- namely,
> > ones that'd select a config or fdt. on the other hand, are there things
> > that we'd want to change that'd affect everything from console_init_f()
> > and earlier ?
> 
> IMO flags should purely update the initial state and therefore we
> don't need the system to be function to parse them. Once the system is
> functional, the various bits that are interested can go and look at
> the state to get the info they want. IOW I don't think we need to
> delay parsing until the system is functional - we just need to take
> 'action' then.

ok, so for example, i updated my spi flash patch to do:

drivers/mtd/spi/sandbox.c:
static int sb_cmdline_cb_spi_sf(struct sandbox_state *state, const char *arg)
{   
    unsigned long bus, cs;
    const char *spec = sb_spi_parse_spec(arg, &bus, &cs);

    if (!spec)
        return 1;

    state->spi[bus][cs][0] = &sb_sf_ops;
    state->spi[bus][cs][1] = spec;
    return 0;
}
SB_CMDLINE_OPT(spi_sf, 1, "connect a SPI flash: <bus>:<cs>:<id>:<file>");

and people could do:
	./u-boot --spi_sf 0:3:m25p16:./some-file.bin
this would connect the spi flash simulation up to the simulated spi bus 0 on cs 
3 and have it simulate a m25p16 flash with "some-file.bin" backing it.

if someone were to enter the wrong value for "0:3:m25p16:./some-file.bin", how 
do you propose we communicate that ?  the existing code can easily signal the 
higher parsing logic via return values, but then we'd just get:
	failed to parse option: 0:3:m25p16:./some-file.bin

but what exactly did the code not like ?  was it the bus # ?  the cs # ?  the 
spi flash id ?  the backing file ?  if the getopt code has access to printf(), 
we can clearly communicate to the user what is going wrong.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120226/7b0cc7d2/attachment.pgp>

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Simon Glass]

Hi Mike,

On Sun, Feb 26, 2012 at 8:42 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Sunday 26 February 2012 23:33:25 Simon Glass wrote:
>> On Sun, Feb 26, 2012 at 8:08 PM, Mike Frysinger wrote:
>> > On Sunday 26 February 2012 21:50:32 Simon Glass wrote:
>> >> Given your efforts on the cmdline parsing I'm beginning to think we
>> >> should perhaps add os_printf() and os_printf_stderr() and provide an
>> >> explicit interface. It might only be useful prior to main(), then
>> >> again I'm not so sure.
>> >
>> > i've been pondering this. ?on one hand, we want to parse flags as soon as
>> > possible, but on the other, we want to be able to not have to worry about
>> > what state the system is in when parsing things. ?maybe we specially
>> > mark the few flags that we need very early on and parse those, and then
>> > parse the rest once the system is mostly functional ? ?i can really only
>> > think of one or two flags that we *might* need very early -- namely,
>> > ones that'd select a config or fdt. on the other hand, are there things
>> > that we'd want to change that'd affect everything from console_init_f()
>> > and earlier ?
>>
>> IMO flags should purely update the initial state and therefore we
>> don't need the system to be function to parse them. Once the system is
>> functional, the various bits that are interested can go and look at
>> the state to get the info they want. IOW I don't think we need to
>> delay parsing until the system is functional - we just need to take
>> 'action' then.
>
> ok, so for example, i updated my spi flash patch to do:
>
> drivers/mtd/spi/sandbox.c:
> static int sb_cmdline_cb_spi_sf(struct sandbox_state *state, const char *arg)
> {
> ? ?unsigned long bus, cs;
> ? ?const char *spec = sb_spi_parse_spec(arg, &bus, &cs);
>
> ? ?if (!spec)
> ? ? ? ?return 1;
>
> ? ?state->spi[bus][cs][0] = &sb_sf_ops;
> ? ?state->spi[bus][cs][1] = spec;
> ? ?return 0;
> }
> SB_CMDLINE_OPT(spi_sf, 1, "connect a SPI flash: <bus>:<cs>:<id>:<file>");

sandbox...give me your address and I'll send you a cheque to cover the
bytes so used :-)

>
> and people could do:
> ? ? ? ?./u-boot --spi_sf 0:3:m25p16:./some-file.bin
> this would connect the spi flash simulation up to the simulated spi bus 0 on cs
> 3 and have it simulate a m25p16 flash with "some-file.bin" backing it.
>
> if someone were to enter the wrong value for "0:3:m25p16:./some-file.bin", how
> do you propose we communicate that ? ?the existing code can easily signal the
> higher parsing logic via return values, but then we'd just get:
> ? ? ? ?failed to parse option: 0:3:m25p16:./some-file.bin
>
> but what exactly did the code not like ? ?was it the bus # ? ?the cs # ? ?the
> spi flash id ? ?the backing file ? ?if the getopt code has access to printf(),
> we can clearly communicate to the user what is going wrong.

Yes I think printf() is useful in getopt, I just would prefer to avoid
using U-Boot's printf. It goes through all the console code, and we
might be running a test that deliberately breaks that, perhaps.
Actually this could be a pretty important thing to sort out - we need
to keep a clear boundary between test code and U-Boot code (as we
discussed over GPIO). Having the test code use U-Boot's printf() is
blurring that.

To your example, if the syntax is correct perhaps, then it got through
initial parsing. If the filename is wrong, or the bus number, then
perhaps there will be a message and failure much later. Perhaps the
initial parsing just does what it can to avoid running past an obvious
syntax error. But we can't really know success until we open the file,
or try the bus.

Regards,
Simon

> -mike

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Mike Frysinger]

On Monday 27 February 2012 00:43:30 Simon Glass wrote:
> On Sun, Feb 26, 2012 at 8:42 PM, Mike Frysinger wrote:
> > drivers/mtd/spi/sandbox.c:
> > static int sb_cmdline_cb_spi_sf(struct sandbox_state *state, const char
> > *arg) {
> >    unsigned long bus, cs;
> >    const char *spec = sb_spi_parse_spec(arg, &bus, &cs);
> > 
> >    if (!spec)
> >        return 1;
> > 
> >    state->spi[bus][cs][0] = &sb_sf_ops;
> >    state->spi[bus][cs][1] = spec;
> >    return 0;
> > }
> > SB_CMDLINE_OPT(spi_sf, 1, "connect a SPI flash: <bus>:<cs>:<id>:<file>");
> 
> sandbox...give me your address and I'll send you a cheque to cover the
> bytes so used :-)

it'll have to be a big one to stop the cascading line wrapping :P

> > and people could do:
> >        ./u-boot --spi_sf 0:3:m25p16:./some-file.bin
> > this would connect the spi flash simulation up to the simulated spi bus 0
> > on cs 3 and have it simulate a m25p16 flash with "some-file.bin" backing
> > it.
> > 
> > if someone were to enter the wrong value for
> > "0:3:m25p16:./some-file.bin", how do you propose we communicate that ?
> >  the existing code can easily signal the higher parsing logic via return
> > values, but then we'd just get:
> >        failed to parse option: 0:3:m25p16:./some-file.bin
> > 
> > but what exactly did the code not like ?  was it the bus # ?  the cs # ?
> >  the spi flash id ?  the backing file ?  if the getopt code has access
> > to printf(), we can clearly communicate to the user what is going wrong.
> 
> Yes I think printf() is useful in getopt, I just would prefer to avoid
> using U-Boot's printf. It goes through all the console code, and we
> might be running a test that deliberately breaks that, perhaps.
> Actually this could be a pretty important thing to sort out - we need
> to keep a clear boundary between test code and U-Boot code (as we
> discussed over GPIO). Having the test code use U-Boot's printf() is
> blurring that.

we probably need to add architecture details like this to doc/README.sandbox 
so we can stay on the same page ...

> To your example, if the syntax is correct perhaps, then it got through
> initial parsing. If the filename is wrong, or the bus number, then
> perhaps there will be a message and failure much later. Perhaps the
> initial parsing just does what it can to avoid running past an obvious
> syntax error. But we can't really know success until we open the file,
> or try the bus.

if the bus/cs are valid, the rest of the parsing is delayed until you do "sf 
probe" at which point you'd get an explanation of what is wrong.  i guess we 
can live with this for now.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120227/d9ad9395/attachment.pgp>

[U-Boot] [PATCH v4 8/8] sandbox: Add basic command line parsing

[Simon Glass]

Hi Mike,

On Mon, Feb 27, 2012 at 10:32 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Monday 27 February 2012 00:43:30 Simon Glass wrote:
>> On Sun, Feb 26, 2012 at 8:42 PM, Mike Frysinger wrote:
>> > drivers/mtd/spi/sandbox.c:
>> > static int sb_cmdline_cb_spi_sf(struct sandbox_state *state, const char
>> > *arg) {
>> > ? ?unsigned long bus, cs;
>> > ? ?const char *spec = sb_spi_parse_spec(arg, &bus, &cs);
>> >
>> > ? ?if (!spec)
>> > ? ? ? ?return 1;
>> >
>> > ? ?state->spi[bus][cs][0] = &sb_sf_ops;
>> > ? ?state->spi[bus][cs][1] = spec;
>> > ? ?return 0;
>> > }
>> > SB_CMDLINE_OPT(spi_sf, 1, "connect a SPI flash: <bus>:<cs>:<id>:<file>");
>>
>> sandbox...give me your address and I'll send you a cheque to cover the
>> bytes so used :-)
>
> it'll have to be a big one to stop the cascading line wrapping :P
>
>> > and people could do:
>> > ? ? ? ?./u-boot --spi_sf 0:3:m25p16:./some-file.bin
>> > this would connect the spi flash simulation up to the simulated spi bus 0
>> > on cs 3 and have it simulate a m25p16 flash with "some-file.bin" backing
>> > it.
>> >
>> > if someone were to enter the wrong value for
>> > "0:3:m25p16:./some-file.bin", how do you propose we communicate that ?
>> > ?the existing code can easily signal the higher parsing logic via return
>> > values, but then we'd just get:
>> > ? ? ? ?failed to parse option: 0:3:m25p16:./some-file.bin
>> >
>> > but what exactly did the code not like ? ?was it the bus # ? ?the cs # ?
>> > ?the spi flash id ? ?the backing file ? ?if the getopt code has access
>> > to printf(), we can clearly communicate to the user what is going wrong.
>>
>> Yes I think printf() is useful in getopt, I just would prefer to avoid
>> using U-Boot's printf. It goes through all the console code, and we
>> might be running a test that deliberately breaks that, perhaps.
>> Actually this could be a pretty important thing to sort out - we need
>> to keep a clear boundary between test code and U-Boot code (as we
>> discussed over GPIO). Having the test code use U-Boot's printf() is
>> blurring that.
>
> we probably need to add architecture details like this to doc/README.sandbox
> so we can stay on the same page ...

Yes I agree.

>
>> To your example, if the syntax is correct perhaps, then it got through
>> initial parsing. If the filename is wrong, or the bus number, then
>> perhaps there will be a message and failure much later. Perhaps the
>> initial parsing just does what it can to avoid running past an obvious
>> syntax error. But we can't really know success until we open the file,
>> or try the bus.
>
> if the bus/cs are valid, the rest of the parsing is delayed until you do "sf
> probe" at which point you'd get an explanation of what is wrong. ?i guess we
> can live with this for now.

Yes, I feel like we have done more than enough plumbing in advance of
use. Let's see how it goes.

Regards,
Simon

> -mike

[U-Boot] [PATCH v4 1/8] sandbox: fdt: Add support for CONFIG_OF_CONTROL

[Mike Frysinger]

On Wednesday 15 February 2012 18:51:11 Simon Glass wrote:
> This adds support for a controlling fdt, mirroring the ARM implementation.

merged into my sandbox branch
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120216/abf6d89e/attachment.pgp>

[U-Boot] [PATCH v4 1/8] sandbox: fdt: Add support for CONFIG_OF_CONTROL

[Marek Vasut]

> This adds support for a controlling fdt, mirroring the ARM implementation.

This is offtopic, but CONFIG OF CONTROL sounds interesting ;-)

btw. aren't you missing this part from ARM implementation?

/* Allow the early environment to override the fdt address */
gd->fdt_blob = (void *)getenv_ulong("fdtcontroladdr", 16, (uintptr_t)gd-
>fdt_blob);

> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v3:
> - Use #if defined()..#elif defined, instead of #ifdef..#elif defined
> 
>  arch/sandbox/include/asm/global_data.h |    1 +
>  arch/sandbox/lib/board.c               |    8 ++++++++
>  2 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/sandbox/include/asm/global_data.h
> b/arch/sandbox/include/asm/global_data.h index 8d47191..01a7063 100644
> --- a/arch/sandbox/include/asm/global_data.h
> +++ b/arch/sandbox/include/asm/global_data.h
> @@ -45,6 +45,7 @@ typedef	struct global_data {
>  	unsigned long	fb_base;	/* base address of frame buffer */
>  	u8		*ram_buf;	/* emulated RAM buffer */
>  	phys_size_t	ram_size;	/* RAM size */
> +	const void	*fdt_blob;	/* Our device tree, NULL if none */
>  	void		**jt;		/* jump table */
>  	char		env_buf[32];	/* buffer for getenv() before reloc. */
>  } gd_t;
> diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c
> index b7997e9..6d464d6 100644
> --- a/arch/sandbox/lib/board.c
> +++ b/arch/sandbox/lib/board.c
> @@ -156,6 +156,14 @@ void board_init_f(ulong bootflag)
> 
>  	memset((void *)gd, 0, sizeof(gd_t));
> 
> +#if defined(CONFIG_OF_EMBED)
> +	/* Get a pointer to the FDT */
> +	gd->fdt_blob = _binary_dt_dtb_start;
> +#elif defined(CONFIG_OF_SEPARATE)
> +	/* FDT is at end of image */
> +	gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
> +#endif
> +
>  	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
>  		if ((*init_fnc_ptr)() != 0)
>  			hang();

[U-Boot] [PATCH v4 1/8] sandbox: fdt: Add support for CONFIG_OF_CONTROL

[Simon Glass]

Hi Marek,

On Feb 16, 2012 2:50 AM, "Marek Vasut" <marek.vasut@gmail.com> wrote:
>
> > This adds support for a controlling fdt, mirroring the ARM
implementation.
>
> This is offtopic, but CONFIG OF CONTROL sounds interesting ;-)

Yes I hope it will be before end of year.

>
> btw. aren't you missing this part from ARM implementation?
>
> /* Allow the early environment to override the fdt address */
> gd->fdt_blob = (void *)getenv_ulong("fdtcontroladdr", 16, (uintptr_t)gd-
> >fdt_blob);

In principle yes. In practice we don't have a simple memory interface for
sandbox yet, so we can't implement this.

Well we did have one but it broke a few boards so was reverted. We should
come up with a way to do this...

Regards,
Simon

>
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> > Changes in v3:
> > - Use #if defined()..#elif defined, instead of #ifdef..#elif defined
> >
> >  arch/sandbox/include/asm/global_data.h |    1 +
> >  arch/sandbox/lib/board.c               |    8 ++++++++
> >  2 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/sandbox/include/asm/global_data.h
> > b/arch/sandbox/include/asm/global_data.h index 8d47191..01a7063 100644
> > --- a/arch/sandbox/include/asm/global_data.h
> > +++ b/arch/sandbox/include/asm/global_data.h
> > @@ -45,6 +45,7 @@ typedef     struct global_data {
> >       unsigned long   fb_base;        /* base address of frame buffer */
> >       u8              *ram_buf;       /* emulated RAM buffer */
> >       phys_size_t     ram_size;       /* RAM size */
> > +     const void      *fdt_blob;      /* Our device tree, NULL if none
*/
> >       void            **jt;           /* jump table */
> >       char            env_buf[32];    /* buffer for getenv() before
reloc. */
> >  } gd_t;
> > diff --git a/arch/sandbox/lib/board.c b/arch/sandbox/lib/board.c
> > index b7997e9..6d464d6 100644
> > --- a/arch/sandbox/lib/board.c
> > +++ b/arch/sandbox/lib/board.c
> > @@ -156,6 +156,14 @@ void board_init_f(ulong bootflag)
> >
> >       memset((void *)gd, 0, sizeof(gd_t));
> >
> > +#if defined(CONFIG_OF_EMBED)
> > +     /* Get a pointer to the FDT */
> > +     gd->fdt_blob = _binary_dt_dtb_start;
> > +#elif defined(CONFIG_OF_SEPARATE)
> > +     /* FDT is at end of image */
> > +     gd->fdt_blob = (void *)(_end_ofs + _TEXT_BASE);
> > +#endif
> > +
> >       for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr)
{
> >               if ((*init_fnc_ptr)() != 0)
> >                       hang();