Hi Tom,
Sorry for being late..
On 2016-1-26 3:07, Tom Rini wrote:
> 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...
> So, I've poked aruond a bit more on some of my TI platforms at least.
> Can you look at the following things on yours?
> 1) Do you still see the problem on top of tree? Masahiro fixed at least
> one problem just before v2016.01
Yes, after rebase to tag "v2016.01-rc4", i could still repro it with the
normal bootm cmd.
It seems we would call boot_ramdisk_high if fdt enabled in the prep stage:
vi common/image.c
#ifdef CONFIG_LMB
int image_setup_linux(bootm_headers_t *images)
{
...
if (IMAGE_ENABLE_RAMDISK_HIGH) {
rd_len = images->rd_end - images->rd_start;
ret = boot_ramdisk_high(lmb, images->rd_start, rd_len,
initrd_start, initrd_end);
if (ret)
return ret;
}
vi arch/arm/lib/bootm.c
/* Subcommand: PREP */
static void boot_prep_linux(bootm_headers_t *images)
if (IMAGE_ENABLE_OF_LIBFDT && images->ft_len) {
#ifdef CONFIG_OF_LIBFDT
debug("using: FDT\n");
if (image_setup_linux(images)) {
printf("FDT creation failed! hanging...");
hang();
}
#endif
And as Daniel said, these would "leads to a different behaviour when
calling bootm as a single command and calling bootm with sub-steps."
If IMAGE_ENABLE_OF_LIBFDT is enabled & images->ft_len is not zero,
boot_ramdisk_high would be called twice with splited bootm cmds. And in
our kylin case (IMAGE_ENABLE_OF_LIBFDT is disabled), the
boot_ramdisk_high would not be called with normal bootm cmd :(
> 2) Instead of fdt_high / initrd_high can you look at bootm_low /
> bootm_size ? include/configs/ti_armv7_common.h has these along with a
> big comment about what the overall constraints are.
On current kylin board, we're not using fdt_high or initrd_high.
The bootm_low / bootm_size would be CONFIG_SYS_SDRAM_BASE /
gd->bd->bi_dram[0].size by default(if not define those env), and arm
arch will reserve 4K at the end for stack, so the ramdisk would be
relocated to around (dram_end - 4K) :)
> Thanks!
>
