Re: [Buildroot] [PATCH v2 15/18] package/qoriq-mc-utils: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Vlad,

Thanks for the v2, which I have applied. I have some comments below,
though.

On Fri, 28 Mar 2025 20:04:23 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> The configuration files for the MC firmware binary are distributed
> through a separate repository on GitHub, and need a different package.
> They are licensed differently than the firmware itself, and unlike the
> firmware, they are customizable.
> 
> There are two ways for a board to use this package - similar to
> qoriq-rcw. If it is an NXP reference board or if the example files
> otherwise work fine with it, it is recommended to set the _INTREE
> variables to select a pre-existing DPL and DPC. Otherwise, if it is a
> custom board, the best solution is to just provide the DPL and DPC dts
> files in board/, and set the _CUSTOM_PATH variables to point to them.
> 
> There are also two ways to deploy to the target.
> 
> Traditionally in NXP BSPs, U-Boot loads the MC firmware, DPL and DPC
> from given offsets in the storage medium (outside of the filesystem).
> But this is not hardcoded and it doesn't have to be the case - the
> mcinitcmd U-Boot environment variable is freely customizable. What can
> also be done, and is done for the LX2160A-RDB, is to deploy multiple DPL
> and DPC files (all the files available for a board) to a folder of the
> rootfs, and just have two symlinks: dpl.dtb and dpc.dtb which point to
> the currently active files. This makes easier the processes of
> upgrading, downgrading and keeping multiple file versions.
> 
> Nonetheless, the "traditional" method of deploying to the target is also
> possible. The selected DPL and DPC files are deployed to the "images"
> folder and are freely usable with genimage or other post-image scripts.

Very nice commit log, really. It does help people like me, who are not
familiar with the details of QorIQ platforms, understand why the
package is done like this. So clearly thanks a lot for this commit log
(the one on qoriq-mc-binary was equally useful).


> +QORIQ_MC_UTILS_VERSION = 10.39.0
> +QORIQ_MC_UTILS_SITE = $(call github,nxp-qoriq,mc-utils,mc_release_$(QORIQ_MC_UTILS_VERSION))
> +QORIQ_MC_UTILS_LICENSE = BSD-3-Clause
> +QORIQ_MC_UTILS_INSTALL_IMAGES = YES
> +ifeq ($(BR2_PACKAGE_QORIQ_MC_UTILS_TARGET_INSTALL_PATH),)
> +QORIQ_MC_UTILS_INSTALL_TARGET = NO
> +endif

For the same reason as for qoriq-mc-binary, this couldn't work as
$(BR2_PACKAGE_QORIQ_MC_UTILS_TARGET_INSTALL_PATH) is never empty.

> +QORIQ_MC_UTILS_DEPENDENCIES = host-dtc
> +
> +QORIQ_MC_UTILS_DPC_FILES = $(call qstrip,$(BR2_PACKAGE_QORIQ_MC_UTILS_DPC_CUSTOM_PATH))
> +QORIQ_MC_UTILS_DPC_INTREE = $(call qstrip,$(BR2_PACKAGE_QORIQ_MC_UTILS_DPC_INTREE))
> +QORIQ_MC_UTILS_DPL_FILES = $(call qstrip,$(BR2_PACKAGE_QORIQ_MC_UTILS_DPL_CUSTOM_PATH))
> +QORIQ_MC_UTILS_DPL_INTREE = $(call qstrip,$(BR2_PACKAGE_QORIQ_MC_UTILS_DPL_INTREE))
> +QORIQ_MC_UTILS_INSTALL_PATH = $(call qstrip,$(BR2_PACKAGE_QORIQ_MC_UTILS_TARGET_INSTALL_PATH))
> +
> +define QORIQ_MC_UTILS_INSTALL_FILES
> +	$(foreach file, $(QORIQ_MC_UTILS_DPC_FILES) $(QORIQ_MC_UTILS_DPL_FILES), \
> +		PATH=$(BR_PATH) dtc -I dts -O dtb $(file).dts -o $(1)/$(notdir $(file)).dtb; )
> +	$(foreach file, $(QORIQ_MC_UTILS_DPC_INTREE) $(QORIQ_MC_UTILS_DPL_INTREE), \
> +		$(INSTALL) -D $(@D)/config/$(file).dtb $(1)/$(notdir $(file)).dtb; )

Nit: I dropped the ; and moved the parenthesis to the next line, as
this is usually how we format such loops in Buildroot.

However, initially I was confused: you were not doing anything with the
external DPC/DPL files, so I was wondering how the BUILD_CMDS could do
something with them. Turns out you can actually build them "manually"
with dtc at install time.

So I was initially a bit hesitant: I think it would have made more
sense to actually build those files in the build step, and only do the
installation at the install step. Right now, you might be building them
twice (once when installing to images, once when installing to target).
Sure thing, a dtc run isn't long, so performance-wise not a problem.
But if we imagine that DTC was stored a timestamp in the .dtb, then the
.dtb in images/ and target/ would be different. Not super nice, but
overall I decided that it was a bit bike-shedding, so I applied your
patch as-is. But I thought it was worth sharing my thoughts
nevertheless.

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot