[U-Boot] [PATCH] image: fix bootm failure for FIT image

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Bryan Wu]

Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
booting FIT image. It's because calling fit_parse_config() twice will
give us wrong value in img_addr.

Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
return fit_uname_config and fit_uname_kernel for CONFIG_FIT.

Reported-by: York Sun <yorksun@freescale.com>
Signed-off-by: Bryan Wu <pengw@nvidia.com>
---
 common/bootm.c               |  9 +++++----
 common/cmd_pxe.c             |  9 +++++++++
 common/image.c               | 19 ++++++++++---------
 include/configs/jetson-tk1.h |  2 ++
 include/image.h              |  6 ++++++
 5 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/common/bootm.c b/common/bootm.c
index 76d811c..85b71ba 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -731,7 +731,12 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 	int		os_noffset;
 #endif
 
+#if defined(CONFIG_FIT)
+	img_addr = genimg_get_kernel_addr(argv[0], &fit_uname_config,
+					  &fit_uname_kernel);
+#else
 	img_addr = genimg_get_kernel_addr(argv[0]);
+#endif
 
 	bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
 
@@ -788,10 +793,6 @@ 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,
diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
index c816339..9a2c370 100644
--- a/common/cmd_pxe.c
+++ b/common/cmd_pxe.c
@@ -612,6 +612,10 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	int len = 0;
 	ulong kernel_addr;
 	void *buf;
+#if defined(CONFIG_FIT)
+	const char *fit_uname_config = NULL;
+	const char *fit_uname_kernel = NULL;
+#endif
 
 	label_print(label);
 
@@ -774,7 +778,12 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	if (bootm_argv[3])
 		bootm_argc = 4;
 
+#if defined(CONFIG_FIT)
+	kernel_addr = genimg_get_kernel_addr(bootm_argv[1], &fit_uname_config,
+					     &fit_uname_kernel);
+#else
 	kernel_addr = genimg_get_kernel_addr(bootm_argv[1]);
+#endif
 	buf = map_sysmem(kernel_addr, 0);
 	/* Try bootm for legacy and FIT format image */
 	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID)
diff --git a/common/image.c b/common/image.c
index a2999c0..ea980cb 100644
--- a/common/image.c
+++ b/common/image.c
@@ -652,13 +652,14 @@ int genimg_get_comp_id(const char *name)
  * 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;
+ulong genimg_get_kernel_addr(char * const img_addr,
+			     const char **fit_uname_config,
+			     const char **fit_uname_kernel)
+#else
+ulong genimg_get_kernel_addr(char * const img_addr)
 #endif
-
+{
 	ulong kernel_addr;
 
 	/* find out kernel image address */
@@ -668,13 +669,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);
diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
index 68b6fb6..b68d58e 100644
--- a/include/configs/jetson-tk1.h
+++ b/include/configs/jetson-tk1.h
@@ -27,6 +27,8 @@
 #define CONFIG_OF_LIBFDT
 #define CONFIG_OF_BOARD_SETUP
 
+#define CONFIG_FIT
+
 #define CONFIG_SERIAL_TAG
 #define CONFIG_TEGRA_SERIAL_HIGH	0x01770000
 #define CONFIG_TEGRA_SERIAL_LOW		0x034200FF
diff --git a/include/image.h b/include/image.h
index ca2fe86..a47c146 100644
--- a/include/image.h
+++ b/include/image.h
@@ -424,7 +424,13 @@ enum fit_load_op {
 #define IMAGE_FORMAT_FIT	0x02	/* new, libfdt based format */
 #define IMAGE_FORMAT_ANDROID	0x03	/* Android boot image */
 
+#if defined(CONFIG_FIT)
+ulong genimg_get_kernel_addr(char * const img_addr,
+			     const char	**fit_uname_config,
+			     const char	**fit_uname_kernel);
+#else
 ulong genimg_get_kernel_addr(char * const img_addr);
+#endif
 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] image: fix bootm failure for FIT image

[Bryan Wu]

York,

Could you please to try this patch on your platform? I tested on mine
but it's not the same as your setup.

