Change in coreboot[master]: soc/intel/common/systemagent: add Kconfig guard

[Andrey Petrov (Code Review)]

Andrey Petrov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

soc/intel/common/systemagent: add Kconfig guard

Looks like having this in the tree force-sets MMCONF_BASE_ADDR to
some value which can't be overriden anywhere.

TEST=build test on wip platform

Change-Id: Ia160444e8ac7cac55153f659f4d98f4f77f0d467
---
M src/mainboard/intel/harcuvar/Kconfig
M src/mainboard/scaleway/tagada/Kconfig
M src/soc/intel/common/block/systemagent/Kconfig
3 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/38639/1

diff --git a/src/mainboard/intel/harcuvar/Kconfig b/src/mainboard/intel/harcuvar/Kconfig
index 271ff81..e5f7dad 100644
--- a/src/mainboard/intel/harcuvar/Kconfig
+++ b/src/mainboard/intel/harcuvar/Kconfig
@@ -42,4 +42,8 @@
    help
      Location of SPD binary for memory down function.

+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 endif # BOARD_INTEL_HARCUVAR
diff --git a/src/mainboard/scaleway/tagada/Kconfig b/src/mainboard/scaleway/tagada/Kconfig
index 21088cb..5db6048 100644
--- a/src/mainboard/scaleway/tagada/Kconfig
+++ b/src/mainboard/scaleway/tagada/Kconfig
@@ -42,4 +42,8 @@
    hex
    default 0x19	# SMBIOS_ENCLOSURE_MULTI_SYSTEM_CHASSIS

+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 endif # BOARD_SCALEWAY_TAGADA
diff --git a/src/soc/intel/common/block/systemagent/Kconfig b/src/soc/intel/common/block/systemagent/Kconfig
index 1222573..6dd1f3b 100644
--- a/src/soc/intel/common/block/systemagent/Kconfig
+++ b/src/soc/intel/common/block/systemagent/Kconfig
@@ -3,6 +3,8 @@
    help
      Intel Processor common System Agent support

+if SOC_INTEL_COMMON_BLOCK_SA
+
 config MMCONF_BASE_ADDRESS
    hex
    default 0xe0000000
@@ -36,3 +38,5 @@
    default n
    help
      This option allows you to add the DMA Protected Range (DPR).
+
+endif

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

Patch Set 1:

was originally https://review.coreboot.org/c/coreboot/+/38013

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/38639/1//COMMIT_MSG@9 
PS1, Line 9: Looks like having this in the tree force-sets MMCONF_BASE_ADDR to
           : some value which can't be overriden anywhe
That's surprising. IIUC, the first default value is picked up by Kconfig and the mainboard Kconfig files are sourced before soc/ ones.

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

Patch Set 1:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/38639/1//COMMIT_MSG@9 
PS1, Line 9: Looks like having this in the tree force-sets MMCONF_BASE_ADDR to
           : some value which can't be overriden anywhe
> That's surprising. […]

there as discussion in https://review.coreboot.org/c/coreboot/+/38013 (force abandoned)
it looks like if your platform is not in src/soc you can't really override it.
I do not really understand all the kconfig magic

[Andrey Petrov (Code Review)]

Hello Patrick Rudolph, David Hendricks, Vanessa Eusebio, David Hendricks, build bot (Jenkins), Nico Huber, David Guckian,

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

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

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

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

soc/intel/common/systemagent: add Kconfig guard

Looks like having this in the tree force-sets MMCONF_BASE_ADDR to
some value which can't be overriden anywhere.

TEST=build test on wip platform

Change-Id: Ia160444e8ac7cac55153f659f4d98f4f77f0d467
Signed-off-by: Andrey Petrov 
---
M src/mainboard/intel/harcuvar/Kconfig
M src/mainboard/scaleway/tagada/Kconfig
M src/soc/intel/common/block/systemagent/Kconfig
3 files changed, 12 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/38639/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/38639/1//COMMIT_MSG@9 
PS1, Line 9: Looks like having this in the tree force-sets MMCONF_BASE_ADDR to
           : some value which can't be overriden anywhe
> there as discussion in https://review.coreboot.org/c/coreboot/+/38013 (force abandoned) […]

I am still confused how this change would help. If your platform is not in src/soc, then I assume you are not selecting SOC_INTEL_COMMON. If that is the case, then this check should fail: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/Kcon...

https://review.coreboot.org/c/coreboot/+/38639/2/src/mainboard/intel/harcuva... 
File src/mainboard/intel/harcuvar/Kconfig:

https://review.coreboot.org/c/coreboot/+/38639/2/src/mainboard/intel/harcuva... 
PS2, Line 45: config MMCONF_BASE_ADDRESS
Instead of adding these to mainboard, I think it would be better to add these to soc/intel/denverton_ns/Kconfig

[Andrey Petrov (Code Review)]

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: add Kconfig guard
......................................................................

Patch Set 2:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/38639/1//COMMIT_MSG@9 
PS1, Line 9: Looks like having this in the tree force-sets MMCONF_BASE_ADDR to
           : some value which can't be overriden anywhe
> I am still confused how this change would help. […]

actually I am selecting SOC_INTEL_COMMON, because I need to select quite a bit of code in soc/intel/common/block

https://review.coreboot.org/c/coreboot/+/38639/2/src/mainboard/intel/harcuva... 
File src/mainboard/intel/harcuvar/Kconfig:

https://review.coreboot.org/c/coreboot/+/38639/2/src/mainboard/intel/harcuva... 
PS2, Line 45: config MMCONF_BASE_ADDRESS
> Instead of adding these to mainboard, I think it would be better to add these to soc/intel/denverton […]

the best idea would be for denverton to select SOC_INTEL_COMMON_BLOCK_SA and start using "common" systemagent code, if it is possible. But ok, I just moved the definition to Kconfig which is far better place for it, thanks

Change in coreboot[master]: soc/intel/common/systemagent: Add Kconfig guard

[Andrey Petrov (Code Review)]

Hello Patrick Rudolph, David Hendricks, Vanessa Eusebio, David Hendricks, build bot (Jenkins), Nico Huber, David Guckian,

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

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

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

Change subject: soc/intel/common/systemagent: Add Kconfig guard
......................................................................

soc/intel/common/systemagent: Add Kconfig guard

Looks like selecting SOC_INTEL_COMMON force-sets MMCONF_BASE_ADDR to
some value which can't be overriden outside of soc/intel/common. So
adding a non-SoC platform thats uses code from soc/intel/common is not
possible.

TEST=build test on wip platform

Change-Id: Ia160444e8ac7cac55153f659f4d98f4f77f0d467
Signed-off-by: Andrey Petrov 
---
M src/soc/intel/common/block/systemagent/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 8 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/38639/3

Change in coreboot[master]: soc/intel/common/systemagent: Add Kconfig guard

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: Add Kconfig guard
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: soc/intel/common/systemagent: Add Kconfig guard

[David Guckian (Code Review)]

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

Change subject: soc/intel/common/systemagent: Add Kconfig guard
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: soc/intel/common/systemagent: Add Kconfig guard

[Andrey Petrov (Code Review)]

Andrey Petrov has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: Add Kconfig guard
......................................................................

soc/intel/common/systemagent: Add Kconfig guard

Looks like selecting SOC_INTEL_COMMON force-sets MMCONF_BASE_ADDR to
some value which can't be overriden outside of soc/intel/common. So
adding a non-SoC platform thats uses code from soc/intel/common is not
possible.

TEST=build test on wip platform

Change-Id: Ia160444e8ac7cac55153f659f4d98f4f77f0d467
Signed-off-by: Andrey Petrov 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38639
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
Reviewed-by: David Guckian
---
M src/soc/intel/common/block/systemagent/Kconfig
M src/soc/intel/denverton_ns/Kconfig
2 files changed, 8 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  David Guckian: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/common/block/systemagent/Kconfig b/src/soc/intel/common/block/systemagent/Kconfig
index 1222573..6dd1f3b 100644
--- a/src/soc/intel/common/block/systemagent/Kconfig
+++ b/src/soc/intel/common/block/systemagent/Kconfig
@@ -3,6 +3,8 @@
    help
      Intel Processor common System Agent support

+if SOC_INTEL_COMMON_BLOCK_SA
+
 config MMCONF_BASE_ADDRESS
    hex
    default 0xe0000000
@@ -36,3 +38,5 @@
    default n
    help
      This option allows you to add the DMA Protected Range (DPR).
+
+endif
diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig
index fa49eb0..aed2beb 100644
--- a/src/soc/intel/denverton_ns/Kconfig
+++ b/src/soc/intel/denverton_ns/Kconfig
@@ -54,6 +54,10 @@
    select UDK_2015_BINDING
    select CPU_INTEL_FIRMWARE_INTERFACE_TABLE

+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 config FSP_T_ADDR
    hex "Intel FSP-T (temp ram init) binary location"
    depends on ADD_FSP_BINARIES && FSP_CAR

Change in coreboot[master]: soc/intel/common/systemagent: Add Kconfig guard

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38639 )

Change subject: soc/intel/common/systemagent: Add Kconfig guard
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/360
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/359
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/358

Please note: This test is under development and might not be accurate at all!