mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] scripts/make_fit: Support decomposing DTBs
@ 2024-06-05  9:48 Chen-Yu Tsai
  2024-06-10 15:16 ` Simon Glass
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Chen-Yu Tsai @ 2024-06-05  9:48 UTC (permalink / raw)
  To: Simon Glass, Masahiro Yamada, Nathan Chancellor, Nicolas Schier
  Cc: Chen-Yu Tsai, linux-arm-kernel, linux-kernel, linux-kbuild

The kernel tree builds some "composite" DTBs, where the final DTB is the
result of applying one or more DTB overlays on top of a base DTB with
fdtoverlay.

The FIT image specification already supports configurations having one
base DTB and overlays applied on top. It is then up to the bootloader to
apply said overlays and either use or pass on the final result. This
allows the FIT image builder to reuse the same FDT images for multiple
configurations, if such cases exist.

The decomposition function depends on the kernel build system, reading
back the .cmd files for the to-be-packaged DTB files to check for the
fdtoverlay command being called. This will not work outside the kernel
tree. The function is off by default to keep compatibility with possible
existing users.

To facilitate the decomposition and keep the code clean, the model and
compatitble string extraction have been moved out of the output_dtb
function. The FDT image description is replaced with the base file name
of the included image.

Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
This is a feature I alluded to in my replies to Simon's original
submission of the make_fit.py script [1].

This is again made a runtime argument as not all firmware out there
that boot FIT images support applying overlays. Like my previous
submission for disabling compression for included FDT images, the
bootloader found in RK3399 and MT8173 Chromebooks do not support
applying overlays. Another case of this is U-boot shipped by development
board vendors in binary form (without upstream) in an image or in
SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
These would fail to boot FIT images with DT overlays. One such
example is my Hummingboard Pulse. In these cases the firmware is
either not upgradable or very hard to upgrade.

I believe there is value in supporting these cases. A common script
shipped with the kernel source that can be shared by distros means
the distro people don't have to reimplement this in their downstream
repos or meta-packages. For ChromeOS this means reducing the amount
of package code we have in shell script.

[1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
[2]

 scripts/Makefile.lib |  1 +
 scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
 2 files changed, 49 insertions(+), 22 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 9f06f6aaf7fc..d78b5d38beaa 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
       cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
 		--name '$(UIMAGE_NAME)' \
 		$(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
+		$(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
 		--compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
 
 # XZ
diff --git a/scripts/make_fit.py b/scripts/make_fit.py
index 263147df80a4..120f13e1323c 100755
--- a/scripts/make_fit.py
+++ b/scripts/make_fit.py
@@ -22,6 +22,11 @@ the entire FIT.
 Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
 zstd algorithms.
 
+Use -d to decompose "composite" DTBs into their base components and
+deduplicate the resulting base DTBs and DTB overlays. This requires the
+DTBs to be sourced from the kernel build directory, as the implementation
+looks at the .cmd files produced by the kernel build.
+
 The resulting FIT can be booted by bootloaders which support FIT, such
 as U-Boot, Linuxboot, Tianocore, etc.
 
@@ -64,6 +69,8 @@ def parse_args():
           help='Specifies the architecture')
     parser.add_argument('-c', '--compress', type=str, default='none',
           help='Specifies the compression')
+    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
+          help='Decompose composite DTBs into base DTB and overlays')
     parser.add_argument('-E', '--external', action='store_true',
           help='Convert the FIT to use external data')
     parser.add_argument('-n', '--name', type=str, required=True,
@@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
     fsw.end_node()
     seq = 0
     with fsw.add_node('configurations'):
-        for model, compat in entries:
+        for model, compat, files in entries:
             seq += 1
             with fsw.add_node(f'conf-{seq}'):
                 fsw.property('compatible', bytes(compat))
                 fsw.property_string('description', model)
-                fsw.property_string('fdt', f'fdt-{seq}')
+                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))
                 fsw.property_string('kernel', 'kernel')
     fsw.end_node()
 
@@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
         fname (str): Filename containing the DTB
         arch: FIT architecture, e.g. 'arm64'
         compress (str): Compressed algorithm, e.g. 'gzip'
-
-    Returns:
-        tuple:
-            str: Model name
-            bytes: Compatible stringlist
     """
     with fsw.add_node(f'fdt-{seq}'):
-        # Get the compatible / model information
-        with open(fname, 'rb') as inf:
-            data = inf.read()
-        fdt = libfdt.FdtRo(data)
-        model = fdt.getprop(0, 'model').as_str()
-        compat = fdt.getprop(0, 'compatible')
-
-        fsw.property_string('description', model)
+        fsw.property_string('description', os.path.basename(fname))
         fsw.property_string('type', 'flat_dt')
         fsw.property_string('arch', arch)
         fsw.property_string('compression', compress)
@@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
         with open(fname, 'rb') as inf:
             compressed = compress_data(inf, compress)
         fsw.property('data', compressed)
-    return model, compat
 
 
 def build_fit(args):
@@ -235,6 +229,7 @@ def build_fit(args):
     fsw = libfdt.FdtSw()
     setup_fit(fsw, args.name)
     entries = []
+    fdts = collections.OrderedDict()
 
     # Handle the kernel
     with open(args.kernel, 'rb') as inf:
@@ -243,12 +238,43 @@ def build_fit(args):
     write_kernel(fsw, comp_data, args)
 
     for fname in args.dtbs:
-        # Ignore overlay (.dtbo) files
-        if os.path.splitext(fname)[1] == '.dtb':
-            seq += 1
-            size += os.path.getsize(fname)
-            model, compat = output_dtb(fsw, seq, fname, args.arch, args.compress)
-            entries.append([model, compat])
+        # Ignore non-DTB (*.dtb) files
+        if os.path.splitext(fname)[1] != '.dtb':
+            continue
+
+        # Get the compatible / model information
+        with open(fname, 'rb') as inf:
+            data = inf.read()
+        fdt = libfdt.FdtRo(data)
+        model = fdt.getprop(0, 'model').as_str()
+        compat = fdt.getprop(0, 'compatible')
+
+        if args.decompose_dtbs:
+            # Check if the DTB needs to be decomposed
+            path, basename = os.path.split(fname)
+            cmd_fname = os.path.join(path, f'.{basename}.cmd')
+            with open(cmd_fname, 'r', encoding='ascii') as inf:
+                cmd = inf.read()
+
+            if 'scripts/dtc/fdtoverlay' in cmd:
+                # This depends on the structure of the composite DTB command
+                files = cmd.split()
+                files = files[files.index('-i')+1:]
+            else:
+                files = [fname]
+        else:
+            files = [fname]
+
+        for fn in files:
+            if fn not in fdts:
+                seq += 1
+                size += os.path.getsize(fn)
+                output_dtb(fsw, seq, fn, args.arch, args.compress)
+                fdts[fn] = seq
+
+        files_seq = [fdts[fn] for fn in files]
+
+        entries.append([model, compat, files_seq])
 
     finish_fit(fsw, entries)
 
-- 
2.45.1.288.g0e0cd299f1-goog


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-05  9:48 [PATCH] scripts/make_fit: Support decomposing DTBs Chen-Yu Tsai
@ 2024-06-10 15:16 ` Simon Glass
  2024-06-11  8:51   ` Chen-Yu Tsai
  2024-06-11 14:32 ` Masahiro Yamada
  2024-06-11 20:01 ` Simon Glass
  2 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2024-06-10 15:16 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild

Hi Chen-Yu,

On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> The kernel tree builds some "composite" DTBs, where the final DTB is the
> result of applying one or more DTB overlays on top of a base DTB with
> fdtoverlay.
>
> The FIT image specification already supports configurations having one
> base DTB and overlays applied on top. It is then up to the bootloader to
> apply said overlays and either use or pass on the final result. This
> allows the FIT image builder to reuse the same FDT images for multiple
> configurations, if such cases exist.
>
> The decomposition function depends on the kernel build system, reading
> back the .cmd files for the to-be-packaged DTB files to check for the
> fdtoverlay command being called. This will not work outside the kernel
> tree. The function is off by default to keep compatibility with possible
> existing users.
>
> To facilitate the decomposition and keep the code clean, the model and
> compatitble string extraction have been moved out of the output_dtb
> function. The FDT image description is replaced with the base file name
> of the included image.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This is a feature I alluded to in my replies to Simon's original
> submission of the make_fit.py script [1].
>
> This is again made a runtime argument as not all firmware out there
> that boot FIT images support applying overlays. Like my previous
> submission for disabling compression for included FDT images, the
> bootloader found in RK3399 and MT8173 Chromebooks do not support
> applying overlays. Another case of this is U-boot shipped by development
> board vendors in binary form (without upstream) in an image or in
> SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> These would fail to boot FIT images with DT overlays. One such
> example is my Hummingboard Pulse. In these cases the firmware is
> either not upgradable or very hard to upgrade.
>
> I believe there is value in supporting these cases. A common script
> shipped with the kernel source that can be shared by distros means
> the distro people don't have to reimplement this in their downstream
> repos or meta-packages. For ChromeOS this means reducing the amount
> of package code we have in shell script.
>
> [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> [2]
>
>  scripts/Makefile.lib |  1 +
>  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 49 insertions(+), 22 deletions(-)

This is a clever way to discover the included files. Does it need to
rely on the Linux build information, or could this information somehow
be in the .dtb files? I had expected some sort of overlay scheme in
the source, but perhaps people have given up on that?

Regards,
Simon

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-10 15:16 ` Simon Glass
@ 2024-06-11  8:51   ` Chen-Yu Tsai
  2024-06-11 14:45     ` Masahiro Yamada
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Chen-Yu Tsai @ 2024-06-11  8:51 UTC (permalink / raw)
  To: Simon Glass
  Cc: Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild, Elliot Berman,
	Devicetree List, Rob Herring, Krzysztof Kozlowski, Conor Dooley

