[U-Boot] [PATCH] image: fix bootm failure for FIT image
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);
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
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
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
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.
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.
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
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
participants (4)
-
Bryan Wu -
Simon Glass -
Stephen Warren -
York Sun