Re: [Buildroot] [PATCH v3 2/3] board/raspberrypi: allow non-matching globs for DTBs

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Mon, Feb 17, 2025 at 01:33:52PM +0100, Fiona Klute wrote:
> Hi Niklas!
> 
> Am 10.02.25 um 13:47 schrieb Niklas Cassel:
> > On Fri, Feb 07, 2025 at 06:14:57PM +0100, Fiona Klute via buildroot wrote:
> > > From: "Fiona Klute (WIWA)" <fiona.klute@gmx.de>
> > > 
> > > When installing DTBs from the rpi-firmware package
> > > (BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS=y), there's no DTB directly in
> > > the images directory because the kernel build doesn't provide one.
> > 
> > Your new defconfig does not seem to have BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS
> > set. What am I missing?
> 
> This change is independent of the defconfig, which is aimed at a board
> that's well supported by in-tree DTS (both mainline and RPi downstream).
> The only connection is that both are about support for RPi boards.
> 
> > Regardless, perhaps clarify/rephrase your commit message.
> 
> What needs clarifying in your opinion?

This series adds a new defconfig:
configs/raspberrypicm4io_64_uboot_defconfig

Since these 3 patches are in a series, it is logical that patches 1-2 are
actually needed for patch 3.

The commit message in patch 2 references
BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS=y
However, the defconfig in patch 3 does not select this config.

So, if patch 2 is completely unrelated to patch 3, why submit it in the
same series?


After looking at package/rpi-firmware/Config.in it appears that
BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS is "default y"
(and you new defconfig does select BR2_PACKAGE_RPI_FIRMWARE).

So I guess the answer is: because package/rpi-firmware/Config.in is silly.

A config option to install something is usually never "default y".
To me it is very counter-intuitive that defconfigs that want firmware
installed, but do not want DTBs installed would have to do:
BR2_PACKAGE_RPI_FIRMWARE=y
# BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS is not set

Rather than configs that want both installed would do:
BR2_PACKAGE_RPI_FIRMWARE=y
BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS=y

But this is unrelated to you series, so feel free to ignore my comment.


> 
> > > Using pre-compiled DTBs is discouraged, but useful for using mainline
> > > kernels until suitable DTS can be merged, or to provide U-Boot with a
> > > DTB (via the firmware bootloader) independent of whatever kernel DTB
> > > is in use.
> > 
> > I don't really understand which pre-compiled DTB you are using, since
> > your series is not adding any DTB, and your defconfig does not have
> > BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS set.
> > 
> > However, wouldn't it be much better to rebase this series on either:
> > 
> > 1) Romain's series:
> > https://lore.kernel.org/buildroot/20250204163622.192932-3-romain.naour@smile.fr/
> > which introduces BR2_LINUX_KERNEL_CUSTOM_DTS_PATH
> > 
> > or
> > 
> > 2) Your own series:
> > https://lore.kernel.org/buildroot/20250207183455.1468220-1-fiona.klute@gmx.de/
> > 
> > 
> > Then this series could provide a DT source file (.dts) instead of a binary
> > blob (.dtb). Having a DTS file would make it easier for other buildroot
> > contributors to make changes, and once upstream has a DTS merged, you can
> > remove your DTS file provided by buildroot.
> 
> This patch has nothing to do with either series. Both of those series
> aim to support building out-of-tree DTS and installing the resulting DTB.
> 
> This patch allows the post-image script to work if the user does not
> have the kernel (or tools added by one of those series) build a DTB, and
> instead sets BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS=y. If they have a
> suitable DTS to build, there's no point in using that option. The
> rpi-firmware package installs its pre-compiled DTBs into
> "${BINARIES_DIR}"/rpi-firmware/, so the "${BINARIES_DIR}"/*.dtb glob
> won't match anything, and including the literal glob in the FILES list
> will lead to "file not found" errors.
> 
> In short: The purpose of this patch is only to make
> BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTBS=y work without forcing the user to
> adjust the post-image script.

Ok, thank you for clarifying.


Kind regards,
Niklas
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot