[U-Boot] [PATCH 0/3] introduce genimg_get_kernel_addr()

[U-Boot] [PATCH 0/3] introduce genimg_get_kernel_addr()

[Bryan Wu]

When trying to fix the error message issue in pxe sysboot, we found
it's need a real kernel address for further image format detection.

So I take out such code from boot_get_kernel() of bootm.c and create
the new API functin genimg_get_kernel_addr(). Then convert pxe/sysboot
and bootm.c to use it.

Bryan Wu (3):
  image: introduce genimg_get_kernel_addr()
  pxe: detect image format before calling bootm/bootz
  bootm: use genimg_get_kernel_addr()

 common/bootm.c   | 25 +++++--------------------
 common/cmd_pxe.c | 15 +++++++++++----
 common/image.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/image.h  |  1 +
 4 files changed, 60 insertions(+), 24 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/3] image: introduce genimg_get_kernel_addr()

[Bryan Wu]

Kernel address is normally stored as a string argument of bootm or bootz.
This function is taken out from boot_get_kernel() of bootm.c, which can be
reused by others.

Signed-off-by: Bryan Wu <pengw@nvidia.com>
---
 common/image.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/image.h |  1 +
 2 files changed, 44 insertions(+)

diff --git a/common/image.c b/common/image.c
index 11b3cf5..4e2816a 100644
--- a/common/image.c
+++ b/common/image.c
@@ -643,6 +643,49 @@ int genimg_get_comp_id(const char *name)
 
 #ifndef USE_HOSTCC
 /**
+ * genimg_get_kernel_addr - get the real kernel address
+ * @img_addr: a string might contain real image address
+ *
+ * genimg_get_kernel_addr() get the real kernel start address from a string
+ * which is normally the first argv of bootm/bootz
+ *
+ * returns:
+ *     kernel start address
+ */
+ulong genimg_get_kernel_addr(char * const img_addr)
+{
+#if defined(CONFIG_FIT)
+	const char	*fit_uname_config = NULL;
+	const char	*fit_uname_kernel = NULL;
+#endif
+
+	ulong kernel_addr;
+
+	/* find out kernel image address */
+	if (!img_addr) {
+		kernel_addr = load_addr;
+		debug("*  kernel: default image load address = 0x%08lx\n",
+		      load_addr);
+#if defined(CONFIG_FIT)
+	} else if (fit_parse_conf(img_addr, load_addr, &kernel_addr,
+				  fit_uname_config)) {
+		debug("*  kernel: config '%s' from image at 0x%08lx\n",
+		      *fit_uname_config, kernel_addr);
+	} else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr,
+				     fit_uname_kernel)) {
+		debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
+		      *fit_uname_kernel, kernel_addr);
+#endif
+	} else {
+		kernel_addr = simple_strtoul(img_addr, NULL, 16);
+		debug("*  kernel: cmdline image address = 0x%08lx\n",
+		      kernel_addr);
+	}
+
+	return kernel_addr;
+}
+
+/**
  * genimg_get_format - get image format type
  * @img_addr: image start address
  *
diff --git a/include/image.h b/include/image.h
index 3e8f78d..ca2fe86 100644
--- a/include/image.h
+++ b/include/image.h
@@ -424,6 +424,7 @@ enum fit_load_op {
 #define IMAGE_FORMAT_FIT	0x02	/* new, libfdt based format */
 #define IMAGE_FORMAT_ANDROID	0x03	/* Android boot image */
 
+ulong genimg_get_kernel_addr(char * const img_addr);
 int genimg_get_format(const void *img_addr);
 int genimg_has_config(bootm_headers_t *images);
 ulong genimg_get_image(ulong img_addr);
-- 
1.9.1

[U-Boot] [PATCH 2/3] pxe: detect image format before calling bootm/bootz

[Bryan Wu]

Trying bootm for zImage will print out several error message which
is not necessary for this case. So detect image format firstly, only
try bootm for legacy and FIT format image then try bootz for others.

