| Message ID | 20211021155843.1969401-9-mcgrof@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | firmware_loader: built-in API and make x86 use it | expand |
On Thu, Oct 21, 2021 at 08:58:41AM -0700, Luis R. Rodriguez wrote: > From: Luis Chamberlain <mcgrof@kernel.org> > > If we wanted to use a different directory for building target > builtin firmware it is easier if we just have a shared library > Makefile, and each target directory can then just include it and > populate the respective needed variables. This reduces clutter, > makes things easier to read, and also most importantly allows > us to not have to try to magically adjust only one target > kconfig symbol for built-in firmware files. Trying to do this > can easily end up causing odd build issues if the user is not > careful. > > As an example issue, if we are going to try to extend the > FW_LOADER_BUILTIN_FILES list and FW_LOADER_BUILTIN_DIR in case > of a new test firmware builtin support currently our only option > would be modify the defaults of each of these in case test firmware > builtin support was enabled. Defaults however won't augment a prior > setting, and so if FW_LOADER_BUILTIN_DIR="/lib/firmware" and you > and want this to be changed to something like > FW_LOADER_BUILTIN_DIR="drivers/base/firmware_loader/test-builtin" > the change will not take effect as a prior build already had it > set, and the build would fail. Trying to augment / append the > variables in the Makefile just makes this very difficult to > read. > > Using a library let's us split up possible built-in targets so > that the user does not have to be involved. This will be used > in a subsequent patch which will add another user to this > built-in firmware library Makefile and demo how to use it outside > of the default FW_LOADER_BUILTIN_DIR and FW_LOADER_BUILTIN_FILES. > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> I'm sorry, but I do not understand the need for this change at all. You are now building this as a library, but what uses this library? The patches after this series are just testing patches, to verify that the code previous in this series is working correctly, it should not depend on a new library that only the testing code requires, right? confused, greg k-h
On Fri, Oct 22, 2021 at 02:13:38PM +0200, Greg KH wrote: > On Thu, Oct 21, 2021 at 08:58:41AM -0700, Luis R. Rodriguez wrote: > > From: Luis Chamberlain <mcgrof@kernel.org> > > > > If we wanted to use a different directory for building target > > builtin firmware it is easier if we just have a shared library > > Makefile, and each target directory can then just include it and > > populate the respective needed variables. This reduces clutter, > > makes things easier to read, and also most importantly allows > > us to not have to try to magically adjust only one target > > kconfig symbol for built-in firmware files. Trying to do this > > can easily end up causing odd build issues if the user is not > > careful. > > > > As an example issue, if we are going to try to extend the > > FW_LOADER_BUILTIN_FILES list and FW_LOADER_BUILTIN_DIR in case > > of a new test firmware builtin support currently our only option > > would be modify the defaults of each of these in case test firmware > > builtin support was enabled. Defaults however won't augment a prior > > setting, and so if FW_LOADER_BUILTIN_DIR="/lib/firmware" and you > > and want this to be changed to something like > > FW_LOADER_BUILTIN_DIR="drivers/base/firmware_loader/test-builtin" > > the change will not take effect as a prior build already had it > > set, and the build would fail. Trying to augment / append the > > variables in the Makefile just makes this very difficult to > > read. > > > > Using a library let's us split up possible built-in targets so > > that the user does not have to be involved. This will be used > > in a subsequent patch which will add another user to this > > built-in firmware library Makefile and demo how to use it outside > > of the default FW_LOADER_BUILTIN_DIR and FW_LOADER_BUILTIN_FILES. > > > > Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> > > I'm sorry, but I do not understand the need for this change at all. You > are now building this as a library, but what uses this library? The > patches after this series are just testing patches, to verify that the > code previous in this series is working correctly, it should not depend > on a new library that only the testing code requires, right? The last patch adds support to test built-in firmware, but most kernels will have and do want EXTRA_FIRMWARE="", and so there cannot be anything that can be tested. And so we need aother two pair of kconfig symbols which are independent to test built-in firmware. The reason for this is explained in the commit log, if we try to augment the EXTRA_FIRMWARE when enabling testing built-in firmware we can easily end up with odd build issues. So this patch moves the logic to enable us to re-use the same built-in magic for two independent kconfig test symbols. Luis
diff --git a/drivers/base/firmware_loader/builtin/Makefile b/drivers/base/firmware_loader/builtin/Makefile index 7cdd0b5c7384..9b0dc193f6c7 100644 --- a/drivers/base/firmware_loader/builtin/Makefile +++ b/drivers/base/firmware_loader/builtin/Makefile @@ -1,4 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 +include $(srctree)/drivers/base/firmware_loader/builtin/lib.Makefile + obj-y += main.o # Create $(fwdir) from $(CONFIG_FW_LOADER_BUILTIN_DIR) -- if it doesn't have a @@ -8,35 +10,3 @@ fwdir := $(addprefix $(srctree)/,$(filter-out /%,$(fwdir)))$(filter /%,$(fwdir)) firmware := $(addsuffix .gen.o, $(subst $(quote),,$(CONFIG_FW_LOADER_BUILTIN_FILES))) obj-y += $(firmware) - -FWNAME = $(patsubst $(obj)/%.gen.S,%,$@) -FWSTR = $(subst $(comma),_,$(subst /,_,$(subst .,_,$(subst -,_,$(FWNAME))))) -ASM_WORD = $(if $(CONFIG_64BIT),.quad,.long) -ASM_ALIGN = $(if $(CONFIG_64BIT),3,2) -PROGBITS = $(if $(CONFIG_ARM),%,@)progbits - -filechk_fwbin = \ - echo "/* Generated by $(src)/Makefile */" ;\ - echo " .section .rodata" ;\ - echo " .p2align 4" ;\ - echo "_fw_$(FWSTR)_bin:" ;\ - echo " .incbin \"$(fwdir)/$(FWNAME)\"" ;\ - echo "_fw_end:" ;\ - echo " .section .rodata.str,\"aMS\",$(PROGBITS),1" ;\ - echo " .p2align $(ASM_ALIGN)" ;\ - echo "_fw_$(FWSTR)_name:" ;\ - echo " .string \"$(FWNAME)\"" ;\ - echo " .section .builtin_fw,\"a\",$(PROGBITS)" ;\ - echo " .p2align $(ASM_ALIGN)" ;\ - echo " $(ASM_WORD) _fw_$(FWSTR)_name" ;\ - echo " $(ASM_WORD) _fw_$(FWSTR)_bin" ;\ - echo " $(ASM_WORD) _fw_end - _fw_$(FWSTR)_bin" - -$(obj)/%.gen.S: FORCE - $(call filechk,fwbin) - -# The .o files depend on the binaries directly; the .S files don't. -$(addprefix $(obj)/, $(firmware)): $(obj)/%.gen.o: $(fwdir)/% - -targets := $(patsubst $(obj)/%,%, \ - $(shell find $(obj) -name \*.gen.S 2>/dev/null)) diff --git a/drivers/base/firmware_loader/builtin/lib.Makefile b/drivers/base/firmware_loader/builtin/lib.Makefile new file mode 100644 index 000000000000..e979a67acfa7 --- /dev/null +++ b/drivers/base/firmware_loader/builtin/lib.Makefile @@ -0,0 +1,32 @@ +# SPDX-License-Identifier: GPL-2.0 +FWNAME = $(patsubst $(obj)/%.gen.S,%,$@) +FWSTR = $(subst $(comma),_,$(subst /,_,$(subst .,_,$(subst -,_,$(FWNAME))))) +ASM_WORD = $(if $(CONFIG_64BIT),.quad,.long) +ASM_ALIGN = $(if $(CONFIG_64BIT),3,2) +PROGBITS = $(if $(CONFIG_ARM),%,@)progbits + +filechk_fwbin = \ + echo "/* Generated by $(src)/Makefile */" ;\ + echo " .section .rodata" ;\ + echo " .p2align 4" ;\ + echo "_fw_$(FWSTR)_bin:" ;\ + echo " .incbin \"$(fwdir)/$(FWNAME)\"" ;\ + echo "_fw_end:" ;\ + echo " .section .rodata.str,\"aMS\",$(PROGBITS),1" ;\ + echo " .p2align $(ASM_ALIGN)" ;\ + echo "_fw_$(FWSTR)_name:" ;\ + echo " .string \"$(FWNAME)\"" ;\ + echo " .section .builtin_fw,\"a\",$(PROGBITS)" ;\ + echo " .p2align $(ASM_ALIGN)" ;\ + echo " $(ASM_WORD) _fw_$(FWSTR)_name" ;\ + echo " $(ASM_WORD) _fw_$(FWSTR)_bin" ;\ + echo " $(ASM_WORD) _fw_end - _fw_$(FWSTR)_bin" + +$(obj)/%.gen.S: FORCE + $(call filechk,fwbin) + +# The .o files depend on the binaries directly; the .S files don't. +$(addprefix $(obj)/, $(firmware)): $(obj)/%.gen.o: $(fwdir)/% + +targets := $(patsubst $(obj)/%,%, \ + $(shell find $(obj) -name \*.gen.S 2>/dev/null))