Change in coreboot[master]: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place

[Arthur Heymans (Code Review)]

Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35884 )


Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................

soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place

include/device/pci_mmio_cfg.h now has a compile time error if
MMCONF_BASE_ADDRESS is not defined. This requires soc/intel target to
explicitly set this in the SOC dir, instead of potentially using a
possibly wrong common default.

Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/soc/intel/apollolake/Kconfig
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/common/block/systemagent/Kconfig
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/skylake/Kconfig
5 files changed, 16 insertions(+), 4 deletions(-)



  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/35884/1

diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig
index a1d3c07..24a4b7a 100644
--- a/src/soc/intel/apollolake/Kconfig
+++ b/src/soc/intel/apollolake/Kconfig
@@ -128,6 +128,10 @@
 	 TPM part is conntected on Fast SPI interface, but the LPC MMIO
 	 TPM transactions are decoded and serialized over the SPI interface.
 
+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 config PCR_BASE_ADDRESS
 	hex
 	default 0xd0000000
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index c1fda95..aeef75c 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -109,6 +109,10 @@
 	select FSP_T_XIP if FSP_CAR
 	select HECI_DISABLE_USING_SMM if !SOC_INTEL_COFFEELAKE && !SOC_INTEL_WHISKEYLAKE && !SOC_INTEL_COMETLAKE
 
+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 config DCACHE_RAM_BASE
 	default 0xfef00000
 
diff --git a/src/soc/intel/common/block/systemagent/Kconfig b/src/soc/intel/common/block/systemagent/Kconfig
index 1222573..d7619a0 100644
--- a/src/soc/intel/common/block/systemagent/Kconfig
+++ b/src/soc/intel/common/block/systemagent/Kconfig
@@ -3,10 +3,6 @@
 	help
 	  Intel Processor common System Agent support
 
-config MMCONF_BASE_ADDRESS
-	hex
-	default 0xe0000000
-
 config SA_PCIEX_LENGTH
 	hex
 	default 0x10000000 if (PCIEX_LENGTH_256MB)
diff --git a/src/soc/intel/denverton_ns/Kconfig b/src/soc/intel/denverton_ns/Kconfig
index 2aadcae..d12e4e3 100644
--- a/src/soc/intel/denverton_ns/Kconfig
+++ b/src/soc/intel/denverton_ns/Kconfig
@@ -91,6 +91,10 @@
 	int
 	default 16
 
+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 config PCR_BASE_ADDRESS
 	hex
 	default 0xfd000000
diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig
index 901e5f9..a9b8aaa 100644
--- a/src/soc/intel/skylake/Kconfig
+++ b/src/soc/intel/skylake/Kconfig
@@ -120,6 +120,10 @@
 	hex
 	default 0x200000
 
+config MMCONF_BASE_ADDRESS
+	hex
+	default 0xe0000000
+
 config DCACHE_RAM_BASE
 	hex
 	default 0xfef00000

-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-MessageType: newchange