On Mon, Jun 10, 2024 at 11:16 PM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Chen-Yu,
>
> On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > result of applying one or more DTB overlays on top of a base DTB with
> > fdtoverlay.
> >
> > The FIT image specification already supports configurations having one
> > base DTB and overlays applied on top. It is then up to the bootloader to
> > apply said overlays and either use or pass on the final result. This
> > allows the FIT image builder to reuse the same FDT images for multiple
> > configurations, if such cases exist.
> >
> > The decomposition function depends on the kernel build system, reading
> > back the .cmd files for the to-be-packaged DTB files to check for the
> > fdtoverlay command being called. This will not work outside the kernel
> > tree. The function is off by default to keep compatibility with possible
> > existing users.
> >
> > To facilitate the decomposition and keep the code clean, the model and
> > compatitble string extraction have been moved out of the output_dtb
> > function. The FDT image description is replaced with the base file name
> > of the included image.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > This is a feature I alluded to in my replies to Simon's original
> > submission of the make_fit.py script [1].
> >
> > This is again made a runtime argument as not all firmware out there
> > that boot FIT images support applying overlays. Like my previous
> > submission for disabling compression for included FDT images, the
> > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > applying overlays. Another case of this is U-boot shipped by development
> > board vendors in binary form (without upstream) in an image or in
> > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > These would fail to boot FIT images with DT overlays. One such
> > example is my Hummingboard Pulse. In these cases the firmware is
> > either not upgradable or very hard to upgrade.
> >
> > I believe there is value in supporting these cases. A common script
> > shipped with the kernel source that can be shared by distros means
> > the distro people don't have to reimplement this in their downstream
> > repos or meta-packages. For ChromeOS this means reducing the amount
> > of package code we have in shell script.
> >
> > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > [2]
> >
> >  scripts/Makefile.lib |  1 +
> >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> >  2 files changed, 49 insertions(+), 22 deletions(-)
>
> This is a clever way to discover the included files. Does it need to
> rely on the Linux build information, or could this information somehow
> be in the .dtb files? I had expected some sort of overlay scheme in

(+CC DT folks and mailing list)

I suppose we could make the `fdtoverlay` program embed this data during
the kernel build. That would keep the information together, while also
having one source of truth (the kernel Makefiles). Whether it belongs
in the DTB or not is a separate matter.

> the source, but perhaps people have given up on that?

I wouldn't say given up, since we haven't agreed on anything either.
Elliot had some concerns when I brought this up earlier [1] though.

ChenYu

[1] https://lore.kernel.org/linux-mediatek/20240314113908471-0700.eberman@hu-eberman-lv.qualcomm.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-05  9:48 [PATCH] scripts/make_fit: Support decomposing DTBs Chen-Yu Tsai
  2024-06-10 15:16 ` Simon Glass
@ 2024-06-11 14:32 ` Masahiro Yamada
  2024-06-13  6:06   ` Chen-Yu Tsai
  2024-06-11 20:01 ` Simon Glass
  2 siblings, 1 reply; 12+ messages in thread
From: Masahiro Yamada @ 2024-06-11 14:32 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Simon Glass, Nathan Chancellor, Nicolas Schier, linux-arm-kernel,
	linux-kernel, linux-kbuild