Thanks,
-Bryan

On Fri, Aug 15, 2014 at 10:49 AM, Bryan Wu <cooloney@gmail.com> wrote:
> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
> booting FIT image. It's because calling fit_parse_config() twice will
> give us wrong value in img_addr.
>
> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
> return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
>
> Reported-by: York Sun <yorksun@freescale.com>
> Signed-off-by: Bryan Wu <pengw@nvidia.com>
> ---
>  common/bootm.c               |  9 +++++----
>  common/cmd_pxe.c             |  9 +++++++++
>  common/image.c               | 19 ++++++++++---------
>  include/configs/jetson-tk1.h |  2 ++
>  include/image.h              |  6 ++++++
>  5 files changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/common/bootm.c b/common/bootm.c
> index 76d811c..85b71ba 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -731,7 +731,12 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
>         int             os_noffset;
>  #endif
>
> +#if defined(CONFIG_FIT)
> +       img_addr = genimg_get_kernel_addr(argv[0], &fit_uname_config,
> +                                         &fit_uname_kernel);
> +#else
>         img_addr = genimg_get_kernel_addr(argv[0]);
> +#endif
>
>         bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);
>
> @@ -788,10 +793,6 @@ 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,
> diff --git a/common/cmd_pxe.c b/common/cmd_pxe.c
> index c816339..9a2c370 100644
> --- a/common/cmd_pxe.c
> +++ b/common/cmd_pxe.c
> @@ -612,6 +612,10 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>         int len = 0;
>         ulong kernel_addr;
>         void *buf;
> +#if defined(CONFIG_FIT)
> +       const char *fit_uname_config = NULL;
> +       const char *fit_uname_kernel = NULL;
> +#endif
>
>         label_print(label);
>
> @@ -774,7 +778,12 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>         if (bootm_argv[3])
>                 bootm_argc = 4;
>
> +#if defined(CONFIG_FIT)
> +       kernel_addr = genimg_get_kernel_addr(bootm_argv[1], &fit_uname_config,
> +                                            &fit_uname_kernel);
> +#else
>         kernel_addr = genimg_get_kernel_addr(bootm_argv[1]);
> +#endif
>         buf = map_sysmem(kernel_addr, 0);
>         /* Try bootm for legacy and FIT format image */
>         if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID)
> diff --git a/common/image.c b/common/image.c
> index a2999c0..ea980cb 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -652,13 +652,14 @@ int genimg_get_comp_id(const char *name)
>   * 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;
> +ulong genimg_get_kernel_addr(char * const img_addr,
> +                            const char **fit_uname_config,
> +                            const char **fit_uname_kernel)
> +#else
> +ulong genimg_get_kernel_addr(char * const img_addr)
>  #endif
> -
> +{
>         ulong kernel_addr;
>
>         /* find out kernel image address */
> @@ -668,13 +669,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);
> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
> index 68b6fb6..b68d58e 100644
> --- a/include/configs/jetson-tk1.h
> +++ b/include/configs/jetson-tk1.h
> @@ -27,6 +27,8 @@
>  #define CONFIG_OF_LIBFDT
>  #define CONFIG_OF_BOARD_SETUP
>
> +#define CONFIG_FIT
> +
>  #define CONFIG_SERIAL_TAG
>  #define CONFIG_TEGRA_SERIAL_HIGH       0x01770000
>  #define CONFIG_TEGRA_SERIAL_LOW                0x034200FF
> diff --git a/include/image.h b/include/image.h
> index ca2fe86..a47c146 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -424,7 +424,13 @@ enum fit_load_op {
>  #define IMAGE_FORMAT_FIT       0x02    /* new, libfdt based format */
>  #define IMAGE_FORMAT_ANDROID   0x03    /* Android boot image */
>
> +#if defined(CONFIG_FIT)
> +ulong genimg_get_kernel_addr(char * const img_addr,
> +                            const char **fit_uname_config,
> +                            const char **fit_uname_kernel);
> +#else
>  ulong genimg_get_kernel_addr(char * const img_addr);
> +#endif
>  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] image: fix bootm failure for FIT image

