sandbox: fdt: Add support for CONFIG_OF_CONTROL

+#define CONFIG_LMB

Quoted 1 line · On Tuesday 10 January 2012 19:45:46 Simon Glass wrote:

+#define CONFIG_LMB

do we need this ?

-mike

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Tuesday 10 January 2012 19:45:46 Simon Glass wrote:

+#define CONFIG_LMB

do we need this ?

Without it I think bootm fails to build with FDT enabled.

Quoted 6 lines · On Monday 23 January 2012 00:30:32 Simon Glass wrote:

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Tuesday 10 January 2012 19:45:46 Simon Glass wrote:

+#define CONFIG_LMB

do we need this ?

Quoted 1 line

Without it I think bootm fails to build with FDT enabled.

looks like CONFIG_OF_LIBFDT hard depends on CONFIG_LMB, and a lot of lmb stuff
leaks when it isn't defined. i think i'll send a patch to tighten up the lmb.h
header when that define isn't available.
-mike

Quoted 9 lines · Mike Frysinger

Quoted 7 lines · On Monday 23 January 2012 00:30:32 Simon Glass wrote:

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Tuesday 10 January 2012 19:45:46 Simon Glass wrote:

+#define CONFIG_LMB

do we need this ?

Without it I think bootm fails to build with FDT enabled.

Quoted 6 lines

looks like CONFIG_OF_LIBFDT hard depends on CONFIG_LMB, and a lot of lmb stuff
leaks when it isn't defined. i think i'll send a patch to tighten up the lmb.h
header when that define isn't available.
-mike

OK - did you do this? Just wondering if I should drop this CONFIG now...

Quoted 17 lines · On Wednesday 15 February 2012 01:26:10 Simon Glass wrote:

Quoted 15 lines · Mike Frysinger

Quoted 7 lines · On Monday 23 January 2012 00:30:32 Simon Glass wrote:

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Tuesday 10 January 2012 19:45:46 Simon Glass wrote:

+#define CONFIG_LMB

do we need this ?

Without it I think bootm fails to build with FDT enabled.

looks like CONFIG_OF_LIBFDT hard depends on CONFIG_LMB, and a lot of lmb stuff
leaks when it isn't defined. ?i think i'll send a patch to tighten up the lmb.h
header when that define isn't available.
-mike

Quoted 1 line

OK - did you do this? Just wondering if I should drop this CONFIG now...

i posted a patch, but we'll still need the lmb define

-mike

Quoted 18 lines · Mike Frysinger

Quoted 17 lines · On Wednesday 15 February 2012 01:26:10 Simon Glass wrote:

Quoted 15 lines · Mike Frysinger

Quoted 7 lines · On Monday 23 January 2012 00:30:32 Simon Glass wrote:

Quoted 4 lines · Mike Frysinger

Quoted 1 line · On Tuesday 10 January 2012 19:45:46 Simon Glass wrote:

+#define CONFIG_LMB

do we need this ?

Without it I think bootm fails to build with FDT enabled.

looks like CONFIG_OF_LIBFDT hard depends on CONFIG_LMB, and a lot of lmb stuff
leaks when it isn't defined. i think i'll send a patch to tighten up the lmb.h
header when that define isn't available.
-mike

Quoted 3 lines

Quoted 1 line

OK - did you do this? Just wondering if I should drop this CONFIG now...

i posted a patch, but we'll still need the lmb define

OK. So this patch stays as is?

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

Quoted 2 lines · On Tuesday 10 January 2012 19:45:47 Simon Glass wrote:

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

seems to be a desync in types ... all "gpio" fields should be "unsigned" and not "int"

arch/sandbox/include/asm/gpio.h

Quoted 5 lines

arch/sandbox/include/asm/gpio.h new file +1 -0

 
+int sandbox_gpio_get_value(int gp);

why bother with parallel sandbox gpio API ? ?why can't we just implement the gpio API directly and throw away sandbox_gpio_xxx ? ?then we can also stub out sandbox/include/asm/gpio.h ...

also, missing gpio_status() define to gpio_info()

Quoted 4 lines

drivers/gpio/sandbox.c new file +0 -0

drivers/gpio/sandbox.c

Quoted 6 lines

drivers/gpio/sandbox.c new file +6 -0

+enum {
+ ? ? CMD_INFO,
+ ? ? CMD_PORT,
+ ? ? CMD_OUTPUT,
+ ? ? CMD_INPUT,
+};

