Change in coreboot[master]: soc/intel/Kconfig: Move MMCONF_BASE_ADDRESS out of a common place
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 arthur@aheymans.xyz --- 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
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?
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.
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.
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 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
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?
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).
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
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 arthur@aheymans.xyz --- 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
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.
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
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
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 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 arthur@aheymans.xyz Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- 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
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
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.
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
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 😊
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.
-
Aaron Durbin (Code Review) -
Andrey Petrov (Code Review)
-
Angel Pons (Code Review)
-
Arthur Heymans (Code Review)
-
David Guckian (Code Review)
-
HAOUAS Elyes (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Vanny E (Code Review)