[PATCH v2 4/8] binman: Add QCDT support

[Sam Day]

Greetings Simon,

On Monday, 8 June 2026 at 11:27 PM, Simon Glass <sjg at chromium.org> wrote:

> Hi Sam,
>
> On 2026-06-07T23:37:09, Sam Day via B4 Relay
> <devnull+me.samcday.com at kernel.org> wrote:
> > binman: Add QCDT support
> >
> > This vendor-specific format is used by many bootloaders on older qcom
> > SoCs, such as msm8916. It's a container for N FDTs. Each one is
> > contained in a record that includes metadata about the platform/variant
> > it targets. The previous bootloader picks the 'right' record based on
> > this metadata.
> >
> > This initial impl targets a streamlined v2 path, with no support for
> > different versions or multiple qcom,msm-id/qcom,board-id tuples. If/when
> > that's needed it will be implemented in a follow-up.
> >
> > Signed-off-by: Sam Day <me at samcday.com>
> >
> > tools/binman/etype/android_boot.py           |  30 +++++
> >  tools/binman/etype/qcdt.py                   | 160 +++++++++++++++++++++++++++
> >  tools/binman/ftest.py                        |  22 ++++
> >  tools/binman/test/vendor/qcdt.dts            |  20 ++++
> >  tools/binman/test/vendor/qcdt_bad_msm_id.dts |  17 +++
> >  5 files changed, 249 insertions(+)
>
> Your comment says "A legacy QCDT abootimg, the kind msm8916 bootloaders expect"
>
> Please can you spell out the acronym...

I'd love to, but I think nobody can actually do that. There's not
really any canonical impl or reference point for this stuff. I believe the
reason everyone calls it "QCDT" is because that's the magic identifier in
the header.

Maybe it means "QualComm Device(tree) Table"? Or perhaps it means
"Qualcomm/CodeAurora Device Trees"? We may never truly know :)

> I would also prefer qcom_xxx
> for the etype name where xxx is whatever this means, since presumably
> this is Qualcomm-specific.

Given the acronym doesn't have a canonical definition, and the limited
references to it use the "QCDT" moniker directly, I opted to keep it as-is
for v3, since `qcom_cdt` is confusing and would be a unique reference that
exists only in U-Boot. Or would you prefer qcom_qcdt?

>
> > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > @@ -0,0 +1,160 @@
> > +from binman.entry import Entry
> > +from binman.etype.section import Entry_section
> > +from dtoc import fdt_util
>
> The _align_up()/_pad() helpers at the top are duplicated from
> android_boot.py in patch 1, and will be again in dtbh.py in patch 6.
> Please factor them into a shared helper module (or stick them on
> Entry_section).

I opted for the latter. v3 introduces a new patch to put these
shared helpers in Entry_section.

>
> > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > @@ -0,0 +1,160 @@
> > +    def ReadEntries(self):
> > +        for node in self._node.subnodes:
> > +            if self.IsSpecialSubnode(node):
> > +                continue
> > +
> > +            payloads = self._GetPayloadSubnodes(node)
> > +            if len(payloads) > 1:
> > +                raise ValueError("Node '%s': must contain exactly one DTB "
> > +                                 "payload subnode" % node.path)
>
> Please use self.Raise() throughout - that's the binman convention and
> it adds the node path for you. The static/classmethod helpers should
> become plain methods so they can use self.Raise() too.

Ack, done in v3.

>
> > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > @@ -0,0 +1,160 @@
> > +        dtb_offset = _align_up(12 + len(dtbs) * 24, page_size)
> > +        records = []
> > +        payloads = bytearray()
> > +        for msm_id, board_id, dtb in dtbs:
> > +            dtb_size = _align_up(len(dtb), page_size)
> > +            records.append((*msm_id, *board_id, dtb_offset, dtb_size))
> > +            payloads += _pad(dtb, page_size)
> > +            dtb_offset += dtb_size
> > +
> > +        qcdt = bytearray(struct.pack('<4sII', QCDT_MAGIC, QCDT_VERSION,
> > +                                     len(records)))
> > +        for platform_id, soc_rev, variant_id, subtype, offset, size in records:
> > +            qcdt += struct.pack('<IIIIII', platform_id, variant_id, subtype,
> > +                                soc_rev, offset, size)
>
> The 12 and 24 magic numbers should be struct.calcsize() of named
> header/record format strings, the way android_boot.py does for
> BOOT_IMAGE_HEADER_V0_SIZE. That also documents the layout.

Hm yeah, I got lazy here. v3 removes the magic values and formalizes
the format/size calculations like android_boot does.

>
> Also, unpacking msm-id as (platform_id, soc_rev) and board-id as
> (variant_id, subtype), then packing in the swapped order (platform_id,
> variant_id, subtype, soc_rev) is hard to read. Please build each
> record directly with the on-disk field order and add a comment
> referencing the LK/AOSP source for the QCDT v2 record layout.

Ack - this isn't very readable, I agree. Overhauled in v3.

>
> > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > @@ -0,0 +1,160 @@
> > +    def BuildSectionData(self, required):
> > +        if not self._node.subnodes:
> > +            raise ValueError("Node '%s': Missing required DTB subnodes" %
> > +                             self._node.path)
>
> Redundant with the 'if not dtbs:' check further down (which correctly
> handles the case where the only subnodes are special). Please drop it.

Ack. v3 refactors this heavily and the redundant check is gone.

>
> > diff --git a/tools/binman/etype/qcdt.py b/tools/binman/etype/qcdt.py
> > @@ -0,0 +1,160 @@
> > +    def _GetPageSize(self):
> > +        if self._page_size is not None:
> > +            return self._page_size
> > +
> > +        return getattr(self.section, 'page_size', 2048)
>
> The docstring says "defaults to the parent android-boot page size",
> but in practice this picks it up from the vendor-dt section that
> _ReadVendorDtEntries() stamps with a page_size attribute. Please
> update the docstring - or better, but really you should look up the
> enclosing android-boot section directly rather than relying on an
> attribute being monkey-patched onto an intermediate generic section.

Indeed. v3 refactors this heavily to introduce a shared base class, which
prefers a locally set page-size, falling back to traversing the ancestry
to find an android-boot preference (and finally falling back to 2048).

>
> > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> > @@ -5673,6 +5673,28 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
> > +    def testAndroidBootQcdt(self):
> > +        """Test that binman can produce a QCDT container"""
> > +        data, dtb_data, _map, _dtb = self._DoReadFileDtb(
> > +            'vendor/qcdt.dts', use_real_dtb=True)
>
> Only one record is covered. Since the whole point of QCDT is to hold
> multiple DTBs picked by msm-id/board-id, please add a test with at
> least two dtb-* records so the offset/size table-walking code is
> actually exercised.

Agree. Done in v3.

>
> > +    binman: Add QCDT support
> > +
> > +    This initial impl targets a streamlined v2 path, with no support for
> > +    different versions or multiple qcom,msm-id/qcom,board-id tuples. If/when
> > +    that's needed it will be implemented in a follow-up.
>
> How abouta Link: to the LK/AOSP reference for the QCDT v2 record format?

Good idea, done in v3.

Thanks Simon,
-Sam

>
> Regards,
> Simon
>