On Thu, Jun 13, 2024 at 3:45 PM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote: > > On Wed, Jun 12, 2024 at 4:01 AM Simon Glass <sjg@xxxxxxxxxxxx> wrote: > > > > Hi Chen-Yu, > > > > On Wed, 5 Jun 2024 at 03:48, Chen-Yu Tsai <wenst@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx/ > > > [2] > > > > > > scripts/Makefile.lib | 1 + > > > scripts/make_fit.py | 70 ++++++++++++++++++++++++++++++-------------- > > > 2 files changed, 49 insertions(+), 22 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@xxxxxxxxxxxx> > > > > 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
