Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33198
Change subject: sb/intel/common: Add macro to link a file in all stages ......................................................................
sb/intel/common: Add macro to link a file in all stages
It is easy to forget to link some files which breaks the build when some Kconfig options get enabled.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/southbridge/intel/common/Makefile.inc 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/1
diff --git a/src/southbridge/intel/common/Makefile.inc b/src/southbridge/intel/common/Makefile.inc index 4cf6e6f..ab5e5d8 100644 --- a/src/southbridge/intel/common/Makefile.inc +++ b/src/southbridge/intel/common/Makefile.inc @@ -13,6 +13,17 @@ ## GNU General Public License for more details. ##
+# add SPI drivers per stage except smm +# $1 condition +# $2 filename +define add_to_all_stages +bootblock-$(1) += $(2) +verstage-$(1) += $(2) +romstage-$(1) += $(2) +postcar-$(1) += $(2) +ramstage-$(1) += $(2) +endef + # CONFIG_HAVE_INTEL_FIRMWARE protects doing anything to the build. subdirs-y += firmware
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: sb/intel/common: Add macro to link a file in all stages ......................................................................
Patch Set 1:
Why not the toplevel Makefile.inc? somewhere below `classes-y`
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: sb/intel/common: Add macro to link a file in all stages ......................................................................
Patch Set 1:
Could we draw the $(eval) into the macro?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#2).
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Makefile.inc: Add macro to link a file in all stages
Files needed in all stages are quite common, so add a macro as a shorthand for that.
e.g.: to link a file in all stages, add the following to a Makefile.inc "$(call stages,myfile.c)" to unconditionally link it "$(call all_stages,myfile.c,$(CONFIG_MY_KCONFIG_OPTION))" to link it depending on CONFIG_MY_KCONFIG_OPTION.
This does not include smm at the moment, since that often has separate Kconfigs determining whether to link it.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/2
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#3).
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Makefile.inc: Add macro to link a file in all stages
Files needed in all stages are quite common, so add a macro as a shorthand for that.
e.g.: to link a file in all stages, add the following to a Makefile.inc "$(call all_stages,myfile.c)" to unconditionally link it, "$(call all_stages,myfile.c,$(CONFIG_MY_KCONFIG_OPTION))" to link it depending on CONFIG_MY_KCONFIG_OPTION.
This does not include smm at the moment, since that often has separate Kconfigs determining whether to link it.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/3
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#4).
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Makefile.inc: Add macro to link a file in all stages
Files needed in all stages are quite common, so add a macro as a shorthand for that.
e.g.: to link a file in all stages, add the following to a Makefile.inc "$(call all_stages,myfile.c)" to unconditionally link it, "$(call all_stages,myfile.c,$(CONFIG_MY_KCONFIG_OPTION))" to link it depending on CONFIG_MY_KCONFIG_OPTION.
This does not include smm at the moment, since that often has separate Kconfigs determining whether to link it.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/4
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#5).
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Makefile.inc: Add macro to link a file in all stages
Files needed in all stages are quite common, so add a macro as a shorthand for that.
e.g.: to link a file in all stages, add the following to a Makefile.inc "$(call all_stages,myfile.c)" to unconditionally link it, "$(call all_stages,myfile.c,CONFIG_MY_KCONFIG_OPTION)" to link it depending on CONFIG_MY_KCONFIG_OPTION.
This does not include smm at the moment, since that often has separate Kconfigs determining whether to link it.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/5
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#6).
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Makefile.inc: Add macro to link a file in all stages
Files needed in all stages are quite common, so add a macro as a shorthand for that.
e.g.: to link a file in all stages, add the following to a Makefile.inc "$(call all_stages,myfile.c,$(CONFIG_MY_KCONFIG_OPTION))" to link it depending on CONFIG_MY_KCONFIG_OPTION.
This does not include smm at the moment, since that often has separate Kconfigs determining whether to link it.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc 1 file changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Patch Set 6: Code-Review+1
Looks good, just the order of the parameters feels wrong. Maybe that's just me, but... currently the condition comes first:
somestage-$(CONFIG_SOMETHING) += somefile.c
Keeping that order would also help to emphasize the file name, I think. Maybe we could even make that more literal, e.g. `all_stages_if`:
$(call all_stages_if,$(CONFIG_SOMETHING),somefile.c)
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Patch Set 6:
Patch Set 6: Code-Review+1
Looks good, just the order of the parameters feels wrong. Maybe that's just me, but... currently the condition comes first:
somestage-$(CONFIG_SOMETHING) += somefile.cKeeping that order would also help to emphasize the file name, I think. Maybe we could even make that more literal, e.g. `all_stages_if`:
$(call all_stages_if,$(CONFIG_SOMETHING),somefile.c)
I agree with reversing the arguments. I think that makes it more readable.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add macro to link a file in all stages ......................................................................
Patch Set 6:
I'd rather go for a new class ("all"?) with special behavior (namely: paste into all other classes). That way, it's not "$(call mumblejumble,beware,of,the,commas)" but "all-$(CONFIG_FOO) += bar.c"
Hello HAOUAS Elyes, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#7).
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Makefile.inc: Add a class 'all' to link files in all stage
Currently only implemented on x86.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc M src/arch/x86/Makefile.inc 2 files changed, 20 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/7
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33198/7/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/#/c/33198/7/src/arch/x86/Makefile.inc@117 PS7, Line 117: bootblock-srcs += $(all-srcs) I'd put this in basedir Makefile.inc, but our makefiles seem to fragile for that (or my make knowledge is lacking).
Hello HAOUAS Elyes, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#8).
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Makefile.inc: Add a class 'all' to link files in all stage
Currently only implemented on x86.
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile.inc M src/arch/x86/Makefile.inc 2 files changed, 19 insertions(+), 27 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/8
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Patch Set 8:
(1 comment)
I'd rather go for a new class ("all"?) with special behavior (namely: paste into all other classes).
You have to take Patrick more literally here. We have the notion of a `special-class` ;) It simply means for each of its `$(class)-y` files a `$(class)-handler` is called.
The order in `$(includemakefiles)` is unfortunate atm. If we could move the special-class handling one line up, before the `-srcs +=`, something lean like this could do the job:
$(call add-special-class,all) all-handler = $(foreach class,bootblock verstage...,$(class)-y += $(2))
Given the few special classes we have so far, I think changing the order should be safe. Patrick, what do you think?
https://review.coreboot.org/c/coreboot/+/33198/7/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33198/7/src/arch/x86/Makefile.inc@1... PS7, Line 117: bootblock-srcs += $(all-srcs)
I'd put this in basedir Makefile. […]
I don't think it can work like this. `*-srcs` is declared with `:=`, so `+= $(all-srcs)` here would add the contents of `$(all-srcs)` that were gathered up to this point but not beyond.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Patch Set 8:
Patch Set 8: The order in `$(includemakefiles)` is unfortunate atm. If we could move the special-class handling one line up, before the `-srcs +=`, something lean like this could do the job:
$(call add-special-class,all) all-handler = $(foreach class,bootblock verstage...,$(class)-y += $(2))Given the few special classes we have so far, I think changing the order should be safe. Patrick, what do you think?
Yes, sounds good. I wonder if we should enumerate the classes manually in the all-handler or use $(filter-out) to just paste stuff into everything but special classes?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Patch Set 8:
Patch Set 8:
(1 comment)
I'd rather go for a new class ("all"?) with special behavior (namely: paste into all other classes).
You have to take Patrick more literally here. We have the notion of a `special-class` ;) It simply means for each of its `$(class)-y` files a `$(class)-handler` is called.
The order in `$(includemakefiles)` is unfortunate atm. If we could move the special-class handling one line up, before the `-srcs +=`, something lean like this could do the job:
$(call add-special-class,all) all-handler = $(foreach class,bootblock verstage...,$(class)-y += $(2))Given the few special classes we have so far, I think changing the order should be safe. Patrick, what do you think?
No success with this method so far.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage ......................................................................
Patch Set 8:
Patch Set 8:
Patch Set 8:
(1 comment)
I'd rather go for a new class ("all"?) with special behavior (namely: paste into all other classes).
You have to take Patrick more literally here. We have the notion of a `special-class` ;) It simply means for each of its `$(class)-y` files a `$(class)-handler` is called.
The order in `$(includemakefiles)` is unfortunate atm. If we could move the special-class handling one line up, before the `-srcs +=`, something lean like this could do the job:
$(call add-special-class,all) all-handler = $(foreach class,bootblock verstage...,$(class)-y += $(2))Given the few special classes we have so far, I think changing the order should be safe. Patrick, what do you think?
No success with this method so far.
Nevermind. Got it working
Hello HAOUAS Elyes, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#9).
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Makefile.inc: Add a class 'all' to link files in all stage except SMM
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile M Makefile.inc 2 files changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/9
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33198/7/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33198/7/src/arch/x86/Makefile.inc@1... PS7, Line 117: bootblock-srcs += $(all-srcs)
I don't think it can work like this. `*-srcs` is declared with `:=`, so […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 9: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile File Makefile:
PS9: Should we mirror this change to libpayload/Makefile?
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile.inc@112 PS9, Line 112: $$ Do we need the deferred substitution of $(2)? I would guess that it boils down to evaluate it before / after the $(for ) substitution. And shouldn't make a difference.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile.inc@112 PS9, Line 112: $$
Do we need the deferred substitution of $(2)? I would guess that it […]
Actually, before / during the $(eval ), just looked up the rules...
Hello HAOUAS Elyes, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33198
to look at the new patch set (#10).
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Makefile.inc: Add a class 'all' to link files in all stage except SMM
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Makefile M Makefile.inc 2 files changed, 6 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/33198/10
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile File Makefile:
PS9:
Should we mirror this change to libpayload/Makefile?
Do you expect a special-class handler to add classes in libpayload? Or do you speak about an all target in libpayload?
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile.inc File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile.inc@112 PS9, Line 112: $$
Actually, before / during the $(eval ), just looked up the rules...
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile File Makefile:
PS9:
Should we mirror this change to libpayload/Makefile? […]
We already have
payloads/libpayload/Makefile.inc:$(call add-special-class,includes)
in libpayload. What I meant is this particular change of order here. We should keep libpayload's implementation of `includemakefiles` aligned, IMHO.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile File Makefile:
PS9:
We already have
payloads/libpayload/Makefile.inc:$(call add-special-class,includes)in libpayload. What I meant is this particular change of order here. We should keep libpayload's implementation of `includemakefiles` aligned, IMHO.
Done CB:36439.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 11: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33198/9/Makefile File Makefile:
PS9:
We already have […]
Ack
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/33198 )
Change subject: Makefile.inc: Add a class 'all' to link files in all stage except SMM ......................................................................
Makefile.inc: Add a class 'all' to link files in all stage except SMM
Change-Id: I955dd2dc22cb3cfc4fdf1198cfd32f56475f97c9 Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/33198 Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Makefile M Makefile.inc 2 files changed, 6 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved
diff --git a/Makefile b/Makefile index 0172b09..f3f9592 100644 --- a/Makefile +++ b/Makefile @@ -252,13 +252,13 @@ $(foreach class,classes subdirs $(classes) $(special-classes), $(eval $(class)-y:=)) \ $(eval -include $(1)) \ $(foreach class,$(classes-y), $(call add-class,$(class))) \ + $(foreach special,$(special-classes), \ + $(foreach item,$($(special)-y), $(call $(special)-handler,$(dir $(1)),$(item)))) \ $(foreach class,$(classes), \ $(eval $(class)-srcs+= \ $$(subst $(absobj)/,$(obj)/, \ $$(subst $(top)/,, \ $$(abspath $$(subst $(dir $(1))/,/,$$(addprefix $(dir $(1)),$$($(class)-y)))))))) \ - $(foreach special,$(special-classes), \ - $(foreach item,$($(special)-y), $(call $(special)-handler,$(dir $(1)),$(item)))) \ $(eval subdirs+=$$(subst $(CURDIR)/,,$$(wildcard $$(abspath $$(addprefix $(dir $(1)),$$(subdirs-y))))))
# For each path in $(subdirs) call includemakefiles diff --git a/Makefile.inc b/Makefile.inc index f7f3708..04c83d8 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -107,6 +107,10 @@ # Add source classes and their build options classes-y := ramstage romstage bootblock decompressor postcar smm smmstub cpu_microcode verstage
+# Add a special 'all' class to add sources to all stages +$(call add-special-class,all) +all-handler = $(foreach class,bootblock verstage romstage postcar ramstage,$(eval $(class)-y += $(2))) + # Add dynamic classes for rmodules $(foreach supported_arch,$(ARCH_SUPPORTED), \ $(eval $(call define_class,rmodules_$(supported_arch),$(supported_arch))))
-
Arthur Heymans (Code Review) -
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)