Re: [PATCH v2 01/18] binman: Add support for generating TI Board config binaries

Hi Neha,

On Wed, 5 Apr 2023 at 00:13, Neha Malcom Francis n-francis@ti.com wrote:

The ti-board-config entry loads and validates a given YAML config file against a given schema, and generates the board config binary. K3 devices require these generated binaries to be packed into the final system firmware images.

Signed-off-by: Neha Malcom Francis n-francis@ti.com

tools/binman/entries.rst | 50 ++++ tools/binman/etype/ti_board_config.py | 246 ++++++++++++++++++ tools/binman/ftest.py | 11 + tools/binman/test/277_ti_board_cfg.dts | 11 + .../binman/test/278_ti_board_cfg_combined.dts | 25 ++ tools/binman/test/yaml/config.yaml | 11 + tools/binman/test/yaml/schema.yaml | 26 ++ 7 files changed, 380 insertions(+) create mode 100644 tools/binman/etype/ti_board_config.py create mode 100644 tools/binman/test/277_ti_board_cfg.dts create mode 100644 tools/binman/test/278_ti_board_cfg_combined.dts create mode 100644 tools/binman/test/yaml/config.yaml create mode 100644 tools/binman/test/yaml/schema.yaml

This looks pretty good. Some things to fix:

The first line of a function comment should fit on that line. Add a blank line afterwards. Add full comments to all functions, describing the purpose / type of each arg (you have done many of them).

Use 'binman entry-docs >tools/binman/entries.rst' to generate the docs rather than hand editing. Try 'make htmldocs' as this fails for your updates.

Some test coverage is missing - use 'binman test -T' to try it. You will need to add tests for failing cases as well as success, perhaps adding new .dts files for your tests.

The jsonschema module is now required. That needs to be added to the 'dependencies' line of tools/binman/pyproject.toml

A few minor nits below

diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst index b71af801fd..7cfe61dd09 100644 --- a/tools/binman/entries.rst +++ b/tools/binman/entries.rst @@ -2423,3 +2423,53 @@ may be used instead.

+.. _etype_ti_board_config:

+Entry: ti-board-config: Texas Instruments board config binary +-------------------------------------------------------------

+Support for generation of TI schema validated board configuration +binary +This etype supports generation of two kinds of board configuration +binaries: singular board config binary as well as combined board config +binary.

+Properties / Entry arguments:

• • config-file: File containing board configuration data in YAML
• • schema-file: File containing board configuration YAML schema against
• which the config file is validated

+These above parameters are used only when the generated binary is +intended to be a single board configuration binary. Example::

+/* generate a my-ti-board-config.bin generated from a YAML configuration +file validated against the schema*/ +my-ti-board-config {

• ti-board-config {

•    config = "board-config.yaml";
  

•    schema = "schema.yaml";
  

• };

+};

+To generate a combined board configuration binary, we pack the +needed individual binaries into a ti-board-config binary. In this case, +the available supported subnode names are board-cfg, pm-cfg, sec-cfg and +rm-cfg. For example::

+/* generate a my-combined-ti-board-config.bin packed with a header +(containing details about the included board config binaries), along +with the YAML schema validated binaries themselves*/ +my-combined-ti-board-config {

• ti-board-config {

•    board-cfg {
  

•        config = "board-cfg.yaml";
  

•        schema = "schema.yaml";
  

•    };
  

•    sec-cfg {
  

•        config = "sec-cfg.yaml";
  

•        schema = "schema.yaml";
  

•    };
  

• };

+};

diff --git a/tools/binman/etype/ti_board_config.py b/tools/binman/etype/ti_board_config.py new file mode 100644 index 0000000000..0a9be44afc --- /dev/null +++ b/tools/binman/etype/ti_board_config.py @@ -0,0 +1,246 @@ +# SPDX-License-Identifier: GPL-2.0+ +# Copyright (c) 2022 Texas Instruments Incorporated - https://www.ti.com/ +# Written by Neha Malcom Francis n-francis@ti.com +# +# Entry-type module for generating schema validated TI board +# configuration binary +#

+import os +import struct +import tempfile +import yaml

+from collections import OrderedDict +from jsonschema import validate +from shutil import copyfileobj +from shutil import rmtree

+from binman.entry import Entry +from binman.etype.section import Entry_section +from binman.etype.blob_ext import Entry_blob_ext +from binman.etype.blob_ext_list import Entry_blob_ext_list +from dtoc import fdt_util +from u_boot_pylib import tools, tout

+BOARDCFG = 0xB +BOARDCFG_SEC = 0xD +BOARDCFG_PM = 0xE +BOARDCFG_RM = 0xC +BOARDCFG_NUM_ELEMS = 4

+class Entry_ti_board_config(Entry_section):