This patch needs new function genimg_get_kernel_addr().

Signed-off-by: Bryan Wu <pengw@nvidia.com>
---
 common/cmd_pxe.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
index ba48692..59b3598 100644
--- a/common/cmd_pxe.c
+++ b/common/cmd_pxe.c
@@ -1,5 +1,6 @@
 /*
  * Copyright 2010-2011 Calxeda, Inc.
+ * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
  *
  * SPDX-License-Identifier:	GPL-2.0+
  */
@@ -609,6 +610,8 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	char *bootargs;
 	int bootm_argc = 3;
 	int len = 0;
+	ulong kernel_addr;
+	void *buf;
 
 	label_print(label);
 
@@ -771,11 +774,15 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	if (bootm_argv[3])
 		bootm_argc = 4;
 
-	do_bootm(cmdtp, 0, bootm_argc, bootm_argv);
-
+	kernel_addr = genimg_get_kernel_addr(bootm_argv[1]);
+	buf = map_sysmem(kernel_addr, 0);
+	/* Try bootm for legacy and FIT format image */
+	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID)
+		do_bootm(cmdtp, 0, bootm_argc, bootm_argv);
 #ifdef CONFIG_CMD_BOOTZ
-	/* Try booting a zImage if do_bootm returns */
-	do_bootz(cmdtp, 0, bootm_argc, bootm_argv);
+	/* Try booting a zImage */
+	else
+		do_bootz(cmdtp, 0, bootm_argc, bootm_argv);
 #endif
 	return 1;
 }
-- 
1.9.1

[U-Boot] [PATCH 3/3] bootm: use genimg_get_kernel_addr()

[Bryan Wu]

Use the new API which is originally taken out from boot_get_kernel
of bootm.c

Signed-off-by: Bryan Wu <pengw@nvidia.com>
---
 common/bootm.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 7ec2ed8..aee68cd 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -731,26 +731,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 	int		os_noffset;
 #endif
 
-	/* find out kernel image address */
-	if (argc < 1) {
-		img_addr = load_addr;
-		debug("*  kernel: default image load address = 0x%08lx\n",
-		      load_addr);
-#if defined(CONFIG_FIT)
-	} else if (fit_parse_conf(argv[0], load_addr, &img_addr,
-				  &fit_uname_config)) {
-		debug("*  kernel: config '%s' from image at 0x%08lx\n",
-		      fit_uname_config, img_addr);
-	} else if (fit_parse_subimage(argv[0], load_addr, &img_addr,
-				     &fit_uname_kernel)) {
-		debug("*  kernel: subimage '%s' from image@0x%08lx\n",
-		      fit_uname_kernel, img_addr);
-#endif
-	} else {
-		img_addr = simple_strtoul(argv[0], NULL, 16);
-		debug("*  kernel: cmdline image address = 0x%08lx\n",
-		      img_addr);
-	}
+	img_addr = genimg_get_kernel_addr(argv[0]);
 
 	bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
 
@@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
 	case IMAGE_FORMAT_FIT:
+		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
+					fit_uname_config))
+			fit_parse_subimage(argv[0], load_addr, &img_addr,
+					fit_uname_kernel);
 		os_noffset = fit_image_load(images, img_addr,
 				&fit_uname_kernel, &fit_uname_config,
 				IH_ARCH_DEFAULT, IH_TYPE_KERNEL,
-- 
1.9.1

[U-Boot] [PATCH 3/3] bootm: use genimg_get_kernel_addr()

[Stephen Warren]

On 07/31/2014 06:40 PM, Bryan Wu wrote:
> Use the new API which is originally taken out from boot_get_kernel
> of bootm.c

> diff --git a/common/bootm.c b/common/bootm.c

>   	case IMAGE_FORMAT_FIT:
> +		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
> +					fit_uname_config))
> +			fit_parse_subimage(argv[0], load_addr, &img_addr,
> +					fit_uname_kernel);

