[U-Boot] [PATCH v1 0/3] New tag for Flattened Image Trees (FIT) - Booting Xen from a FIT. - U-Boot

newer
[U-Boot] Please pull u-boot-dm.git

[U-Boot] [PATCH v1 0/3] New tag for Flattened Image Trees (FIT) - Booting Xen from a FIT.

older
[U-Boot] [PATCH v2 0/5] Introduce...

Karl Apsite

13 May 2015 13 May '15

2:53 p.m.

The FIT config now supports a tag named "loadables:" which is a comma separated list. Users can add any number of images to the list, and u-boot will move the selected binaries to their listed load_addresses. This allows u-boot to boot xen from using an FIT configuration. Xen expects a kernel to be placed at a predetermined location, however the "kernel" field was already filled by xen itself. This change allows the user to move the required binary before xen boots, all within the FIT's configuration.

Karl Apsite (3): add boot_get_loadables() to load listed images Combine bootm_find_<thing> functions together Remove the bootm_find_other() wrapper

-- 2.3.7

Show replies by date

Karl Apsite

13 May 13 May

2:53 p.m.

New subject: [U-Boot] [PATCH v1 1/3] add boot_get_loadables() to load listed images

From: Karl Apsite karl.apsite@dornerworks.com

Added a trimmed down instance of boot_get_<thing>() to satisfy the minimum requierments of the added feature. The function follows the normal patterns set by other boot_get<thing>'s, which should make it a bit easier to combine them all together into one boot_get_image() function in a later refactor.

Documentation for the new function can be found in source: common/image.c

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com ---

diff --git a/common/bootm.c b/common/bootm.c index 6842029..f04e49b 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -240,6 +240,23 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) } #endif

+#if defined(CONFIG_FIT) +static int bootm_find_loadables(int flag, int argc, char * const argv[]) +{ + int ret; + + /* find all of the loadables */ + ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT, + NULL, NULL); + if (ret) { + puts("Loadable(s) is corrupt or invalid\n"); + return 1; + } + + return 0; +} +#endif + int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) { if (bootm_find_ramdisk(flag, argc, argv)) @@ -250,6 +267,11 @@ int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) return 1; #endif

+#if defined(CONFIG_FIT) + if (bootm_find_loadables(flag, argc, argv)) + return 1; +#endif + return 0; }

diff --git a/common/image-fit.c b/common/image-fit.c index 245264d..fe23c13 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1544,6 +1544,8 @@ static const char *fit_get_image_type_property(int type) return FIT_RAMDISK_PROP; case IH_TYPE_X86_SETUP: return FIT_SETUP_PROP; + case IH_TYPE_LOADABLE: + return FIT_LOADABLE_PROP; }

return "unknown"; @@ -1661,7 +1663,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr, os_ok = image_type == IH_TYPE_FLATDT || fit_image_check_os(fit, noffset, IH_OS_LINUX) || fit_image_check_os(fit, noffset, IH_OS_OPENRTOS); - if (!type_ok || !os_ok) { + + /* If either of the checks fail, we should report an error, but + * if the image type is coming from the "loadables" field, we + * don't care about what it is */ + if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) { fit_image_get_os(fit, noffset, &os); printf("No %s %s %s Image\n", genimg_get_os_name(os), diff --git a/common/image.c b/common/image.c index fdec496..dc7e795 100644 --- a/common/image.c +++ b/common/image.c @@ -1165,6 +1165,99 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, #endif }

+/** + * boot_get_loadable - routine to load a list of binaries to memory + * @argc: Ignored Argument + * @argv: Ignored Argument + * @images: pointer to the bootm images structure + * @arch: expected architecture for the image + * @ld_start: Ignored Argument + * @ld_len: Ignored Argument + * + * boot_get_loadable() will take the given FIT configuration, and look + * for a field named "loadables". Loadables, is a list of elements in + * the FIT given as strings. exe: + * loadables = "linux_kernel@1", "fdt@2"; + * this function will attempt to parse each string, and load the + * corresponding element from the FIT into memory. Once placed, + * no aditional actions are taken. + * + * returns: + * 0, if only valid images or no images are found + * error code, if an error occurs during fit_image_load + */ +#if defined(CONFIG_FIT) +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images, + uint8_t arch, const ulong *ld_start, ulong * const ld_len) +{ + /* + * These variabels are used to hold the current image location + * in system memory. + */ + ulong tmp_img_addr; + /* These are not used */ + ulong img_data, img_len; + void *buf; + char *uname; + char *uname_end; + int len, fit_img_result; + /* Check to see if the images struct has a FIT configuration */ + if (genimg_has_config(images)) { + /* + * Obtain the os FIT header from the images struct + * copy from dataflash if needed + */ + tmp_img_addr = map_to_sysmem(images->fit_hdr_os); + tmp_img_addr = genimg_get_image(tmp_img_addr); + buf = map_sysmem(tmp_img_addr, 0); + /* + * Check image type. For FIT images get FIT node + * and attempt to locate a generic binary. + */ + switch (genimg_get_format(buf)) { + case IMAGE_FORMAT_FIT: + /* + * Try to obtain the 'loadables' field from the + * configuration If it exists, then try and load + * all of the listed strings + * + * This logic assumes a character is one byte + */ + uname = (char *)fdt_getprop(buf, + fit_conf_get_node(buf, images->fit_uname_cfg), + FIT_LOADABLE_PROP, &len); + uname_end = uname + len; + if (NULL != uname) { + do { + fit_img_result = fit_image_load(images, + tmp_img_addr, + (const char **)&uname, + &(images->fit_uname_cfg), arch, + IH_TYPE_LOADABLE, + BOOTSTAGE_ID_FIT_LOADS_START, + FIT_LOAD_OPTIONAL_NON_ZERO, + &img_data, &img_len); + if (fit_img_result < 0) { + /* Something went wrong! */ + return fit_img_result; + } + uname += strnlen(uname, + uname_end - uname) + 1; + } while (uname < uname_end); + } + break; + default: + puts("The given image format is not supported (corrupt?)\n"); + return 1; + } + } else { + debug("## FIT configuration was not specified\n"); + } + + return 0; +} +#endif + #ifdef CONFIG_SYS_BOOT_GET_CMDLINE /** * boot_get_cmdline - allocate and initialize kernel cmdline diff --git a/include/bootstage.h b/include/bootstage.h index be44014..4e2e0fb 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -168,6 +168,7 @@ enum bootstage_id { BOOTSTAGE_ID_NAND_FIT_READ = 150, BOOTSTAGE_ID_NAND_FIT_READ_OK,

+ BOOTSTAGE_ID_FIT_LOADS_START = 160, /* for Loadable Images */ /* * These boot stages are new, higher level, and not directly related * to the old boot progress numbers. They are useful for recording diff --git a/include/image.h b/include/image.h index 97b96b3..0b0b55b 100644 --- a/include/image.h +++ b/include/image.h @@ -244,6 +244,7 @@ struct lmb; #define IH_TYPE_SOCFPGAIMAGE 19 /* Altera SOCFPGA Preloader */ #define IH_TYPE_X86_SETUP 20 /* x86 setup.bin Image */ #define IH_TYPE_LPC32XXIMAGE 21 /* x86 setup.bin Image */ +#define IH_TYPE_LOADABLE 22 /* A list of typeless images */

/* * Compression Types @@ -455,7 +456,9 @@ ulong genimg_get_image(ulong img_addr);

int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, ulong *rd_start, ulong *rd_end); -#endif +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images, + uint8_t arch, const ulong *ld_start, ulong * const ld_len); +#endif /* !USE_HOSTCC */

int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch, ulong *setup_start, ulong *setup_len);

-- 2.3.7

Simon Glass

15 May 15 May

3:57 p.m.

New subject: [U-Boot] [PATCH v1 1/3] add boot_get_loadables() to load listed images

Hi Karl,

On 13 May 2015 at 06:53, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...

From: Karl Apsite karl.apsite@dornerworks.com

Added a trimmed down instance of boot_get_<thing>() to satisfy the minimum requierments of the added feature. The function follows the normal patterns set by other boot_get<thing>'s, which should make it a bit easier to combine them all together into one boot_get_image() function in a later refactor.

Documentation for the new function can be found in source: common/image.c

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 22 +++++++++++++ common/image-fit.c | 8 ++++- common/image.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/bootstage.h | 1 + include/image.h | 5 ++- 5 files changed, 127 insertions(+), 2 deletions(-)

This looks fine to me. Can you please add documentation - doc/uImage.FIT

A few bits below.

...

diff --git a/common/bootm.c b/common/bootm.c index 6842029..f04e49b 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -240,6 +240,23 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) } #endif

