Change in ...coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Felix Singer (Code Review)]

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/30931

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 12 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/1

diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig
index 8156d18..6f5ed2a 100644
--- a/src/drivers/intel/fsp2_0/Kconfig
+++ b/src/drivers/intel/fsp2_0/Kconfig
@@ -85,7 +85,8 @@
    bool "Use the IntelFSP based binaries"
    depends on ADD_FSP_BINARIES
    depends on SOC_INTEL_APOLLOLAKE || SOC_INTEL_SKYLAKE || \
-		SOC_INTEL_KABYLAKE || SOC_INTEL_COFFEELAKE
+		SOC_INTEL_KABYLAKE || SOC_INTEL_COFFEELAKE || \
+		SOC_INTEL_DENVERTON_NS
    help
      When selecting this option, the SoC must set FSP_HEADER_PATH
      and FSP_FD_PATH correctly so FSP splitting works.
diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig
index 1096549..dfb5c37 100644
--- a/src/soc/intel/denverton_ns/Kconfig
+++ b/src/soc/intel/denverton_ns/Kconfig
@@ -78,6 +78,16 @@
    help
      The memory location of the Intel FSP-S binary for this platform.

+config FSP_HEADER_PATH
+	string "Location of FSP headers"
+	depends on MAINBOARD_USES_FSP2_0
+	default "3rdparty/fsp/DenvertonNSFspBinPkg/Include/"
+
+config FSP_FD_PATH
+	string
+	depends on FSP_USE_REPO
+	default "3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"
+
 # CAR memory layout on DENVERTON_NS hardware:
 ## CAR base address - 0xfef00000
 ## CAR size 1MB - 0x100 (0xfff00)

[Felix Singer (Code Review)]

Hello Patrick Rudolph, Vanny E, build bot (Jenkins), David Guckian,

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

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

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/2

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 3:

(1 comment)

There's a reference to vendorcode/ in denverton_ns/Makefile.inc that
should be disabled in case of FSP_USE_REPO.

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig 
File src/soc/intel/denverton_ns/Kconfig:

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig@8... 
PS3, Line 88: 	default "3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"
These are used in drivers/intel/fsp2_0/. So their
declaration, dependencies and prompts should move
there too. Only the defaults should be overridden
here. Actually in case of FSP_USE_REPO, they
shouldn't have a prompt, should they?

Overriding defaults for something with prompts/
dependencies is delicate, though. Please test.

[Felix Singer (Code Review)]

Hello Patrick Rudolph, Vanny E, build bot (Jenkins), David Guckian,

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

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

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/4

[Felix Singer (Code Review)]

Hello Patrick Rudolph, Vanny E, Patrick Rudolph, Julien Viard de Galbert, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, David Guckian,

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

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

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/5

[David Guckian (Code Review)]

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 5: Code-Review+1

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 7:

This change is ready for review.

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig 
File src/soc/intel/denverton_ns/Kconfig:

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig@8... 
PS3, Line 88: 	default "3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"
> These are used in drivers/intel/fsp2_0/. So their […]

I removed the prompt but not the declarations, because it's done like this on all other FSP platforms.

[Felix Singer (Code Review)]

Hello Patrick Rudolph, David Guckian, Vanny E, Patrick Rudolph, Julien Viard de Galbert, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, David Guckian,

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

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

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/denverton_ns/Makefile.inc
3 files changed, 13 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/8

[Felix Singer (Code Review)]

Hello Patrick Rudolph, David Guckian, Vanny E, Patrick Rudolph, Julien Viard de Galbert, Philipp Deppenwiese, build bot (Jenkins), Nico Huber, Patrick Georgi, Martin Roth, David Guckian,

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

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

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 10 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/9

[Vanny E (Code Review)]

Vanny E has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 9: Code-Review+1

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig 
File src/soc/intel/denverton_ns/Kconfig:

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig@8... 
PS3, Line 82: 	string "Location of FSP headers"
I don't understand it. Why does it need a prompt now? and didn't need
one before? How does it work when you don't use the repo?

This triggers a `-I` in a Makefile in `drivers/intel/fsp2_0/`. Where
is the existing `-I` that was used until now? Doesn't it have to be
adapted? Otherwise, we'd have two conflicting include paths.

https://review.coreboot.org/#/c/30931/3/src/soc/intel/denverton_ns/Kconfig@8... 
PS3, Line 88: 	default "3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"
> I removed the prompt but not the declarations, because it's done like this on all other FSP platform […]

I really don't care about the existing weirdness. If you don't want to
make the code base worse, please try to understand what you are doing
and don't just copy-paste.

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo for Denverton

[Mimoja (Code Review)]