CMD_XXX enums are unused -> punt

drivers/gpio/sandbox.c line 33

Quoted 5 lines

drivers/gpio/sandbox.c new file +4 -0

+enum {
+ ? ? GPIOF_OUTPUT ? ?= 1 << 1,
+ ? ? GPIOF_HIGH ? ? ?= 1 << 2,
+};

turn enum bit flags into defines

also, add a "reserved" bit flag and check it in gpio_request()

drivers/gpio/sandbox.c

Quoted 9 lines

drivers/gpio/sandbox.c new file +5 -0

+/* read GPIO IN value of port 'gp' */
+int gpio_get_value(int gp)
...
+ ? ? if (get_gpio_flag(gp, GPIOF_OUTPUT))
...
+int gpio_set_value(int gp, int value)
...
+ ? ? if (get_gpio_flag(gp, GPIOF_OUTPUT)) {

drop valid gpio checking in these funcs ... only the request func should do this

drivers/gpio/sandbox.c line 139

Quoted 6 lines

drivers/gpio/sandbox.c new file +5 -0

+int gpio_request(unsigned gpio, const char *label)
+{
+ ? ? /* for now, do nothing */
+ ? ? return 0;
+}

add (gpio <= CONFIG_SANDBOX_GPIO_COUNT) check to verify the gpio is valid

drivers/gpio/sandbox.c line 152

Quoted 6 lines

drivers/gpio/sandbox.c new file +5 -0

+int gpio_info(int gp)
+{
+ ? ? printf("Sandbox GPIOs\n");
+ ? ? return 0;
+}

implement it ?

-mike

Quoted 9 lines · Mike Frysinger

arch/sandbox/include/asm/gpio.h

Quoted 4 lines · On Tuesday 10 January 2012 19:45:47 Simon Glass wrote:

arch/sandbox/include/asm/gpio.h new file +1 -0

 
+int sandbox_gpio_get_value(int gp);

why bother with parallel sandbox gpio API ? why can't we just implement the gpio API directly and throw away sandbox_gpio_xxx ? then we can also stub out sandbox/include/asm/gpio.h ...

Because the current state of the GPIOs needs to be stored somewhere. Test code which wants a GPIO to appear to be high to U-Boot can call sandbox_gpio_set_value() and that value will be recorded and provided to future gpio_get_value() calls.

Without this virtualisation, the driver would have no purpose.

Quoted 1 line

also, missing gpio_status() define to gpio_info()

I see gpio_info() there at the bottom - gpio_status() is optional I think. Do we need to bring this in?

Quoted 11 lines

arch/sandbox/include/asm/gpio.h

Quoted 8 lines

arch/sandbox/include/asm/gpio.h new file +5 -0

+/* read GPIO IN value of port 'gp' */
+int gpio_get_value(int gp)
...
+     if (get_gpio_flag(gp, GPIOF_OUTPUT))
...
+int gpio_set_value(int gp, int value)
...
+     if (get_gpio_flag(gp, GPIOF_OUTPUT)) {

drop valid gpio checking in these funcs ... only the request func should do this

What if someone passes in garbage value? Don't we want to catch this?

Quoted 16 lines · On Monday 23 January 2012 01:20:16 Simon Glass wrote:

Quoted 9 lines · Mike Frysinger

arch/sandbox/include/asm/gpio.h

Quoted 4 lines · On Tuesday 10 January 2012 19:45:47 Simon Glass wrote:

arch/sandbox/include/asm/gpio.h new file +1 -0

 
+int sandbox_gpio_get_value(int gp);

why bother with parallel sandbox gpio API ? why can't we just implement the gpio API directly and throw away sandbox_gpio_xxx ? then we can also stub out sandbox/include/asm/gpio.h ...

Because the current state of the GPIOs needs to be stored somewhere. Test code which wants a GPIO to appear to be high to U-Boot can call sandbox_gpio_set_value() and that value will be recorded and provided to future gpio_get_value() calls.

Quoted 1 line

Without this virtualisation, the driver would have no purpose.

i'm not seeing it. why does external code need to reach into the guts at all when we have a gpio API for them to use ?

Quoted 17 lines · On Tuesday 24 January 2012 16:35:00 Mike Frysinger wrote:

Quoted 16 lines · On Monday 23 January 2012 01:20:16 Simon Glass wrote:

Quoted 9 lines · Mike Frysinger

arch/sandbox/include/asm/gpio.h

Quoted 4 lines · On Tuesday 10 January 2012 19:45:47 Simon Glass wrote:

arch/sandbox/include/asm/gpio.h new file +1 -0

 
+int sandbox_gpio_get_value(int gp);

why bother with parallel sandbox gpio API ? ?why can't we just implement the gpio API directly and throw away sandbox_gpio_xxx ? then we can also stub out sandbox/include/asm/gpio.h ...

Because the current state of the GPIOs needs to be stored somewhere. Test code which wants a GPIO to appear to be high to U-Boot can call sandbox_gpio_set_value() and that value will be recorded and provided to future gpio_get_value() calls.

Quoted 4 lines

Quoted 1 line

Without this virtualisation, the driver would have no purpose.

i'm not seeing it. ?why does external code need to reach into the guts at all when we have a gpio API for them to use ?

ok, you clarified it for me, so now we just need some comments in asm/gpio.h explaining the expected users of the internal API
-mike

include/configs/sandbox.h

Quoted 4 lines · On Tuesday 10 January 2012 19:45:48 Simon Glass wrote:

include/configs/sandbox.h +1 -0

 
+#define CONFIG_SANDBOX_GPIO_COUNT ? ?224

do we really need 224 examples GPIOs ? ?can't we do with like 20 ?

-mike

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.

Quoted 3 lines · On Tuesday 10 January 2012 19:45:50 Simon Glass wrote:

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.

does this need to be done this early ? ?parsing args can easily be done inside main() (as i showed in my patch that added command line support).

in terms of processing the state, i think we can just make this into an init func that runs before we let board_init_r tail into main_loop

-mike

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?

Quoted 5 lines · On Tuesday 10 January 2012 19:45:51 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?

some of the flags differ between Linux/arches, so i think we'll have to do it now :/

Review

Quoted 6 lines

(quoted diff) +6 -0

+enum {
+ ? ? OS_O_RDONLY,
+ ? ? OS_O_WRONLY,
+ ? ? OS_O_RDWR,
+ ? ? OS_O_CREAT ? ? ?= 0100,
+};

the flags are bit based, so an enum won't work i don't think

-mike

Quoted 9 lines · Mike Frysinger

include/os.h ssize_t os_read(int fd, void *buf, size_t count); · line 54

Quoted 6 lines · On Tuesday 10 January 2012 19:45:51 Simon Glass wrote:

include/os.h +6 -0

+enum {
+     OS_O_RDONLY,
+     OS_O_WRONLY,
+     OS_O_RDWR,
+     OS_O_CREAT      = 0100,
+};

the flags are bit based, so an enum won't work i don't think

Mostly, but in /usr/include/bits/fcntl.h:

#define O_RDONLY 00
#define O_WRONLY 01
#define O_RDWR 02
#define O_CREAT 0100 /* not fcntl */

Quoted 11 lines · On Saturday 14 January 2012 23:36:40 Simon Glass wrote:

Quoted 9 lines · Mike Frysinger

Review

Quoted 6 lines · On Tuesday 10 January 2012 19:45:51 Simon Glass wrote:

(quoted diff) +6 -0

+enum {
+ ? ? OS_O_RDONLY,
+ ? ? OS_O_WRONLY,
+ ? ? OS_O_RDWR,
+ ? ? OS_O_CREAT ? ? ?= 0100,
+};

the flags are bit based, so an enum won't work i don't think

Quoted 6 lines

Mostly, but in /usr/include/bits/fcntl.h:

#define O_RDONLY ? ? ? ? ? 00
#define O_WRONLY ? ? ? ? ? 01
#define O_RDWR ? ? ? ? ? ? ? ? ? ? 02
#define O_CREAT ? ? ? ? ? ? ? ? ?0100 /* not fcntl */

what i mean is that people don't do bitwise operations with enums. ?so to open a few for writing and create it, you have to do:
? ? ? ?open(..., O_CREAT|O_WRONLY)

which is just weird with enums

-mike

arch/sandbox/cpu/os.c

Quoted 4 lines · On Tuesday 10 January 2012 19:45:52 Simon Glass wrote:

arch/sandbox/cpu/os.c +1 -0

 
+static struct option long_options[] = {

constify

Quoted 7 lines

arch/sandbox/cpu/os.c +7 -0

+void os_usage(int err)
+{
+ ? ? if (err < 0)
+ ? ? ? ? ? ? fprintf(stderr, "Try `--help' for more information.\n");
+ ? ? fprintf(stderr, "u-boot, "
+ ? ? ? ? ? ? "a command line test interface to U-Boot\n\n"
+ ? ? ? ? ? ? "usage:\tu-boot [-ch]\n"

Quoted 5 lines

+ ? ? ? ? ? ? "Options:\n"
+ ? ? ? ? ? ? "\t-h\tDisplay help\n"
+ ? ? ? ? ? ? "\t-c <command>\tExecute U-Boot command\n");
+ ? ? exit(1);
+}

os_usage() should only write to stderr/exit(1) if it's an error, otherwise it should use stdout/exit(0)

arch/sandbox/cpu/os.c u64 os_get_nsec(void)

Quoted 6 lines

arch/sandbox/cpu/os.c +4 -0

+int os_parse_args(struct sandbox_state *state, int argc, char *argv[])
+{
...
+ ? ? while ((c = getopt_long(argc, argv, "c:h",
+ ? ? ? ? ? ? long_options, NULL)) != EOF) {

pull the optstring ("c:h") out of the getopt_long call and put it into a const next to the long_options[] variable

and next to opts array

arch/sandbox/cpu/os.c

Quoted 10 lines

+ ? ? /* Execute command if required */
+ ? ? if (state->cmd) {
+ ? ? ? ? ? ? /* TODO: redo this when cmd tidy-up series lands */

arch/sandbox/cpu/os.c +6 -0

+#ifdef CONFIG_SYS_HUSH_PARSER
+ ? ? ? ? ? ? run_command(state->cmd, 0);
+#else
+ ? ? ? ? ? ? parse_string_outer(state->cmd, FLAG_PARSE_SEMICOLON |
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? FLAG_EXIT_FROM_LOOP);
+#endif

i'm not sure how useful -c is since we can already do:
? ? ? ? ?./u-boot <<<"some command"

each to their own i guess ...

-mike

Quoted 4 lines · Mike Frysinger

i'm not sure how useful -c is since we can already do:

         ./u-boot <<<"some command"

each to their own i guess ...

I had trouble getting that to work properly, ending up with a runaway process. Probably we don't handle EOF correctly since we just loop forever.

I do think this argument is useful and it (and help) provide a reasonable excuse for a command line parser.

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

Quoted 1 line · On Tuesday 10 January 2012 19:45:45 Simon Glass wrote:

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

OK, but what's the point if we aren't doing FDT's ?

Review

Quoted 8 lines

(quoted diff) +7 -0

+#ifdef 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

i'd be inclined to do:
+#if defined(CONFIG_OF_EMBED)
? ? ? ?...
+#elif defined(CONFIG_OF_SEPARATE)
? ? ? ?...
+#endif

-mike

Quoted 5 lines · Mike Frysinger

Quoted 2 lines · On Tuesday 10 January 2012 19:45:45 Simon Glass wrote:

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

OK, but what's the point if we aren't doing FDT's ?

Aren't we?

I hope that sandbox provides a way of testing the FDT stuff. I used it for the fdtdec alias stuff which was complicated enough that it needed unit tests.

We need to enable these options to bring in that code, even if we don't actually have a run-time FDT yet.

Quoted 5 lines · On Monday 23 January 2012 00:27:56 Simon Glass wrote:

Quoted 4 lines · Mike Frysinger

Quoted 2 lines · On Tuesday 10 January 2012 19:45:45 Simon Glass wrote:

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

Quoted 3 lines

Quoted 1 line

OK, but what's the point if we aren't doing FDT's ?

Aren't we?

i don't see FDT's getting used anywhere in this patch or this patch series. so no, we aren't currently doing FDT's in sandbox.

Quoted 4 lines

I hope that sandbox provides a way of testing the FDT stuff. I used it for the fdtdec alias stuff which was complicated enough that it needed unit tests.

if it actually can be used/tested under sandbox, then i'm fine with this.
but
can that actually be done now or are other things necessary first ?

Quoted 3 lines

We need to enable these options to bring in that code, even if we don't actually have a run-time FDT yet.

i don't see how run-time FDT's would be used by sandbox ...

-mike