Patchwork [U-Boot] Fix bootm to work on powerpc again (compressed uImage)

login
register
mail settings
Submitter Stefan Roese
Date June 28, 2013, 8:02 a.m.
Message ID <1372406523-5726-1-git-send-email-sr@denx.de>
Download mbox | patch
Permalink /patch/255294/
State Accepted
Delegated to: Tom Rini
Headers show

Comments

Stefan Roese - June 28, 2013, 8:02 a.m.
Patch 35fc84fa1 [Refactor the bootm command to reduce code duplication]
breaks booting Linux (compressed uImage with fdt) on powerpc.

boot_jump_linux() mustn't be called before boot_prep_linux() and
boot_body_linux() have been called. So remove the superfluous call
to boot_jump_linux() in arch/powerpc/lib/bootm.c as its called later on
in this function.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@ti.com>
---
Simon, I'm not 100% sure why this is triggered by your patch.

As you see, my patch only fixes powerpc. ARM might have the same problem
as it has the same code duplication in bootm.c.

Thanks,
Stefan

 arch/powerpc/lib/bootm.c | 5 -----
 1 file changed, 5 deletions(-)
Tom Rini - June 28, 2013, 1:03 p.m.
On Fri, Jun 28, 2013 at 10:02:03AM +0200, Stefan Roese wrote:

> Patch 35fc84fa1 [Refactor the bootm command to reduce code duplication]
> breaks booting Linux (compressed uImage with fdt) on powerpc.
> 
> boot_jump_linux() mustn't be called before boot_prep_linux() and
> boot_body_linux() have been called. So remove the superfluous call
> to boot_jump_linux() in arch/powerpc/lib/bootm.c as its called later on
> in this function.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@ti.com>
> ---
> Simon, I'm not 100% sure why this is triggered by your patch.
> 
> As you see, my patch only fixes powerpc. ARM might have the same problem
> as it has the same code duplication in bootm.c.

Progress!  On PowerPC, does bootm work like before?  I still have to
issue a separate 'bootm go', but that now boots, rather than hangs.
Stefan Roese - June 28, 2013, 1:11 p.m.
On 06/28/2013 03:03 PM, Tom Rini wrote:
>> Patch 35fc84fa1 [Refactor the bootm command to reduce code duplication]
>> breaks booting Linux (compressed uImage with fdt) on powerpc.
>>
>> boot_jump_linux() mustn't be called before boot_prep_linux() and
>> boot_body_linux() have been called. So remove the superfluous call
>> to boot_jump_linux() in arch/powerpc/lib/bootm.c as its called later on
>> in this function.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@ti.com>
>> ---
>> Simon, I'm not 100% sure why this is triggered by your patch.
>>
>> As you see, my patch only fixes powerpc. ARM might have the same problem
>> as it has the same code duplication in bootm.c.
> 
> Progress!  On PowerPC, does bootm work like before?

Yes. I'm using the same environment (boot_cmd) as before. And without
this patch "bootm kernel_addr - fdt_addr' hangs. I might only affect  DT
booting, as without this patch boot_jump_linux() is called without
calling boot_prep_linux() and boot_body_linux() before. One of those (I
didn't check which one) is responsible for this line:

   Loading Device Tree to 007f8000, end 007ffdd4 ... OK

And this is missing when this patch is not applied.

Thanks,
Stefan
Simon Glass - June 28, 2013, 9:46 p.m.
Hi Stefan,

On Fri, Jun 28, 2013 at 1:02 AM, Stefan Roese <sr@denx.de> wrote:

> Patch 35fc84fa1 [Refactor the bootm command to reduce code duplication]
> breaks booting Linux (compressed uImage with fdt) on powerpc.
>
> boot_jump_linux() mustn't be called before boot_prep_linux() and
> boot_body_linux() have been called. So remove the superfluous call
> to boot_jump_linux() in arch/powerpc/lib/bootm.c as its called later on
> in this function.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@ti.com>
>

Acked-by: Simon Glass <sjg@chromium.org>


> ---
> Simon, I'm not 100% sure why this is triggered by your patch.
>

I'm very grateful for your time spent looking at this and getting to the
bottom of it!

Maybe because do_bootm_linux() is a funny function - you can call it with a
mask in which case it will do that step, but if you call it with 0 it will
do all steps. Seems like something that should be tidied up.


>
> As you see, my patch only fixes powerpc. ARM might have the same problem
> as it has the same code duplication in bootm.c.
>
> Thanks,
> Stefan
>
>  arch/powerpc/lib/bootm.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
> index dd6c98c..d4ad323 100644
> --- a/arch/powerpc/lib/bootm.c
> +++ b/arch/powerpc/lib/bootm.c
> @@ -256,11 +256,6 @@ int do_bootm_linux(int flag, int argc, char * const
> argv[], bootm_headers_t *ima
>                 return 0;
>         }
>
> -       if (flag & BOOTM_STATE_OS_GO) {
> -               boot_jump_linux(images);
> -               return 0;
> -       }
> -
>         boot_prep_linux(images);
>         ret = boot_body_linux(images);
>         if (ret)
> --
> 1.8.3.1
>
>
Regards,
Simon
Tom Rini - June 28, 2013, 10:08 p.m.
On Fri, Jun 28, 2013 at 10:02:03AM +0200, Stefan Roese wrote:

> Patch 35fc84fa1 [Refactor the bootm command to reduce code duplication]
> breaks booting Linux (compressed uImage with fdt) on powerpc.
> 
> boot_jump_linux() mustn't be called before boot_prep_linux() and
> boot_body_linux() have been called. So remove the superfluous call
> to boot_jump_linux() in arch/powerpc/lib/bootm.c as its called later on
> in this function.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@ti.com>

Applied to u-boot/master, thanks!

Patch

diff --git a/arch/powerpc/lib/bootm.c b/arch/powerpc/lib/bootm.c
index dd6c98c..d4ad323 100644
--- a/arch/powerpc/lib/bootm.c
+++ b/arch/powerpc/lib/bootm.c
@@ -256,11 +256,6 @@  int do_bootm_linux(int flag, int argc, char * const argv[], bootm_headers_t *ima
 		return 0;
 	}
 
-	if (flag & BOOTM_STATE_OS_GO) {
-		boot_jump_linux(images);
-		return 0;
-	}
-
 	boot_prep_linux(images);
 	ret = boot_body_linux(images);
 	if (ret)