Change in coreboot[master]: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options

[Mario Scheithauer (Code Review)]

Mario Scheithauer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options

The commit (1058dd84f06fa2fcbdd99eb99da07dccdf5b9722) breaks the Siemens
APL mainboards as FSP-M never returns once it is called.
This patch fixes an incomplete implementation of this change.

TEST=tested on siemens/mc_apl5

Change-Id: I48e5fa36e1ad799d09714f53a3041f73b8ec3550
Signed-off-by: Mario Scheithauer 
---
M src/soc/intel/apollolake/Makefile.inc
M src/soc/intel/denverton_ns/Makefile.inc
M src/soc/intel/quark/Makefile.inc
3 files changed, 5 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36645/1

diff --git a/src/soc/intel/apollolake/Makefile.inc b/src/soc/intel/apollolake/Makefile.inc
index 42cbab0..ef81e32 100644
--- a/src/soc/intel/apollolake/Makefile.inc
+++ b/src/soc/intel/apollolake/Makefile.inc
@@ -108,7 +108,7 @@
 CPPFLAGS_common += -I$(src)/soc/intel/apollolake/include

# Since FSP-M runs in CAR we need to relocate it to a specific address
-$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_M_ADDR)
+$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR)

# Handle GLK paging requirements
 ifeq ($(CONFIG_PAGING_IN_CACHE_AS_RAM),y)
diff --git a/src/soc/intel/denverton_ns/Makefile.inc b/src/soc/intel/denverton_ns/Makefile.inc
index 51ae136..4050f61 100644
--- a/src/soc/intel/denverton_ns/Makefile.inc
+++ b/src/soc/intel/denverton_ns/Makefile.inc
@@ -90,8 +90,8 @@

##Set FSP binary blobs memory location

-$(FSP_T_CBFS)-options := -b $(CONFIG_FSP_T_ADDR) --xip
-$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_M_ADDR) --xip
-$(FSP_S_CBFS)-options := -b $(CONFIG_FSP_S_ADDR) --xip
+$(call strip_quotes,$(CONFIG_FSP_T_CBFS))-options := -b $(CONFIG_FSP_T_ADDR) --xip
+$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR) --xip
+$(call strip_quotes,$(CONFIG_FSP_S_CBFS))-options := -b $(CONFIG_FSP_S_ADDR) --xip

endif ## CONFIG_SOC_INTEL_DENVERTON_NS
diff --git a/src/soc/intel/quark/Makefile.inc b/src/soc/intel/quark/Makefile.inc
index f2413c8..bd120ab 100644
--- a/src/soc/intel/quark/Makefile.inc
+++ b/src/soc/intel/quark/Makefile.inc
@@ -71,7 +71,7 @@
 CPPFLAGS_common += -I3rdparty/blobs/soc/intel/quark

# Since FSP-M runs in CAR we need to relocate it to a specific address
-$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_ESRAM_LOC)
+$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_ESRAM_LOC)

# Add the FSP binary to the CBFS image
 cbfs-files-$(CONFIG_ADD_FSP_RAW_BIN) += fsp.bin

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG@9 
PS1, Line 9: 1058dd84f06fa2fcbdd99eb99da07dccdf5b9722
Do you mean 8fc523e3137cfdde970a3c87e22b8bbc586a3f7e?
Also, if you remove the parentheses, "commit 12345" will link to the commit in the Gerrit UI.

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG@11 
PS1, Line 11: incomplete
The assumption that FSP_M_CBFS, set from src/drivers/intel/fsp2_0/Makefile.inc is available in the SoCs seemed to be deliberate, but it's wrong.

[Mario Scheithauer (Code Review)]

Hello Patrick Rudolph, Vanessa Eusebio, build bot (Jenkins), Patrick Georgi, David Guckian, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36645