• """
• Support for generation of TI schema validated board configuration
• binary
• This etype supports generation of two kinds of board configuration
• binaries: singular board config binary as well as combined board config
• binary.
• Available parameters are:
• config-file

•    File containing board configuration data in YAML
  

• schema-file

•    File containing board configuration YAML schema against which the
  

•    config file is validated
  

• These above parameters are used only when the generated binary is
• intended to be a single board configuration binary. Example::
• /* generate a my-ti-board-config.bin generated from a YAML configuration
• file validated against the schema*/
• my-ti-board-config {

•    ti-board-config {
  

•        config = "board-config.yaml";
  

•        schema = "schema.yaml";
  

•    };
  

• };
• To generate a combined board configuration binary, we pack the
• needed individual binaries into a ti-board-config binary. In this case,
• the available supported subnode names are board-cfg, pm-cfg, sec-cfg and
• rm-cfg. For example::
• /* generate a my-combined-ti-board-config.bin packed with a header
• (containing details about the included board config binaries), along
• with the YAML schema validated binaries themselves*/
• my-combined-ti-board-config {

•    ti-board-config {
  

•        board-cfg {
  

•            config = "board-cfg.yaml";
  

•            schema = "schema.yaml";
  

•        };
  

•        sec-cfg {
  

•            config = "sec-cfg.yaml";
  

•            schema = "schema.yaml";
  

•        };
  

•    }
  

• }
• """
• def __init__(self, section, etype, node):

•    super().__init__(section, etype, node)
  

•    self.config_file = None
  

•    self.schema_file = None
  

•    self._entries = OrderedDict()
  

•    self._entries_data = OrderedDict()
  

•    self.num_elems = BOARDCFG_NUM_ELEMS
  

•    self.fmt = '<HHHBB'
  

•    self.index = 0
  

•    self.binary_offset = 0
  

•    self.sw_rev = 1
  

•    self.devgrp = 0
  

