On Wed Oct 8, 2025 at 12:20 PM UTC, Evgeny Bachinin wrote: > <...> >> diff --git a/arch/arm/include/asm/arch-meson/boot.h >> b/arch/arm/include/asm/arch-meson/boot.h >> index >> a11dfde719e3e48e10bcb1f6b1b84eb8586ca9e7..e66b45983fe2f5d81f83642c01502a72773213b2 >> 100644 >> --- a/arch/arm/include/asm/arch-meson/boot.h >> +++ b/arch/arm/include/asm/arch-meson/boot.h >> @@ -21,6 +21,8 @@ int meson_get_boot_device(void); >> >> int meson_get_soc_rev(char *buff, size_t buff_len); >> >> +void meson_setup_capsule(void); >> + > > minor: maybe, add kernel-doc description ? > (up to you & maintainers) > >> /** >> * meson_get_socinfo - retrieve cpu_id of the Amlogic SoC >> * >> diff --git a/arch/arm/mach-meson/Makefile b/arch/arm/mach-meson/Makefile >> index >> 535b0878b9105e7a83729bea65fa4cd70cd4beac..640921e2b8e9b4777f0b212991edea26de2987d4 >> 100644 >> --- a/arch/arm/mach-meson/Makefile >> +++ b/arch/arm/mach-meson/Makefile >> @@ -2,7 +2,7 @@ >> # >> # Copyright (c) 2016 Beniamino Galvani <[email protected]> >> >> -obj-y += board-common.o sm.o board-info.o >> +obj-y += board-common.o sm.o board-info.o capsule.o >> obj-$(CONFIG_MESON_GX) += board-gx.o >> obj-$(CONFIG_MESON_AXG) += board-axg.o >> obj-$(CONFIG_MESON_G12A) += board-g12a.o >> diff --git a/arch/arm/mach-meson/board-common.c >> b/arch/arm/mach-meson/board-common.c >> index >> 39774c43049a40ed11578086603717571bedd23b..df9f576e7d2a1ce77b9a98259de4c3abf87784ed >> 100644 >> --- a/arch/arm/mach-meson/board-common.c >> +++ b/arch/arm/mach-meson/board-common.c >> @@ -145,6 +145,9 @@ int board_late_init(void) >> { >> meson_set_boot_source(); >> >> + /* Generate dfu_string for EFI capsule updates */ >> + meson_setup_capsule(); >> + > > How many AMlogic boards in the wild support DFU by default and 'EFI > capsule updates'? > > I assume the answer: "not all boards", because meson arch does not > select CONFIG_DFU (or similar) for all meson-boards by default in > related Kconfig files. CMIIW, please. >
As far as I know, many of the Amlogic boards' defconfigs enable CONFIG_DFU... > board-common.c - is a common file for any Amlogic board, hence any > board-specific addition must be at least under ifdef guards. > But ifdefs suggest that something wrong is here. > ... but just to make sure, I think guarding this inside an #ifdef is a good idea, just in case. > From architecture p.o.v the board specific code should go in a strong > implementation of, say, meson_board_late_init(), residing in your > board_specific.c, where board_specific.c can be actually common for all > your Libre Computer boards. > The goal of this patch is to make it unnecessary to add "board-specific" code to support EFI capsule updates, which, chances are, would be mostly copy-paste. So that just enabling the required kconfigs would make things work OOTB. > P.S> sorry, if my assumption about question above is wrong. > No worries, I'm glad you took the time to review my patch in any case. :) > >> return meson_board_late_init(); >> } >> >> diff --git a/arch/arm/mach-meson/capsule.c b/arch/arm/mach-meson/capsule.c >> new file mode 100644 >> index >> 0000000000000000000000000000000000000000..a798f8a3e8c3a8807720af4ef89ccfc20f22f2f8 >> --- /dev/null >> +++ b/arch/arm/mach-meson/capsule.c >> @@ -0,0 +1,59 @@ >> +// SPDX-License-Identifier: GPL-2.0+ >> +/* >> + * Copyright (c) 2025, Ferass El Hafidi <[email protected]> >> + */ >> + >> +#include <dm.h> >> +#include <asm/arch/boot.h> >> +#include <efi_loader.h> >> +#include <mmc.h> > > According to coding style, ABC order is needed here > https://docs.u-boot.org/en/latest/develop/codingstyle.html#include-files > Overlooked, sorry! I'll fix this for next revision >> + >> +/* >> + * To be able to support multiple devices and flash to the correct one we >> need >> + * to automatically generate the dfu_string and fw_name to match the device >> we >> + * are booted from. This is done by meson_setup_capsule() which is then >> called >> + * in board_late_init(). Right now we support EFI capsule updates on SPI >> flash, > > Extra space after `in board_late_init(). Right...` > and `booted from. This...` > As far as I know it's common in english to separate sentences with two spaces. So that's what I did there. >> + * eMMC and SD cards. >> + */ >> +struct efi_fw_image fw_images[] = { >> + { >> + .image_index = 1, >> + }, >> +}; >> + >> +struct efi_capsule_update_info update_info = { >> + .dfu_string = NULL, /* to be set in meson_capsule_setup */ >> + .num_images = ARRAY_SIZE(fw_images), >> + .images = fw_images, >> +}; >> + >> +/* >> + * TODO: Support usecase e.g. FIT image on eMMC + SPL on SD. >> + */ >> +void meson_setup_capsule(void) >> +{ >> + static char dfu_string[32] = { 0 }; >> + int mmc_devnum = 0; /* mmc0 => SD card */ >> + static uint32_t max_size = 0x2000; /* 4 MB */ > > 1) JFYI, checkpatch warns: > ``` > CHECK: Prefer kernel type 'u32' over 'uint32_t' > #125: FILE: arch/arm/mach-meson/capsule.c:37: > + static uint32_t max_size = 0x2000; /* 4 MB */ > ``` > I think uint32_t is fine here, but I can use u32 instead. > 2) BTW, why is it static? Looks like it could be just local var > > 3) 0x2000 does not match 4 MB or I did not get. > 0x2000 is 4 MB in MMC sectors. If you multiply it by 512 bytes, you will get 4 MB. (I should mention that in the comment, this is only used for eMMC/SD flashing) > >> + uint32_t offset = 0x1; /* offset for flashing to eMMC/SD */ > > > ``` > CHECK: Prefer kernel type 'u32' over 'uint32_t' > #126: FILE: arch/arm/mach-meson/capsule.c:38: > + uint32_t offset = 0x1; /* offset for flashing to eMMC/SD */ > ``` > > >> + int boot_device = meson_get_boot_device(); >> + >> + switch (boot_device) { >> + case BOOT_DEVICE_EMMC: >> + mmc_devnum = 1; /* mmc1 is always eMMC */ > > Is it intentional omission of `break;` here? If yes, I'd suggest to > explicitly write (for readability): /* fall through */ > Here it is intentional, yes. I should make that clearer. > >> + case BOOT_DEVICE_SD: >> + snprintf(dfu_string, 32, "mmc %d=u-boot.bin raw %d %d", >> mmc_devnum, offset, max_size); >> + fw_images[0].fw_name = u"U_BOOT_MESON_MMC"; >> + break; >> + case BOOT_DEVICE_SPI: >> + /* We assume there's only one SPI flash */ >> + fw_images[0].fw_name = u"U_BOOT_MESON_SPI"; >> + snprintf(dfu_string, 32, "sf 0:0=u-boot.bin raw 0 %d", >> max_size); >> + break; >> + default: >> + debug("Boot device %d unsupported\n", boot_device); > > minor: > is it intended to execute below code in a case when boot device is > unsupported? may be just return, here? or `/* fall through */` ? > I could return there, but this is quite minor, because if there's no return there it'll just do `update_info.dfu_string = NULL` essentially. But for clarity I'll add a return on the next revision. Best regards, Ferass