[York Sun]

On 08/15/2014 10:50 AM, Bryan Wu wrote:
> York,
> 
> Could you please to try this patch on your platform? I tested on mine
> but it's not the same as your setup.

I am doing it now.

York

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[York Sun]

On 08/15/2014 10:50 AM, Bryan Wu wrote:
> York,
> 
> Could you please to try this patch on your platform? I tested on mine
> but it's not the same as your setup.

Bryan,

I did a quick test and this patch fixed the problem I had. Please continue code
review process.

I noticed a change in include/configs/jetson-tk1.h. Does it have anything to do
with this patch?

York

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Bryan Wu]

On Fri, Aug 15, 2014 at 11:06 AM, York Sun <yorksun@freescale.com> wrote:
> On 08/15/2014 10:50 AM, Bryan Wu wrote:
>> York,
>>
>> Could you please to try this patch on your platform? I tested on mine
>> but it's not the same as your setup.
>
> Bryan,
>
> I did a quick test and this patch fixed the problem I had. Please continue code
> review process.
>
> I noticed a change in include/configs/jetson-tk1.h. Does it have anything to do
> with this patch?
>

Yeah, I forget to remove that change after I tested CONFIG_FIT.

I have more clean up patches after this one and looks like Tom merged
my old version patch instead the one I updated according to Simon's
review.

Thanks,
-Bryan

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Stephen Warren]

On 08/15/2014 11:49 AM, Bryan Wu wrote:
> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for

Can we spell out the commit description too, so it's easier to know what 
it refers too, and is useful if someone cherry-picks the commit so the 
commit ID changes:

Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...

> booting FIT image. It's because calling fit_parse_config() twice will
> give us wrong value in img_addr.
>
> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
> return fit_uname_config and fit_uname_kernel for CONFIG_FIT.

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

> -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;
> +ulong genimg_get_kernel_addr(char * const img_addr,
> +			     const char **fit_uname_config,
> +			     const char **fit_uname_kernel)
> +#else
> +ulong genimg_get_kernel_addr(char * const img_addr)
>   #endif

Indentation looks wrong on that #endif.

Wouldn't it be better to avoid repeating the common parts, so this instead:

ulong genimg_get_kernel_addr(char * const img_addr,
#if defined(CONFIG_FIT)
			     const char **fit_uname_config,
			     const char **fit_uname_kernel)
#endif
)
{

> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h

> +#define CONFIG_FIT

I think that crept in by mistake.

> diff --git a/include/image.h b/include/image.h
> index ca2fe86..a47c146 100644

> +#if defined(CONFIG_FIT)
> +ulong genimg_get_kernel_addr(char * const img_addr,
> +			     const char	**fit_uname_config,
> +			     const char	**fit_uname_kernel);
> +#else
>   ulong genimg_get_kernel_addr(char * const img_addr);
> +#endif

Same comment about #ifdef placement here.

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Bryan Wu]

On Fri, Aug 15, 2014 at 11:04 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/15/2014 11:49 AM, Bryan Wu wrote:
>>
>> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
>
>
> Can we spell out the commit description too, so it's easier to know what it
> refers too, and is useful if someone cherry-picks the commit so the commit
> ID changes:
>
> Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
>
>
>> booting FIT image. It's because calling fit_parse_config() twice will
>> give us wrong value in img_addr.
>>
>> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
>> return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
>
>
>> diff --git a/common/image.c b/common/image.c
>
>
>> -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;
>> +ulong genimg_get_kernel_addr(char * const img_addr,
>> +                            const char **fit_uname_config,
>> +                            const char **fit_uname_kernel)
>> +#else
>> +ulong genimg_get_kernel_addr(char * const img_addr)
>>   #endif
>
>
> Indentation looks wrong on that #endif.
>
> Wouldn't it be better to avoid repeating the common parts, so this instead:
>
> ulong genimg_get_kernel_addr(char * const img_addr,
> #if defined(CONFIG_FIT)
>                              const char **fit_uname_config,
>                              const char **fit_uname_kernel)
> #endif
> )
> {
>

I got compiling error like this when I try this method then I moved to
2 current one:

---
ulong genimg_get_kernel_addr(char * const img_addr,
#if defined(CONFIG_FIT)
                             const char **fit_uname_config,
                             const char **fit_uname_kernel
#endif
);
---
include/image.h:432:1: error: expected declaration specifiers or ?...?
before ?)? token
 );
 ^

