| Message ID | 20200404025824.66971-1-pcc@google.com |
|---|---|
| State | Accepted |
| Commit | ec8eef5e71e4d876324aa81b3c394822d8ef1eb6 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | fvp: Add support for loading Android boot images via semihosting | expand |
On Mon, Apr 6, 2020 at 10:40 AM Ryan Harkin <ryan.harkin@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 <pcc@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 <pcc@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. 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 >> >>
On Mon, 6 Apr 2020 at 19:25, Peter Collingbourne <pcc@google.com> wrote: > On Mon, Apr 6, 2020 at 10:40 AM Ryan Harkin <ryan.harkin@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 <pcc@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 <pcc@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.harkin@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 >>> >>>
On Mon, Apr 6, 2020 at 11:30 AM Ryan Harkin <ryan.harkin@linaro.org> wrote: > > On Mon, 6 Apr 2020 at 19:25, Peter Collingbourne <pcc@google.com> wrote: > >> On Mon, Apr 6, 2020 at 10:40 AM Ryan Harkin <ryan.harkin@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 <pcc@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 <pcc@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.harkin@linaro.org> > Thanks for the review! Do you know what is the next step for getting this patch landed in master? I read https://www.denx.de/wiki/U-Boot/Patches but unfortunately I did not get a clear idea of what the next step is. Peter > >> >> 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 >>>> >>>>
On Tue, Apr 14, 2020 at 10:11:06AM -0700, Peter Collingbourne wrote: > On Mon, Apr 6, 2020 at 11:30 AM Ryan Harkin <ryan.harkin@linaro.org> wrote: > > > > > On Mon, 6 Apr 2020 at 19:25, Peter Collingbourne <pcc@google.com> wrote: > > > >> On Mon, Apr 6, 2020 at 10:40 AM Ryan Harkin <ryan.harkin@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 <pcc@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 <pcc@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.harkin@linaro.org> > > > > Thanks for the review! Do you know what is the next step for getting this > patch landed in master? I read https://www.denx.de/wiki/U-Boot/Patches but > unfortunately I did not get a clear idea of what the next step is. It's on me to pick up and I will soon, thanks!
On Sat, Apr 4, 2020 at 4:58 AM Peter Collingbourne <pcc@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 <pcc@google.com> That's very helpful when doing Android development. Thanks for doing this. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Fri, Apr 03, 2020 at 07:58:24PM -0700, Peter Collingbourne 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 <pcc@google.com> > Reviewed-by: Ryan Harkin <ryan.harkin@linaro.org> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Applied to u-boot/master, thanks!
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" - -#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
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 <pcc@google.com> --- configs/vexpress_aemv8a_semi_defconfig | 2 ++ include/configs/vexpress_aemv8a.h | 30 +++++++++++++++++--------- 2 files changed, 22 insertions(+), 10 deletions(-)