+#if defined(CONFIG_FIT) +static int bootm_find_loadables(int flag, int argc, char * const argv[]) +{
  int ret;
  /* find all of the loadables */
  ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT,
                         NULL, NULL);
  if (ret) {
          puts("Loadable(s) is corrupt or invalid\n");

printf() here (we are trying to avoid using puts() so that one day we can make it compatible with the C library puts in that it automatically outputs \n).

...

          return 1;
  }
  return 0;
+} +#endif

int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) { if (bootm_find_ramdisk(flag, argc, argv)) @@ -250,6 +267,11 @@ int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) return 1; #endif

+#if defined(CONFIG_FIT)
  if (bootm_find_loadables(flag, argc, argv))
          return 1;
+#endif
  return 0;
}

diff --git a/common/image-fit.c b/common/image-fit.c index 245264d..fe23c13 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1544,6 +1544,8 @@ static const char *fit_get_image_type_property(int type) return FIT_RAMDISK_PROP; case IH_TYPE_X86_SETUP: return FIT_SETUP_PROP;
  case IH_TYPE_LOADABLE:
          return FIT_LOADABLE_PROP;
  }

  return "unknown";
@@ -1661,7 +1663,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr, os_ok = image_type == IH_TYPE_FLATDT || fit_image_check_os(fit, noffset, IH_OS_LINUX) || fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
  if (!type_ok || !os_ok) {
  /* If either of the checks fail, we should report an error, but

/* * If either ... * what it is */

...

   * if the image type is coming from the "loadables" field, we
   * don't care about what it is */
  if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) {
          fit_image_get_os(fit, noffset, &os);
          printf("No %s %s %s Image\n",
                 genimg_get_os_name(os),
diff --git a/common/image.c b/common/image.c index fdec496..dc7e795 100644 --- a/common/image.c +++ b/common/image.c @@ -1165,6 +1165,99 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, #endif }

+/**

boot_get_loadable - routine to load a list of binaries to memory

@argc: Ignored Argument

@argv: Ignored Argument

@images: pointer to the bootm images structure

@arch: expected architecture for the image

@ld_start: Ignored Argument

@ld_len: Ignored Argument

boot_get_loadable() will take the given FIT configuration, and look

for a field named "loadables". Loadables, is a list of elements in

the FIT given as strings. exe:

loadables = "linux_kernel@1", "fdt@2";

this function will attempt to parse each string, and load the

corresponding element from the FIT into memory. Once placed,

no aditional actions are taken.

returns:

@return

...

0, if only valid images or no images are found

error code, if an error occurs during fit_image_load

Great docs.

...

+#if defined(CONFIG_FIT) +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,

          uint8_t arch, const ulong *ld_start, ulong * const ld_len)

```
  /*
```

   * These variabels are used to hold the current image location

```
   * in system memory.
```
```
   */
```
```
  ulong tmp_img_addr;
```
```
  /* These are not used */
```
```
  ulong img_data, img_len;
```
```
  void *buf;
```
```
  char *uname;
```
```
  char *uname_end;
```
```
  int len, fit_img_result;
```

blank line

...

  /* Check to see if the images struct has a FIT configuration */

```
  if (genimg_has_config(images)) {
```

Can we do...

if (!genimg_has_config(images)) { debug("## FIT configuration was not specified\n"); return 0; }

just to limit the indent level?

...

```
          /*
```

           * Obtain the os FIT header from the images struct

           * copy from dataflash if needed

```
           */
```

          tmp_img_addr = map_to_sysmem(images->fit_hdr_os);

          tmp_img_addr = genimg_get_image(tmp_img_addr);

          buf = map_sysmem(tmp_img_addr, 0);

```
          /*
```

           * Check image type. For FIT images get FIT node

           * and attempt to locate a generic binary.

```
           */
```

          switch (genimg_get_format(buf)) {

```
          case IMAGE_FORMAT_FIT:
```
```
                  /*
```

                   * Try to obtain the 'loadables' field from the

                   * configuration If it exists, then try and load

                   * all of the listed strings

```
                   *
```

                   * This logic assumes a character is one byte

```
                   */
```

                  uname = (char *)fdt_getprop(buf,

do you need the cast?

...

                          fit_conf_get_node(buf, images->fit_uname_cfg),

                          FIT_LOADABLE_PROP, &len);

                  uname_end = uname + len;

```
                  if (NULL != uname) {
```

if (uname)

but perhaps you should use:

for (index = 0; !fdt_get_string_index(...index...); index++) { process string

...

```
                          do {
```

                                  fit_img_result = fit_image_load(images,

                                          tmp_img_addr,

                                          (const char **)&uname,

                                          &(images->fit_uname_cfg), arch,

                                          IH_TYPE_LOADABLE,

                                          BOOTSTAGE_ID_FIT_LOADS_START,

                                          FIT_LOAD_OPTIONAL_NON_ZERO,

                                          &img_data, &img_len);

                                  if (fit_img_result < 0) {

                                          /* Something went wrong! */

                                          return fit_img_result;

```
                                  }
```

                                  uname += strnlen(uname,

                                          uname_end - uname) + 1;

                          } while (uname < uname_end);

```
                  }
```
```
                  break;
```
```
          default:
```

                  puts("The given image format is not supported (corrupt?)\n");

```
                  return 1;
```
```
          }
```
```
  } else {
```

          debug("## FIT configuration was not specified\n");

```
  }
```
```
  return 0;
```

+} +#endif

#ifdef CONFIG_SYS_BOOT_GET_CMDLINE /**

boot_get_cmdline - allocate and initialize kernel cmdline

diff --git a/include/bootstage.h b/include/bootstage.h index be44014..4e2e0fb 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -168,6 +168,7 @@ enum bootstage_id { BOOTSTAGE_ID_NAND_FIT_READ = 150, BOOTSTAGE_ID_NAND_FIT_READ_OK,

  BOOTSTAGE_ID_FIT_LOADS_START = 160,     /* for Loadable Images */

FIT_LOADABLE_START?

...

    /*
     * These boot stages are new, higher level, and not directly related
     * to the old boot progress numbers. They are useful for recording
diff --git a/include/image.h b/include/image.h index 97b96b3..0b0b55b 100644 --- a/include/image.h +++ b/include/image.h @@ -244,6 +244,7 @@ struct lmb; #define IH_TYPE_SOCFPGAIMAGE 19 /* Altera SOCFPGA Preloader */ #define IH_TYPE_X86_SETUP 20 /* x86 setup.bin Image */ #define IH_TYPE_LPC32XXIMAGE 21 /* x86 setup.bin Image */ +#define IH_TYPE_LOADABLE 22 /* A list of typeless images */

/*

Compression Types

@@ -455,7 +456,9 @@ ulong genimg_get_image(ulong img_addr);

int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, ulong *rd_start, ulong *rd_end); -#endif +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
          uint8_t arch, const ulong *ld_start, ulong * const ld_len);

Function comment should go here since this is the API definition (move it from the C file).

...

+#endif /* !USE_HOSTCC */

int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch, ulong *setup_start, ulong *setup_len); -- 2.3.7

Regards, Simon

Karl Apsite

10:17 p.m.

New subject: [U-Boot] [PATCH v1 1/3] add boot_get_loadables() to load listed images

Hi Simon,

On 05/15/2015 09:57 AM, Simon Glass wrote:

...

Hi Karl,

On 13 May 2015 at 06:53, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...
From: Karl Apsite karl.apsite@dornerworks.com

Added a trimmed down instance of boot_get_<thing>() to satisfy the minimum requierments of the added feature. The function follows the normal patterns set by other boot_get<thing>'s, which should make it a bit easier to combine them all together into one boot_get_image() function in a later refactor.

Documentation for the new function can be found in source: common/image.c

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 22 +++++++++++++ common/image-fit.c | 8 ++++- common/image.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/bootstage.h | 1 + include/image.h | 5 ++- 5 files changed, 127 insertions(+), 2 deletions(-)

This looks fine to me. Can you please add documentation - doc/uImage.FIT

This will be included in one of the missing commits

...

A few bits below.

...
diff --git a/common/bootm.c b/common/bootm.c index 6842029..f04e49b 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -240,6 +240,23 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) } #endif

+#if defined(CONFIG_FIT) +static int bootm_find_loadables(int flag, int argc, char * const argv[]) +{
  int ret;
  /* find all of the loadables */
  ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT,
                         NULL, NULL);
  if (ret) {
          puts("Loadable(s) is corrupt or invalid\n");
printf() here (we are trying to avoid using puts() so that one day we can make it compatible with the C library puts in that it automatically outputs \n).

I will convert puts to printf in all 3 cases.

...

...
          return 1;
  }
  return 0;
+} +#endif

int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) { if (bootm_find_ramdisk(flag, argc, argv)) @@ -250,6 +267,11 @@ int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) return 1; #endif

+#if defined(CONFIG_FIT)
  if (bootm_find_loadables(flag, argc, argv))
          return 1;
+#endif
  return 0;
}

diff --git a/common/image-fit.c b/common/image-fit.c index 245264d..fe23c13 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1544,6 +1544,8 @@ static const char *fit_get_image_type_property(int type) return FIT_RAMDISK_PROP; case IH_TYPE_X86_SETUP: return FIT_SETUP_PROP;
  case IH_TYPE_LOADABLE:
          return FIT_LOADABLE_PROP;
  }

  return "unknown";
@@ -1661,7 +1663,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr, os_ok = image_type == IH_TYPE_FLATDT || fit_image_check_os(fit, noffset, IH_OS_LINUX) || fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
  if (!type_ok || !os_ok) {
  /* If either of the checks fail, we should report an error, but
/*

If either ...

what it is

*/

I'll correct the comment

...

...
   * if the image type is coming from the "loadables" field, we
   * don't care about what it is */
  if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) {
          fit_image_get_os(fit, noffset, &os);
          printf("No %s %s %s Image\n",
                 genimg_get_os_name(os),
diff --git a/common/image.c b/common/image.c index fdec496..dc7e795 100644 --- a/common/image.c +++ b/common/image.c @@ -1165,6 +1165,99 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, #endif }

+/**

boot_get_loadable - routine to load a list of binaries to memory

@argc: Ignored Argument

@argv: Ignored Argument

@images: pointer to the bootm images structure

@arch: expected architecture for the image

@ld_start: Ignored Argument

@ld_len: Ignored Argument

boot_get_loadable() will take the given FIT configuration, and look

for a field named "loadables". Loadables, is a list of elements in

the FIT given as strings. exe:

loadables = "linux_kernel@1", "fdt@2";

this function will attempt to parse each string, and load the

corresponding element from the FIT into memory. Once placed,

no aditional actions are taken.

returns:
@return

I did not see @return in the linked documentation: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Document...

I will change it to @return

...

0, if only valid images or no images are found

error code, if an error occurs during fit_image_load

Great docs.

...

*/

+#if defined(CONFIG_FIT) +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
          uint8_t arch, const ulong *ld_start, ulong * const ld_len)
+{
  /*
   * These variabels are used to hold the current image location
   * in system memory.
   */

s/variabels/variables

...

```
  ulong tmp_img_addr;
```
```
  /* These are not used */
```

I will make this comment a bit more clear

...

```
  ulong img_data, img_len;
```
```
  void *buf;
```
```
  char *uname;
```
```
  char *uname_end;
```
```
  int len, fit_img_result;
```

blank line

I looked, and didn't see a blank line, so I take this to mean you WANT a line here.

...

...
  /* Check to see if the images struct has a FIT configuration */
  if (genimg_has_config(images)) {
Can we do...

if (!genimg_has_config(images)) { debug("## FIT configuration was not specified\n"); return 0; }

just to limit the indent level?

Totally!

...

```
          /*
```

           * Obtain the os FIT header from the images struct

           * copy from dataflash if needed

```
           */
```

          tmp_img_addr = map_to_sysmem(images->fit_hdr_os);

          tmp_img_addr = genimg_get_image(tmp_img_addr);

          buf = map_sysmem(tmp_img_addr, 0);

```
          /*
```

           * Check image type. For FIT images get FIT node

           * and attempt to locate a generic binary.

```
           */
```

          switch (genimg_get_format(buf)) {

```
          case IMAGE_FORMAT_FIT:
```
```
                  /*
```

                   * Try to obtain the 'loadables' field from the

                   * configuration If it exists, then try and load

                   * all of the listed strings

```
                   *
```

                   * This logic assumes a character is one byte

```
                   */
```

                  uname = (char *)fdt_getprop(buf,

do you need the cast?

It is a (const void *) without the cast. An error is tossed if I don't have the cast because I am discarding the ‘const’ qualifier. Probably a moot point.

...

                          fit_conf_get_node(buf, images->fit_uname_cfg),

                          FIT_LOADABLE_PROP, &len);

                  uname_end = uname + len;

```
                  if (NULL != uname) {
```

if (uname)

but perhaps you should use:

for (index = 0; !fdt_get_string_index(...index...); index++) { process string

Sure!

...

...
[snip] +#endif

#ifdef CONFIG_SYS_BOOT_GET_CMDLINE /**

boot_get_cmdline - allocate and initialize kernel cmdline

diff --git a/include/bootstage.h b/include/bootstage.h index be44014..4e2e0fb 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -168,6 +168,7 @@ enum bootstage_id { BOOTSTAGE_ID_NAND_FIT_READ = 150, BOOTSTAGE_ID_NAND_FIT_READ_OK,
  BOOTSTAGE_ID_FIT_LOADS_START = 160,     /* for Loadable Images */
FIT_LOADABLE_START?

The name is getting a bit long, but I can change it.

...

...
    /*
     * These boot stages are new, higher level, and not directly related
     * to the old boot progress numbers. They are useful for recording
diff --git a/include/image.h b/include/image.h index 97b96b3..0b0b55b 100644 --- a/include/image.h +++ b/include/image.h @@ -244,6 +244,7 @@ struct lmb; #define IH_TYPE_SOCFPGAIMAGE 19 /* Altera SOCFPGA Preloader */ #define IH_TYPE_X86_SETUP 20 /* x86 setup.bin Image */ #define IH_TYPE_LPC32XXIMAGE 21 /* x86 setup.bin Image */ +#define IH_TYPE_LOADABLE 22 /* A list of typeless images */

/*

Compression Types

@@ -455,7 +456,9 @@ ulong genimg_get_image(ulong img_addr);

int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, ulong *rd_start, ulong *rd_end); -#endif +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
          uint8_t arch, const ulong *ld_start, ulong * const ld_len);
Function comment should go here since this is the API definition (move it from the C file).

I agree, however by that logic, ALL of the comments on non-static functions in common/image.c should move. If that's correct, then it might be a good idea to defer this until someone can get around to moving the lot of them together, to maintain consistency.

Current examples: http://git.denx.de/?p=u-boot.git;a=blob;f=common/image.c;h=fdec496c4bf0e936f...

http://git.denx.de/?p=u-boot.git;a=blob;f=common/image-fdt.c;h=7e2da7b3b7218...

...

...
+#endif /* !USE_HOSTCC */

int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch, ulong *setup_start, ulong *setup_len); -- 2.3.7

Regards, Simon

Simon Glass

10:38 p.m.

New subject: [U-Boot] [PATCH v1 1/3] add boot_get_loadables() to load listed images

Hi Karl,

On 15 May 2015 at 14:17, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...

Hi Simon,

On 05/15/2015 09:57 AM, Simon Glass wrote:

...
Hi Karl,

On 13 May 2015 at 06:53, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...
From: Karl Apsite karl.apsite@dornerworks.com

Added a trimmed down instance of boot_get_<thing>() to satisfy the minimum requierments of the added feature. The function follows the normal patterns set by other boot_get<thing>'s, which should make it a bit easier to combine them all together into one boot_get_image() function in a later refactor.

Documentation for the new function can be found in source: common/image.c

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 22 +++++++++++++ common/image-fit.c | 8 ++++- common/image.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/bootstage.h | 1 + include/image.h | 5 ++- 5 files changed, 127 insertions(+), 2 deletions(-)

This looks fine to me. Can you please add documentation - doc/uImage.FIT

This will be included in one of the missing commits

...
A few bits below.

...
diff --git a/common/bootm.c b/common/bootm.c index 6842029..f04e49b 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -240,6 +240,23 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) } #endif

+#if defined(CONFIG_FIT) +static int bootm_find_loadables(int flag, int argc, char * const argv[]) +{
  int ret;
  /* find all of the loadables */
  ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT,
                         NULL, NULL);
  if (ret) {
          puts("Loadable(s) is corrupt or invalid\n");
printf() here (we are trying to avoid using puts() so that one day we can make it compatible with the C library puts in that it automatically outputs \n).
I will convert puts to printf in all 3 cases.

...
...
          return 1;
  }
  return 0;
+} +#endif

int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) { if (bootm_find_ramdisk(flag, argc, argv)) @@ -250,6 +267,11 @@ int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) return 1; #endif

+#if defined(CONFIG_FIT)
  if (bootm_find_loadables(flag, argc, argv))
          return 1;
+#endif
  return 0;
}

diff --git a/common/image-fit.c b/common/image-fit.c index 245264d..fe23c13 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1544,6 +1544,8 @@ static const char *fit_get_image_type_property(int type) return FIT_RAMDISK_PROP; case IH_TYPE_X86_SETUP: return FIT_SETUP_PROP;
  case IH_TYPE_LOADABLE:
          return FIT_LOADABLE_PROP;
  }

  return "unknown";
@@ -1661,7 +1663,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr, os_ok = image_type == IH_TYPE_FLATDT || fit_image_check_os(fit, noffset, IH_OS_LINUX) || fit_image_check_os(fit, noffset, IH_OS_OPENRTOS);
  if (!type_ok || !os_ok) {
  /* If either of the checks fail, we should report an error, but
/*

If either ...

what it is

*/
I'll correct the comment

...
...
   * if the image type is coming from the "loadables" field, we
   * don't care about what it is */
  if ((!type_ok || !os_ok) && image_type != IH_TYPE_LOADABLE) {
          fit_image_get_os(fit, noffset, &os);
          printf("No %s %s %s Image\n",
                 genimg_get_os_name(os),
diff --git a/common/image.c b/common/image.c index fdec496..dc7e795 100644 --- a/common/image.c +++ b/common/image.c @@ -1165,6 +1165,99 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, #endif }

+/**

boot_get_loadable - routine to load a list of binaries to memory

@argc: Ignored Argument

@argv: Ignored Argument

@images: pointer to the bootm images structure

@arch: expected architecture for the image

@ld_start: Ignored Argument

@ld_len: Ignored Argument

boot_get_loadable() will take the given FIT configuration, and look

for a field named "loadables". Loadables, is a list of elements in

the FIT given as strings. exe:

loadables = "linux_kernel@1", "fdt@2";

this function will attempt to parse each string, and load the

corresponding element from the FIT into memory. Once placed,

no aditional actions are taken.

returns:
@return
I did not see @return in the linked documentation: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Document...

I will change it to @return

...
...
0, if only valid images or no images are found
error code, if an error occurs during fit_image_load
Great docs.

...
*/

+#if defined(CONFIG_FIT) +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
          uint8_t arch, const ulong *ld_start, ulong * const ld_len)
+{
  /*
   * These variabels are used to hold the current image location
   * in system memory.
   */
s/variabels/variables

...
...
  ulong tmp_img_addr;
  /* These are not used */
I will make this comment a bit more clear

...
...
  ulong img_data, img_len;
  void *buf;
  char *uname;
  char *uname_end;
  int len, fit_img_result;
blank line
I looked, and didn't see a blank line, so I take this to mean you WANT a line here.

...
...
  /* Check to see if the images struct has a FIT configuration */
  if (genimg_has_config(images)) {
Can we do...

if (!genimg_has_config(images)) { debug("## FIT configuration was not specified\n"); return 0; }

just to limit the indent level?
Totally!

...
...
          /*
           * Obtain the os FIT header from the images struct
           * copy from dataflash if needed
           */
          tmp_img_addr = map_to_sysmem(images->fit_hdr_os);
          tmp_img_addr = genimg_get_image(tmp_img_addr);
          buf = map_sysmem(tmp_img_addr, 0);
          /*
           * Check image type. For FIT images get FIT node
           * and attempt to locate a generic binary.
           */
          switch (genimg_get_format(buf)) {
          case IMAGE_FORMAT_FIT:
                  /*
                   * Try to obtain the 'loadables' field from the
                   * configuration If it exists, then try and load
                   * all of the listed strings
                   *
                   * This logic assumes a character is one byte
                   */
                  uname = (char *)fdt_getprop(buf,
do you need the cast?
It is a (const void *) without the cast. An error is tossed if I don't have the cast because I am discarding the ‘const’ qualifier. Probably a moot point.

...
...
                          fit_conf_get_node(buf, images->fit_uname_cfg),
                          FIT_LOADABLE_PROP, &len);
                  uname_end = uname + len;
                  if (NULL != uname) {
if (uname)

but perhaps you should use:

for (index = 0; !fdt_get_string_index(...index...); index++) { process string
Sure!

...
...
[snip] +#endif

#ifdef CONFIG_SYS_BOOT_GET_CMDLINE /**

boot_get_cmdline - allocate and initialize kernel cmdline

diff --git a/include/bootstage.h b/include/bootstage.h index be44014..4e2e0fb 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -168,6 +168,7 @@ enum bootstage_id { BOOTSTAGE_ID_NAND_FIT_READ = 150, BOOTSTAGE_ID_NAND_FIT_READ_OK,
  BOOTSTAGE_ID_FIT_LOADS_START = 160,     /* for Loadable Images */
FIT_LOADABLE_START?
The name is getting a bit long, but I can change it.

...
...
    /*
     * These boot stages are new, higher level, and not directly related
     * to the old boot progress numbers. They are useful for recording
diff --git a/include/image.h b/include/image.h index 97b96b3..0b0b55b 100644 --- a/include/image.h +++ b/include/image.h @@ -244,6 +244,7 @@ struct lmb; #define IH_TYPE_SOCFPGAIMAGE 19 /* Altera SOCFPGA Preloader */ #define IH_TYPE_X86_SETUP 20 /* x86 setup.bin Image */ #define IH_TYPE_LPC32XXIMAGE 21 /* x86 setup.bin Image */ +#define IH_TYPE_LOADABLE 22 /* A list of typeless images */

/*

Compression Types

@@ -455,7 +456,9 @@ ulong genimg_get_image(ulong img_addr);

int boot_get_ramdisk(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, ulong *rd_start, ulong *rd_end); -#endif +int boot_get_loadable(int argc, char * const argv[], bootm_headers_t *images,
          uint8_t arch, const ulong *ld_start, ulong * const ld_len);
Function comment should go here since this is the API definition (move it from the C file).
I agree, however by that logic, ALL of the comments on non-static functions in common/image.c should move. If that's correct, then it might be a good idea to defer this until someone can get around to moving the lot of them together, to maintain consistency.

Current examples: http://git.denx.de/?p=u-boot.git;a=blob;f=common/image.c;h=fdec496c4bf0e936f...

http://git.denx.de/?p=u-boot.git;a=blob;f=common/image-fdt.c;h=7e2da7b3b7218...

Perhaps. But most of the comments probably need redoing to fit the right style. So my approach is normally to try to do things nicely for new code even if old code doesn't.

...

...
...
+#endif /* !USE_HOSTCC */

int boot_get_setup_fit(bootm_headers_t *images, uint8_t arch, ulong *setup_start, ulong *setup_len); -- 2.3.7

Regards, Simon

Karl Apsite

13 May 13 May

2:53 p.m.

New subject: [U-Boot] [PATCH v1 2/3] Combine bootm_find_<thing> functions together

bootm_find_ramdisk_fdt() renamed to bootm_find_images() for readability.

The function bootm_find_ramdisk_fdt() appears to be a simple wrapper for bootm_find_ramdisk(), bootm_find_fdt(), and now bootm_find_loadables(). I didn't see any other callers entering a bootm_find<thing>, so removing the wrapper, and condensing these together hopefully makes the code a little simpler.

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com ---

common/bootm.c | 37 ++----------------------------------- common/cmd_bootm.c | 4 ++-- include/bootm.h | 2 +- 3 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c index f04e49b..ae0d674 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -206,7 +206,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc, return 0; }

-static int bootm_find_ramdisk(int flag, int argc, char * const argv[]) +int bootm_find_images(int flag, int argc, char * const argv[]) { int ret;

@@ -218,14 +218,7 @@ static int bootm_find_ramdisk(int flag, int argc, char * const argv[]) return 1; }

- return 0; -} - #if defined(CONFIG_OF_LIBFDT) -static int bootm_find_fdt(int flag, int argc, char * const argv[]) -{ - int ret; - /* find flattened device tree */ ret = boot_get_fdt(flag, argc, argv, IH_ARCH_DEFAULT, &images, &images.ft_addr, &images.ft_len); @@ -233,18 +226,10 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) puts("Could not find a valid device tree\n"); return 1; } - set_working_fdt_addr((ulong)images.ft_addr); - - return 0; -} #endif

#if defined(CONFIG_FIT) -static int bootm_find_loadables(int flag, int argc, char * const argv[]) -{ - int ret; - /* find all of the loadables */ ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT, NULL, NULL); @@ -252,24 +237,6 @@ static int bootm_find_loadables(int flag, int argc, char * const argv[]) puts("Loadable(s) is corrupt or invalid\n"); return 1; } - - return 0; -} -#endif - -int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) -{ - if (bootm_find_ramdisk(flag, argc, argv)) - return 1; - -#if defined(CONFIG_OF_LIBFDT) - if (bootm_find_fdt(flag, argc, argv)) - return 1; -#endif - -#if defined(CONFIG_FIT) - if (bootm_find_loadables(flag, argc, argv)) - return 1; #endif

return 0; @@ -283,7 +250,7 @@ static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc, (images.os.type == IH_TYPE_MULTI)) && (images.os.os == IH_OS_LINUX || images.os.os == IH_OS_VXWORKS)) - return bootm_find_ramdisk_fdt(flag, argc, argv); + return bootm_find_images(flag, argc, argv);

return 0; } diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 6b6aca6..48738ac 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -580,7 +580,7 @@ static int bootz_start(cmd_tbl_t *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */ - if (bootm_find_ramdisk_fdt(flag, argc, argv)) + if (bootm_find_images(flag, argc, argv)) return 1;

return 0; @@ -721,7 +721,7 @@ static int booti_start(cmd_tbl_t *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */ - if (bootm_find_ramdisk_fdt(flag, argc, argv)) + if (bootm_find_images(flag, argc, argv)) return 1;

return 0; diff --git a/include/bootm.h b/include/bootm.h index 6181488..4981377 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -49,7 +49,7 @@ int boot_selected_os(int argc, char * const argv[], int state, ulong bootm_disable_interrupts(void);

/* This is a special function used by booti/bootz */ -int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]); +int bootm_find_images(int flag, int argc, char * const argv[]);

int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int states, bootm_headers_t *images, int boot_progress);

-- 2.3.7

Simon Glass

15 May 15 May

3:57 p.m.

New subject: [U-Boot] [PATCH v1 2/3] Combine bootm_find_<thing> functions together

Hi Karl,

On 13 May 2015 at 06:53, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...

bootm_find_ramdisk_fdt() renamed to bootm_find_images() for readability.

The function bootm_find_ramdisk_fdt() appears to be a simple wrapper for bootm_find_ramdisk(), bootm_find_fdt(), and now bootm_find_loadables(). I didn't see any other callers entering a bootm_find<thing>, so removing the wrapper, and condensing these together hopefully makes the code a little simpler.

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 37 ++----------------------------------- common/cmd_bootm.c | 4 ++-- include/bootm.h | 2 +- 3 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c index f04e49b..ae0d674 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -206,7 +206,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc, return 0; }

-static int bootm_find_ramdisk(int flag, int argc, char * const argv[]) +int bootm_find_images(int flag, int argc, char * const argv[]) { int ret;

@@ -218,14 +218,7 @@ static int bootm_find_ramdisk(int flag, int argc, char * const argv[]) return 1; }
  return 0;
-}

#if defined(CONFIG_OF_LIBFDT) -static int bootm_find_fdt(int flag, int argc, char * const argv[]) -{
  int ret;
  /* find flattened device tree */
  ret = boot_get_fdt(flag, argc, argv, IH_ARCH_DEFAULT, &images,
                     &images.ft_addr, &images.ft_len);
@@ -233,18 +226,10 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) puts("Could not find a valid device tree\n"); return 1; }
  set_working_fdt_addr((ulong)images.ft_addr);
  return 0;
-} #endif

#if defined(CONFIG_FIT) -static int bootm_find_loadables(int flag, int argc, char * const argv[]) -{
  int ret;
  /* find all of the loadables */
  ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT,
                         NULL, NULL);
@@ -252,24 +237,6 @@ static int bootm_find_loadables(int flag, int argc, char * const argv[]) puts("Loadable(s) is corrupt or invalid\n"); return 1; }
  return 0;
-} -#endif

-int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) -{
  if (bootm_find_ramdisk(flag, argc, argv))
          return 1;
-#if defined(CONFIG_OF_LIBFDT)
  if (bootm_find_fdt(flag, argc, argv))
          return 1;
-#endif

-#if defined(CONFIG_FIT)
  if (bootm_find_loadables(flag, argc, argv))
          return 1;
#endif
    return 0;
@@ -283,7 +250,7 @@ static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc, (images.os.type == IH_TYPE_MULTI)) && (images.os.os == IH_OS_LINUX || images.os.os == IH_OS_VXWORKS))
          return bootm_find_ramdisk_fdt(flag, argc, argv);
          return bootm_find_images(flag, argc, argv);

  return 0;
} diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 6b6aca6..48738ac 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -580,7 +580,7 @@ static int bootz_start(cmd_tbl_t *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */
  if (bootm_find_ramdisk_fdt(flag, argc, argv))
  if (bootm_find_images(flag, argc, argv))
          return 1;

  return 0;
@@ -721,7 +721,7 @@ static int booti_start(cmd_tbl_t *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */
  if (bootm_find_ramdisk_fdt(flag, argc, argv))
  if (bootm_find_images(flag, argc, argv))
          return 1;

  return 0;
diff --git a/include/bootm.h b/include/bootm.h index 6181488..4981377 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -49,7 +49,7 @@ int boot_selected_os(int argc, char * const argv[], int state, ulong bootm_disable_interrupts(void);

/* This is a special function used by booti/bootz */

...

-int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]); +int bootm_find_images(int flag, int argc, char * const argv[]);

Can you please add a proper functoin comment for this function? Parameters, what it does, return value etc...

...

int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int states, bootm_headers_t *images, int boot_progress); -- 2.3.7

Regards, Simon

Karl Apsite

10:17 p.m.

New subject: [U-Boot] [PATCH v1 2/3] Combine bootm_find_<thing> functions together

On 05/15/2015 09:57 AM, Simon Glass wrote:

...

Hi Karl,

On 13 May 2015 at 06:53, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...
bootm_find_ramdisk_fdt() renamed to bootm_find_images() for readability.

The function bootm_find_ramdisk_fdt() appears to be a simple wrapper for bootm_find_ramdisk(), bootm_find_fdt(), and now bootm_find_loadables(). I didn't see any other callers entering a bootm_find<thing>, so removing the wrapper, and condensing these together hopefully makes the code a little simpler.

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 37 ++----------------------------------- common/cmd_bootm.c | 4 ++-- include/bootm.h | 2 +- 3 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c index f04e49b..ae0d674 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -206,7 +206,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc, return 0; }

-static int bootm_find_ramdisk(int flag, int argc, char * const argv[]) +int bootm_find_images(int flag, int argc, char * const argv[]) { int ret;

@@ -218,14 +218,7 @@ static int bootm_find_ramdisk(int flag, int argc, char * const argv[]) return 1; }
  return 0;
-}

#if defined(CONFIG_OF_LIBFDT) -static int bootm_find_fdt(int flag, int argc, char * const argv[]) -{
  int ret;
  /* find flattened device tree */
  ret = boot_get_fdt(flag, argc, argv, IH_ARCH_DEFAULT, &images,
                     &images.ft_addr, &images.ft_len);
@@ -233,18 +226,10 @@ static int bootm_find_fdt(int flag, int argc, char * const argv[]) puts("Could not find a valid device tree\n"); return 1; }
  set_working_fdt_addr((ulong)images.ft_addr);
  return 0;
-} #endif

#if defined(CONFIG_FIT) -static int bootm_find_loadables(int flag, int argc, char * const argv[]) -{
  int ret;
  /* find all of the loadables */
  ret = boot_get_loadable(argc, argv, &images, IH_ARCH_DEFAULT,
                         NULL, NULL);
@@ -252,24 +237,6 @@ static int bootm_find_loadables(int flag, int argc, char * const argv[]) puts("Loadable(s) is corrupt or invalid\n"); return 1; }
  return 0;
-} -#endif

-int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]) -{
  if (bootm_find_ramdisk(flag, argc, argv))
          return 1;
-#if defined(CONFIG_OF_LIBFDT)
  if (bootm_find_fdt(flag, argc, argv))
          return 1;
-#endif

-#if defined(CONFIG_FIT)
  if (bootm_find_loadables(flag, argc, argv))
          return 1;
#endif
    return 0;
@@ -283,7 +250,7 @@ static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc, (images.os.type == IH_TYPE_MULTI)) && (images.os.os == IH_OS_LINUX || images.os.os == IH_OS_VXWORKS))
          return bootm_find_ramdisk_fdt(flag, argc, argv);
          return bootm_find_images(flag, argc, argv);

  return 0;
} diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index 6b6aca6..48738ac 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -580,7 +580,7 @@ static int bootz_start(cmd_tbl_t *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */
  if (bootm_find_ramdisk_fdt(flag, argc, argv))
  if (bootm_find_images(flag, argc, argv))
          return 1;

  return 0;
@@ -721,7 +721,7 @@ static int booti_start(cmd_tbl_t *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */
  if (bootm_find_ramdisk_fdt(flag, argc, argv))
  if (bootm_find_images(flag, argc, argv))
          return 1;

  return 0;
diff --git a/include/bootm.h b/include/bootm.h index 6181488..4981377 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -49,7 +49,7 @@ int boot_selected_os(int argc, char * const argv[], int state, ulong bootm_disable_interrupts(void);

/* This is a special function used by booti/bootz */
...
-int bootm_find_ramdisk_fdt(int flag, int argc, char * const argv[]); +int bootm_find_images(int flag, int argc, char * const argv[]);

Can you please add a proper functoin comment for this function? Parameters, what it does, return value etc...

Sure thing.

...

...
int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int states, bootm_headers_t *images, int boot_progress); -- 2.3.7

Regards, Simon

Karl Apsite

13 May 13 May

2:54 p.m.

New subject: [U-Boot] [PATCH v1 3/3] Remove the bootm_find_other() wrapper

The bootm_find_other() function was only called once, and ran a quick check before it called bootm_find_images(). This wrapper function was removed by moving the conditional to do_bootm_states (where find_other was being called) and calling bootm_find_images directly.

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com ---

common/bootm.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c index ae0d674..13f2bc9 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -241,19 +241,6 @@ int bootm_find_images(int flag, int argc, char * const argv[])

return 0; } - -static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc, - char * const argv[]) -{ - if (((images.os.type == IH_TYPE_KERNEL) || - (images.os.type == IH_TYPE_KERNEL_NOLOAD) || - (images.os.type == IH_TYPE_MULTI)) && - (images.os.os == IH_OS_LINUX || - images.os.os == IH_OS_VXWORKS)) - return bootm_find_images(flag, argc, argv); - - return 0; -} #endif /* USE_HOSTC */

/** @@ -581,7 +568,15 @@ int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], ret = bootm_find_os(cmdtp, flag, argc, argv);

if (!ret && (states & BOOTM_STATE_FINDOTHER)) { - ret = bootm_find_other(cmdtp, flag, argc, argv); + if (((images.os.type == IH_TYPE_KERNEL) || + (images.os.type == IH_TYPE_KERNEL_NOLOAD) || + (images.os.type == IH_TYPE_MULTI)) && + (images.os.os == IH_OS_LINUX || + images.os.os == IH_OS_VXWORKS)) + ret = bootm_find_images(flag, argc, argv); + else + ret = 0; + argc = 0; /* consume the args */ }

-- 2.3.7

Simon Glass

15 May 15 May

3:58 p.m.

New subject: [U-Boot] [PATCH v1 3/3] Remove the bootm_find_other() wrapper

Hi Karl,

On 13 May 2015 at 06:54, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...

The bootm_find_other() function was only called once, and ran a quick check before it called bootm_find_images(). This wrapper function was removed by moving the conditional to do_bootm_states (where find_other was being called) and calling bootm_find_images directly.

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c index ae0d674..13f2bc9 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -241,19 +241,6 @@ int bootm_find_images(int flag, int argc, char * const argv[])
    return 0;
}

-static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc,
                      char * const argv[])
-{
  if (((images.os.type == IH_TYPE_KERNEL) ||
       (images.os.type == IH_TYPE_KERNEL_NOLOAD) ||
       (images.os.type == IH_TYPE_MULTI)) &&
      (images.os.os == IH_OS_LINUX ||
           images.os.os == IH_OS_VXWORKS))
          return bootm_find_images(flag, argc, argv);
  return 0;
-} #endif /* USE_HOSTC */

/** @@ -581,7 +568,15 @@ int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], ret = bootm_find_os(cmdtp, flag, argc, argv);
    if (!ret && (states & BOOTM_STATE_FINDOTHER)) {
          ret = bootm_find_other(cmdtp, flag, argc, argv);
          if (((images.os.type == IH_TYPE_KERNEL) ||
               (images.os.type == IH_TYPE_KERNEL_NOLOAD) ||
               (images.os.type == IH_TYPE_MULTI)) &&
              (images.os.os == IH_OS_LINUX ||
                   images.os.os == IH_OS_VXWORKS))
                  ret = bootm_find_images(flag, argc, argv);
          else
                  ret = 0;

do_bootm_states() is already too long. Can we keep the bootm_find_other() function? Calling it here matches the rest of the code within do_bootm_states().

...

            argc = 0;       /* consume the args */
    }

-- 2.3.7

Regards, Simon

Simon Glass

3:58 p.m.

New subject: [U-Boot] [PATCH v1 3/3] Remove the bootm_find_other() wrapper

Hi Karl,

On 15 May 2015 at 07:58, Simon Glass sjg@chromium.org wrote:

...

Hi Karl,

On 13 May 2015 at 06:54, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...
The bootm_find_other() function was only called once, and ran a quick check before it called bootm_find_images(). This wrapper function was removed by moving the conditional to do_bootm_states (where find_other was being called) and calling bootm_find_images directly.

Signed-off-by: Karl Apsite Karl.Apsite@dornerworks.com

common/bootm.c | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c index ae0d674..13f2bc9 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -241,19 +241,6 @@ int bootm_find_images(int flag, int argc, char * const argv[])
    return 0;
}

-static int bootm_find_other(cmd_tbl_t *cmdtp, int flag, int argc,
                      char * const argv[])
-{
  if (((images.os.type == IH_TYPE_KERNEL) ||
       (images.os.type == IH_TYPE_KERNEL_NOLOAD) ||
       (images.os.type == IH_TYPE_MULTI)) &&
      (images.os.os == IH_OS_LINUX ||
           images.os.os == IH_OS_VXWORKS))
          return bootm_find_images(flag, argc, argv);
  return 0;
-} #endif /* USE_HOSTC */

/** @@ -581,7 +568,15 @@ int do_bootm_states(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], ret = bootm_find_os(cmdtp, flag, argc, argv);
    if (!ret && (states & BOOTM_STATE_FINDOTHER)) {
          ret = bootm_find_other(cmdtp, flag, argc, argv);
          if (((images.os.type == IH_TYPE_KERNEL) ||
               (images.os.type == IH_TYPE_KERNEL_NOLOAD) ||
               (images.os.type == IH_TYPE_MULTI)) &&
              (images.os.os == IH_OS_LINUX ||
                   images.os.os == IH_OS_VXWORKS))
                  ret = bootm_find_images(flag, argc, argv);
          else
                  ret = 0;
do_bootm_states() is already too long. Can we keep the bootm_find_other() function? Calling it here matches the rest of the code within do_bootm_states().

It's fine to rename if i you like.

...

            argc = 0;       /* consume the args */
    }

-- 2.3.7

Regards, Simon

Karl Apsite

4:33 p.m.

New subject: [U-Boot] [PATCH v1 0/3] New tag for Flattened Image Trees (FIT) - Booting Xen from a FIT.

Hi Simon,

I read your initial comments, and I immediately realized I made a mistake. I had initially submitted my series on a local branch, but my mailing client didn't send out anything past the cover letter. (I think Patman had CC'd too many people, and my mailer was preventing spam) When you first responded, I then rushed out the patch again, trimming down what Patman was trying to do. I was on a different branch and had made a couple small efforts towards refactoring. On that branch, I had intended to submit commits A, B, and C, but instead, sent you C, D, and E.

Commit A will have the tests I added to the test runner. Commit B will have some of the missing documentation you had mentioned. And you responded favorably to Commit D.

I will submit a new patch series with the correct commits (A, B, C, and D), with modifications based on your review comments. I will not be pursuing a larger refactor at this time.

Thank you, and I apologize for any confusion I may have caused.

On 05/13/2015 08:53 AM, Karl Apsite wrote:

...

The FIT config now supports a tag named "loadables:" which is a comma separated list. Users can add any number of images to the list, and u-boot will move the selected binaries to their listed load_addresses. This allows u-boot to boot xen from using an FIT configuration. Xen expects a kernel to be placed at a predetermined location, however the "kernel" field was already filled by xen itself. This change allows the user to move the required binary before xen boots, all within the FIT's configuration.

Karl Apsite (3): add boot_get_loadables() to load listed images Combine bootm_find_<thing> functions together Remove the bootm_find_other() wrapper

common/bootm.c | 50 ++++++++++------------------ common/cmd_bootm.c | 4 +-- common/image-fit.c | 8 ++++- common/image.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/bootm.h | 2 +- include/bootstage.h | 1 + include/image.h | 5 ++- 7 files changed, 125 insertions(+), 38 deletions(-)

Simon Glass

4:52 p.m.

New subject: [U-Boot] [PATCH v1 0/3] New tag for Flattened Image Trees (FIT) - Booting Xen from a FIT.

Hi Karl,

On 15 May 2015 at 08:33, Karl Apsite Karl.Apsite@dornerworks.com wrote:

...

Hi Simon,

I read your initial comments, and I immediately realized I made a mistake. I had initially submitted my series on a local branch, but my mailing client didn't send out anything past the cover letter. (I think Patman had CC'd too many people, and my mailer was preventing spam) When you first responded, I then rushed out the patch again, trimming down what Patman was trying to do. I was on a different branch and had made a couple small efforts towards refactoring. On that branch, I had intended to submit commits A, B, and C, but instead, sent you C, D, and E.

Commit A will have the tests I added to the test runner. Commit B will have some of the missing documentation you had mentioned. And you responded favorably to Commit D.

I will submit a new patch series with the correct commits (A, B, C, and D), with modifications based on your review comments. I will not be pursuing a larger refactor at this time.

Thank you, and I apologize for any confusion I may have caused.

OK thanks. If you can use get 'git send-email' to work for you then patman will send the patches automatically.

...

On 05/13/2015 08:53 AM, Karl Apsite wrote:

...
The FIT config now supports a tag named "loadables:" which is a comma separated list. Users can add any number of images to the list, and u-boot will move the selected binaries to their listed load_addresses. This allows u-boot to boot xen from using an FIT configuration. Xen expects a kernel to be placed at a predetermined location, however the "kernel" field was already filled by xen itself. This change allows the user to move the required binary before xen boots, all within the FIT's configuration.

Karl Apsite (3): add boot_get_loadables() to load listed images Combine bootm_find_<thing> functions together Remove the bootm_find_other() wrapper

common/bootm.c | 50 ++++++++++------------------ common/cmd_bootm.c | 4 +-- common/image-fit.c | 8 ++++- common/image.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/bootm.h | 2 +- include/bootstage.h | 1 + include/image.h | 5 ++- 7 files changed, 125 insertions(+), 38 deletions(-)

Regards, Simon

3652

Age (days ago)

3654

Last active (days ago)

List overview

Download

12 comments

2 participants

tags (0)

participants (2)

Karl Apsite
Simon Glass