But if I use this:

----
ulong genimg_get_kernel_addr(char * const img_addr
#if defined(CONFIG_FIT)
                             , const char **fit_uname_config,
                             const char **fit_uname_kernel
#endif
);
----
Then compiler is happy, but the style looks weird to me.

This is for declaration but definition has the same problem.

-Bryan


>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
>
>
>> +#define CONFIG_FIT
>
>
> I think that crept in by mistake.
>
>
>> diff --git a/include/image.h b/include/image.h
>> index ca2fe86..a47c146 100644
>
>
>> +#if defined(CONFIG_FIT)
>> +ulong genimg_get_kernel_addr(char * const img_addr,
>> +                            const char **fit_uname_config,
>> +                            const char **fit_uname_kernel);
>> +#else
>>   ulong genimg_get_kernel_addr(char * const img_addr);
>> +#endif
>
>
> Same comment about #ifdef placement here.

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Simon Glass]

Hi,

On 15 August 2014 12:25, Bryan Wu <cooloney@gmail.com> wrote:
> On Fri, Aug 15, 2014 at 11:04 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/15/2014 11:49 AM, Bryan Wu wrote:
>>>
>>> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
>>
>>
>> Can we spell out the commit description too, so it's easier to know what it
>> refers too, and is useful if someone cherry-picks the commit so the commit
>> ID changes:
>>
>> Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
>>
>>
>>> booting FIT image. It's because calling fit_parse_config() twice will
>>> give us wrong value in img_addr.
>>>
>>> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
>>> return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
>>
>>
>>> diff --git a/common/image.c b/common/image.c
>>
>>
>>> -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;
>>> +ulong genimg_get_kernel_addr(char * const img_addr,
>>> +                            const char **fit_uname_config,
>>> +                            const char **fit_uname_kernel)
>>> +#else

Let's avoid these #ifdefs inside a function signature, just too ugly.
If the extra arguments are a big problem you could create a new
function perhaps?

Also, how about updating test/image/fit.py to test this behaviour?
Then we can catch the bug.

>>> +ulong genimg_get_kernel_addr(char * const img_addr)
>>>   #endif
>>
>>
>> Indentation looks wrong on that #endif.
>>
>> Wouldn't it be better to avoid repeating the common parts, so this instead:
>>
>> ulong genimg_get_kernel_addr(char * const img_addr,
>> #if defined(CONFIG_FIT)
>>                              const char **fit_uname_config,
>>                              const char **fit_uname_kernel)
>> #endif
>> )
>> {
>>
>
> I got compiling error like this when I try this method then I moved to
> 2 current one:
>
> ---
> ulong genimg_get_kernel_addr(char * const img_addr,
> #if defined(CONFIG_FIT)
>                              const char **fit_uname_config,
>                              const char **fit_uname_kernel
> #endif
> );
> ---
> include/image.h:432:1: error: expected declaration specifiers or ?...?
> before ?)? token
>  );
>  ^
>
> But if I use this:
>
> ----
> ulong genimg_get_kernel_addr(char * const img_addr
> #if defined(CONFIG_FIT)
>                              , const char **fit_uname_config,
>                              const char **fit_uname_kernel
> #endif
> );
> ----
> Then compiler is happy, but the style looks weird to me.
>
> This is for declaration but definition has the same problem.
>
> -Bryan
>
>
>>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
>>
>>
>>> +#define CONFIG_FIT
>>
>>
>> I think that crept in by mistake.
>>
>>
>>> diff --git a/include/image.h b/include/image.h
>>> index ca2fe86..a47c146 100644
>>
>>
>>> +#if defined(CONFIG_FIT)
>>> +ulong genimg_get_kernel_addr(char * const img_addr,
>>> +                            const char **fit_uname_config,
>>> +                            const char **fit_uname_kernel);
>>> +#else
>>>   ulong genimg_get_kernel_addr(char * const img_addr);
>>> +#endif
>>
>>
>> Same comment about #ifdef placement here.