On Wed, Jun 5, 2024 at 6:48 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> The kernel tree builds some "composite" DTBs, where the final DTB is the
> result of applying one or more DTB overlays on top of a base DTB with
> fdtoverlay.
>
> The FIT image specification already supports configurations having one
> base DTB and overlays applied on top. It is then up to the bootloader to
> apply said overlays and either use or pass on the final result. This
> allows the FIT image builder to reuse the same FDT images for multiple
> configurations, if such cases exist.
>
> The decomposition function depends on the kernel build system, reading
> back the .cmd files for the to-be-packaged DTB files to check for the
> fdtoverlay command being called. This will not work outside the kernel
> tree. The function is off by default to keep compatibility with possible
> existing users.
>
> To facilitate the decomposition and keep the code clean, the model and
> compatitble string extraction have been moved out of the output_dtb
> function. The FDT image description is replaced with the base file name
> of the included image.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This is a feature I alluded to in my replies to Simon's original
> submission of the make_fit.py script [1].
>
> This is again made a runtime argument as not all firmware out there
> that boot FIT images support applying overlays. Like my previous
> submission for disabling compression for included FDT images, the
> bootloader found in RK3399 and MT8173 Chromebooks do not support
> applying overlays. Another case of this is U-boot shipped by development
> board vendors in binary form (without upstream) in an image or in
> SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> These would fail to boot FIT images with DT overlays. One such
> example is my Hummingboard Pulse. In these cases the firmware is
> either not upgradable or very hard to upgrade.
>
> I believe there is value in supporting these cases. A common script
> shipped with the kernel source that can be shared by distros means
> the distro people don't have to reimplement this in their downstream
> repos or meta-packages. For ChromeOS this means reducing the amount
> of package code we have in shell script.
>
> [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> [2]
>
>  scripts/Makefile.lib |  1 +
>  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 49 insertions(+), 22 deletions(-)
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 9f06f6aaf7fc..d78b5d38beaa 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
>        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
>                 --name '$(UIMAGE_NAME)' \
>                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
>                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
>
>  # XZ
> diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> index 263147df80a4..120f13e1323c 100755
> --- a/scripts/make_fit.py
> +++ b/scripts/make_fit.py
> @@ -22,6 +22,11 @@ the entire FIT.
>  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
>  zstd algorithms.
>
> +Use -d to decompose "composite" DTBs into their base components and
> +deduplicate the resulting base DTBs and DTB overlays. This requires the
> +DTBs to be sourced from the kernel build directory, as the implementation
> +looks at the .cmd files produced by the kernel build.
> +
>  The resulting FIT can be booted by bootloaders which support FIT, such
>  as U-Boot, Linuxboot, Tianocore, etc.
>
> @@ -64,6 +69,8 @@ def parse_args():
>            help='Specifies the architecture')
>      parser.add_argument('-c', '--compress', type=str, default='none',
>            help='Specifies the compression')
> +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> +          help='Decompose composite DTBs into base DTB and overlays')
>      parser.add_argument('-E', '--external', action='store_true',
>            help='Convert the FIT to use external data')
>      parser.add_argument('-n', '--name', type=str, required=True,
> @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
>      fsw.end_node()
>      seq = 0
>      with fsw.add_node('configurations'):
> -        for model, compat in entries:
> +        for model, compat, files in entries:
>              seq += 1
>              with fsw.add_node(f'conf-{seq}'):
>                  fsw.property('compatible', bytes(compat))
>                  fsw.property_string('description', model)
> -                fsw.property_string('fdt', f'fdt-{seq}')
> +                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))
>                  fsw.property_string('kernel', 'kernel')
>      fsw.end_node()
>
> @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
>          fname (str): Filename containing the DTB
>          arch: FIT architecture, e.g. 'arm64'
>          compress (str): Compressed algorithm, e.g. 'gzip'
> -
> -    Returns:
> -        tuple:
> -            str: Model name
> -            bytes: Compatible stringlist
>      """
>      with fsw.add_node(f'fdt-{seq}'):
> -        # Get the compatible / model information
> -        with open(fname, 'rb') as inf:
> -            data = inf.read()
> -        fdt = libfdt.FdtRo(data)
> -        model = fdt.getprop(0, 'model').as_str()
> -        compat = fdt.getprop(0, 'compatible')
> -
> -        fsw.property_string('description', model)
> +        fsw.property_string('description', os.path.basename(fname))
>          fsw.property_string('type', 'flat_dt')
>          fsw.property_string('arch', arch)
>          fsw.property_string('compression', compress)
> @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
>          with open(fname, 'rb') as inf:
>              compressed = compress_data(inf, compress)
>          fsw.property('data', compressed)
> -    return model, compat
>
>
>  def build_fit(args):
> @@ -235,6 +229,7 @@ def build_fit(args):
>      fsw = libfdt.FdtSw()
>      setup_fit(fsw, args.name)
>      entries = []
> +    fdts = collections.OrderedDict()


I am fine with this patch.

Just a nit.

Is there any reason why you used OrderedDict() instead of
the normal dictionary, "fdts = {}" ?





-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-11  8:51   ` Chen-Yu Tsai
@ 2024-06-11 14:45     ` Masahiro Yamada
  2024-06-13  8:49       ` Chen-Yu Tsai
  2024-06-11 19:54     ` Simon Glass
  2024-06-11 19:59     ` Elliot Berman
  2 siblings, 1 reply; 12+ messages in thread
From: Masahiro Yamada @ 2024-06-11 14:45 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Simon Glass, Nathan Chancellor, Nicolas Schier, linux-arm-kernel,
	linux-kernel, linux-kbuild, Elliot Berman, Devicetree List,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley

On Tue, Jun 11, 2024 at 5:52 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Mon, Jun 10, 2024 at 11:16 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Chen-Yu,
> >
> > On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > > result of applying one or more DTB overlays on top of a base DTB with
> > > fdtoverlay.
> > >
> > > The FIT image specification already supports configurations having one
> > > base DTB and overlays applied on top. It is then up to the bootloader to
> > > apply said overlays and either use or pass on the final result. This
> > > allows the FIT image builder to reuse the same FDT images for multiple
> > > configurations, if such cases exist.
> > >
> > > The decomposition function depends on the kernel build system, reading
> > > back the .cmd files for the to-be-packaged DTB files to check for the
> > > fdtoverlay command being called. This will not work outside the kernel
> > > tree. The function is off by default to keep compatibility with possible
> > > existing users.
> > >
> > > To facilitate the decomposition and keep the code clean, the model and
> > > compatitble string extraction have been moved out of the output_dtb
> > > function. The FDT image description is replaced with the base file name
> > > of the included image.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > This is a feature I alluded to in my replies to Simon's original
> > > submission of the make_fit.py script [1].
> > >
> > > This is again made a runtime argument as not all firmware out there
> > > that boot FIT images support applying overlays. Like my previous
> > > submission for disabling compression for included FDT images, the
> > > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > > applying overlays. Another case of this is U-boot shipped by development
> > > board vendors in binary form (without upstream) in an image or in
> > > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > > These would fail to boot FIT images with DT overlays. One such
> > > example is my Hummingboard Pulse. In these cases the firmware is
> > > either not upgradable or very hard to upgrade.
> > >
> > > I believe there is value in supporting these cases. A common script
> > > shipped with the kernel source that can be shared by distros means
> > > the distro people don't have to reimplement this in their downstream
> > > repos or meta-packages. For ChromeOS this means reducing the amount
> > > of package code we have in shell script.
> > >
> > > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > > [2]
> > >
> > >  scripts/Makefile.lib |  1 +
> > >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> > >  2 files changed, 49 insertions(+), 22 deletions(-)
> >
> > This is a clever way to discover the included files. Does it need to
> > rely on the Linux build information, or could this information somehow
> > be in the .dtb files? I had expected some sort of overlay scheme in
>
> (+CC DT folks and mailing list)
>
> I suppose we could make the `fdtoverlay` program embed this data during
> the kernel build. That would keep the information together, while also
> having one source of truth (the kernel Makefiles). Whether it belongs
> in the DTB or not is a separate matter.


Some time ago, I asked a similar question
with a similar motivation.

https://lore.kernel.org/devicetree-compiler/CAK7LNARV8Bo-tBXMdOu55Wg9uZRXvNiRdkDJ4LH8PwVMnMp4cA@mail.gmail.com/







>
> > the source, but perhaps people have given up on that?
>
> I wouldn't say given up, since we haven't agreed on anything either.
> Elliot had some concerns when I brought this up earlier [1] though.
>
> ChenYu
>
> [1] https://lore.kernel.org/linux-mediatek/20240314113908471-0700.eberman@hu-eberman-lv.qualcomm.com/
>

--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-11  8:51   ` Chen-Yu Tsai
  2024-06-11 14:45     ` Masahiro Yamada
@ 2024-06-11 19:54     ` Simon Glass
  2024-06-11 19:59     ` Elliot Berman
  2 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2024-06-11 19:54 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild, Elliot Berman,
	Devicetree List, Rob Herring, Krzysztof Kozlowski, Conor Dooley

On Tue, 11 Jun 2024 at 02:52, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Mon, Jun 10, 2024 at 11:16 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Chen-Yu,
> >
> > On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > > result of applying one or more DTB overlays on top of a base DTB with
> > > fdtoverlay.
> > >
> > > The FIT image specification already supports configurations having one
> > > base DTB and overlays applied on top. It is then up to the bootloader to
> > > apply said overlays and either use or pass on the final result. This
> > > allows the FIT image builder to reuse the same FDT images for multiple
> > > configurations, if such cases exist.
> > >
> > > The decomposition function depends on the kernel build system, reading
> > > back the .cmd files for the to-be-packaged DTB files to check for the
> > > fdtoverlay command being called. This will not work outside the kernel
> > > tree. The function is off by default to keep compatibility with possible
> > > existing users.
> > >
> > > To facilitate the decomposition and keep the code clean, the model and
> > > compatitble string extraction have been moved out of the output_dtb
> > > function. The FDT image description is replaced with the base file name
> > > of the included image.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > This is a feature I alluded to in my replies to Simon's original
> > > submission of the make_fit.py script [1].
> > >
> > > This is again made a runtime argument as not all firmware out there
> > > that boot FIT images support applying overlays. Like my previous
> > > submission for disabling compression for included FDT images, the
> > > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > > applying overlays. Another case of this is U-boot shipped by development
> > > board vendors in binary form (without upstream) in an image or in
> > > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > > These would fail to boot FIT images with DT overlays. One such
> > > example is my Hummingboard Pulse. In these cases the firmware is
> > > either not upgradable or very hard to upgrade.
> > >
> > > I believe there is value in supporting these cases. A common script
> > > shipped with the kernel source that can be shared by distros means
> > > the distro people don't have to reimplement this in their downstream
> > > repos or meta-packages. For ChromeOS this means reducing the amount
> > > of package code we have in shell script.
> > >
> > > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > > [2]
> > >
> > >  scripts/Makefile.lib |  1 +
> > >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> > >  2 files changed, 49 insertions(+), 22 deletions(-)
> >
> > This is a clever way to discover the included files. Does it need to
> > rely on the Linux build information, or could this information somehow
> > be in the .dtb files? I had expected some sort of overlay scheme in
>
> (+CC DT folks and mailing list)
>
> I suppose we could make the `fdtoverlay` program embed this data during
> the kernel build. That would keep the information together, while also
> having one source of truth (the kernel Makefiles). Whether it belongs
> in the DTB or not is a separate matter.

OK, well we can always look at that later.

>
> > the source, but perhaps people have given up on that?
>
> I wouldn't say given up, since we haven't agreed on anything either.
> Elliot had some concerns when I brought this up earlier [1] though.

OK.

Regards,
Simon

>
> ChenYu
>
> [1] https://lore.kernel.org/linux-mediatek/20240314113908471-0700.eberman@hu-eberman-lv.qualcomm.com/

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-11  8:51   ` Chen-Yu Tsai
  2024-06-11 14:45     ` Masahiro Yamada
  2024-06-11 19:54     ` Simon Glass
@ 2024-06-11 19:59     ` Elliot Berman
  2 siblings, 0 replies; 12+ messages in thread
From: Elliot Berman @ 2024-06-11 19:59 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Simon Glass, Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild, Devicetree List,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley

On Tue, Jun 11, 2024 at 04:51:52PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jun 10, 2024 at 11:16 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Chen-Yu,
> >
> > On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > > result of applying one or more DTB overlays on top of a base DTB with
> > > fdtoverlay.
> > >
> > > The FIT image specification already supports configurations having one
> > > base DTB and overlays applied on top. It is then up to the bootloader to
> > > apply said overlays and either use or pass on the final result. This
> > > allows the FIT image builder to reuse the same FDT images for multiple
> > > configurations, if such cases exist.
> > >
> > > The decomposition function depends on the kernel build system, reading
> > > back the .cmd files for the to-be-packaged DTB files to check for the
> > > fdtoverlay command being called. This will not work outside the kernel
> > > tree. The function is off by default to keep compatibility with possible
> > > existing users.
> > >
> > > To facilitate the decomposition and keep the code clean, the model and
> > > compatitble string extraction have been moved out of the output_dtb
> > > function. The FDT image description is replaced with the base file name
> > > of the included image.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > This is a feature I alluded to in my replies to Simon's original
> > > submission of the make_fit.py script [1].
> > >
> > > This is again made a runtime argument as not all firmware out there
> > > that boot FIT images support applying overlays. Like my previous
> > > submission for disabling compression for included FDT images, the
> > > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > > applying overlays. Another case of this is U-boot shipped by development
> > > board vendors in binary form (without upstream) in an image or in
> > > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > > These would fail to boot FIT images with DT overlays. One such
> > > example is my Hummingboard Pulse. In these cases the firmware is
> > > either not upgradable or very hard to upgrade.
> > >
> > > I believe there is value in supporting these cases. A common script
> > > shipped with the kernel source that can be shared by distros means
> > > the distro people don't have to reimplement this in their downstream
> > > repos or meta-packages. For ChromeOS this means reducing the amount
> > > of package code we have in shell script.
> > >
> > > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > > [2]
> > >
> > >  scripts/Makefile.lib |  1 +
> > >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> > >  2 files changed, 49 insertions(+), 22 deletions(-)
> >
> > This is a clever way to discover the included files. Does it need to
> > rely on the Linux build information, or could this information somehow
> > be in the .dtb files? I had expected some sort of overlay scheme in
> 
> (+CC DT folks and mailing list)
> 
> I suppose we could make the `fdtoverlay` program embed this data during
> the kernel build. That would keep the information together, while also
> having one source of truth (the kernel Makefiles). Whether it belongs
> in the DTB or not is a separate matter.
> 
> > the source, but perhaps people have given up on that?
> 
> I wouldn't say given up, since we haven't agreed on anything either.
> Elliot had some concerns when I brought this up earlier [1] though.
> 

I'd be happy if it was from the DTS.

Thanks,
Elliot


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-05  9:48 [PATCH] scripts/make_fit: Support decomposing DTBs Chen-Yu Tsai
  2024-06-10 15:16 ` Simon Glass
  2024-06-11 14:32 ` Masahiro Yamada
@ 2024-06-11 20:01 ` Simon Glass
  2024-06-13  7:45   ` Chen-Yu Tsai
  2 siblings, 1 reply; 12+ messages in thread
From: Simon Glass @ 2024-06-11 20:01 UTC (permalink / raw)
  To: Chen-Yu Tsai
  Cc: Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild

Hi Chen-Yu,

On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> The kernel tree builds some "composite" DTBs, where the final DTB is the
> result of applying one or more DTB overlays on top of a base DTB with
> fdtoverlay.
>
> The FIT image specification already supports configurations having one
> base DTB and overlays applied on top. It is then up to the bootloader to
> apply said overlays and either use or pass on the final result. This
> allows the FIT image builder to reuse the same FDT images for multiple
> configurations, if such cases exist.
>
> The decomposition function depends on the kernel build system, reading
> back the .cmd files for the to-be-packaged DTB files to check for the
> fdtoverlay command being called. This will not work outside the kernel
> tree. The function is off by default to keep compatibility with possible
> existing users.
>
> To facilitate the decomposition and keep the code clean, the model and
> compatitble string extraction have been moved out of the output_dtb
> function. The FDT image description is replaced with the base file name
> of the included image.
>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> This is a feature I alluded to in my replies to Simon's original
> submission of the make_fit.py script [1].
>
> This is again made a runtime argument as not all firmware out there
> that boot FIT images support applying overlays. Like my previous
> submission for disabling compression for included FDT images, the
> bootloader found in RK3399 and MT8173 Chromebooks do not support
> applying overlays. Another case of this is U-boot shipped by development
> board vendors in binary form (without upstream) in an image or in
> SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> These would fail to boot FIT images with DT overlays. One such
> example is my Hummingboard Pulse. In these cases the firmware is
> either not upgradable or very hard to upgrade.
>
> I believe there is value in supporting these cases. A common script
> shipped with the kernel source that can be shared by distros means
> the distro people don't have to reimplement this in their downstream
> repos or meta-packages. For ChromeOS this means reducing the amount
> of package code we have in shell script.
>
> [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> [2]
>
>  scripts/Makefile.lib |  1 +
>  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 49 insertions(+), 22 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Some possible nits / changes below

>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 9f06f6aaf7fc..d78b5d38beaa 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
>        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
>                 --name '$(UIMAGE_NAME)' \
>                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
>                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
>
>  # XZ
> diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> index 263147df80a4..120f13e1323c 100755
> --- a/scripts/make_fit.py
> +++ b/scripts/make_fit.py
> @@ -22,6 +22,11 @@ the entire FIT.
>  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
>  zstd algorithms.
>
> +Use -d to decompose "composite" DTBs into their base components and
> +deduplicate the resulting base DTBs and DTB overlays. This requires the
> +DTBs to be sourced from the kernel build directory, as the implementation
> +looks at the .cmd files produced by the kernel build.
> +
>  The resulting FIT can be booted by bootloaders which support FIT, such
>  as U-Boot, Linuxboot, Tianocore, etc.
>
> @@ -64,6 +69,8 @@ def parse_args():
>            help='Specifies the architecture')
>      parser.add_argument('-c', '--compress', type=str, default='none',
>            help='Specifies the compression')
> +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> +          help='Decompose composite DTBs into base DTB and overlays')

I wonder if we should reserve -d for --debug? I don't have a strong
opinion though.

>      parser.add_argument('-E', '--external', action='store_true',
>            help='Convert the FIT to use external data')
>      parser.add_argument('-n', '--name', type=str, required=True,
> @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
>      fsw.end_node()
>      seq = 0
>      with fsw.add_node('configurations'):
> -        for model, compat in entries:
> +        for model, compat, files in entries:
>              seq += 1
>              with fsw.add_node(f'conf-{seq}'):
>                  fsw.property('compatible', bytes(compat))
>                  fsw.property_string('description', model)
> -                fsw.property_string('fdt', f'fdt-{seq}')
> +                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))

This looks right to me. It would be nice to use an f string but I
don't know how to do that with bytes.

But do you need the inner [] ?

>                  fsw.property_string('kernel', 'kernel')
>      fsw.end_node()
>
> @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
>          fname (str): Filename containing the DTB
>          arch: FIT architecture, e.g. 'arm64'
>          compress (str): Compressed algorithm, e.g. 'gzip'
> -
> -    Returns:
> -        tuple:
> -            str: Model name
> -            bytes: Compatible stringlist
>      """
>      with fsw.add_node(f'fdt-{seq}'):
> -        # Get the compatible / model information
> -        with open(fname, 'rb') as inf:
> -            data = inf.read()
> -        fdt = libfdt.FdtRo(data)
> -        model = fdt.getprop(0, 'model').as_str()
> -        compat = fdt.getprop(0, 'compatible')
> -
> -        fsw.property_string('description', model)
> +        fsw.property_string('description', os.path.basename(fname))
>          fsw.property_string('type', 'flat_dt')
>          fsw.property_string('arch', arch)
>          fsw.property_string('compression', compress)
> @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
>          with open(fname, 'rb') as inf:
>              compressed = compress_data(inf, compress)
>          fsw.property('data', compressed)
> -    return model, compat
>
>
>  def build_fit(args):
> @@ -235,6 +229,7 @@ def build_fit(args):
>      fsw = libfdt.FdtSw()
>      setup_fit(fsw, args.name)
>      entries = []
> +    fdts = collections.OrderedDict()
>
>      # Handle the kernel
>      with open(args.kernel, 'rb') as inf:
> @@ -243,12 +238,43 @@ def build_fit(args):
>      write_kernel(fsw, comp_data, args)
>
>      for fname in args.dtbs:
> -        # Ignore overlay (.dtbo) files
> -        if os.path.splitext(fname)[1] == '.dtb':
> -            seq += 1
> -            size += os.path.getsize(fname)
> -            model, compat = output_dtb(fsw, seq, fname, args.arch, args.compress)
> -            entries.append([model, compat])
> +        # Ignore non-DTB (*.dtb) files
> +        if os.path.splitext(fname)[1] != '.dtb':
> +            continue
> +
> +        # Get the compatible / model information
> +        with open(fname, 'rb') as inf:
> +            data = inf.read()
> +        fdt = libfdt.FdtRo(data)
> +        model = fdt.getprop(0, 'model').as_str()
> +        compat = fdt.getprop(0, 'compatible')
> +
> +        if args.decompose_dtbs:
> +            # Check if the DTB needs to be decomposed
> +            path, basename = os.path.split(fname)
> +            cmd_fname = os.path.join(path, f'.{basename}.cmd')
> +            with open(cmd_fname, 'r', encoding='ascii') as inf:
> +                cmd = inf.read()
> +
> +            if 'scripts/dtc/fdtoverlay' in cmd:
> +                # This depends on the structure of the composite DTB command
> +                files = cmd.split()
> +                files = files[files.index('-i')+1:]

