On Mon, 6 Apr 2020 at 19:25, Peter Collingbourne <p...@google.com> wrote:
> On Mon, Apr 6, 2020 at 10:40 AM Ryan Harkin <ryan.har...@linaro.org>
> wrote:
>
>> Hi Peter,
>>
>> This looks good to me, but I have a quick question below.
>>
>> On Sat, 4 Apr 2020 at 03:58, Peter Collingbourne <p...@google.com> wrote:
>>
>>> FVP now loads an Android boot image named boot.img if available,
>>> otherwise it falls back to the existing code path.
>>>
>>> Signed-off-by: Peter Collingbourne <p...@google.com>
>>> ---
>>> configs/vexpress_aemv8a_semi_defconfig | 2 ++
>>> include/configs/vexpress_aemv8a.h | 30 +++++++++++++++++---------
>>> 2 files changed, 22 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/configs/vexpress_aemv8a_semi_defconfig
>>> b/configs/vexpress_aemv8a_semi_defconfig
>>> index f31baab197..b52c761dee 100644
>>> --- a/configs/vexpress_aemv8a_semi_defconfig
>>> +++ b/configs/vexpress_aemv8a_semi_defconfig
>>> @@ -14,6 +14,8 @@ CONFIG_BOOTARGS="console=ttyAMA0
>>> earlycon=pl011,0x1c090000 debug user_debug=31 l
>>> # CONFIG_DISPLAY_CPUINFO is not set
>>> # CONFIG_DISPLAY_BOARDINFO is not set
>>> CONFIG_SYS_PROMPT="VExpress64# "
>>> +CONFIG_ANDROID_BOOT_IMAGE=y
>>> +CONFIG_CMD_ABOOTIMG=y
>>> # CONFIG_CMD_CONSOLE is not set
>>> # CONFIG_CMD_XIMG is not set
>>> # CONFIG_CMD_EDITENV is not set
>>> diff --git a/include/configs/vexpress_aemv8a.h
>>> b/include/configs/vexpress_aemv8a.h
>>> index 9a9cec414c..4f3a792f49 100644
>>> --- a/include/configs/vexpress_aemv8a.h
>>> +++ b/include/configs/vexpress_aemv8a.h
>>> @@ -177,16 +177,26 @@
>>> "initrd_addr=0x88000000\0" \
>>> "fdtfile=devtree.dtb\0" \
>>> "fdt_addr=0x83000000\0" \
>>> - "fdt_high=0xffffffffffffffff\0" \
>>> - "initrd_high=0xffffffffffffffff\0"
>>>
>>
>> Why did you move these two inside the 'if' statement below? Is it because
>> you explicitly don't want them set when booting Android?
>>
>
> Yes. We can't have these set when loading an Android boot image because
> they instruct the bootloader to use the device tree/initrd in place instead
> of copying them to another location, and since we're already using the
> kernel in place this may result in the kernel overwriting the device tree
> or initrd when it initializes its own BSS since they appear right after the
> kernel in the boot image format.
>
Ok, thanks for the clarification. That's fine by me.
Reviewed-by: Ryan Harkin <ryan.har...@linaro.org>
>
> Peter
>
>>
>>
>>
>>> -
>>> -#define CONFIG_BOOTCOMMAND "smhload ${kernel_name} ${kernel_addr};
>>> " \
>>> - "smhload ${fdtfile} ${fdt_addr}; " \
>>> - "smhload ${initrd_name} ${initrd_addr} "\
>>> - "initrd_end; " \
>>> - "fdt addr ${fdt_addr}; fdt resize; " \
>>> - "fdt chosen ${initrd_addr}
>>> ${initrd_end}; " \
>>> - "booti $kernel_addr - $fdt_addr"
>>> + "boot_name=boot.img\0" \
>>> + "boot_addr=0x8007f800\0"
>>> +
>>> +#define CONFIG_BOOTCOMMAND "if smhload ${boot_name} ${boot_addr};
>>> then " \
>>> + " set bootargs; " \
>>> + " abootimg addr ${boot_addr}; " \
>>> + " abootimg get dtb --index=0 fdt_addr;
>>> " \
>>> + " bootm ${boot_addr} ${boot_addr} " \
>>> + " ${fdt_addr}; " \
>>> + "else; " \
>>> + " set fdt_high 0xffffffffffffffff; " \
>>> + " set initrd_high 0xffffffffffffffff; "
>>> \
>>> + " smhload ${kernel_name}
>>> ${kernel_addr}; " \
>>> + " smhload ${fdtfile} ${fdt_addr}; " \
>>> + " smhload ${initrd_name} ${initrd_addr}
>>> "\
>>> + " initrd_end; " \
>>> + " fdt addr ${fdt_addr}; fdt resize; " \
>>> + " fdt chosen ${initrd_addr}
>>> ${initrd_end}; " \
>>> + " booti $kernel_addr - $fdt_addr; " \
>>> + "fi"
>>>
>>>
>>> #endif
>>> --
>>> 2.26.0.292.g33ef6b2f38-goog
>>>
>>>