I'd be tempted to try and rework patch 1/3 so that it could "return" the 
fit_uname_* values too. That way, you wouldn't need to call 
fit_parse_*() a second time here. This probably isn't a big issue though.

The series,
Tested-by: Stephen Warren <swarren@nvidia.com>
Reviewed-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH 1/3] image: introduce genimg_get_kernel_addr()

[Simon Glass]

Hi Bryan,

On 31 July 2014 18:39, Bryan Wu <cooloney@gmail.com> wrote:
> Kernel address is normally stored as a string argument of bootm or bootz.
> This function is taken out from boot_get_kernel() of bootm.c, which can be
> reused by others.
>
> Signed-off-by: Bryan Wu <pengw@nvidia.com>
> ---
>  common/image.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/image.h |  1 +
>  2 files changed, 44 insertions(+)
>
> diff --git a/common/image.c b/common/image.c
> index 11b3cf5..4e2816a 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -643,6 +643,49 @@ int genimg_get_comp_id(const char *name)
>
>  #ifndef USE_HOSTCC
>  /**
> + * genimg_get_kernel_addr - get the real kernel address
> + * @img_addr: a string might contain real image address
> + *
> + * genimg_get_kernel_addr() get the real kernel start address from a string
> + * which is normally the first argv of bootm/bootz
> + *
> + * returns:
> + *     kernel start address
> + */

I know you are being consistent, but actually I think this should be
in the header file - you could move some of the other functions
comments there too if you like (in a separate patch).

Also you should document the behaviour when @img_addr is NULL.

> +ulong genimg_get_kernel_addr(char * const img_addr)
> +{
> +#if defined(CONFIG_FIT)
> +       const char      *fit_uname_config = NULL;
> +       const char      *fit_uname_kernel = NULL;

I don't think we need the tabs after 'char'.

> +#endif
> +

nit: Remove this blank line.

> +       ulong kernel_addr;
> +
> +       /* find out kernel image address */
> +       if (!img_addr) {
> +               kernel_addr = load_addr;
> +               debug("*  kernel: default image load address = 0x%08lx\n",
> +                     load_addr);
> +#if defined(CONFIG_FIT)
> +       } else if (fit_parse_conf(img_addr, load_addr, &kernel_addr,
> +                                 fit_uname_config)) {
> +               debug("*  kernel: config '%s' from image at 0x%08lx\n",
> +                     *fit_uname_config, kernel_addr);
> +       } else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr,
> +                                    fit_uname_kernel)) {
> +               debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
> +                     *fit_uname_kernel, kernel_addr);
> +#endif
> +       } else {
> +               kernel_addr = simple_strtoul(img_addr, NULL, 16);
> +               debug("*  kernel: cmdline image address = 0x%08lx\n",
> +                     kernel_addr);
> +       }
> +
> +       return kernel_addr;
> +}
> +
> +/**
>   * genimg_get_format - get image format type
>   * @img_addr: image start address
>   *
> diff --git a/include/image.h b/include/image.h
> index 3e8f78d..ca2fe86 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -424,6 +424,7 @@ enum fit_load_op {
>  #define IMAGE_FORMAT_FIT       0x02    /* new, libfdt based format */
>  #define IMAGE_FORMAT_ANDROID   0x03    /* Android boot image */
>
> +ulong genimg_get_kernel_addr(char * const img_addr);
>  int genimg_get_format(const void *img_addr);
>  int genimg_has_config(bootm_headers_t *images);
>  ulong genimg_get_image(ulong img_addr);
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 3/3] bootm: use genimg_get_kernel_addr()

[Simon Glass]

Hi Bryan,

