[PATCH RESEND 12/16] mach-snapdragon: parse incoming ARM32 ATAGS
Sam Day
me at samcday.com
Sun Jun 7 05:41:08 CEST 2026
Hej Casey,
On Friday, 5 June 2026 at 1:01 AM, Casey Connolly <casey.connolly at linaro.org> wrote:
>
>
> On 6/4/26 00:27, Sam Day wrote:
> > Heya Casey,
> >
> > On Wednesday, 3 June 2026 at 11:47 PM, Casey Connolly <casey.connolly at linaro.org> 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 <me at samcday.com>
> >>>
> >>> 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 <me at samcday.com>
> >>> ---
> >>> 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;
> >>> }
> >>>
> >>> /*
> >>>
> >>
> >>
>
>
More information about the U-Boot
mailing list