[Nico Huber (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@10 
PS1, Line 10: This requires soc/intel target to
            : explicitly set this in the SOC dir
I see the sanity, but don't follow conclusion, why is it require by
"this"?


https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@12 
PS1, Line 12: wrong
How can it be wrong? does the FSP binary depend on a specific
location?



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Tue, 08 Oct 2019 12:56:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@10 
PS1, Line 10: This requires soc/intel target to
            : explicitly set this in the SOC dir
> I see the sanity, but don't follow conclusion, why is it require by […]
> It's a bit like the overuse of __weak in soc/intel to make stuff build without being correct. Requiring things to de explicitly done in the soc dir could avoid that? I'll try to better state that.

https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@12 
PS1, Line 12: wrong
> How can it be wrong? does the FSP binary depend on a specific
> location?

The integration guide recommends (I looked at kabylake) against using non default values and if FSP-T is used it needs to match what is programmed in that UPD, so I think it sufficiently safe to say there are values that should not be tried out.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Tue, 08 Oct 2019 13:28:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <[e-mail]>
Gerrit-MessageType: comment

[Nico Huber (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 1:

(2 comments)

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

https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@10 
PS1, Line 10: This requires soc/intel target to
            : explicitly set this in the SOC dir
> It's a bit like the overuse of __weak in soc/intel to make stuff build without being correct. […]
> To be clear, I'm just bikeshedding the relation to the previous sentence:
> if the fact that you added the compile time error would really require
> this change, Jenkins wouldn't have agreed on the original change :-p

What I read between the lines is roughly that "[To be really useful] this
requires [...]".


https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@12 
PS1, Line 12: wrong
> > How can it be wrong? does the FSP binary depend on a specific […]
> > I agree but still wonder: With a change like this, that has no technical
> > requirement, how can we prevent that it's not turned back. e.g. somebody
> > sees there are 8 out of 9 platforms that use the same value and puts it
> > back into common/...

I don't like comments, but in this case it could help to add something like

    Needs to be what individual FSP versions expect. Keep this in the
    individual soc/ directory.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 1
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Tue, 08 Oct 2019 13:44:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Arthur Heymans <[e-mail]>
Comment-In-Reply-To: Nico Huber <[e-mail]>
Gerrit-MessageType: comment

[David Guckian (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 2: Code-Review+1

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35884/1//COMMIT_MSG@12 
PS1, Line 12: wrong
> I agree but still wonder: With a change like this, that has no technical […]
> Agreed, SOC specific FSP-T will set a value for this, and no guarantee that all SOCs will use the same value.

[Vanny E (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 2: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:08:02 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 2:

off-topic: Is there easy way to replace Name(PCBA, CONFIG_MMCONF_BASE_ADDRESS) in asl?

Surely, there must be way to pass runtime C variable value in there instead?


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Kyösti Mälkki <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-Comment-Date: Tue, 05 Nov 2019 16:16:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 2:
> Patch Set 2:
>
> off-topic: Is there easy way to replace Name(PCBA, CONFIG_MMCONF_BASE_ADDRESS) in asl?
>
> Surely, there must be way to pass runtime C variable value in there instead?

We either build the tables at runtime or pull the value from gnvs (assuming Name() allows that).


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Aaron Durbin <[e-mail]>
Gerrit-CC: Kyösti Mälkki <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 06 Nov 2019 16:54:17 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 2: Code-Review+2


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 2
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanny E <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Kyösti Mälkki <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 06 Nov 2019 16:54:42 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded a new patch set (#3) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................

soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place

include/device/pci_mmio_cfg.h now has a compile time error if
MMCONF_BASE_ADDRESS is not defined. This requires soc/intel target to
explicitly set this in the SOC dir, instead of potentially using a
possibly wrong common default.

Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Signed-off-by: Arthur Heymans <[e-mail]>
---
M src/soc/intel/apollolake/Kconfig
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/denverton_ns/Kconfig
M src/soc/intel/skylake/Kconfig
4 files changed, 16 insertions(+), 0 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/35884/3
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 3:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/denverton_ns/... 
PS3, Line 45: config MMCONF_BASE_ADDRESS
            : 	hex
            : 	default 0xe0000000
See this.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 22 Jul 2020 21:59:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Andrey Petrov (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 3: Code-Review+2

thank you for doing this


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 22 Jul 2020 22:18:29 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/apollolake/Kc... 
File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/apollolake/Kc... 
PS3, Line 84: SOC_INTEL_COMMON_BLOCK_SA
This already defines MMCONF_BASE_ADDRESS


https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/cannonlake/Kc... 
File src/soc/intel/cannonlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/cannonlake/Kc... 
PS3, Line 98: SOC_INTEL_COMMON_BLOCK_SA
This already defines MMCONF_BASE_ADDRESS


https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/skylake/Kconf... 
File src/soc/intel/skylake/Kconfig:

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/skylake/Kconf... 
PS3, Line 63: SOC_INTEL_COMMON_BLOCK_SA
This already defines MMCONF_BASE_ADDRESS



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 3
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Wed, 22 Jul 2020 22:24:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 3:

(4 comments)

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/apollolake/Kc... 
File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/apollolake/Kc... 
PS3, Line 84: SOC_INTEL_COMMON_BLOCK_SA
> This already defines MMCONF_BASE_ADDRESS
> Done

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/cannonlake/Kc... 
File src/soc/intel/cannonlake/Kconfig:

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/cannonlake/Kc... 
PS3, Line 98: SOC_INTEL_COMMON_BLOCK_SA
> This already defines MMCONF_BASE_ADDRESS
> Done

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

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/denverton_ns/... 
PS3, Line 45: config MMCONF_BASE_ADDRESS
            : 	hex
            : 	default 0xe0000000
> See this.
> Done

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/skylake/Kconf... 
File src/soc/intel/skylake/Kconfig:

https://review.coreboot.org/c/coreboot/+/35884/3/src/soc/intel/skylake/Kconf... 
PS3, Line 63: SOC_INTEL_COMMON_BLOCK_SA
> This already defines MMCONF_BASE_ADDRESS
> Done

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has uploaded a new patch set (#4) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................

soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place

include/device/pci_mmio_cfg.h now has a compile time error if
MMCONF_BASE_ADDRESS is not defined. This requires soc/intel target to
explicitly set this in the SOC dir, instead of potentially using a
possibly wrong common default.

Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Signed-off-by: Arthur Heymans <[e-mail]>
Signed-off-by: Elyes HAOUAS <[e-mail]>
---
M src/soc/intel/apollolake/Kconfig
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/common/block/systemagent/Kconfig
M src/soc/intel/skylake/Kconfig
4 files changed, 12 insertions(+), 4 deletions(-)


  git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/35884/4
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: newpatchset

[David Guckian (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 4: Code-Review+1


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Angel Pons <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 23 Jul 2020 08:04:32 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 4: Code-Review-1

No. It doesn't make sense.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 23 Jul 2020 08:24:50 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 4:
> Patch Set 4: Code-Review-1
>
> No. It doesn't make sense.

not sure to understand 

anyway:
src/soc/intel/icelake/Kconfig will select SOC_INTEL_COMMON_BLOCK_SA but 
MMCONF_BASE_ADDRESS = 0xc0000000


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 23 Jul 2020 08:48:49 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Angel Pons (Code Review)]

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

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Patch Set 4:
> Patch Set 4:
> > Patch Set 4: Code-Review-1
> >
> > No. It doesn't make sense.
>
> not sure to understand
>
> anyway:
> src/soc/intel/icelake/Kconfig will select SOC_INTEL_COMMON_BLOCK_SA but 
> MMCONF_BASE_ADDRESS = 0xc0000000

See the reason why `soc/intel/common/Kconfig.common` is named like that in "soc/intel/Kconfig": the common Kconfig options will be picked up last 😊


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: HAOUAS Elyes <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-Comment-Date: Thu, 23 Jul 2020 09:09:06 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

[Arthur Heymans (Code Review)]

Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35884 )

Change subject: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
......................................................................


Abandoned

Fixed in master.
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/35884
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Idcfea1718cc36ae16b491604786c26c6ed320f06
Gerrit-Change-Number: 35884
Gerrit-PatchSet: 4
Gerrit-Owner: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: Aaron Durbin <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Angel Pons <[e-mail]>
Gerrit-Reviewer: Arthur Heymans <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: David Guckian <[e-mail]>
Gerrit-Reviewer: Patrick Rudolph <[e-mail]>
Gerrit-Reviewer: Vanessa Eusebio <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Nico Huber <[e-mail]>
Gerrit-CC: Paul Menzel <[e-mail]>
Gerrit-MessageType: abandon