Hello Marco,
On 03.07.23 08:25, Marco Felsch wrote:
> Hi Ahmad,
>
> On 23-06-26, Ahmad Fatoum wrote:
>> Previous commit turned compile-time errors into link-time errors.
>> This commit goes a step further and allows the link to succeed
>> unconditionally for build coverage and then dependent on the newly
>> introduced CONFIG_MISSING_FIRMWARE_ERROR option abort the build with an
>> error code and a listing of missing firmware printed to stderr.
>>
>> In any case, barebox images which contain firmware in their PBL
>> that's not available will be marked specially to reduce the risk
>> of accidentally putting them to use:
>>
>> * They're truncated to zero size
>>
>> * The final "images built:" section marks them as having firmware
>> missing
>>
>> * They are omitted from the listing in the barebox-flash-images file
>>
>> * Each barebox-broken.img is accompanied with a
>> barebox-broken.img.missing-firmware containing a newline delimited
>> list of missing firmware images
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
>> ---
>> firmware/Kconfig | 14 ++++++++++++++
>> firmware/Makefile | 5 +++++
>> images/Makefile | 21 ++++++++++++++++-----
>> scripts/Makefile.lib | 3 +++
>> 4 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/firmware/Kconfig b/firmware/Kconfig
>> index 56ced00bc430..56d6d0d6c030 100644
>> --- a/firmware/Kconfig
>> +++ b/firmware/Kconfig
>> @@ -6,6 +6,20 @@ config EXTRA_FIRMWARE_DIR
>> string "Firmware blobs root directory"
>> default "firmware"
>>
>> +config MISSING_FIRMWARE_ERROR
>> + bool "Fail the build when required firmware is missing"
>> + default y
>
> Do we need to make this default y? IMHO multi_v8_defconfig will set this
> to 'n' anyway and so the 'n' will become the de-facto default.
Existing users will get the new default. Users that build in a BSP
will want y too as they may not see warnings. It doesn't help people
copying the multi defconfig and not changing it, but that's not a reason
for having existing configs to not fail when they lack firmware.
>
>> + help
>> + In-tree Defconfigs that enable multiple boards with different firmware
>> + binary requirements would say y here, so you don't need unrelated firmware
>> + for the build to succeed.
>> +
>> + Defconfigs custom-tailored to products would say n here as all boards
>> + being built should be functional and have their firmware available.
>> +
>> + If in doubt, say Y and refer to the documentation on where to acquire the
>> + needed firmware.
>> +
>> config HAVE_FIRMWARE_IMX_LPDDR4_PMU_TRAIN
>> bool
>> default y
>> diff --git a/firmware/Makefile b/firmware/Makefile
>> index c66d19c677e8..f9490a8e7a15 100644
>> --- a/firmware/Makefile
>> +++ b/firmware/Makefile
>> @@ -58,6 +58,11 @@ filechk_fwbin = { \
>> echo "\#endif" ;\
>> echo ".global _fw_$(FWSTR)_end" ;\
>> echo "_fw_$(FWSTR)_end:" ;\
>> + echo "\#ifdef __PBL__" ;\
>> + echo " .section .missing_fw,\"a\"" ;\
>> + echo "_fwname_$(FWSTR):" ;\
>> + echo ".ascii \"firmware/$(FWNAME)\\n\"" ;\
>> + echo "\#endif" ;\
>
> Shouldn't this be part of the #ifdef no-firmware? This way we do add the
> .missing_fw section always which shouldn't be the case, right?
It doesn't matter much. Linker will garbage collect the unused section.
>
>> }
>>
>> __fwbin_sha = { \
>> diff --git a/images/Makefile b/images/Makefile
>> index c93f9e268978..16d86a5a5da4 100644
>> --- a/images/Makefile
>> +++ b/images/Makefile
>> @@ -72,6 +72,8 @@ $(obj)/%.pbl: $(pbl-lds) $(BAREBOX_PBL_OBJS) $(obj)/piggy.o $(obj)/sha_sum.o FOR
>>
>> $(obj)/%.pblb: $(obj)/%.pbl FORCE
>> $(call if_changed,objcopy_bin,$(*F))
>> + $(Q)$(OBJCOPY) -O binary --only-section=.missing_fw $< $@.missing-firmware
>> + $(Q)[ -s $@.missing-firmware ] || rm -f $@.missing-firmware
>> $(call cmd,check_file_size,$@,$(CONFIG_BAREBOX_MAX_IMAGE_SIZE))
>>
>> #
>> @@ -127,10 +129,14 @@ $(obj)/barebox.z: $(obj)/../barebox.bin FORCE
>>
>> # %.img - create a copy from another file
>> # ----------------------------------------------------------------
>> +
>> +missing_fw = $(strip $(wildcard $(obj)/$(FILE_$(@F)).missing-firmware $(basename $(obj)/$(FILE_$(@F))).missing-firmware))
>> +
>> .SECONDEXPANSION:
>> $(obj)/%.img: $(obj)/$$(FILE_$$(@F))
>> $(Q)if [ -z $(FILE_$(@F)) ]; then echo "FILE_$(@F) empty!"; false; fi
>> - $(call if_changed,shipped)
>> + $(Q)$(if $(missing_fw),cat $(missing_fw) >$@.missing-firmware,rm -f $@.missing-firmware)
>
> Okay, since we always add the missing-firmware section we need to clean
> it up here, right? Isn't it easier to add the missing-firmware section
> only if detected? If this is possible.
It's possible and I don't feel strong one way or the other.
>
>> + $(call if_changed,$(if $(missing_fw),0size,shipped))
>>
>> board = $(srctree)/arch/$(SRCARCH)/boards
>> objboard = $(objtree)/arch/$(SRCARCH)/boards
>> @@ -194,10 +200,15 @@ multi-image-build:
>>
>> images: $(image-y-path) $(flash-link) $(flash-list) FORCE
>> @echo "images built:"
>> - @for i in $(image-y); do echo $$i; done
>> + @for i in $(image-y); do \
>> + if [ -s $(obj)/$$i ]; then echo $$i; \
>> + else >&2 echo "** firmware missing for $$i **"; \
>
> Not sure if we should print an error if the user set
> CONFIG_MISSING_FIRMWARE_ERROR to n.
We need to alert the user somehow that the image is missing/non-functional.
It's just a warning message.
>
> Regards,
> Marco
>
>> + $(if $(CONFIG_MISSING_FIRMWARE_ERROR), >&2 sed 's/^/\t/' <$(obj)/$${i}.missing-firmware; missing=1;) \
>> + fi; done; test -n "$$missing" && \
>> + echo >&2 "Firmware missing in CONFIG_MISSING_FIRMWARE_ERROR=y build" && false
>>
>> __images_install: images
>> - @for i in $(image-y-path); do install -t "$(INSTALL_PATH)" $$i; done
>> + @for i in $(image-y-path); do if [ -s $$i ]; then install -t "$(INSTALL_PATH)" $$i; fi; done
>>
>> PHONY += __images_install
>>
>> @@ -205,10 +216,10 @@ $(flash-link): $(link-dest) FORCE
>> $(call if_changed,ln)
>>
>> $(flash-list): $(image-y-path)
>> - @for i in $^; do echo $$i; done > $@
>> + @for i in $^; do if [ -s $$i ]; then echo $$i; fi; done > $@
>>
>> clean-files := *.pbl *.pblb *.map start_*.imximg *.img barebox.z start_*.kwbimg \
>> start_*.kwbuartimg *.socfpgaimg *.mlo *.t20img *.t20img.cfg *.t30img \
>> *.t30img.cfg *.t124img *.t124img.cfg *.mlospi *.mlo *.mxsbs *.mxssd \
>> - start_*.simximg start_*.usimximg *.zynqimg *.image *.swapped
>> + start_*.simximg start_*.usimximg *.zynqimg *.image *.swapped *.missing-firmware
>> clean-files += pbl.lds
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 42ee27499561..b8fb2684421e 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -236,6 +236,9 @@ endef
>> # Shipped files
>> # ===========================================================================
>>
>> +quiet_cmd_0size = 0SIZE $@
>> +cmd_0size = : > $@
>> +
>> quiet_cmd_shipped = SHIPPED $@
>> cmd_shipped = cat $< > $@
>>
>> --
>> 2.39.2
>>
>>
>>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