On 31 July 2014 18:40, Bryan Wu <cooloney@gmail.com> wrote:
> Use the new API which is originally taken out from boot_get_kernel
> of bootm.c
>
> Signed-off-by: Bryan Wu <pengw@nvidia.com>
> ---
>  common/bootm.c | 25 +++++--------------------
>  1 file changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index 7ec2ed8..aee68cd 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -731,26 +731,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>         int             os_noffset;
>  #endif
>
> -       /* find out kernel image address */
> -       if (argc < 1) {
> -               img_addr = load_addr;
> -               debug("*  kernel: default image load address = 0x%08lx\n",
> -                     load_addr);
> -#if defined(CONFIG_FIT)
> -       } else if (fit_parse_conf(argv[0], load_addr, &img_addr,
> -                                 &fit_uname_config)) {
> -               debug("*  kernel: config '%s' from image at 0x%08lx\n",
> -                     fit_uname_config, img_addr);
> -       } else if (fit_parse_subimage(argv[0], load_addr, &img_addr,
> -                                    &fit_uname_kernel)) {
> -               debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
> -                     fit_uname_kernel, img_addr);
> -#endif
> -       } else {
> -               img_addr = simple_strtoul(argv[0], NULL, 16);
> -               debug("*  kernel: cmdline image address = 0x%08lx\n",
> -                     img_addr);
> -       }
> +       img_addr = genimg_get_kernel_addr(argv[0]);

Do you think it would be clearer to say

          img_addr = genimg_get_kernel_addr(arg < 1 ? NULL : argv[0]);

I don't think it matters in practice, just a little nervous about
people going off the end of the args.

>
>         bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
>
> @@ -807,6 +788,10 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>  #endif
>  #if defined(CONFIG_FIT)
>         case IMAGE_FORMAT_FIT:
> +               if (!fit_parse_conf(argv[0], load_addr, &img_addr,
> +                                       fit_uname_config))
> +                       fit_parse_subimage(argv[0], load_addr, &img_addr,
> +                                       fit_uname_kernel);
>                 os_noffset = fit_image_load(images, img_addr,
>                                 &fit_uname_kernel, &fit_uname_config,
>                                 IH_ARCH_DEFAULT, IH_TYPE_KERNEL,
> --
> 1.9.1
>

Regards,
Simon

[U-Boot] [PATCH 3/3] bootm: use genimg_get_kernel_addr()

[Bryan Wu]

On Fri, Aug 1, 2014 at 12:18 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/31/2014 06:40 PM, Bryan Wu wrote:
>>
>> Use the new API which is originally taken out from boot_get_kernel
>> of bootm.c
>
>
>> diff --git a/common/bootm.c b/common/bootm.c
>
>
>>         case IMAGE_FORMAT_FIT:
>> +               if (!fit_parse_conf(argv[0], load_addr, &img_addr,
>> +                                       fit_uname_config))
>> +                       fit_parse_subimage(argv[0], load_addr, &img_addr,
>> +                                       fit_uname_kernel);
>
>
> I'd be tempted to try and rework patch 1/3 so that it could "return" the
> fit_uname_* values too. That way, you wouldn't need to call fit_parse_*() a
> second time here. This probably isn't a big issue though.
>

I actually tried to do that at beginning but it makes a little bit
complicated for the caller like pxe code. I believe most of users
don't care about those 2 fit uname arguments but only the kernel
address. So I think we can make it simpler firstly.

Thanks,
-Bryan

[U-Boot] [U-Boot, 1/3] image: introduce genimg_get_kernel_addr()

[Tom Rini]

On Thu, Jul 31, 2014 at 05:39:58PM -0700, Bryan Wu wrote:

> Kernel address is normally stored as a string argument of bootm or bootz.
> This function is taken out from boot_get_kernel() of bootm.c, which can be
> reused by others.
> 
> Signed-off-by: Bryan Wu <pengw@nvidia.com>

After doing the following to fix warnings:
diff --git a/common/image.c b/common/image.c
index 4e2816a..a2999c0 100644
--- a/common/image.c
+++ b/common/image.c
@@ -668,13 +668,13 @@ ulong genimg_get_kernel_addr(char * const img_addr)
 		      load_addr);
 #if defined(CONFIG_FIT)
 	} else if (fit_parse_conf(img_addr, load_addr, &kernel_addr,
-				  fit_uname_config)) {
+				  &fit_uname_config)) {
 		debug("*  kernel: config '%s' from image at 0x%08lx\n",
-		      *fit_uname_config, kernel_addr);
+		      fit_uname_config, kernel_addr);
 	} else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr,