Regards,
Simon

[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Bryan Wu]

On Fri, Aug 15, 2014 at 12:33 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 15 August 2014 12:25, Bryan Wu <cooloney@gmail.com> wrote:
>> On Fri, Aug 15, 2014 at 11:04 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 08/15/2014 11:49 AM, Bryan Wu wrote:
>>>>
>>>> Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
>>>
>>>
>>> Can we spell out the commit description too, so it's easier to know what it
>>> refers too, and is useful if someone cherry-picks the commit so the commit
>>> ID changes:
>>>
>>> Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
>>>
>>>
>>>> booting FIT image. It's because calling fit_parse_config() twice will
>>>> give us wrong value in img_addr.
>>>>
>>>> Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and
>>>> return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
>>>
>>>
>>>> diff --git a/common/image.c b/common/image.c
>>>
>>>
>>>> -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;
>>>> +ulong genimg_get_kernel_addr(char * const img_addr,
>>>> +                            const char **fit_uname_config,
>>>> +                            const char **fit_uname_kernel)
>>>> +#else
>
> Let's avoid these #ifdefs inside a function signature, just too ugly.
> If the extra arguments are a big problem you could create a new
> function perhaps?
>

OK, I will try to define a new function.

> Also, how about updating test/image/fit.py to test this behaviour?
> Then we can catch the bug.
>

York, I might need your information to add some test in the test/image/fit.py.
So a) you download a FIT image to somewhere
     b) what's kind of FIT image you're testing? how many configs?
     c) and you don't have load_addr?

-Bryan

>>>> +ulong genimg_get_kernel_addr(char * const img_addr)
>>>>   #endif
>>>
>>>
>>> Indentation looks wrong on that #endif.
>>>
>>> Wouldn't it be better to avoid repeating the common parts, so this instead:
>>>
>>> ulong genimg_get_kernel_addr(char * const img_addr,
>>> #if defined(CONFIG_FIT)
>>>                              const char **fit_uname_config,
>>>                              const char **fit_uname_kernel)
>>> #endif
>>> )
>>> {
>>>
>>
>> I got compiling error like this when I try this method then I moved to
>> 2 current one:
>>
>> ---
>> ulong genimg_get_kernel_addr(char * const img_addr,
>> #if defined(CONFIG_FIT)
>>                              const char **fit_uname_config,
>>                              const char **fit_uname_kernel
>> #endif
>> );
>> ---
>> include/image.h:432:1: error: expected declaration specifiers or ?...?
>> before ?)? token
>>  );
>>  ^
>>
>> But if I use this:
>>
>> ----
>> ulong genimg_get_kernel_addr(char * const img_addr
>> #if defined(CONFIG_FIT)
>>                              , const char **fit_uname_config,
>>                              const char **fit_uname_kernel
>> #endif
>> );
>> ----
>> Then compiler is happy, but the style looks weird to me.
>>
>> This is for declaration but definition has the same problem.
>>
>> -Bryan
>>
>>
>>>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
>>>
>>>
>>>> +#define CONFIG_FIT
>>>
>>>
>>> I think that crept in by mistake.
>>>
>>>
>>>> diff --git a/include/image.h b/include/image.h
>>>> index ca2fe86..a47c146 100644
>>>
>>>
>>>> +#if defined(CONFIG_FIT)
>>>> +ulong genimg_get_kernel_addr(char * const img_addr,
>>>> +                            const char **fit_uname_config,
>>>> +                            const char **fit_uname_kernel);
>>>> +#else
>>>>   ulong genimg_get_kernel_addr(char * const img_addr);
>>>> +#endif
>>>
>>>
>>> Same comment about #ifdef placement here.
>
> Regards,
> Simon