to look at the new patch set (#2).

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options

The commit 8fc523e3137cfdde970a3c87e22b8bbc586a3f7e breaks the Siemens
APL mainboards as FSP-M never returns once it is called.
This patch fixes this issue.

TEST=tested on siemens/mc_apl5

Change-Id: I48e5fa36e1ad799d09714f53a3041f73b8ec3550
Signed-off-by: Mario Scheithauer 
---
M src/soc/intel/apollolake/Makefile.inc
M src/soc/intel/denverton_ns/Makefile.inc
M src/soc/intel/quark/Makefile.inc
3 files changed, 5 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36645/2

[David Guckian (Code Review)]

David Guckian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 2: Code-Review+1

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG@9 
PS1, Line 9: 1058dd84f06fa2fcbdd99eb99da07dccdf5b9722
> Do you mean 8fc523e3137cfdde970a3c87e22b8bbc586a3f7e? […]

Done

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 2: Code-Review+2

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36645/1//COMMIT_MSG@9 
PS1, Line 9: 1058dd84f06fa2fcbdd99eb99da07dccdf5b9722
> Done

Sorry about the missing answer, I had to rush to another appointment and thanks.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36645/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36645/2//COMMIT_MSG@9 
PS2, Line 9: The commit 8fc523e3137cfdde970a3c87e22b8bbc586a3f7e breaks the Siemens
Commit 8fc523e3 (drivers/intel/fsp2_0: Use strip_quotes for cbfs filenames) …

https://review.coreboot.org/c/coreboot/+/36645/2//COMMIT_MSG@11 
PS2, Line 11: This patch fixes this issue.
Please extend that the `-b` switch is not added.

[Mario Scheithauer (Code Review)]

Hello Patrick Rudolph, David Guckian, Vanessa Eusebio, build bot (Jenkins), Patrick Georgi, David Guckian, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/36645

to look at the new patch set (#3).

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options

The commit 8fc523e3 (drivers/intel/fsp2_0: Use strip_quotes for cbfs
filenames) breaks the Siemens APL mainboards as FSP-M never returns once
it is called. The reason for this is that the -b option is missing when
adding the FSP package to cbfs via cbfstool.
This patch fixes this issue.

TEST=tested on siemens/mc_apl5

Change-Id: I48e5fa36e1ad799d09714f53a3041f73b8ec3550
Signed-off-by: Mario Scheithauer 
---
M src/soc/intel/apollolake/Makefile.inc
M src/soc/intel/denverton_ns/Makefile.inc
M src/soc/intel/quark/Makefile.inc
3 files changed, 5 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36645/3

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/36645/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/36645/2//COMMIT_MSG@9 
PS2, Line 9: The commit 8fc523e3137cfdde970a3c87e22b8bbc586a3f7e breaks the Siemens
> Commit 8fc523e3 (drivers/intel/fsp2_0: Use strip_quotes for cbfs filenames) …

Done

https://review.coreboot.org/c/coreboot/+/36645/2//COMMIT_MSG@11 
PS2, Line 11: This patch fixes this issue.
> Please extend that the `-b` switch is not added.

Done

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

Patch Set 3: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36645 )

Change subject: soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options
......................................................................

soc/intel/{apl,dnv,quark}: Use strip_quotes for FSP options

The commit 8fc523e3 (drivers/intel/fsp2_0: Use strip_quotes for cbfs
filenames) breaks the Siemens APL mainboards as FSP-M never returns once
it is called. The reason for this is that the -b option is missing when
adding the FSP package to cbfs via cbfstool.
This patch fixes this issue.

TEST=tested on siemens/mc_apl5

Change-Id: I48e5fa36e1ad799d09714f53a3041f73b8ec3550
Signed-off-by: Mario Scheithauer 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/36645
Reviewed-by: Subrata Banik 
Reviewed-by: David Guckian
Reviewed-by: Patrick Georgi 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/apollolake/Makefile.inc
M src/soc/intel/denverton_ns/Makefile.inc
M src/soc/intel/quark/Makefile.inc
3 files changed, 5 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Subrata Banik: Looks good to me, approved
  David Guckian: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/apollolake/Makefile.inc b/src/soc/intel/apollolake/Makefile.inc
index 42cbab0..ef81e32 100644
--- a/src/soc/intel/apollolake/Makefile.inc
+++ b/src/soc/intel/apollolake/Makefile.inc
@@ -108,7 +108,7 @@
 CPPFLAGS_common += -I$(src)/soc/intel/apollolake/include

# Since FSP-M runs in CAR we need to relocate it to a specific address
-$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_M_ADDR)
+$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR)

# Handle GLK paging requirements
 ifeq ($(CONFIG_PAGING_IN_CACHE_AS_RAM),y)
diff --git a/src/soc/intel/denverton_ns/Makefile.inc b/src/soc/intel/denverton_ns/Makefile.inc
index 51ae136..4050f61 100644
--- a/src/soc/intel/denverton_ns/Makefile.inc
+++ b/src/soc/intel/denverton_ns/Makefile.inc
@@ -90,8 +90,8 @@

##Set FSP binary blobs memory location

-$(FSP_T_CBFS)-options := -b $(CONFIG_FSP_T_ADDR) --xip
-$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_M_ADDR) --xip
-$(FSP_S_CBFS)-options := -b $(CONFIG_FSP_S_ADDR) --xip
+$(call strip_quotes,$(CONFIG_FSP_T_CBFS))-options := -b $(CONFIG_FSP_T_ADDR) --xip
+$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_M_ADDR) --xip
+$(call strip_quotes,$(CONFIG_FSP_S_CBFS))-options := -b $(CONFIG_FSP_S_ADDR) --xip

endif ## CONFIG_SOC_INTEL_DENVERTON_NS
diff --git a/src/soc/intel/quark/Makefile.inc b/src/soc/intel/quark/Makefile.inc
index f2413c8..bd120ab 100644
--- a/src/soc/intel/quark/Makefile.inc
+++ b/src/soc/intel/quark/Makefile.inc
@@ -71,7 +71,7 @@
 CPPFLAGS_common += -I3rdparty/blobs/soc/intel/quark

# Since FSP-M runs in CAR we need to relocate it to a specific address
-$(FSP_M_CBFS)-options := -b $(CONFIG_FSP_ESRAM_LOC)
+$(call strip_quotes,$(CONFIG_FSP_M_CBFS))-options := -b $(CONFIG_FSP_ESRAM_LOC)

# Add the FSP binary to the CBFS image
 cbfs-files-$(CONFIG_ADD_FSP_RAW_BIN) += fsp.bin