Hi Alice,
On Thu, 19 Dec 2024 at 19:56, Alice Guo <alice....@oss.nxp.com> wrote:
>
> From: Alice Guo <alice....@nxp.com>
>
> i.MX95 needs to combine DDR PHY firmware images and their byte counts
> together, so add a new entry type nxp-append-ddrfw for this requirement.
>
> Signed-off-by: Alice Guo <alice....@nxp.com>
> ---
> tools/binman/entries.rst | 14 ++++++
> tools/binman/etype/nxp_append_ddrfw.py | 78
> ++++++++++++++++++++++++++++++++++
> 2 files changed, 92 insertions(+)
>
Please note that you should create a test[1] for any new Binman etype.
You can see the current work on this[2], which failed apparently due
to a bug in the 'cst' tool.
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index
> 83068ba6d39d9a5b586312fd4a062b0da31eb595..061bd2f1da9812d36c90b1fb10a919667e36ceaa
> 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -1658,6 +1658,20 @@ Properties / Entry arguments:
>
>
>
> +.. _etype_nxp_append_ddrfw:
> +
> +Entry: nxp-append-ddrfw: pack NXP LPDDR firmware
> +---------------------------------------------------
> +
> +Properties / Entry arguments:
> + - oei_m33_ddr_image - M33 DDR IMAGE
> + - lpddr_imem - Synopsys LPDDR PHY firmware
> + - lpddr_dmem - Synopsys LPDDR PHY firmware
> + - lpddr_imem_qb - Synopsys LPDDR quick boot firmware
> + - lpddr_dmem_qb - Synopsys LPDDR quick boot firmware
> +
> +
> +
> .. _etype_opensbi:
>
> Entry: opensbi: RISC-V OpenSBI fw_dynamic blob
> diff --git a/tools/binman/etype/nxp_append_ddrfw.py
> b/tools/binman/etype/nxp_append_ddrfw.py
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..bc23a7611eaf823651a8fcbc9898c9140c277d7f
> --- /dev/null
> +++ b/tools/binman/etype/nxp_append_ddrfw.py
> @@ -0,0 +1,78 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +#
> +# Copyright 2024 NXP
> +
> +from binman.etype.section import Entry_section
> +from dtoc import fdt_util
> +import os
> +
> +class Entry_nxp_append_ddrfw(Entry_section):
> + """NXP M33 OEI DDRFW
Here you should describe the entry type so people know what it is for.
Is the word 'append' needed in the name?
> +
> + Properties / Entry arguments:
> + - oei_m33_ddr_image: M33 DDR IMAGE
> + - lpddr_imem: Synopsys LPDDR PHY firmware
> + - lpddr_dmem: Synopsys LPDDR PHY firmware
> + - lpddr_imem_qb: Synopsys LPDDR quick boot firmware
> + - lpddr_dmem_qb: Synopsys LPDDR quick boot firmware
It isn't clear what these things are. Please add a bit more detail
about how to get these things and what they are.
>From what I can tell, I think you should add each of these as a new
etype. See for example Entry_intel_fsp_m.
Then in your image description you specify how these are packed together.
> + """
> +
> + def __init__(self, section, etype, node):
> + super().__init__(section, etype, node)
> + self.required_props = ['oei_m33_ddr_image', 'lpddr_imem',
> 'lpddr_dmem',
> + 'lpddr_imem_qb', 'lpddr_dmem_qb']
Properties should use hyphens rather than underscores, to fit with
devicetree usage.
> +
> + def ReadNode(self):
> + super().ReadNode()
> + self.oei_m33_ddr_image = fdt_util.GetString(self._node,
> 'oei_m33_ddr_image')
This looks like an output file. Binman handles this side of things and
you should not have etypes writing out files themselves.
> + self.lpddr_imem = fdt_util.GetString(self._node, 'lpddr_imem')
> + self.lpddr_dmem = fdt_util.GetString(self._node, 'lpddr_dmem')
> + self.lpddr_imem_qb = fdt_util.GetString(self._node, 'lpddr_imem_qb')
> + self.lpddr_dmem_qb = fdt_util.GetString(self._node, 'lpddr_dmem_qb')
> + self.ReadEntries()
> +
> + def BuildSectionData(self, required):
> + u_boot_path = os.path.abspath('.')
> +
> + oei_m33_ddr_image_path = os.path.join(u_boot_path,
> self.oei_m33_ddr_image)
> + lpddr_imem_path = os.path.join(u_boot_path, self.lpddr_imem)
> + lpddr_dmem_path = os.path.join(u_boot_path, self.lpddr_dmem)
> + lpddr_imem_qb_path = os.path.join(u_boot_path, self.lpddr_imem_qb)
> + lpddr_dmem_qb_path = os.path.join(u_boot_path, self.lpddr_dmem_qb)
> +
> + if (not (os.path.exists(oei_m33_ddr_image_path) and
> + os.path.exists(lpddr_imem_path) and
> os.path.exists(lpddr_dmem_path) and
> + os.path.exists(lpddr_imem_qb_path) and
> os.path.exists(lpddr_dmem_qb_path))):
> + print("Please check if there are any external blobs that do not
> exist.")
> + return b''
Missing blobs are handled automatically by Binman.[3]
> +
> + lpddr_imem_size = os.path.getsize(lpddr_imem_path)
> + lpddr_dmem_size = os.path.getsize(lpddr_dmem_path)
> + lpddr_imem_qb_size = os.path.getsize(lpddr_imem_qb_path)
> + lpddr_dmem_qb_size = os.path.getsize(lpddr_dmem_qb_path)
> +
> + with open(oei_m33_ddr_image_path, 'rb') as file:
> + oei_m33_ddr_image = file.read()
oei_m33_ddr_image = tools.read_file(oei_m33_ddr_image_path)
> +
> + length = len(oei_m33_ddr_image)
> + if (length % 4 != 0):
> + oei_m33_ddr_image += bytes(4 - length % 4)
Binman handles alignment automatically. It just needs to be in the description
It seems that this etype is putting some files together with a little header.
> +
> + fw_header = lpddr_imem_size.to_bytes(4, 'little') +
> lpddr_dmem_size.to_bytes(4, 'little')
> + with open(lpddr_imem_path, 'rb') as file:
> + lpddr_imem = file.read()
> + with open(lpddr_dmem_path, 'rb') as file:
> + lpddr_dmem = file.read()
> +
> + fw_header_qb = lpddr_imem_qb_size.to_bytes(4, 'little') +
> lpddr_dmem_qb_size.to_bytes(4, 'little')
> + with open(lpddr_imem_qb_path, 'rb') as file:
> + lpddr_imem_qb = file.read()
> + with open(lpddr_dmem_qb_path, 'rb') as file:
> + lpddr_dmem_qb = file.read()
> +
> + data = oei_m33_ddr_image + fw_header + lpddr_imem + lpddr_dmem +
> fw_header_qb + lpddr_imem_qb + lpddr_dmem_qb
So ddr-image should be its own etype (probably just a one-line thing like fsp-s
The next bit is a fw_header plus two blobs, so that should be in an
'lpddr' etype
The final bit is qb (?) which should be in a 'qb' etype, or some better name.
> + length = len(data)
> + if (length % 8 != 0):
> + data += bytes(8 - length % 8)
In the end you will have very little code and an image description like:
oei-m33-ddr {
align-size = <4>;
};
imx-lpddr {
align-size = <4>;
imx-lpddr-imem {
};
imx-lpddr-dmem {
};
};
imx-lpddr-qb {
align-size = <4>;
imx-lpddr-imem-qb {
};
imx-lpddr-dmem-qb {
};
};
Please let me know if you need help with any of this.
> +
> + return data
>
> --
> 2.34.1
>
Regards,
SImon
[1] https://docs.u-boot.org/en/latest/develop/binman_tests.html
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=430428&state=*
[3] https://docs.u-boot.org/en/latest/develop/package/binman.html#external-blobs
