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