spaces around +

> +            else:
> +                files = [fname]
> +        else:
> +            files = [fname]

I do wonder if the code from '# Get the compatible' to here would be
better in a separate, documented function, to keep things easier to
understand?

> +
> +        for fn in files:
> +            if fn not in fdts:
> +                seq += 1
> +                size += os.path.getsize(fn)
> +                output_dtb(fsw, seq, fn, args.arch, args.compress)
> +                fdts[fn] = seq
> +
> +        files_seq = [fdts[fn] for fn in files]
> +
> +        entries.append([model, compat, files_seq])
>
>      finish_fit(fsw, entries)
>
> --
> 2.45.1.288.g0e0cd299f1-goog
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-11 14:32 ` Masahiro Yamada
@ 2024-06-13  6:06   ` Chen-Yu Tsai
  0 siblings, 0 replies; 12+ messages in thread
From: Chen-Yu Tsai @ 2024-06-13  6:06 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Simon Glass, Nathan Chancellor, Nicolas Schier, linux-arm-kernel,
	linux-kernel, linux-kbuild

On Tue, Jun 11, 2024 at 10:33 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Jun 5, 2024 at 6:48 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > result of applying one or more DTB overlays on top of a base DTB with
> > fdtoverlay.
> >
> > The FIT image specification already supports configurations having one
> > base DTB and overlays applied on top. It is then up to the bootloader to
> > apply said overlays and either use or pass on the final result. This
> > allows the FIT image builder to reuse the same FDT images for multiple
> > configurations, if such cases exist.
> >
> > The decomposition function depends on the kernel build system, reading
> > back the .cmd files for the to-be-packaged DTB files to check for the
> > fdtoverlay command being called. This will not work outside the kernel
> > tree. The function is off by default to keep compatibility with possible
> > existing users.
> >
> > To facilitate the decomposition and keep the code clean, the model and
> > compatitble string extraction have been moved out of the output_dtb
> > function. The FDT image description is replaced with the base file name
> > of the included image.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > This is a feature I alluded to in my replies to Simon's original
> > submission of the make_fit.py script [1].
> >
> > This is again made a runtime argument as not all firmware out there
> > that boot FIT images support applying overlays. Like my previous
> > submission for disabling compression for included FDT images, the
> > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > applying overlays. Another case of this is U-boot shipped by development
> > board vendors in binary form (without upstream) in an image or in
> > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > These would fail to boot FIT images with DT overlays. One such
> > example is my Hummingboard Pulse. In these cases the firmware is
> > either not upgradable or very hard to upgrade.
> >
> > I believe there is value in supporting these cases. A common script
> > shipped with the kernel source that can be shared by distros means
> > the distro people don't have to reimplement this in their downstream
> > repos or meta-packages. For ChromeOS this means reducing the amount
> > of package code we have in shell script.
> >
> > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > [2]
> >
> >  scripts/Makefile.lib |  1 +
> >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> >  2 files changed, 49 insertions(+), 22 deletions(-)
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 9f06f6aaf7fc..d78b5d38beaa 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
> >        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
> >                 --name '$(UIMAGE_NAME)' \
> >                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> > +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
> >                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
> >
> >  # XZ
> > diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> > index 263147df80a4..120f13e1323c 100755
> > --- a/scripts/make_fit.py
> > +++ b/scripts/make_fit.py
> > @@ -22,6 +22,11 @@ the entire FIT.
> >  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
> >  zstd algorithms.
> >
> > +Use -d to decompose "composite" DTBs into their base components and
> > +deduplicate the resulting base DTBs and DTB overlays. This requires the
> > +DTBs to be sourced from the kernel build directory, as the implementation
> > +looks at the .cmd files produced by the kernel build.
> > +
> >  The resulting FIT can be booted by bootloaders which support FIT, such
> >  as U-Boot, Linuxboot, Tianocore, etc.
> >
> > @@ -64,6 +69,8 @@ def parse_args():
> >            help='Specifies the architecture')
> >      parser.add_argument('-c', '--compress', type=str, default='none',
> >            help='Specifies the compression')
> > +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> > +          help='Decompose composite DTBs into base DTB and overlays')
> >      parser.add_argument('-E', '--external', action='store_true',
> >            help='Convert the FIT to use external data')
> >      parser.add_argument('-n', '--name', type=str, required=True,
> > @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
> >      fsw.end_node()
> >      seq = 0
> >      with fsw.add_node('configurations'):
> > -        for model, compat in entries:
> > +        for model, compat, files in entries:
> >              seq += 1
> >              with fsw.add_node(f'conf-{seq}'):
> >                  fsw.property('compatible', bytes(compat))
> >                  fsw.property_string('description', model)
> > -                fsw.property_string('fdt', f'fdt-{seq}')
> > +                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))
> >                  fsw.property_string('kernel', 'kernel')
> >      fsw.end_node()
> >
> > @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
> >          fname (str): Filename containing the DTB
> >          arch: FIT architecture, e.g. 'arm64'
> >          compress (str): Compressed algorithm, e.g. 'gzip'
> > -
> > -    Returns:
> > -        tuple:
> > -            str: Model name
> > -            bytes: Compatible stringlist
> >      """
> >      with fsw.add_node(f'fdt-{seq}'):
> > -        # Get the compatible / model information
> > -        with open(fname, 'rb') as inf:
> > -            data = inf.read()
> > -        fdt = libfdt.FdtRo(data)
> > -        model = fdt.getprop(0, 'model').as_str()
> > -        compat = fdt.getprop(0, 'compatible')
> > -
> > -        fsw.property_string('description', model)
> > +        fsw.property_string('description', os.path.basename(fname))
> >          fsw.property_string('type', 'flat_dt')
> >          fsw.property_string('arch', arch)
> >          fsw.property_string('compression', compress)
> > @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
> >          with open(fname, 'rb') as inf:
> >              compressed = compress_data(inf, compress)
> >          fsw.property('data', compressed)
> > -    return model, compat
> >
> >
> >  def build_fit(args):
> > @@ -235,6 +229,7 @@ def build_fit(args):
> >      fsw = libfdt.FdtSw()
> >      setup_fit(fsw, args.name)
> >      entries = []
> > +    fdts = collections.OrderedDict()
>
>
> I am fine with this patch.
>
> Just a nit.
>
> Is there any reason why you used OrderedDict() instead of
> the normal dictionary, "fdts = {}" ?

I had wanted to use it as the main list of entries; using OrderedDict()
preserves the order that the DTBs were given. That didn't pan out.

I'll replace it with the standard dictionary.


ChenYu

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-11 20:01 ` Simon Glass
@ 2024-06-13  7:45   ` Chen-Yu Tsai
  2024-06-13  8:36     ` Chen-Yu Tsai
  0 siblings, 1 reply; 12+ messages in thread
From: Chen-Yu Tsai @ 2024-06-13  7:45 UTC (permalink / raw)
  To: Simon Glass
  Cc: Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild

On Wed, Jun 12, 2024 at 4:01 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Chen-Yu,
>
> On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > result of applying one or more DTB overlays on top of a base DTB with
> > fdtoverlay.
> >
> > The FIT image specification already supports configurations having one
> > base DTB and overlays applied on top. It is then up to the bootloader to
> > apply said overlays and either use or pass on the final result. This
> > allows the FIT image builder to reuse the same FDT images for multiple
> > configurations, if such cases exist.
> >
> > The decomposition function depends on the kernel build system, reading
> > back the .cmd files for the to-be-packaged DTB files to check for the
> > fdtoverlay command being called. This will not work outside the kernel
> > tree. The function is off by default to keep compatibility with possible
> > existing users.
> >
> > To facilitate the decomposition and keep the code clean, the model and
> > compatitble string extraction have been moved out of the output_dtb
> > function. The FDT image description is replaced with the base file name
> > of the included image.
> >
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > This is a feature I alluded to in my replies to Simon's original
> > submission of the make_fit.py script [1].
> >
> > This is again made a runtime argument as not all firmware out there
> > that boot FIT images support applying overlays. Like my previous
> > submission for disabling compression for included FDT images, the
> > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > applying overlays. Another case of this is U-boot shipped by development
> > board vendors in binary form (without upstream) in an image or in
> > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > These would fail to boot FIT images with DT overlays. One such
> > example is my Hummingboard Pulse. In these cases the firmware is
> > either not upgradable or very hard to upgrade.
> >
> > I believe there is value in supporting these cases. A common script
> > shipped with the kernel source that can be shared by distros means
> > the distro people don't have to reimplement this in their downstream
> > repos or meta-packages. For ChromeOS this means reducing the amount
> > of package code we have in shell script.
> >
> > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > [2]
> >
> >  scripts/Makefile.lib |  1 +
> >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> >  2 files changed, 49 insertions(+), 22 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Some possible nits / changes below
>
> >
> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 9f06f6aaf7fc..d78b5d38beaa 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
> >        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
> >                 --name '$(UIMAGE_NAME)' \
> >                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> > +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
> >                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
> >
> >  # XZ
> > diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> > index 263147df80a4..120f13e1323c 100755
> > --- a/scripts/make_fit.py
> > +++ b/scripts/make_fit.py
> > @@ -22,6 +22,11 @@ the entire FIT.
> >  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
> >  zstd algorithms.
> >
> > +Use -d to decompose "composite" DTBs into their base components and
> > +deduplicate the resulting base DTBs and DTB overlays. This requires the
> > +DTBs to be sourced from the kernel build directory, as the implementation
> > +looks at the .cmd files produced by the kernel build.
> > +
> >  The resulting FIT can be booted by bootloaders which support FIT, such
> >  as U-Boot, Linuxboot, Tianocore, etc.
> >
> > @@ -64,6 +69,8 @@ def parse_args():
> >            help='Specifies the architecture')
> >      parser.add_argument('-c', '--compress', type=str, default='none',
> >            help='Specifies the compression')
> > +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> > +          help='Decompose composite DTBs into base DTB and overlays')
>
> I wonder if we should reserve -d for --debug? I don't have a strong
> opinion though.

Seems reasonable. I'll make it use the capital -D then.

> >      parser.add_argument('-E', '--external', action='store_true',
> >            help='Convert the FIT to use external data')
> >      parser.add_argument('-n', '--name', type=str, required=True,
> > @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
> >      fsw.end_node()
> >      seq = 0
> >      with fsw.add_node('configurations'):
> > -        for model, compat in entries:
> > +        for model, compat, files in entries:
> >              seq += 1
> >              with fsw.add_node(f'conf-{seq}'):
> >                  fsw.property('compatible', bytes(compat))
> >                  fsw.property_string('description', model)
> > -                fsw.property_string('fdt', f'fdt-{seq}')
> > +                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))
>
> This looks right to me. It would be nice to use an f string but I
> don't know how to do that with bytes.

Me neither. Switching the format to an f string doesn't work:

  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 324, in <module>
    sys.exit(run_make_fit())
             ^^^^^^^^^^^^^^
  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 298, in run_make_fit
    out_data, count, size = build_fit(args)
                            ^^^^^^^^^^^^^^^
  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 288, in build_fit
    finish_fit(fsw, entries)
  File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 161, in finish_fit
    fsw.property('fdt', b''.join([f'fdt-%d\x00' % x for x in files]))
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: sequence item 0: expected a bytes-like object, str found

> But do you need the inner [] ?

Nope. Will remove.

>
> >                  fsw.property_string('kernel', 'kernel')
> >      fsw.end_node()
> >
> > @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
> >          fname (str): Filename containing the DTB
> >          arch: FIT architecture, e.g. 'arm64'
> >          compress (str): Compressed algorithm, e.g. 'gzip'
> > -
> > -    Returns:
> > -        tuple:
> > -            str: Model name
> > -            bytes: Compatible stringlist
> >      """
> >      with fsw.add_node(f'fdt-{seq}'):
> > -        # Get the compatible / model information
> > -        with open(fname, 'rb') as inf:
> > -            data = inf.read()
> > -        fdt = libfdt.FdtRo(data)
> > -        model = fdt.getprop(0, 'model').as_str()
> > -        compat = fdt.getprop(0, 'compatible')
> > -
> > -        fsw.property_string('description', model)
> > +        fsw.property_string('description', os.path.basename(fname))
> >          fsw.property_string('type', 'flat_dt')
> >          fsw.property_string('arch', arch)
> >          fsw.property_string('compression', compress)
> > @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
> >          with open(fname, 'rb') as inf:
> >              compressed = compress_data(inf, compress)
> >          fsw.property('data', compressed)
> > -    return model, compat
> >
> >
> >  def build_fit(args):
> > @@ -235,6 +229,7 @@ def build_fit(args):
> >      fsw = libfdt.FdtSw()
> >      setup_fit(fsw, args.name)
> >      entries = []
> > +    fdts = collections.OrderedDict()
> >
> >      # Handle the kernel
> >      with open(args.kernel, 'rb') as inf:
> > @@ -243,12 +238,43 @@ def build_fit(args):
> >      write_kernel(fsw, comp_data, args)
> >
> >      for fname in args.dtbs:
> > -        # Ignore overlay (.dtbo) files
> > -        if os.path.splitext(fname)[1] == '.dtb':
> > -            seq += 1
> > -            size += os.path.getsize(fname)
> > -            model, compat = output_dtb(fsw, seq, fname, args.arch, args.compress)
> > -            entries.append([model, compat])
> > +        # Ignore non-DTB (*.dtb) files
> > +        if os.path.splitext(fname)[1] != '.dtb':
> > +            continue
> > +
> > +        # Get the compatible / model information
> > +        with open(fname, 'rb') as inf:
> > +            data = inf.read()
> > +        fdt = libfdt.FdtRo(data)
> > +        model = fdt.getprop(0, 'model').as_str()
> > +        compat = fdt.getprop(0, 'compatible')
> > +
> > +        if args.decompose_dtbs:
> > +            # Check if the DTB needs to be decomposed
> > +            path, basename = os.path.split(fname)
> > +            cmd_fname = os.path.join(path, f'.{basename}.cmd')
> > +            with open(cmd_fname, 'r', encoding='ascii') as inf:
> > +                cmd = inf.read()
> > +
> > +            if 'scripts/dtc/fdtoverlay' in cmd:
> > +                # This depends on the structure of the composite DTB command
> > +                files = cmd.split()
> > +                files = files[files.index('-i')+1:]
>
> spaces around +

