On Fri, Jan 15, 2016 at 04:42:21PM +0100, Daniel Schwierzeck wrote:

Am Freitag, den 15.01.2016, 09:42 -0500 schrieb Tom Rini:

quoted

On Fri, Jan 15, 2016 at 10:20:43AM +0800, Jeffy Chen wrote:

quoted

Hi Tom,

On 2016-1-15 8:59, Tom Rini wrote:

quoted

On Fri, Jan 15, 2016 at 08:53:06AM +0800, Jeffy Chen wrote:

quoted

Hi Tom,

On 2016-1-15 0:22, Tom Rini wrote:

quoted

On Thu, Jan 14, 2016 at 10:31:34AM +0800, Jeffy Chen wrote:

quoted

Hi Tom,

On 2016-1-13 23:28, Tom Rini wrote:

quoted

On Wed, Jan 13, 2016 at 04:53:19PM +0800, Jeffy Chen
wrote:

quoted

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 <redacted>
Acked-by: Simon Glass <sjg@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...

quoted

quoted

+#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.

Yes, I bet ARM and PowerPC and SPARC and m68k are broken now.

quoted

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

There's certainly room to make the whole bootm sequence shared better.
But I also see now that in the cleanup we moved from always calling
bootm_ramdisk_high to, well, not always calling it in all cases as you
found (but it looks like it should have been, but isn't).  I'm trying to
wrap my head around how to fix this.  Thanks!

-- 
Tom