[Buildroot] [PATCH 1/1] package/rpi-firmware: fix missing files in overlays

[Buildroot] [PATCH 1/1] package/rpi-firmware: fix missing files in overlays

[Sassen, Rutger]

[-- Attachment #1.1: Type: text/plain, Size: 1642 bytes --]

Not all files in boot/overlays are copied to the image, which may result
in overlays not being loaded at all, or a wrong overlay being loaded.

When using os_prefix and/or overlay_prefix in the raspberry pi
config.txt, it checks for the existence of README in the overlays
directory. If it is missing, overlays will not be loaded.

The overlay_map.dtb file contains rules for overlays that require
special handling depending on the specific type of raspberry pi
hardware. When this file is missing, the wrong overlay may be loaded.

Fixed by copying all files from boot/overlays, instead of only *.dtbo

Signed-off-by: Rutger Sassen <rsassen@comecer.com>
---
 package/rpi-firmware/rpi-firmware.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/rpi-firmware/rpi-firmware.mk b/package/rpi-firmware/rpi-firmware.mk
index b46a7f5270..4b9a442bb6 100644
--- a/package/rpi-firmware/rpi-firmware.mk
+++ b/package/rpi-firmware/rpi-firmware.mk
@@ -45,7 +45,7 @@ endif
 
 ifeq ($(BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAYS),y)
 define RPI_FIRMWARE_INSTALL_DTB_OVERLAYS
-	$(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo), \
+$(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*), \
		$(INSTALL) -D -m 0644 $(ovldtb) $(BINARIES_DIR)/rpi-firmware/overlays/$(notdir $(ovldtb))
	)
 endef
-- 
2.20.1

This email communication is CONFIDENTIAL. If you are not the intended recipient, you may not use, copy or disclose to anyone the message or any information contained in the message and I ask that you please notify me by return email and delete this communication immediately. Thank you.

[-- Attachment #1.2: Type: text/html, Size: 2051 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/rpi-firmware: fix missing files in overlays

[Peter Seiderer]

Hello Rutger,

On Tue, 8 Mar 2022 12:40:14 +0000, "Sassen, Rutger" <rsassen@comecer.com> wrote:

> Not all files in boot/overlays are copied to the image, which may result
> in overlays not being loaded at all, or a wrong overlay being loaded.
>
> When using os_prefix and/or overlay_prefix in the raspberry pi
> config.txt, it checks for the existence of README in the overlays
> directory. If it is missing, overlays will not be loaded.

Any prove for this behavior (could not find anything about it in
the docs)?

>
> The overlay_map.dtb file contains rules for overlays that require
> special handling depending on the specific type of raspberry pi
> hardware. When this file is missing, the wrong overlay may be loaded.

Only if you use this feature, see [1], [2] (building multiple rpi targeting sdcard)
instead of determining the right overlay (exactly targeting one rpi flavour sdcard,
as the buildroot rpi example config files do)...

>
> Fixed by copying all files from boot/overlays, instead of only *.dtbo
>
> Signed-off-by: Rutger Sassen <rsassen@comecer.com>
> ---
>  package/rpi-firmware/rpi-firmware.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/package/rpi-firmware/rpi-firmware.mk b/package/rpi-firmware/rpi-firmware.mk
> index b46a7f5270..4b9a442bb6 100644
> --- a/package/rpi-firmware/rpi-firmware.mk
> +++ b/package/rpi-firmware/rpi-firmware.mk
> @@ -45,7 +45,7 @@ endif
>
>  ifeq ($(BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAYS),y)
>  define RPI_FIRMWARE_INSTALL_DTB_OVERLAYS
> -	$(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo), \
> +$(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*), \
> 		$(INSTALL) -D -m 0644 $(ovldtb) $(BINARIES_DIR)/rpi-firmware/overlays/$(notdir $(ovldtb))
> 	)
>  endef

Would prefer to only/explicitly add the overlay_map.dtb file:

       $(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo) $(@D)/boot/overlays/overlay_map.dtb, \

