Hi Mattijs, Thank you for your review.
ср, 22 мая 2024 г. в 17:43, Mattijs Korpershoek <mkorpersh...@baylibre.com>: > > Hi Roman, > > Thank you for the patch. > > On dim., mai 19, 2024 at 12:53, Roman Stratiienko <r.stratiie...@gmail.com> > wrote: > > > Quote from [1]: > > > > "For devices launching with Android 13, the generic ramdisk is removed > > from the boot image and placed in a separate init_boot image. > > This change leaves the boot image with only the GKI kernel." > > > > [1]: > > https://source.android.com/docs/core/architecture/partitions/generic-boot > > Signed-off-by: Roman Stratiienko <r.stratiie...@gmail.com> > > --- > > boot/image-board.c | 5 ++++- > > cmd/abootimg.c | 26 +++++++++++++++++++++----- > > include/image.h | 7 +++++++ > > 3 files changed, 32 insertions(+), 6 deletions(-) > > This patch does not apply on: > - next: 7d24c3e06fa9 ("Merge patch series "scripts/setlocalversion sync with > linux 6.9"") > - master: a7f0154c4128 ("Prepare v2024.07-rc3") > > Please consider rebasing when resending. V2 Rebased > > More review below > > > > > diff --git a/boot/image-board.c b/boot/image-board.c > > index 75f6906cd5..715654ff01 100644 > > --- a/boot/image-board.c > > +++ b/boot/image-board.c > > @@ -414,9 +414,12 @@ static int select_ramdisk(struct bootm_headers > > *images, const char *select, u8 a > > int ret; > > if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) { > > void *boot_img = > > map_sysmem(get_abootimg_addr(), 0); > > + void *init_boot_img = > > map_sysmem(get_ainit_bootimg_addr(), 0); > > void *vendor_boot_img = > > map_sysmem(get_avendor_bootimg_addr(), 0); > > + void *ramdisk_img = (init_boot_img == -1) ? > > boot_img : > > + > > init_boot_img; > > This line introduces a build warning: > > ../boot/image-board.c: In function ‘select_ramdisk’: > ../boot/image-board.c:412:68: warning: comparison between pointer and integer > 412 | void *ramdisk_img = (init_boot_img == > -1) ? boot_img : > | ^~ > > Can we please fix it in v2 ? Fixed > > > > > - ret = android_image_get_ramdisk(boot_img, > > vendor_boot_img, > > + ret = android_image_get_ramdisk(ramdisk_img, > > vendor_boot_img, > > rd_datap, > > rd_lenp); > > unmap_sysmem(vendor_boot_img); > > Can we please add unmap_sysmem(init_boot_img); here as well ? Done. > > > unmap_sysmem(boot_img); > > diff --git a/cmd/abootimg.c b/cmd/abootimg.c > > index e68c523705..7c450a3b2d 100644 > > --- a/cmd/abootimg.c > > +++ b/cmd/abootimg.c > > @@ -17,6 +17,7 @@ > > > > /* Please use abootimg_addr() macro to obtain the boot image address */ > > static ulong _abootimg_addr = -1; > > +static ulong _ainit_bootimg_addr = -1; > > static ulong _avendor_bootimg_addr = -1; > > > > ulong get_abootimg_addr(void) > > @@ -24,6 +25,11 @@ ulong get_abootimg_addr(void) > > return (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr); > > } > > > > +ulong get_ainit_bootimg_addr(void) > > +{ > > + return _ainit_bootimg_addr; > > +} > > + > > ulong get_avendor_bootimg_addr(void) > > { > > return _avendor_bootimg_addr; > > @@ -209,7 +215,7 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int > > flag, int argc, > > char *endp; > > ulong img_addr; > > > > - if (argc < 2 || argc > 3) > > + if (argc < 2 || argc > 4) > > return CMD_RET_USAGE; > > > > img_addr = hextoul(argv[1], &endp); > > @@ -220,16 +226,26 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, > > int flag, int argc, > > > > _abootimg_addr = img_addr; > > > > - if (argc == 3) { > > + if (argc > 2) { > > img_addr = simple_strtoul(argv[2], &endp, 16); > > if (*endp != '\0') { > > - printf("Error: Wrong vendor image address\n"); > > + printf("Error: Wrong vendor_boot image address\n"); > > Nitpick: this seems like a harmless change but could be done in a > separate patch. > > To me, it's fine to include this hunk, but can we mention it in the > commit message? > > Something along the lines as "While at it, update wrong error handling > message when vendor_boot cannot be loaded". Added. > > > return CMD_RET_FAILURE; > > } > > > > _avendor_bootimg_addr = img_addr; > > } > > > > + if (argc == 4) { > > + img_addr = simple_strtoul(argv[3], &endp, 16); > > + if (*endp != '\0') { > > + printf("Error: Wrong init_boot image address\n"); > > + return CMD_RET_FAILURE; > > + } > > + > > + _ainit_bootimg_addr = img_addr; > > + } > > + > > return CMD_RET_SUCCESS; > > } > > > > @@ -346,7 +362,7 @@ fail: > > } > > > > static struct cmd_tbl cmd_abootimg_sub[] = { > > - U_BOOT_CMD_MKENT(addr, 3, 1, do_abootimg_addr, "", ""), > > + U_BOOT_CMD_MKENT(addr, 4, 1, do_abootimg_addr, "", ""), > > U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""), > > U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""), > > U_BOOT_CMD_MKENT(load, 5, 1, do_abootimg_load, "", ""), > > @@ -375,7 +391,7 @@ static int do_abootimg(struct cmd_tbl *cmdtp, int flag, > > int argc, > > U_BOOT_CMD( > > abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg, > > "manipulate Android Boot Image", > > - "addr <boot_img_addr> [<vendor_boot_img_addr>]>\n" > > + "addr <boot_img_addr> [<vendor_boot_img_addr> > > [<init_boot_img_addr>]]\n" > > " - set the address in RAM where boot image is located\n" > > " ($loadaddr is used by default)\n" > > Please include the help text update in the documentation: > doc/android/boot-image.rst That documentation does not mention 'abootimg addr' command at the moment. I do not see an easy way to add it in a way that makes sense. > > > "abootimg dump dtb\n" > > diff --git a/include/image.h b/include/image.h > > index be3c73a72e..b1fd6b3149 100644 > > --- a/include/image.h > > +++ b/include/image.h > > @@ -1982,6 +1982,13 @@ bool is_android_vendor_boot_image_header(const void > > *vendor_boot_img); > > */ > > ulong get_abootimg_addr(void); > > > > +/** > > + * get_ainit_bootimg_addr() - Get Android init boot image address > > + * > > + * Return: Android init boot image address > > + */ > > +ulong get_ainit_bootimg_addr(void); > > + > > /** > > * get_avendor_bootimg_addr() - Get Android vendor boot image address > > * > > -- > > 2.40.1