[U-Boot] [PATCH v4 5/6] rockchip: kylin: Enable boot with android boot image

[Simon Glass]

Hi,

On 15 January 2016 at 18:18, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 15 January 2016 at 08:42, Daniel Schwierzeck
> <daniel.schwierzeck at gmail.com> wrote:
>> Am Freitag, den 15.01.2016, 09:42 -0500 schrieb Tom Rini:
>>> On Fri, Jan 15, 2016 at 10:20:43AM +0800, Jeffy Chen wrote:
>>> > Hi Tom,
>>> >
>>> > On 2016-1-15 8:59, Tom Rini wrote:
>>> > > On Fri, Jan 15, 2016 at 08:53:06AM +0800, Jeffy Chen wrote:
>>> > > > Hi Tom,
>>> > > >
>>> > > > On 2016-1-15 0:22, Tom Rini wrote:
>>> > > > > On Thu, Jan 14, 2016 at 10:31:34AM +0800, Jeffy Chen wrote:
>>> > > > > > Hi Tom,
>>> > > > > >
>>> > > > > > On 2016-1-13 23:28, Tom Rini wrote:
>>> > > > > > > On Wed, Jan 13, 2016 at 04:53:19PM +0800, Jeffy Chen
>>> > > > > > > wrote:
>>> > > > > > >
>>> > > > > > > > The android kernel is using appended dtb by default,
>>> > > > > > > > and store
>>> > > > > > > > ramdisk right after kernel & dtb.
>>> > > > > > > > So we needs to relocate ramdisk, and use atags to pass
>>> > > > > > > > params.
>>> > > > > > > >
>>> > > > > > > > Signed-off-by: Jeffy Chen <jeffy.chen at rock-chips.com>
>>> > > > > > > > Acked-by: Simon Glass <sjg at chromium.org>
>>> > > > > > > > ---
>>> > > > > > > >
>>> > > > > > > > Changes in v4: None
>>> > > > > > > > Changes in v3: None
>>> > > > > > > > Changes in v2: None
>>> > > > > > > >
>>> > > > > > > >  include/configs/kylin_rk3036.h | 23
>>> > > > > > > > +++++++++++++++++++++++
>>> > > > > > > >  1 file changed, 23 insertions(+)
>>> > > > > > > >
>>> > > > > > > > diff --git a/include/configs/kylin_rk3036.h
>>> > > > > > > > b/include/configs/kylin_rk3036.h
>>> > > > > > > > index b750b26..49997ec 100644
>>> > > > > > > > --- a/include/configs/kylin_rk3036.h
>>> > > > > > > > +++ b/include/configs/kylin_rk3036.h
>>> > > > > > > > @@ -35,6 +35,29 @@
>>> > > > > > > >  #undef CONFIG_EXTRA_ENV_SETTINGS
>>> > > > > > > >  #define CONFIG_EXTRA_ENV_SETTINGS \
>>> > > > > > > >         "partitions=" PARTS_DEFAULT \
>>> > > > > > > > +       "mmcdev=0\0" \
>>> > > > > > > > +       "mmcpart=5\0" \
>>> > > > > > > > +       "loadaddr=" __stringify(CONFIG_SYS_LOAD_ADDR)
>>> > > > > > > > "\0" \
>>> > > > > > > > +
>>> > > > > > > > +#define CONFIG_ANDROID_BOOT_IMAGE
>>> > > > > > > > +#define CONFIG_SYS_BOOT_RAMDISK_HIGH
>>> > > > > > > This should already be set.
>>> > > > > > Right, i'll remove it...
>>> > > > > > > > +#define CONFIG_SYS_HUSH_PARSER
>>> > > > > > > > +
>>> > > > > > > > +#undef CONFIG_BOOTCOMMAND
>>> > > > > > > > +#define CONFIG_BOOTCOMMAND \
>>> > > > > > > > +       "mmc dev ${mmcdev}; if mmc rescan; then " \
>>> > > > > > > > +               "part start mmc ${mmcdev} ${mmcpart}
>>> > > > > > > > boot_start;" \
>>> > > > > > > > +               "part size mmc ${mmcdev} ${mmcpart}
>>> > > > > > > > boot_size;" \
>>> > > > > > > > +               "mmc read ${loadaddr} ${boot_start}
>>> > > > > > > > ${boot_size};" \
>>> > > > > > > > +               "bootm start ${loadaddr}; bootm
>>> > > > > > > > ramdisk;" \
>>> > > > > > > > +               "bootm prep; bootm go;" \
>>> > > > > > > > +       "fi;" \
>>> > > > > > > > +
>>> > > > > > > > +/* Enable atags */
>>> > > > > > > > +#define CONFIG_SYS_BOOTPARAMS_LEN      (64*1024)
>>> > > > > > > > +#define CONFIG_INITRD_TAG
>>> > > > > > > > +#define CONFIG_SETUP_MEMORY_TAGS
>>> > > > > > > > +#define CONFIG_CMDLINE_TAG
>>> > > > > > > But I'm confused as to what exactly is going on here.
>>> > > > > > >  Appended dtb is
>>> > > > > > > not the same as ATAGS.  And you shouldn't need to split
>>> > > > > > > up bootm like
>>> > > > > > > that.  Can you please explain a bit more?  Thanks!
>>> > > > > > The u-boot will pass atags to kernel, and kernel will merge
>>> > > > > > those
>>> > > > > > atags into the appended dtb(fdt).
>>> > > > > >
>>> > > > > > The default bootm flow would not pass ramdisk state, but we
>>> > > > > > need it,
>>> > > > > > so we should add this state into default flow, or just use
>>> > > > > > split
>>> > > > > > bootm cmds :)
>>> > > > > That seems very strange.  Is the ramdisk concatenated with
>>> > > > > the kernel
>>> > > > > and dtb as well (and that's why bootm ramdisk somehow finds
>>> > > > > it but
>>> > > > > normal bootm doesn't as you aren't passing in a ramdisk
>>> > > > > address) ?
>>> > > > Yes, the ramdisk concatenated with the kernel and dtb as
>>> > > > well(u-boot/include/android_image.h: struct andr_img_hdr).
>>> > > >
>>> > > > And the normal bootm cmd would find it by parsing andr_img_hdr
>>> > > > struct.
>>> > > > But we still need bootm ramdisk state, because it will call
>>> > > > boot_ramdisk_high to relocate ramdisk area :)
>>> > > >
>>> > > > I found if not relocate it to somewhere else, it would be
>>> > > > corrupted
>>> > > > after kernel's decompressing(during update fdt area).
>>> > > So 'bootm $loadaddr' of an Android image sees, but does not
>>> > > relocate the
>>> > > ramdisk that is included in the image, but bootm ramdisk does?
>>> > >  That
>>> > > sounds like a bug in the regular bootm handling.
>>> > Yep, the default bootm flow would not contain ramdisk relocate
>>> > state:
>>> >
>>> > vi common/cmd_bootm.c
>>> >         return do_bootm_states(cmdtp, flag, argc, argv,
>>> > BOOTM_STATE_START |
>>> >                 BOOTM_STATE_FINDOS | BOOTM_STATE_FINDOTHER |
>>> >                 BOOTM_STATE_LOADOS |
>>> > #if defined(CONFIG_PPC) || defined(CONFIG_MIPS)
>>> >                 BOOTM_STATE_OS_CMDLINE |
>>> > #endif
>>> >                 BOOTM_STATE_OS_PREP | BOOTM_STATE_OS_FAKE_GO |
>>> >                 BOOTM_STATE_OS_GO, &images, 1);
>>> >
>>> > But i'm not sure if it's ok to add it here...
>>>
>>> Actually, maybe so.  This made me do some digging again back into
>>> Simon's series that refactored the bootm code.  In the long long ago,
>>> nearly every architecture would call bootm_ramdisk_high to relocate
>>> the
>>> ramdisk and set the environment (and poke the DT).  But it wasn't
>>> always
>>> clear when / why it would do this.  And it got consolidated.  And
>>> here's
>>> where it's tricky.  It looks correct today, still, to make sure that
>>> we
>>> relocate the ramdisk.  image_setup_linux() checks
>>> IMAGE_ENABLE_RAMDISK_HIGH which is toggled by
>>> CONFIG_SYS_BOOT_RAMDISK_HIGH.  But for example, MIPS whacked _back_
>>> in a
>>> local call to make sure the ramdisk was being relocated because it
>>> wasn't back in 2014 into boot_prep_linux.  It may even be related to
>>> some of the initrd rated things Masahiro has found recently.  So
>>> something is wrong down in this core code.
>>
>> one problem is the different behaviour of bootm between legacy uImages
>> and FIT uImages. In case of legacy uImages, the core code automatically
>> relocates initramfs and DTB. In case of FIT uImages, the arch-specific
>> do_bootm_linux() still has to do that. This was only somehow addressed
>> by adding the helper function image_setup_linux() and calling it from
>> do_bootm_linux(). But you can't use that function with legacy uImages,
>> because initramfs and DTB would be relocated twice because the states
>> BOOTM_STATE_RAMDISK and BOOTM_STATE_FDT are not checked. Because of
>> that image_setup_linux() also leads to a different behaviour when
>> calling bootm as a single command and calling bootm with sub-steps.
>>
>> So the current bootm implementation on MIPS is the only way for me to
>> make bootm working with all possible combinations of legacy/FIT uImage,
>> with/without initramfs, with/without DTB and single/multiple bootm
>> calls. I suspect that the current implementation on ARM does not work
>> with all possible combinations.
>>
>>> Jeffy, can you try and debug
>>> this a bit since you have a failing case in front of you?  Otherwise
>>> I'll put it on my TODO list.  Thanks!
>>>
>>
>> Some ideas for a possible refactoring of the bootm core code:
>> - remove arch-specific do_bootm_linux() and image_setup_linux()
>> - rename functions like boot_prep_linux() and boot_jump_linux() to
>> arch_boot_prep_linux() and arch_boot_jump_linux() and declare them as
>> __weak and add a default implementation, the arch code can overwrite
>> them
>> - refactor the core code to call those functions at the appropriate
>> locations
>
> Perhaps we need a more formal API between bootm and the arch-specific
> implementation of the various pieces.
>
> Also it would be great to add more tests for bootm. I made a start
> based on what I could figure out about the behaviour
> (tests/image/test-fit.py), but we should do more.
>
> Regards,
> Simon

I'm going to hold off on this patch as there seems to be some other root cause.

Regards,
Simon