or

	$(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo $(@D)/boot/overlays/*.dtb), \

or as it is a (maybe unwanted target feature) add a new buildroot rpi-firmware
configure option for adding the overlay_map.dtb file...

Regards,
Peter

[1] https://github.com/raspberrypi/documentation/blob/develop/documentation/asciidoc/computers/configuration/device-tree.adoc
[2] https://github.com/raspberrypi/documentation/commit/67f5d3b4ff8b2e52239a5415c4b0272a85a910fc

Please avoid your footer message...

> --
> 2.20.1
>
> This email communication is CONFIDENTIAL. If you are not the intended recipient, you may not use, copy or disclose to anyone
> the message or any information contained in the message and I ask that you please notify me by return email and delete this
> communication immediately. Thank you.
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/rpi-firmware: fix missing files in overlays

[Peter Seiderer]

Hello Rutger,

On Wed, 9 Mar 2022 10:25:24 +0000, "Sassen, Rutger" <rsassen@comecer.com> wrote:

> Hi Peter,
>
> > > Not all files in boot/overlays are copied to the image, which may result
> > > in overlays not being loaded at all, or a wrong overlay being loaded.
> > >
> > > When using os_prefix and/or overlay_prefix in the raspberry pi
> > > config.txt, it checks for the existence of README in the overlays
> > > directory. If it is missing, overlays will not be loaded.
>
> > Any prove for this behavior (could not find anything about it in
> > the docs)?
>
> Yes, it is documented in the raspberry pi docs for config.txt [3], 2nd paragraph:
>

Thanks for the link, but as I read it (and I am pretty sure the given overlays
are loaded from the default location without the README file) the README file is
only needed in case os_prefix is is (overlays stored in a non default location)?

> I've also tested it myself and can confirm that e.g. when using os_prefix=custom/ , and /custom/overlays/README does not exist, that overlays will be loaded from the root /overlays instead, or when /overlays does not exist, no overlays will be loaded at all.
>

Seems to validate the above..., a README file in the default boot partition overlays directory is
not needed (the default buildroot/rpi-firmware/genimage-configs location)..., and the special
cases (os_prefix use cases) are not supported by buildroot out-of-the-box, so I would omit
the README case...

>
> > > The overlay_map.dtb file contains rules for overlays that require
> > > special handling depending on the specific type of raspberry pi
> > > hardware. When this file is missing, the wrong overlay may be loaded.
> >
> > Only if you use this feature, see [1], [2] (building multiple rpi targeting sdcard)
> > instead of determining the right overlay (exactly targeting one rpi flavour sdcard,
> > as the buildroot rpi example config files do)...
>
> That is correct. You have to be careful however when e.g. switching to a new rpi target. When no overlay map is present, you'll have to manually change config.txt to use the correct overlays for your platform instead of relying on the overlay map.

As the buildroot default configs are per raspberry pi model this is no use-case/problem...., but optional
support for multiple targeting sdcards is welcome ;-)

>
> > Would prefer to only/explicitly add the overlay_map.dtb file:
> >
> > $(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo) $(@D)/boot/overlays/overlay_map.dtb, \
> >
> > or
> >
> > $(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo $(@D)/boot/overlays/*.dtb), \
> >
> > or as it is a (maybe unwanted target feature) add a new buildroot rpi-firmware
> > configure option for adding the overlay_map.dtb file...
>
> I had also considered adding only the README and overlay_map.dtb. But I figured if the raspberry pi firmware adds extra files, they would probably be important. Not copying them might introduce unexpected behavior. When not copying all files, each time when bumping rpi-firmware to a new version one should check if files were added, and if they are worth copying.
>
> I could add a configure option to also copy overlay_map.dtb and README like you suggested (probably two separate options then; default on?). E.g.
>
> BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAY_MAP

Yes (default off)..., as it is a easy/simple option to enable one implementation
of basic multi-target sd-cards (the other option would be the use of 'Model Filters'
([4]) in config.txt).

> BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAY_README

Omit (see above)...

>
> How about instead adding a configure option for specifying a list of files to be copied instead of all files? That way you could also specify exactly which dtbo files should be copied, instead of copying all of them. Currently there are a lot of overlays copied that we don't use for our system configuration. And if in the future new files are added to the rpi-firmware overlays, we don't have to introduce yet another configure option for it. E.g.
>
> BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAY_FILES

Matter of taste (as with many options available in buildroot)...., I believe the _FILES option is
overkill/too-complex for 1.1M overlays files (and will not fix/enable all possibles use-cases,
see os_prefix above..., very special cases needs special hand-crafted solutions/scripts/genimage-configs,
etc.)...

Regards,
Peter

[4] https://www.raspberrypi.com/documentation/computers/config_txt.html#model-filters


>
> Thanks for the feedback, regards,
>
> Rutger
>
> [3] https://www.raspberrypi.com/documentation/computers/config_txt.html#os_prefix
>
> This email communication is CONFIDENTIAL. If you are not the intended recipient, you may not use, copy or disclose to anyone the message or any information contained in the message and I ask that you please notify me by return email and delete this communication immediately. Thank you.

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/rpi-firmware: fix missing files in overlays

[Sassen, Rutger]

[-- Attachment #1.1: Type: text/plain, Size: 3466 bytes --]

Hi Peter,

> > Not all files in boot/overlays are copied to the image, which may result
> > in overlays not being loaded at all, or a wrong overlay being loaded.
> >
> > When using os_prefix and/or overlay_prefix in the raspberry pi
> > config.txt, it checks for the existence of README in the overlays
> > directory. If it is missing, overlays will not be loaded.

> Any prove for this behavior (could not find anything about it in
> the docs)?

Yes, it is documented in the raspberry pi docs for config.txt [3], 2nd paragraph:

I've also tested it myself and can confirm that e.g. when using os_prefix=custom/ , and /custom/overlays/README does not exist, that overlays will be loaded from the root /overlays instead, or when /overlays does not exist, no overlays will be loaded at all.


> > The overlay_map.dtb file contains rules for overlays that require
> > special handling depending on the specific type of raspberry pi
> > hardware. When this file is missing, the wrong overlay may be loaded.
>
> Only if you use this feature, see [1], [2] (building multiple rpi targeting sdcard)
> instead of determining the right overlay (exactly targeting one rpi flavour sdcard,
> as the buildroot rpi example config files do)...

That is correct. You have to be careful however when e.g. switching to a new rpi target. When no overlay map is present, you'll have to manually change config.txt to use the correct overlays for your platform instead of relying on the overlay map.

> Would prefer to only/explicitly add the overlay_map.dtb file:
>
> $(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo) $(@D)/boot/overlays/overlay_map.dtb, \
>
> or
>
> $(foreach ovldtb,$(wildcard $(@D)/boot/overlays/*.dtbo $(@D)/boot/overlays/*.dtb), \
>
> or as it is a (maybe unwanted target feature) add a new buildroot rpi-firmware
> configure option for adding the overlay_map.dtb file...

I had also considered adding only the README and overlay_map.dtb. But I figured if the raspberry pi firmware adds extra files, they would probably be important. Not copying them might introduce unexpected behavior. When not copying all files, each time when bumping rpi-firmware to a new version one should check if files were added, and if they are worth copying.

I could add a configure option to also copy overlay_map.dtb and README like you suggested (probably two separate options then; default on?). E.g.

BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAY_MAP
BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAY_README

How about instead adding a configure option for specifying a list of files to be copied instead of all files? That way you could also specify exactly which dtbo files should be copied, instead of copying all of them. Currently there are a lot of overlays copied that we don't use for our system configuration. And if in the future new files are added to the rpi-firmware overlays, we don't have to introduce yet another configure option for it. E.g.

BR2_PACKAGE_RPI_FIRMWARE_INSTALL_DTB_OVERLAY_FILES

Thanks for the feedback, regards,

Rutger

[3] https://www.raspberrypi.com/documentation/computers/config_txt.html#os_prefix

This email communication is CONFIDENTIAL. If you are not the intended recipient, you may not use, copy or disclose to anyone the message or any information contained in the message and I ask that you please notify me by return email and delete this communication immediately. Thank you.

[-- Attachment #1.2: Type: text/html, Size: 4163 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot