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

[Simon Glass]

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 would also prefer qcom_xxx
for the etype name where xxx is whatever this means, since presumably
this is Qualcomm-specific.

> 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).

> 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.

> 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.

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.

> 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.

> 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.

> 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.

> +    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?

Regards,
Simon