Mimoja has uploaded a new patch set (#10) to the change originally created by Felix Singer. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo for Denverton
......................................................................

soc/intel/denverton_ns: Allow using FSP repo for Denverton

This commit is adding a dependency check for the FSP_USE_REPO
config option which so far was not able to deal with Denverton
systems.

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
Signed-off-by: Johanna Schander 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/10

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo for Denverton

[Mimoja (Code Review)]

Mimoja has uploaded a new patch set (#11) to the change originally created by Felix Singer. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo for Denverton
......................................................................

soc/intel/denverton_ns: Allow using FSP repo for Denverton

This commit is adding a dependency check for the FSP_USE_REPO
config option which so far was not able to deal with Denverton
systems.

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
Signed-off-by: Johanna Schander 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 11 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/11

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo for Denverton

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo for Denverton
......................................................................

Patch Set 11: Code-Review+2

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo for Denverton

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo for Denverton
......................................................................

Patch Set 11: -Code-Review

wooops

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo for Denverton

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo for Denverton
......................................................................

Patch Set 11:

The Makefile.inc still needs to be adapted to remove the hard-coded
vendorcode/ include path.

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Felix Singer (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Patrick Rudolph, Mimoja, Patrick Rudolph, Philipp Deppenwiese, Nico Huber, Martin Roth, David Guckian, Julien Viard de Galbert, Vanessa Eusebio, David Guckian,

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

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

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

This commit is adding a dependency check for the FSP_USE_REPO
config option which so far was not able to deal with Denverton
systems.

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
Signed-off-by: Johanna Schander 
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/denverton_ns/Makefile.inc
3 files changed, 11 insertions(+), 3 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/31/30931/12

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 12:

(2 comments)

https://review.coreboot.org/c/coreboot/+/30931/3/src/soc/intel/denverton_ns/... 
File src/soc/intel/denverton_ns/Kconfig:

https://review.coreboot.org/c/coreboot/+/30931/3/src/soc/intel/denverton_ns/... 
PS3, Line 82: 	string "Location of FSP headers"
> I don't understand it. Why does it need a prompt now? and didn't need […]

Done

https://review.coreboot.org/c/coreboot/+/30931/3/src/soc/intel/denverton_ns/... 
PS3, Line 88: 	default "3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"
> I really don't care about the existing weirdness. If you don't want to […]

I can do that in a follow-up patch, but I don't think this is the right patch to change this, since it would break the consistency compared to the other FSP platforms.

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 12: Code-Review+2

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/30931/12/src/soc/intel/denverton_ns... 
File src/soc/intel/denverton_ns/Kconfig:

https://review.coreboot.org/c/coreboot/+/30931/12/src/soc/intel/denverton_ns... 
PS12, Line 23: CPU_SPECIFIC_OPTIONS
hmm, don't we need FSP_M_XIP here?

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 12: Code-Review+1

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Felix Singer (Code Review)]

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30931 )

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/30931/12/src/soc/intel/denverton_ns... 
File src/soc/intel/denverton_ns/Kconfig:

https://review.coreboot.org/c/coreboot/+/30931/12/src/soc/intel/denverton_ns... 
PS12, Line 23: CPU_SPECIFIC_OPTIONS
> hmm, don't we need FSP_M_XIP here?

No idea, but if so this should be fixed by another patch.

Change in coreboot[master]: soc/intel/denverton_ns: Allow using FSP repo

[Patrick Georgi (Code Review)]

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

Change subject: soc/intel/denverton_ns: Allow using FSP repo
......................................................................

soc/intel/denverton_ns: Allow using FSP repo

This commit is adding a dependency check for the FSP_USE_REPO
config option which so far was not able to deal with Denverton
systems.

Change-Id: I615305da5865bef305f560f5c90482cf0937b25a
Signed-off-by: Felix Singer 
Signed-off-by: Johanna Schander 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/30931
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Michael Niewöhner
---
M src/drivers/intel/fsp2_0/Kconfig
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/denverton_ns/Makefile.inc
3 files changed, 11 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Michael Niewöhner: Looks good to me, but someone else must approve

diff --git a/src/drivers/intel/fsp2_0/Kconfig b/src/drivers/intel/fsp2_0/Kconfig
index 2d45343..2624644 100644
--- a/src/drivers/intel/fsp2_0/Kconfig
+++ b/src/drivers/intel/fsp2_0/Kconfig
@@ -55,7 +55,8 @@
    depends on ADD_FSP_BINARIES
    depends on SOC_INTEL_APOLLOLAKE || SOC_INTEL_SKYLAKE || \
    	SOC_INTEL_KABYLAKE || SOC_INTEL_COFFEELAKE || \
-		SOC_INTEL_ICELAKE  || SOC_INTEL_WHISKEYLAKE
+		SOC_INTEL_ICELAKE  || SOC_INTEL_WHISKEYLAKE || \
+		SOC_INTEL_DENVERTON_NS
    help
      When selecting this option, the SoC must set FSP_HEADER_PATH
      and FSP_FD_PATH correctly so FSP splitting works.
diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig
index 9a61127..a74250b 100644
--- a/src/soc/intel/denverton_ns/Kconfig
+++ b/src/soc/intel/denverton_ns/Kconfig
@@ -79,6 +79,15 @@
    help
      The memory location of the Intel FSP-S binary for this platform.

+config FSP_HEADER_PATH
+	string
+	default "3rdparty/fsp/DenvertonNSFspBinPkg/Include/"
+
+config FSP_FD_PATH
+	string
+	depends on FSP_USE_REPO
+	default "3rdparty/fsp/DenvertonNSFspBinPkg/FspBin/DenvertonNSFsp.fd"
+
 # CAR memory layout on DENVERTON_NS hardware:
 ## CAR base address - 0xfef00000
 ## CAR size 1MB - 0x100 (0xfff00)
diff --git a/src/soc/intel/denverton_ns/Makefile.inc b/src/soc/intel/denverton_ns/Makefile.inc
index 4050f61..7529892 100644
--- a/src/soc/intel/denverton_ns/Makefile.inc
+++ b/src/soc/intel/denverton_ns/Makefile.inc
@@ -86,10 +86,8 @@
 verstage-$(CONFIG_DRIVERS_UART_8250MEM) += uart_debug.c

CPPFLAGS_common += -I$(src)/soc/intel/denverton_ns/include
-CPPFLAGS_common += -I$(src)/vendorcode/intel/fsp/fsp2_0/denverton_ns

##Set FSP binary blobs memory location
-
 $(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