-				     fit_uname_kernel)) {
+				     &fit_uname_kernel)) {
 		debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
-		      *fit_uname_kernel, kernel_addr);
+		      fit_uname_kernel, kernel_addr);
 #endif
 	} else {
 		kernel_addr = simple_strtoul(img_addr, NULL, 16);

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140810/74876541/attachment.pgp>

[U-Boot] [U-Boot, 2/3] pxe: detect image format before calling bootm/bootz

[Tom Rini]

On Thu, Jul 31, 2014 at 05:39:59PM -0700, Bryan Wu wrote:

> Trying bootm for zImage will print out several error message which
> is not necessary for this case. So detect image format firstly, only
> try bootm for legacy and FIT format image then try bootz for others.
> 
> This patch needs new function genimg_get_kernel_addr().
> 
> Signed-off-by: Bryan Wu <pengw@nvidia.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140810/535482d9/attachment.pgp>

[U-Boot] [U-Boot,3/3] bootm: use genimg_get_kernel_addr()

[Tom Rini]

On Thu, Jul 31, 2014 at 05:40:00PM -0700, Bryan Wu wrote:

> Use the new API which is originally taken out from boot_get_kernel
> of bootm.c
> 
> Signed-off-by: Bryan Wu <pengw@nvidia.com>
> Tested-by: Stephen Warren <swarren@nvidia.com>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

After doing the following to fix warnings:
diff --git a/common/bootm.c b/common/bootm.c
index aee68cd..76d811c 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -789,9 +789,9 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #if defined(CONFIG_FIT)
 	case IMAGE_FORMAT_FIT:
 		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
-					fit_uname_config))
+					&fit_uname_config))
 			fit_parse_subimage(argv[0], load_addr, &img_addr,
-					fit_uname_kernel);
+					&fit_uname_kernel);
 		os_noffset = fit_image_load(images, img_addr,
 				&fit_uname_kernel, &fit_uname_config,
 				IH_ARCH_DEFAULT, IH_TYPE_KERNEL,
Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140810/838245b3/attachment.pgp>

[U-Boot] [U-Boot, 1/3] image: introduce genimg_get_kernel_addr()

[Bryan Wu]

On Sun, Aug 10, 2014 at 3:21 PM, Tom Rini <trini@ti.com> wrote:
> On Thu, Jul 31, 2014 at 05:39:58PM -0700, Bryan Wu wrote:
>
>> Kernel address is normally stored as a string argument of bootm or bootz.
>> This function is taken out from boot_get_kernel() of bootm.c, which can be
>> reused by others.
>>
>> Signed-off-by: Bryan Wu <pengw@nvidia.com>
>
> After doing the following to fix warnings:
> diff --git a/common/image.c b/common/image.c
> index 4e2816a..a2999c0 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -668,13 +668,13 @@ ulong genimg_get_kernel_addr(char * const img_addr)
>                       load_addr);
>  #if defined(CONFIG_FIT)
>         } else if (fit_parse_conf(img_addr, load_addr, &kernel_addr,
> -                                 fit_uname_config)) {
> +                                 &fit_uname_config)) {

My bad, thanks for fixing this.

-Bryan

>                 debug("*  kernel: config '%s' from image at 0x%08lx\n",
> -                     *fit_uname_config, kernel_addr);
> +                     fit_uname_config, kernel_addr);
>         } else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr,
> -                                    fit_uname_kernel)) {
> +                                    &fit_uname_kernel)) {
>                 debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
> -                     *fit_uname_kernel, kernel_addr);
> +                     fit_uname_kernel, kernel_addr);
>  #endif
>         } else {
>                 kernel_addr = simple_strtoul(img_addr, NULL, 16);
>
> Applied to u-boot/master, thanks!
>
> --
> Tom