Will fix.

> > +            else:
> > +                files = [fname]
> > +        else:
> > +            files = [fname]
>
> I do wonder if the code from '# Get the compatible' to here would be
> better in a separate, documented function, to keep things easier to
> understand?

I'll see what I can do. In that case I'll drop your Reviewed-by.


Thanks
ChenYu

> > +
> > +        for fn in files:
> > +            if fn not in fdts:
> > +                seq += 1
> > +                size += os.path.getsize(fn)
> > +                output_dtb(fsw, seq, fn, args.arch, args.compress)
> > +                fdts[fn] = seq
> > +
> > +        files_seq = [fdts[fn] for fn in files]
> > +
> > +        entries.append([model, compat, files_seq])
> >
> >      finish_fit(fsw, entries)
> >
> > --
> > 2.45.1.288.g0e0cd299f1-goog
> >
>
> Regards,
> Simon

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-13  7:45   ` Chen-Yu Tsai
@ 2024-06-13  8:36     ` Chen-Yu Tsai
  0 siblings, 0 replies; 12+ messages in thread
From: Chen-Yu Tsai @ 2024-06-13  8:36 UTC (permalink / raw)
  To: Simon Glass
  Cc: Masahiro Yamada, Nathan Chancellor, Nicolas Schier,
	linux-arm-kernel, linux-kernel, linux-kbuild

