Hej Casey, On Friday, 5 June 2026 at 1:01 AM, Casey Connolly <[email protected]> wrote:
> > > On 6/4/26 00:27, Sam Day wrote: > > Heya Casey, > > > > On Wednesday, 3 June 2026 at 11:47 PM, Casey Connolly > > <[email protected]> wrote: > > > >> Hi Sam, > >> > >> (CC list definitely needs some tuning on the next revision XD) > > > > Hmm? Is it incorrect to use `b4 prep --auto-to-cc` nowadays? > > Urgh yeah I think this is a U-Boot issue with MAINTAINERS, needs some > improvement but not your fault. > > > >> > >> On 6/1/26 10:12, Sam Day via B4 Relay wrote: > >>> From: Sam Day <[email protected]> > >>> > >>> Many (often fused) bootloaders on older ARMv7 qcom SoCs do not support > >>> FDTs at all, and instead pass along ATAGS in r2. > >>> > >>> Minimal support for parsing memory info out of these tags is introduced, > >>> so that we can support mainline devicetrees, which expect the bootloader > >>> to populate /memory nodes. > >> > >> I think this should go its own armv7-specific file. It would probably > >> also make sense to just have a separate implementation of > >> board_fdt_blob_setup() since it's already really quite complicated. > > > > I was considering that as well but decided against it, since sprinkling > > #ifdef > > everywhere is quite ugly, and the ATAGS handling code is quite minimal. Are > > you > > sure it really needs to be split out? > > I would have the two implementations in different files > > > >> > >> The SMEM series will do a bit of rework, in the next spin I'll probably > >> move the caching stuff out as well, maybe I should also take > >> board_fdt_blob_setup() then the file can be arm64 specific? > > > > I'm not sure how we can have a separate version of board_fdt_blob_setup. On > > ARMv7, sometimes you'll br receiving ATAGS in r2, sometimes it'll be FDT. It > > depends on which bootloader you're chaining from. > > I see, can we impose any limitations on this in practise? I'm not sure what you mean here. If we want U-Boot to support chaining from fused bootloaders on ARMv7 qcom devices, then we need to meet them where they're at. > If not then I > guess this is fine. I'd like to see all this logic cleaned up a bit as > I'm just worried about introducing bugs FWIW, I share your concern (remember that my fingerprints are on this unhinged function too xD) and I spent quite a bit of time looking at this before proposing it. I also tested it on sdm845/sdm670. Happy to address any further specific concerns you have, though. Otherwise, it's starting to look like maybe we should abandon the "big tent" aspirations? I have no issues with respinning this series to introduce a separate mach-snapdragon-armv7 to avoid any potential breakages for newer hardware. As you can see, these older SoCs are different enough from "modern qcom" that we're not really gaining much by co-habitating everything. > > At the very least I'd like it if the atags parsing was moved to a > different file then here we can do if > (CONFIG_IS_ENABLED(...SNAPDRAGON_ARM32) > qcom_parse_atags() I've queued up that change for v2. Note that it's still slightly messy. The anonymous prevbl_ddr_banks struct definition and ddr_bank_cmp prototype need to be hoisted into qcom-priv.h so that atags.c can use them. > > the compiler will optimise out the function call so there's no need for > a stub version Huh, TIL. That's very useful to know thanks :D Peace, -Sam > > > > > e.g my nokia-fame can run U-Boot directly chained from SBL3 since the > > bootchain > > on this device has been broken enough, in which case U-Boot gets ATAGS. My > > samsung-expressltexx chains from lk2nd, in which case U-Boot gets an FDT. > > > > Cheers, > > -Sam > > > >> > >>> > >>> Signed-off-by: Sam Day <[email protected]> > >>> --- > >>> arch/arm/mach-snapdragon/board.c | 119 > >>> ++++++++++++++++++++++++++++++--------- > >>> 1 file changed, 91 insertions(+), 28 deletions(-) > >>> > >>> diff --git a/arch/arm/mach-snapdragon/board.c > >>> b/arch/arm/mach-snapdragon/board.c > >>> index 627b566d178..753983467a0 100644 > >>> --- a/arch/arm/mach-snapdragon/board.c > >>> +++ b/arch/arm/mach-snapdragon/board.c > >>> @@ -11,6 +11,7 @@ > >>> > >>> #include <asm/gpio.h> > >>> #include <asm/io.h> > >>> +#include <asm/setup.h> > >>> #include <asm/system.h> > >>> #include <dm/device.h> > >>> #include <dm/pinctrl.h> > >>> @@ -175,6 +176,67 @@ static int qcom_parse_memory(const void *fdt) > >>> return 0; > >>> } > >>> > >>> +static bool qcom_atags_valid(const phys_addr_t p) > >>> +{ > >>> + const struct tag *tags = (const struct tag *)p; > >>> + > >>> + return tags && tags->hdr.tag == ATAG_CORE && > >>> + tags->hdr.size >= sizeof(struct tag_header) / sizeof(u32); > >>> +} > >>> + > >>> +static int qcom_parse_atags(const struct tag *tags) > >>> +{ > >>> + phys_addr_t ram_end = 0; > >>> + const struct tag *t; > >>> + bool atags_end = false; > >>> + u32 words = 0; > >>> + int j = 0; > >>> + > >>> + memset(prevbl_ddr_banks, 0, sizeof(prevbl_ddr_banks)); > >>> + > >>> + for (t = tags; words < SZ_16K / sizeof(u32); t = tag_next(t)) { > >>> + if (t->hdr.tag == ATAG_NONE) { > >>> + atags_end = true; > >>> + break; > >>> + } > >>> + if (t->hdr.size < sizeof(struct tag_header) / sizeof(u32)) > >>> + return -EINVAL; > >>> + if (t->hdr.size > SZ_16K / sizeof(u32) - words) > >>> + return -EINVAL; > >>> + > >>> + words += t->hdr.size; > >>> + > >>> + if (t->hdr.tag != ATAG_MEM) > >>> + continue; > >>> + if (t->hdr.size < tag_size(tag_mem32)) > >>> + return -EINVAL; > >>> + if (!t->u.mem.size) > >>> + continue; > >>> + > >>> + prevbl_ddr_banks[j].start = t->u.mem.start; > >>> + prevbl_ddr_banks[j].size = t->u.mem.size; > >>> + ram_end = max(ram_end, (phys_addr_t)t->u.mem.start + > >>> t->u.mem.size); > >>> + j++; > >>> + > >>> + if (j == CONFIG_NR_DRAM_BANKS) > >>> + break; > >>> + } > >>> + > >>> + if (!atags_end && j < CONFIG_NR_DRAM_BANKS) { > >>> + log_err("Provided more memory banks than we can handle\n"); > >>> + return -EINVAL; > >>> + } > >>> + if (!j) > >>> + return -ENODATA; > >>> + > >>> + qsort(prevbl_ddr_banks, j, sizeof(prevbl_ddr_banks[0]), ddr_bank_cmp); > >>> + > >>> + gd->ram_base = prevbl_ddr_banks[0].start; > >>> + gd->ram_size = ram_end - gd->ram_base; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static void show_psci_version(void) > >>> { > >>> #ifdef CONFIG_ARM64 > >>> @@ -227,42 +289,52 @@ static void qcom_psci_fixup(void *fdt) > >>> */ > >>> int board_fdt_blob_setup(void **fdtp) > >>> { > >>> - struct fdt_header *external_fdt, *internal_fdt; > >>> - bool internal_valid, external_valid; > >>> - int ret = -ENODATA; > >>> - > >>> - internal_fdt = (struct fdt_header *)*fdtp; > >>> - external_fdt = (struct fdt_header *)get_prev_bl_fdt_addr(); > >>> - external_valid = external_fdt && !fdt_check_header(external_fdt); > >>> - internal_valid = !fdt_check_header(internal_fdt); > >>> + int ret = -ENODATA, setup_ret = -EEXIST; > >>> + struct fdt_header *internal_fdt = *fdtp; > >>> + phys_addr_t prev_bl_arg = get_prev_bl_fdt_addr(); > >>> + bool internal_valid = internal_fdt && !fdt_check_header(internal_fdt); > >>> + bool external_is_fdt = prev_bl_arg && > >>> + !fdt_check_header((const void *)prev_bl_arg); > >>> > >>> - /* > >>> - * There is no point returning an error here, U-Boot can't do anything > >>> useful in this situation. > >>> - * Bail out while we can still print a useful error message. > >>> - */ > >>> - if (!internal_valid && !external_valid) > >>> + if (internal_valid) { > >>> + debug("Using built in FDT\n"); > >>> + } else if (external_is_fdt) { > >>> + debug("Using external FDT\n"); > >>> + *fdtp = (void *)prev_bl_arg; > >>> + setup_ret = 0; > >>> + } else { > >>> + /* > >>> + * There is no point returning an error here, U-Boot can't do > >>> + * anything useful in this situation. Bail out while we can > >>> + * still print a useful error message. > >>> + */ > >>> panic("Internal FDT is invalid and no external FDT was > >>> provided! (fdt=%p)\n", > >>> - external_fdt); > >>> + (void *)prev_bl_arg); > >>> + } > >>> > >>> /* Prefer memory information from internal DT if it's present */ > >>> if (internal_valid) > >>> ret = qcom_parse_memory(internal_fdt); > >>> > >>> - if (ret < 0 && external_valid) { > >>> + if (ret < 0 && prev_bl_arg) { > >>> /* No internal FDT or it lacks a proper /memory node. > >>> * The previous bootloader handed us something, let's > >>> try that. > >>> */ > >>> if (internal_valid) > >>> debug("No memory info in internal FDT, falling > >>> back to external\n"); > >>> > >>> - ret = qcom_parse_memory(external_fdt); > >>> + /* the prev BL arg is either an FDT or ATAGS */ > >>> + if (external_is_fdt) > >>> + ret = qcom_parse_memory((const void *)prev_bl_arg); > >>> + if (ret < 0 && qcom_atags_valid(prev_bl_arg)) > >>> + ret = qcom_parse_atags((const struct tag *)prev_bl_arg); > >>> } > >>> > >>> if (ret < 0) > >>> panic("No valid memory ranges found!\n"); > >>> > >>> - /* If we have an external FDT, it can only have come from the Android > >>> bootloader. */ > >>> - if (external_valid) > >>> + /* If we have a prev BL arg, we assume it came from ABL */ > >>> + if (prev_bl_arg) > >>> qcom_boot_source = QCOM_BOOT_SOURCE_ANDROID; > >>> else > >>> qcom_boot_source = QCOM_BOOT_SOURCE_XBL; > >>> @@ -270,18 +342,9 @@ int board_fdt_blob_setup(void **fdtp) > >>> debug("ram_base = %#011lx, ram_size = %#011llx\n", > >>> gd->ram_base, (unsigned long long)gd->ram_size); > >>> > >>> - if (internal_valid) { > >>> - debug("Using built in FDT\n"); > >>> - ret = -EEXIST; > >>> - } else { > >>> - debug("Using external FDT\n"); > >>> - *fdtp = external_fdt; > >>> - ret = 0; > >>> - } > >>> - > >>> qcom_psci_fixup(*fdtp); > >>> > >>> - return ret; > >>> + return setup_ret; > >>> } > >>> > >>> /* > >>> > >> > >> > >