Better to use a _ prefix with private properties, ie.. self._devgr[

• def ReadNode(self):

•    super().ReadNode()
  

•    self.config_file = fdt_util.GetString(self._node, 'config')
  

•    self.schema_file = fdt_util.GetString(self._node, 'schema')
  

•    if self.config_file is None:
  

•        self.ReadEntries()
  

Why is this not done if self.config_file is not None? Please add a comment.

• def ReadEntries(self):

•    """Read the subnodes to find out what should go in this image"""
  

•    num_cfgs = 0
  

•    for node in self._node.subnodes:
  

•        if 'type' not in node.props:
  

•            num_cfgs += 1
  

•            etype = 'ti-board-config'
  

Can you drop that and just use the string in the next line?

•            entry = Entry.Create(self, node, etype)
  

•            entry.ReadNode()
  

•            cfg_data = entry.BuildSectionData(True)
  

•            self._entries[entry.name] = entry
  

•            self._entries_data[entry.name] = cfg_data
  

You could add your data to the entry if you like?

entry._cfg_data = cfg_data

?

•    self.num_elems = num_cfgs
  

Isn't that len(self._node.subnodes) ?

• def _convert_to_byte_chunk(self, val, data_type):

•    """Convert value into byte array"""
  

•    size = 0
  

•    if (data_type == "#/definitions/u8"):
  

•        size = 1
  

•    elif (data_type == "#/definitions/u16"):
  

•        size = 2
  

•    elif (data_type == "#/definitions/u32"):
  

•        size = 4
  

•    else:
  

•        raise Exception("Data type not present in definitions")
  

ValueError()

Please use single quotes where possible except for function docs (please fix throughout)

•    if type(val) == int:
  

•        br = val.to_bytes(size, byteorder="little")
  

•    return br
  

• def _compile_yaml(self, schema_yaml, file_yaml):

•    """Convert YAML file into byte array based on YAML schema"""
  

•    br = bytearray()
  

•    for key in file_yaml.keys():
  

•        node = file_yaml[key]
  

•        node_schema = schema_yaml['properties'][key]
  

•        node_type = node_schema.get('type')
  

•        if not 'type' in node_schema:
  

•            br += self._convert_to_byte_chunk(node,
  

•                                              node_schema.get('$ref'))
  

•        elif node_type == 'object':
  

•            br += self._compile_yaml(node_schema, node)
  

•        elif node_type == 'array':
  

•            for item in node:
  

•                if not isinstance(item, dict):
  

•                    br += self._convert_to_byte_chunk(
  

•                        item, schema_yaml['properties'][key]['items']["$ref"])
  

•                else:
  

•                    br += self._compile_yaml(node_schema.get('items'), item)
  

•    return br
  

• def _generate_binaries(self):

•    """Generate config binary artifacts from the loaded YAML configuration file"""
  

Return:

•    try:
  

•        cfg_binary = bytearray()
  

•        for key in self.file_yaml.keys():
  

•            node = self.file_yaml[key]
  

You can do:

for key, node in self.file_yaml.items():

•            node_schema = self.schema_yaml['properties'][key]
  

•            br = self._compile_yaml(node_schema, node)
  

•            cfg_binary += br
  

•    except Exception as e:
  

•        tout.warning("Combined board config binary was not generated properly")
  

•        cfg_binary = tools.get_bytes(0, 512)
  

•    return cfg_binary
  

• def _add_boardcfg(self, bcfgtype, bcfgdata):

comment

•    size = len(bcfgdata)
  

•    desc = struct.pack(self.fmt, bcfgtype,
  

•                        self.binary_offset, size, self.devgrp, 0)
  

•    with open(self.descfile, "ab+") as desc_fh:
  

•        desc_fh.write(desc)
  

•    with open(self.bcfgfile, "ab+") as bcfg_fh:
  

•        bcfg_fh.write(bcfgdata)
  

•    self.binary_offset += size
  

•    self.index += 1
  

• def _finalize(self):

•    try:
  

•        with open(self.descfile, "rb") as desc_fh:
  

•            with open(self.bcfgfile, "rb") as bcfg_fh:
  

•                with open(self.fh_file, 'ab+') as fh:
  

•                    desc_fh.seek(0)
  

•                    bcfg_fh.seek(0)
  

But if they are just opened you don't need to seek. Can't you drop those two lines?

•                    copyfileobj(desc_fh, fh)
  

•                    copyfileobj(bcfg_fh, fh)
  

•        data = tools.read_file(self.fh_file)
  

•    except Exception as e:
  

You should not catch Exception since this will cause anything to be caught. Here I think you want IOError ?

•        tout.warning("Combined board config binary was not generated properly")
  

•        data = tools.get_bytes(0, 512)
  

•    rmtree(self.tmpdir)
  

•    return data
  

• def BuildSectionData(self, required):

•    if self.config_file is None:
  

•        self.binary_offset = 0
  

•        self.tmpdir = tempfile.mkdtemp()
  

This should use tools.get_output_filename() rather than create a temp dir. We want the output to be visible for debugging, etc.

•        self.fh_file = os.path.join(self.tmpdir, "fh")
  

•        self.descfile = os.path.join(self.tmpdir, "desc")
  

•        self.bcfgfile = os.path.join(self.tmpdir, "bcfg")
  

•        try:
  

•            with open(self.fh_file, 'wb') as f:
  

•                t_bytes = f.write(struct.pack(
  

•                    '<BB', self.num_elems, self.sw_rev))
  

•            self.binary_offset += t_bytes
  

•            self.binary_offset += self.num_elems * struct.calcsize(self.fmt)
  

•        except Exception as e:
  

•            tout.warning("Combined board config header was not generated properly")
  

•        if 'board-cfg' in self._entries:
  

•            self._add_boardcfg(BOARDCFG, self._entries_data['board-cfg'])
  

•        if 'sec-cfg' in self._entries:
  

•            self._add_boardcfg(BOARDCFG_SEC, self._entries_data['sec-cfg'])
  

•        if 'pm-cfg' in self._entries:
  

•            self._add_boardcfg(BOARDCFG_PM, self._entries_data['pm-cfg'])
  

•        if 'rm-cfg' in self._entries:
  

•            self._add_boardcfg(BOARDCFG_RM, self._entries_data['rm-cfg'])
  

•        data = self._finalize()
  

•        return data
  

•    else:
  

•        with open(self.config_file, 'r') as f:
  

•            self.file_yaml = yaml.safe_load(f)
  

•        with open(self.schema_file, 'r') as sch:
  

•            self.schema_yaml = yaml.safe_load(sch)
  

•        try:
  

•            validate(self.file_yaml, self.schema_yaml)
  

•        except Exception as e:
  

•            tout.error(f"Schema validation error: {e}")
  

•        data = self._generate_binaries()
  

•        return data
  

• def SetImagePos(self, image_pos):

•    Entry.SetImagePos(self, image_pos)
  

• def SetCalculatedProperties(self):

•    Entry.SetCalculatedProperties(self)
  

• def CheckEntries(self):

•    Entry.CheckEntries(self)
  

[..]

• def testTIBoardConfig(self):

•    """Test that a schema validated board config file can be generated"""
  

•    data = self._DoReadFile('277_ti_board_cfg.dts')
  

•    self.assertEqual(TI_BOARD_CONFIG_DATA, data)
  

• def testTIBoardConfig(self):

•    """Test that a schema validated combined board config file can be generated"""
  

•    data = self._DoReadFile('278_ti_board_cfg_combined.dts')
  

•    configlen_noheader = TI_BOARD_CONFIG_DATA*4
  

spaces around *

•    self.assertGreater(data, configlen_noheader)
  
  

if __name__ == "__main__": unittest.main()

Regards, Simon