On Thu, Jun 13, 2024 at 3:45 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
>
> On Wed, Jun 12, 2024 at 4:01 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Chen-Yu,
> >
> > On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > >
> > > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > > result of applying one or more DTB overlays on top of a base DTB with
> > > fdtoverlay.
> > >
> > > The FIT image specification already supports configurations having one
> > > base DTB and overlays applied on top. It is then up to the bootloader to
> > > apply said overlays and either use or pass on the final result. This
> > > allows the FIT image builder to reuse the same FDT images for multiple
> > > configurations, if such cases exist.
> > >
> > > The decomposition function depends on the kernel build system, reading
> > > back the .cmd files for the to-be-packaged DTB files to check for the
> > > fdtoverlay command being called. This will not work outside the kernel
> > > tree. The function is off by default to keep compatibility with possible
> > > existing users.
> > >
> > > To facilitate the decomposition and keep the code clean, the model and
> > > compatitble string extraction have been moved out of the output_dtb
> > > function. The FDT image description is replaced with the base file name
> > > of the included image.
> > >
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > This is a feature I alluded to in my replies to Simon's original
> > > submission of the make_fit.py script [1].
> > >
> > > This is again made a runtime argument as not all firmware out there
> > > that boot FIT images support applying overlays. Like my previous
> > > submission for disabling compression for included FDT images, the
> > > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > > applying overlays. Another case of this is U-boot shipped by development
> > > board vendors in binary form (without upstream) in an image or in
> > > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > > These would fail to boot FIT images with DT overlays. One such
> > > example is my Hummingboard Pulse. In these cases the firmware is
> > > either not upgradable or very hard to upgrade.
> > >
> > > I believe there is value in supporting these cases. A common script
> > > shipped with the kernel source that can be shared by distros means
> > > the distro people don't have to reimplement this in their downstream
> > > repos or meta-packages. For ChromeOS this means reducing the amount
> > > of package code we have in shell script.
> > >
> > > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > > [2]
> > >
> > >  scripts/Makefile.lib |  1 +
> > >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> > >  2 files changed, 49 insertions(+), 22 deletions(-)
> >
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Some possible nits / changes below
> >
> > >
> > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > index 9f06f6aaf7fc..d78b5d38beaa 100644
> > > --- a/scripts/Makefile.lib
> > > +++ b/scripts/Makefile.lib
> > > @@ -522,6 +522,7 @@ quiet_cmd_fit = FIT     $@
> > >        cmd_fit = $(MAKE_FIT) -o $@ --arch $(UIMAGE_ARCH) --os linux \
> > >                 --name '$(UIMAGE_NAME)' \
> > >                 $(if $(findstring 1,$(KBUILD_VERBOSE)),-v) \
> > > +               $(if $(FIT_DECOMPOSE_DTBS),--decompose-dtbs) \
> > >                 --compress $(FIT_COMPRESSION) -k $< @$(word 2,$^)
> > >
> > >  # XZ
> > > diff --git a/scripts/make_fit.py b/scripts/make_fit.py
> > > index 263147df80a4..120f13e1323c 100755
> > > --- a/scripts/make_fit.py
> > > +++ b/scripts/make_fit.py
> > > @@ -22,6 +22,11 @@ the entire FIT.
> > >  Use -c to compress the data, using bzip2, gzip, lz4, lzma, lzo and
> > >  zstd algorithms.
> > >
> > > +Use -d to decompose "composite" DTBs into their base components and
> > > +deduplicate the resulting base DTBs and DTB overlays. This requires the
> > > +DTBs to be sourced from the kernel build directory, as the implementation
> > > +looks at the .cmd files produced by the kernel build.
> > > +
> > >  The resulting FIT can be booted by bootloaders which support FIT, such
> > >  as U-Boot, Linuxboot, Tianocore, etc.
> > >
> > > @@ -64,6 +69,8 @@ def parse_args():
> > >            help='Specifies the architecture')
> > >      parser.add_argument('-c', '--compress', type=str, default='none',
> > >            help='Specifies the compression')
> > > +    parser.add_argument('-d', '--decompose-dtbs', action='store_true',
> > > +          help='Decompose composite DTBs into base DTB and overlays')
> >
> > I wonder if we should reserve -d for --debug? I don't have a strong
> > opinion though.
>
> Seems reasonable. I'll make it use the capital -D then.
>
> > >      parser.add_argument('-E', '--external', action='store_true',
> > >            help='Convert the FIT to use external data')
> > >      parser.add_argument('-n', '--name', type=str, required=True,
> > > @@ -140,12 +147,12 @@ def finish_fit(fsw, entries):
> > >      fsw.end_node()
> > >      seq = 0
> > >      with fsw.add_node('configurations'):
> > > -        for model, compat in entries:
> > > +        for model, compat, files in entries:
> > >              seq += 1
> > >              with fsw.add_node(f'conf-{seq}'):
> > >                  fsw.property('compatible', bytes(compat))
> > >                  fsw.property_string('description', model)
> > > -                fsw.property_string('fdt', f'fdt-{seq}')
> > > +                fsw.property('fdt', b''.join([b'fdt-%d\x00' % x for x in files]))
> >
> > This looks right to me. It would be nice to use an f string but I
> > don't know how to do that with bytes.
>
> Me neither. Switching the format to an f string doesn't work:
>
>   File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 324, in <module>
>     sys.exit(run_make_fit())
>              ^^^^^^^^^^^^^^
>   File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 298, in run_make_fit
>     out_data, count, size = build_fit(args)
>                             ^^^^^^^^^^^^^^^
>   File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 288, in build_fit
>     finish_fit(fsw, entries)
>   File "/ssd1/wenst/linux/src/scripts/make_fit.py", line 161, in finish_fit
>     fsw.property('fdt', b''.join([f'fdt-%d\x00' % x for x in files]))
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> TypeError: sequence item 0: expected a bytes-like object, str found

    bytes(''.join(f'fdt-{x}\x00' for x in files), "ascii")

Seems to work.

ChenYu

> > But do you need the inner [] ?
>
> Nope. Will remove.
>
> >
> > >                  fsw.property_string('kernel', 'kernel')
> > >      fsw.end_node()
> > >
> > > @@ -193,21 +200,9 @@ def output_dtb(fsw, seq, fname, arch, compress):
> > >          fname (str): Filename containing the DTB
> > >          arch: FIT architecture, e.g. 'arm64'
> > >          compress (str): Compressed algorithm, e.g. 'gzip'
> > > -
> > > -    Returns:
> > > -        tuple:
> > > -            str: Model name
> > > -            bytes: Compatible stringlist
> > >      """
> > >      with fsw.add_node(f'fdt-{seq}'):
> > > -        # Get the compatible / model information
> > > -        with open(fname, 'rb') as inf:
> > > -            data = inf.read()
> > > -        fdt = libfdt.FdtRo(data)
> > > -        model = fdt.getprop(0, 'model').as_str()
> > > -        compat = fdt.getprop(0, 'compatible')
> > > -
> > > -        fsw.property_string('description', model)
> > > +        fsw.property_string('description', os.path.basename(fname))
> > >          fsw.property_string('type', 'flat_dt')
> > >          fsw.property_string('arch', arch)
> > >          fsw.property_string('compression', compress)
> > > @@ -215,7 +210,6 @@ def output_dtb(fsw, seq, fname, arch, compress):
> > >          with open(fname, 'rb') as inf:
> > >              compressed = compress_data(inf, compress)
> > >          fsw.property('data', compressed)
> > > -    return model, compat
> > >
> > >
> > >  def build_fit(args):
> > > @@ -235,6 +229,7 @@ def build_fit(args):
> > >      fsw = libfdt.FdtSw()
> > >      setup_fit(fsw, args.name)
> > >      entries = []
> > > +    fdts = collections.OrderedDict()
> > >
> > >      # Handle the kernel
> > >      with open(args.kernel, 'rb') as inf:
> > > @@ -243,12 +238,43 @@ def build_fit(args):
> > >      write_kernel(fsw, comp_data, args)
> > >
> > >      for fname in args.dtbs:
> > > -        # Ignore overlay (.dtbo) files
> > > -        if os.path.splitext(fname)[1] == '.dtb':
> > > -            seq += 1
> > > -            size += os.path.getsize(fname)
> > > -            model, compat = output_dtb(fsw, seq, fname, args.arch, args.compress)
> > > -            entries.append([model, compat])
> > > +        # Ignore non-DTB (*.dtb) files
> > > +        if os.path.splitext(fname)[1] != '.dtb':
> > > +            continue
> > > +
> > > +        # Get the compatible / model information
> > > +        with open(fname, 'rb') as inf:
> > > +            data = inf.read()
> > > +        fdt = libfdt.FdtRo(data)
> > > +        model = fdt.getprop(0, 'model').as_str()
> > > +        compat = fdt.getprop(0, 'compatible')
> > > +
> > > +        if args.decompose_dtbs:
> > > +            # Check if the DTB needs to be decomposed
> > > +            path, basename = os.path.split(fname)
> > > +            cmd_fname = os.path.join(path, f'.{basename}.cmd')
> > > +            with open(cmd_fname, 'r', encoding='ascii') as inf:
> > > +                cmd = inf.read()
> > > +
> > > +            if 'scripts/dtc/fdtoverlay' in cmd:
> > > +                # This depends on the structure of the composite DTB command
> > > +                files = cmd.split()
> > > +                files = files[files.index('-i')+1:]
> >
> > spaces around +
>
> Will fix.
>
> > > +            else:
> > > +                files = [fname]
> > > +        else:
> > > +            files = [fname]
> >
> > I do wonder if the code from '# Get the compatible' to here would be
> > better in a separate, documented function, to keep things easier to
> > understand?
>
> I'll see what I can do. In that case I'll drop your Reviewed-by.
>
>
> Thanks
> ChenYu
>
> > > +
> > > +        for fn in files:
> > > +            if fn not in fdts:
> > > +                seq += 1
> > > +                size += os.path.getsize(fn)
> > > +                output_dtb(fsw, seq, fn, args.arch, args.compress)
> > > +                fdts[fn] = seq
> > > +
> > > +        files_seq = [fdts[fn] for fn in files]
> > > +
> > > +        entries.append([model, compat, files_seq])
> > >
> > >      finish_fit(fsw, entries)
> > >
> > > --
> > > 2.45.1.288.g0e0cd299f1-goog
> > >
> >
> > Regards,
> > Simon

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] scripts/make_fit: Support decomposing DTBs
  2024-06-11 14:45     ` Masahiro Yamada
@ 2024-06-13  8:49       ` Chen-Yu Tsai
  0 siblings, 0 replies; 12+ messages in thread
From: Chen-Yu Tsai @ 2024-06-13  8:49 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Simon Glass, Nathan Chancellor, Nicolas Schier, linux-arm-kernel,
	linux-kernel, linux-kbuild, Elliot Berman, Devicetree List,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley

On Tue, Jun 11, 2024 at 10:45 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Tue, Jun 11, 2024 at 5:52 PM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > On Mon, Jun 10, 2024 at 11:16 PM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Chen-Yu,
> > >
> > > On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@chromium.org> wrote:
> > > >
> > > > The kernel tree builds some "composite" DTBs, where the final DTB is the
> > > > result of applying one or more DTB overlays on top of a base DTB with
> > > > fdtoverlay.
> > > >
> > > > The FIT image specification already supports configurations having one
> > > > base DTB and overlays applied on top. It is then up to the bootloader to
> > > > apply said overlays and either use or pass on the final result. This
> > > > allows the FIT image builder to reuse the same FDT images for multiple
> > > > configurations, if such cases exist.
> > > >
> > > > The decomposition function depends on the kernel build system, reading
> > > > back the .cmd files for the to-be-packaged DTB files to check for the
> > > > fdtoverlay command being called. This will not work outside the kernel
> > > > tree. The function is off by default to keep compatibility with possible
> > > > existing users.
> > > >
> > > > To facilitate the decomposition and keep the code clean, the model and
> > > > compatitble string extraction have been moved out of the output_dtb
> > > > function. The FDT image description is replaced with the base file name
> > > > of the included image.
> > > >
> > > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > > ---
> > > > This is a feature I alluded to in my replies to Simon's original
> > > > submission of the make_fit.py script [1].
> > > >
> > > > This is again made a runtime argument as not all firmware out there
> > > > that boot FIT images support applying overlays. Like my previous
> > > > submission for disabling compression for included FDT images, the
> > > > bootloader found in RK3399 and MT8173 Chromebooks do not support
> > > > applying overlays. Another case of this is U-boot shipped by development
> > > > board vendors in binary form (without upstream) in an image or in
> > > > SPI flash on the board that were built with OF_LIBFDT_OVERLAY=n.
> > > > These would fail to boot FIT images with DT overlays. One such
> > > > example is my Hummingboard Pulse. In these cases the firmware is
> > > > either not upgradable or very hard to upgrade.
> > > >
> > > > I believe there is value in supporting these cases. A common script
> > > > shipped with the kernel source that can be shared by distros means
> > > > the distro people don't have to reimplement this in their downstream
> > > > repos or meta-packages. For ChromeOS this means reducing the amount
> > > > of package code we have in shell script.
> > > >
> > > > [1] https://lore.kernel.org/linux-kbuild/20231207142723.GA3187877@google.com/
> > > > [2]
> > > >
> > > >  scripts/Makefile.lib |  1 +
> > > >  scripts/make_fit.py  | 70 ++++++++++++++++++++++++++++++--------------
> > > >  2 files changed, 49 insertions(+), 22 deletions(-)
> > >
> > > This is a clever way to discover the included files. Does it need to
> > > rely on the Linux build information, or could this information somehow
> > > be in the .dtb files? I had expected some sort of overlay scheme in
> >
> > (+CC DT folks and mailing list)
> >
> > I suppose we could make the `fdtoverlay` program embed this data during
> > the kernel build. That would keep the information together, while also
> > having one source of truth (the kernel Makefiles). Whether it belongs
> > in the DTB or not is a separate matter.
>
>
> Some time ago, I asked a similar question
> with a similar motivation.
>
> https://lore.kernel.org/devicetree-compiler/CAK7LNARV8Bo-tBXMdOu55Wg9uZRXvNiRdkDJ4LH8PwVMnMp4cA@mail.gmail.com/

I think this discussion was geared towards "unapplying" overlays. What
we would like is for metadata to be available, and preferably not tied
to the kernel build system generated metadata file, so that with the same
bunch of DTB + DTBO files, we could figure out how to decompose them and
just bundle the components.

If the metadata is embedded within the composite DTB, then given a DTB
bundle (such as installed with `make dtbs_install`) the make_fit.py
script could go and do the decomposition without the *.cmd files
from the kernel build. This is assuming all the component parts are
installed together.

ChenYu

> >
> > > the source, but perhaps people have given up on that?
> >
> > I wouldn't say given up, since we haven't agreed on anything either.
> > Elliot had some concerns when I brought this up earlier [1] though.
> >
> > ChenYu
> >
> > [1] https://lore.kernel.org/linux-mediatek/20240314113908471-0700.eberman@hu-eberman-lv.qualcomm.com/
> >
>
> --
> Best Regards
> Masahiro Yamada

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2024-06-13  8:49 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-06-05  9:48 [PATCH] scripts/make_fit: Support decomposing DTBs Chen-Yu Tsai
2024-06-10 15:16 ` Simon Glass
2024-06-11  8:51   ` Chen-Yu Tsai
2024-06-11 14:45     ` Masahiro Yamada
2024-06-13  8:49       ` Chen-Yu Tsai
2024-06-11 19:54     ` Simon Glass
2024-06-11 19:59     ` Elliot Berman
2024-06-11 14:32 ` Masahiro Yamada
2024-06-13  6:06   ` Chen-Yu Tsai
2024-06-11 20:01 ` Simon Glass
2024-06-13  7:45   ` Chen-Yu Tsai
2024-06-13  8:36     ` Chen-Yu Tsai

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®