Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33116
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on ARCH_RAMSTAGE_X86_32 ......................................................................
Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on ARCH_RAMSTAGE_X86_32
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/x4x/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 14 files changed, 14 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/1
diff --git a/src/cpu/intel/model_2065x/Kconfig b/src/cpu/intel/model_2065x/Kconfig index d8c0168..e7679a6 100644 --- a/src/cpu/intel/model_2065x/Kconfig +++ b/src/cpu/intel/model_2065x/Kconfig @@ -21,7 +21,7 @@ select CPU_INTEL_COMMON select NO_FIXED_XIP_ROM_SIZE select PARALLEL_MP - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32
config BOOTBLOCK_CPU_INIT string diff --git a/src/cpu/intel/model_206ax/Kconfig b/src/cpu/intel/model_206ax/Kconfig index dbb8982..951f45b 100644 --- a/src/cpu/intel/model_206ax/Kconfig +++ b/src/cpu/intel/model_206ax/Kconfig @@ -20,7 +20,7 @@ #select AP_IN_SIPI_WAIT select TSC_SYNC_MFENCE select CPU_INTEL_COMMON - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select PARALLEL_MP select NO_FIXED_XIP_ROM_SIZE
diff --git a/src/northbridge/intel/gm45/Kconfig b/src/northbridge/intel/gm45/Kconfig index f1318eb..a704467 100644 --- a/src/northbridge/intel/gm45/Kconfig +++ b/src/northbridge/intel/gm45/Kconfig @@ -30,7 +30,7 @@ select POSTCAR_CONSOLE select SMM_TSEG select PARALLEL_MP - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32
config CBFS_SIZE hex diff --git a/src/northbridge/intel/haswell/Kconfig b/src/northbridge/intel/haswell/Kconfig index e1067c5..3081c28 100644 --- a/src/northbridge/intel/haswell/Kconfig +++ b/src/northbridge/intel/haswell/Kconfig @@ -19,7 +19,7 @@ select CACHE_MRC_SETTINGS select INTEL_DDI select INTEL_GMA_ACPI - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select POSTCAR_STAGE select POSTCAR_CONSOLE select C_ENVIRONMENT_BOOTBLOCK diff --git a/src/northbridge/intel/i945/Kconfig b/src/northbridge/intel/i945/Kconfig index 2c21420..d3b760d 100644 --- a/src/northbridge/intel/i945/Kconfig +++ b/src/northbridge/intel/i945/Kconfig @@ -31,7 +31,7 @@ select POSTCAR_CONSOLE select SMM_TSEG select PARALLEL_MP - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32
config NORTHBRIDGE_INTEL_SUBTYPE_I945GC def_bool n diff --git a/src/northbridge/intel/pineview/Kconfig b/src/northbridge/intel/pineview/Kconfig index 2b4f502..b8f6761 100644 --- a/src/northbridge/intel/pineview/Kconfig +++ b/src/northbridge/intel/pineview/Kconfig @@ -32,7 +32,7 @@ select POSTCAR_CONSOLE select SMM_TSEG select PARALLEL_MP - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select C_ENVIRONMENT_BOOTBLOCK
config BOOTBLOCK_NORTHBRIDGE_INIT diff --git a/src/northbridge/intel/x4x/Kconfig b/src/northbridge/intel/x4x/Kconfig index 7cae91e..a8b492c 100644 --- a/src/northbridge/intel/x4x/Kconfig +++ b/src/northbridge/intel/x4x/Kconfig @@ -30,7 +30,7 @@ select POSTCAR_CONSOLE select SMM_TSEG select PARALLEL_MP - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32
config CBFS_SIZE hex diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index d4e1feb..99fd33b 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -55,7 +55,7 @@ select C_ENVIRONMENT_BOOTBLOCK select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select PARALLEL_MP select PARALLEL_MP_AP_WORK select HAVE_SMI_HANDLER diff --git a/src/soc/intel/apollolake/Kconfig b/src/soc/intel/apollolake/Kconfig index 217d1ea..0bfbf03 100644 --- a/src/soc/intel/apollolake/Kconfig +++ b/src/soc/intel/apollolake/Kconfig @@ -40,7 +40,7 @@ # Misc options select C_ENVIRONMENT_BOOTBLOCK select CACHE_MRC_SETTINGS - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select COLLECT_TIMESTAMPS select COMMON_FADT select FSP_PLATFORM_MEMORY_SETTINGS_VERSIONS diff --git a/src/soc/intel/braswell/Kconfig b/src/soc/intel/braswell/Kconfig index ed5c972..0c082d0 100644 --- a/src/soc/intel/braswell/Kconfig +++ b/src/soc/intel/braswell/Kconfig @@ -14,7 +14,7 @@ select ARCH_VERSTAGE_X86_32 select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select COLLECT_TIMESTAMPS select SUPPORT_CPU_UCODE_IN_CBFS select MICROCODE_BLOB_NOT_IN_BLOB_REPO diff --git a/src/soc/intel/broadwell/Kconfig b/src/soc/intel/broadwell/Kconfig index d81ab8a..9c99569 100644 --- a/src/soc/intel/broadwell/Kconfig +++ b/src/soc/intel/broadwell/Kconfig @@ -15,7 +15,7 @@ select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS select MRC_SETTINGS_PROTECT - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select SUPPORT_CPU_UCODE_IN_CBFS select HAVE_MONOTONIC_TIMER diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index e524275..7448e3f 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -57,7 +57,7 @@ select BOOT_DEVICE_SUPPORTS_WRITES select C_ENVIRONMENT_BOOTBLOCK select CACHE_MRC_SETTINGS - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select COMMON_FADT select CPU_INTEL_COMMON select CPU_INTEL_FIRMWARE_INTERFACE_TABLE diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index a4b46ba..5e7a659 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -16,7 +16,7 @@ select BOOT_DEVICE_SUPPORTS_WRITES select C_ENVIRONMENT_BOOTBLOCK select CACHE_MRC_SETTINGS - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select COMMON_FADT select CPU_INTEL_FIRMWARE_INTERFACE_TABLE select FSP_M_XIP diff --git a/src/soc/intel/skylake/Kconfig b/src/soc/intel/skylake/Kconfig index fcfe2b6..b403540 100644 --- a/src/soc/intel/skylake/Kconfig +++ b/src/soc/intel/skylake/Kconfig @@ -23,7 +23,7 @@ select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH select BOOT_DEVICE_SUPPORTS_WRITES select CACHE_MRC_SETTINGS - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if ARCH_RAMSTAGE_X86_32 select COLLECT_TIMESTAMPS select COMMON_FADT select CPU_INTEL_COMMON
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on ARCH_RAMSTAGE_X86_32 ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33116/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33116/1//COMMIT_MSG@7 PS1, Line 7: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on ARCH_RAMSTAGE_X86_32 why? That's not detailed here. if you want to make a dependency then add it at the site of the Kconfig definition.
Hello ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#2).
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on STAGE_RAMSTAGE ......................................................................
Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on STAGE_RAMSTAGE
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/x4x/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 15 files changed, 4 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/2
Hello ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#3).
Change subject: Kconfig: Enable CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM based on CONFIG_STAGE_RAMSTAGE ......................................................................
Kconfig: Enable CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM based on CONFIG_STAGE_RAMSTAGE
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/3
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Enable CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM based on CONFIG_STAGE_RAMSTAGE ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33116/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/3/src/Kconfig@281 PS3, Line 281: config CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM What about this Kconfig entry? You are adding the same one below.
https://review.coreboot.org/#/c/33116/3/src/Kconfig@1214 PS3, Line 1214: ARCH_X86 This option isn't really dependent on x86.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Enable CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM based on CONFIG_STAGE_RAMSTAGE ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/33116/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/3/src/Kconfig@281 PS3, Line 281: config CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
What about this Kconfig entry? You are adding the same one below.
i will add default here then
https://review.coreboot.org/#/c/33116/3/src/Kconfig@1214 PS3, Line 1214: ARCH_X86
This option isn't really dependent on x86.
true, so far only x86 platform has selected this config
Hello ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#4).
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
This patch sets default value for CONFIG_CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM based on CONFIG_ENABLE_STAGE_RAMSTAGE
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/4
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 4: Code-Review+2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33116/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33116/4//COMMIT_MSG@7 PS4, Line 7: Set default value Could you state the reason to set this default?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33116/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33116/4//COMMIT_MSG@7 PS4, Line 7: Set default value
Could you state the reason to set this default?
It's really adding the proper dependency...
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33116/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33116/4//COMMIT_MSG@7 PS4, Line 7: Set default value
It's really adding the proper dependency...
if we don't have ramstage enable then does it make sense to have a config (CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) enable which relies on ramstage ?
i guess i have added 1 line below with same statement.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 4:
willn't merge unless below patchsets are +2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33116/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/7/src/Kconfig@252 PS7, Line 252: default n if !ENABLE_STAGE_RAMSTAGE This is already default n implicitly, so what does this line add?
Also, this option doesn't appear in menuconfig and only seems to be designed to be 'select'ed by other options. 'select' always overrides everything else (both defaults and 'depends on'), so adding this here is pointless and potentially misleading, because it won't prevent the option from being on even if ENABLE_STAGE_RAMSTAGE is off.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33116/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/7/src/Kconfig@252 PS7, Line 252: default n if !ENABLE_STAGE_RAMSTAGE
This is already default n implicitly, so what does this line add? […]
i was just trying to make sure all root dependent kconfig doesn't get default select. I understand your point, do you have some suggestion to make those root dependent config uncheck when root is not select ?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33116/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/7/src/Kconfig@252 PS7, Line 252: default n if !ENABLE_STAGE_RAMSTAGE
i was just trying to make sure all root dependent kconfig doesn't get default select. […]
I don't understand what you're asking. What does "root dependent" mean? This has a 'depends on RELOCATABLE_RAMSTAGE', so it already transitively depends on ENABLE_RAMSTAGE anyway, but as I said dependencies don't matter for select. You cannot make sure this is not selected by another option. Kconfig just doesn't allow you to control that.
Just leave this like it was, I don't understand why anything has to change here. This isn't menuconfig-visible, and what value this config has for builds where it doesn't have any effect either way doesn't matter.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Abandoned
Subrata Banik has restored this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Set default value for CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM ......................................................................
Restored
Hello Aaron Durbin, ron minnich, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#8).
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE
Although CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM has internal dependencies (through kconfig) on RELOCATABLE_RAMSTAGE, so it already transitively depends on ENABLE_RAMSTAGE anyway. Irrespective of that SoC users are selecting CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM config unconditionally.
This patch allows SoC user to select CONFIG_CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM based on ENABLE_STAGE_RAMSTAGE.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/x4x/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 14 files changed, 14 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/8
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/#/c/33116/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/7/src/Kconfig@252 PS7, Line 252: default n if !ENABLE_STAGE_RAMSTAGE
I don't understand what you're asking. […]
can you please look at newer patchset, i have put explicit dependency from soc select
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Stage cache is used by 3 components: 1. Refcode --> Added/used by romstage and/or ramstage 2. Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists) 3. Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations: 1. External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region 2. Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like: 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
2. Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
3. Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
4. Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
5. Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9: Code-Review-1
this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Stage cache is used by 3 components:
- Refcode --> Added/used by romstage and/or ramstage
- Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists)
- Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations:
- External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region
- Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like:
Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
There already is the Kconfig option NO_STAGE_CACHE which defaults to 'y' on !HAVE_ACPI_RESUME. The program loader functions already use that Kconfig option to determine whether to load stages into the cache. To deal with platforms that HAVE_ACPI_RESUME but no relocatable ramstage (but HUGE_ACPI_BACKUP) those weak functions exist. This is however something we ought to deprecate...
I think it is a good idea to reverse the logic of that Kconfig option and go with your suggestion.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
> Patch Set 9: Code-Review-1 > > this is also used to cache postcar stage uitside cbmem
this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Stage cache is used by 3 components:
- Refcode --> Added/used by romstage and/or ramstage
- Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists)
- Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations:
- External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region
- Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like:
Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
There already is the Kconfig option NO_STAGE_CACHE which defaults to 'y' on !HAVE_ACPI_RESUME. The program loader functions already use that Kconfig option to determine whether to load stages into the cache. To deal with platforms that HAVE_ACPI_RESUME but no relocatable ramstage (but HUGE_ACPI_BACKUP) those weak functions exist. This is however something we ought to deprecate...
I think it is a good idea to reverse the logic of that Kconfig option and go with your suggestion.
Thanks Furquan for writing such detailed review. Yes, today coreboot stage_cache location is unnecessarily depends on certain stage (ramstage) which shouldn't be case.
i will try to make changes as below
1. Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) INTERNAL/CBMEM_STAGE_CACHE which are dependent only on !NO_STAGE_CACHE and are mutually exclusive 2. Platform should able to select (A) or (B) 3. Modify .inc as applicable with those configs. 4. Move FSP stage_cache logic into common code
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
> Patch Set 9: > > > Patch Set 9: Code-Review-1 > > > > this is also used to cache postcar stage uitside cbmem > > this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage
postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Stage cache is used by 3 components:
- Refcode --> Added/used by romstage and/or ramstage
- Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists)
- Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations:
- External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region
- Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like:
Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
There already is the Kconfig option NO_STAGE_CACHE which defaults to 'y' on !HAVE_ACPI_RESUME. The program loader functions already use that Kconfig option to determine whether to load stages into the cache. To deal with platforms that HAVE_ACPI_RESUME but no relocatable ramstage (but HUGE_ACPI_BACKUP) those weak functions exist. This is however something we ought to deprecate...
I think it is a good idea to reverse the logic of that Kconfig option and go with your suggestion.
Thanks Furquan for writing such detailed review. Yes, today coreboot stage_cache location is unnecessarily depends on certain stage (ramstage) which shouldn't be case.
i will try to make changes as below
- Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) INTERNAL/CBMEM_STAGE_CACHE which are dependent only on !NO_STAGE_CACHE and are mutually exclusive
- Platform should able to select (A) or (B)
- Modify .inc as applicable with those configs.
- Move FSP stage_cache logic into common code
Furquan, one question, do you think (A) or (B) can be also optional? i mean platform can decide to skip stage_cache as well ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
> Patch Set 9: > > > Patch Set 9: > > > > > Patch Set 9: Code-Review-1 > > > > > > this is also used to cache postcar stage uitside cbmem > > > > this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage > > postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage
then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Stage cache is used by 3 components:
- Refcode --> Added/used by romstage and/or ramstage
- Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists)
- Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations:
- External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region
- Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like:
Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
There already is the Kconfig option NO_STAGE_CACHE which defaults to 'y' on !HAVE_ACPI_RESUME. The program loader functions already use that Kconfig option to determine whether to load stages into the cache. To deal with platforms that HAVE_ACPI_RESUME but no relocatable ramstage (but HUGE_ACPI_BACKUP) those weak functions exist. This is however something we ought to deprecate...
I think it is a good idea to reverse the logic of that Kconfig option and go with your suggestion.
Thanks Furquan for writing such detailed review. Yes, today coreboot stage_cache location is unnecessarily depends on certain stage (ramstage) which shouldn't be case.
i will try to make changes as below
- Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) INTERNAL/CBMEM_STAGE_CACHE which are dependent only on !NO_STAGE_CACHE and are mutually exclusive
- Platform should able to select (A) or (B)
- Modify .inc as applicable with those configs.
- Move FSP stage_cache logic into common code
Furquan, one question, do you think (A) or (B) can be also optional? i mean platform can decide to skip stage_cache as well ?
If NO_STAGE_CACHE is true, then yes, there is no stage cache and so (A) and (B) don't make sense. But, if a platform does not select NO_STAGE_CACHE, it will need either (A) or (B).
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM selection based on ENABLE_STAGE_RAMSTAGE ......................................................................
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
Patch Set 9:
> Patch Set 9: > > > Patch Set 9: > > > > > Patch Set 9: > > > > > > > Patch Set 9: Code-Review-1 > > > > > > > > this is also used to cache postcar stage uitside cbmem > > > > > > this config is mainly depends on relocatable ranstage config, not sure why we have created a config which has root dependency on ramstage but use across romstage, postcar and ramstage > > > > postcarstage is always relocatable. so the depency is on either have_relocatable_ramstage or postcar_stage > > then, i guess kconfig name should reflect the same. isn't it ?
Yes. How about renaming it to "config EXTERNAL_STAGE_CACHE" that depends on RELOCATABLE_RAMSTAGE || POSTCAR_STAGE ?
Stage cache is used by 3 components:
- Refcode --> Added/used by romstage and/or ramstage
- Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists)
- Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations:
- External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region
- Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like:
Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
There already is the Kconfig option NO_STAGE_CACHE which defaults to 'y' on !HAVE_ACPI_RESUME. The program loader functions already use that Kconfig option to determine whether to load stages into the cache. To deal with platforms that HAVE_ACPI_RESUME but no relocatable ramstage (but HUGE_ACPI_BACKUP) those weak functions exist. This is however something we ought to deprecate...
I think it is a good idea to reverse the logic of that Kconfig option and go with your suggestion.
Thanks Furquan for writing such detailed review. Yes, today coreboot stage_cache location is unnecessarily depends on certain stage (ramstage) which shouldn't be case.
i will try to make changes as below
- Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) INTERNAL/CBMEM_STAGE_CACHE which are dependent only on !NO_STAGE_CACHE and are mutually exclusive
- Platform should able to select (A) or (B)
- Modify .inc as applicable with those configs.
- Move FSP stage_cache logic into common code
Furquan, one question, do you think (A) or (B) can be also optional? i mean platform can decide to skip stage_cache as well ?
If NO_STAGE_CACHE is true, then yes, there is no stage cache and so (A) and (B) don't make sense. But, if a platform does not select NO_STAGE_CACHE, it will need either (A) or (B).
got it. not sure if my observation is correct, I could see ~100ms lesser in boot time if platform doesn't select (A) or (B). I will dig deep on this.
Hello Aaron Durbin, ron minnich, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#10).
Change subject: Kconfig: Create STAGE_CACHE kconfig for platform to select ......................................................................
Kconfig: Create STAGE_CACHE kconfig for platform to select
This patch makes below code changes:
1. Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE and are mutually exclusive 2. Platform can select (A) or (B) 3. Modify .inc as applicable with those configs. 4. Move FSP stage_cache logic into common code (WIP)
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 81 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/10
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Create STAGE_CACHE kconfig for platform to select ......................................................................
Patch Set 10:
Furquan, can you please check if newer patchset aligns with your view
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Create STAGE_CACHE kconfig for platform to select ......................................................................
Patch Set 10:
Patch Set 10:
Furquan, can you please check if newer patchset aligns with your view
Hi Subrata, I will get back to you in couple of days. Thanks!
Hello Aaron Durbin, ron minnich, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#11).
Change subject: Kconfig: Create STAGE_CACHE kconfig for platform to select ......................................................................
Kconfig: Create STAGE_CACHE kconfig for platform to select
This patch makes below code changes:
1. Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE and are mutually exclusive 2. Platform can select (A) or (B) 3. Modify .inc as applicable with those configs. 4. Move FSP stage_cache logic into common code (WIP)
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 107 insertions(+), 83 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/11
Hello Aaron Durbin, ron minnich, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#12).
Change subject: Kconfig: Create STAGE_CACHE kconfig for platform to select ......................................................................
Kconfig: Create STAGE_CACHE kconfig for platform to select
This patch makes below code changes:
1. Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE and are mutually exclusive 2. Platform can select (A) or (B) 3. Modify .inc as applicable with those configs. 4. Move FSP stage_cache logic into common code (WIP)
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 96 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/12
Hello Aaron Durbin, ron minnich, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#13).
Change subject: Kconfig: Create STAGE_CACHE kconfig for platform to select ......................................................................
Kconfig: Create STAGE_CACHE kconfig for platform to select
This patch makes below code changes:
1. Define 2 kconfig (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE and are mutually exclusive 2. Platform can select (A) or (B) 3. Modify .inc as applicable with those configs. 4. Move FSP stage_cache logic into common code (WIP)
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 95 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/13
Hello Aaron Durbin, ron minnich, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#14).
Change subject: Kconfig: Create choice for platform stage cache selection ......................................................................
Kconfig: Create choice for platform stage cache selection
This patch makes stage cache kconfig proper
1. Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE (C) NO_STAGE_CACHE - Selecting (A) or (B) will enable CONFIG_STAGE_CACHE 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 94 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/14
Hello Aaron Durbin, ron minnich, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#15).
Change subject: Kconfig: Create choice for platform stage cache selection ......................................................................
Kconfig: Create choice for platform stage cache selection
This patch makes stage cache kconfig proper
1. Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE (C) NO_STAGE_CACHE - Selecting (A) or (B) will enable CONFIG_STAGE_CACHE 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 94 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/15
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Create choice for platform stage cache selection ......................................................................
Patch Set 15: Code-Review-1
(1 comment)
https://review.coreboot.org/#/c/33116/15/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/15/src/Kconfig@259 PS15, Line 259: prompt "Stage Cache Options" Why should this become a user selectable option?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Create choice for platform stage cache selection ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/#/c/33116/15/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/15/src/Kconfig@259 PS15, Line 259: prompt "Stage Cache Options"
Why should this become a user selectable option?
so you don't think "choice" might be a good option? I can remove choice, please review the whole philosophy and let me know if this is what we like to achieve ?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Create choice for platform stage cache selection ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/#/c/33116/15/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/15/src/Kconfig@259 PS15, Line 259: prompt "Stage Cache Options"
so you don't think "choice" might be a good option? […]
The platform either has relocatable stages (postcar/ramstage) and then it should use stage cache in cbmem or an external if implemented or it does not have relocatable stages and then no cache should be implemented at all. Neither option should be user visible.
Hello Aaron Durbin, ron minnich, Arthur Heymans, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#16).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper
1. Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE (C) NO_STAGE_CACHE - Selecting (A) or (B) will enable CONFIG_STAGE_CACHE 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 33 files changed, 88 insertions(+), 72 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/16
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 16:
(1 comment)
https://review.coreboot.org/#/c/33116/15/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/33116/15/src/Kconfig@259 PS15, Line 259: prompt "Stage Cache Options"
The platform either has relocatable stages (postcar/ramstage) and then it should use stage cache in […]
Done
Hello Aaron Durbin, ron minnich, Arthur Heymans, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#17).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper
1. Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE (C) NO_STAGE_CACHE - Selecting (A) or (B) will enable CONFIG_STAGE_CACHE 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/security/memory/Kconfig M src/security/memory/memory_clear.c M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 36 files changed, 91 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/17
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make? I understand that it is true at this point for the current platforms. But what about this: If a platform does not support relocatable ramstage, but still wants to use stage cache to stash refcode/S3 data for example, would that be a valid use case?
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@248 PS17, Line 248: ramstage to be built as a : relocatable module Is it right to mention ramstage here since you seem to be making it more of a generic relocatable stages option now?
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@258 PS17, Line 258: . It would be good to mention that the platform is responsible for providing storage for this external stage cache.
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@262 PS17, Line 262: select RELOCATABLE_MODULES RELOCATABLE_MODULES can be set to true based on !NO_RELOCATABLE_STAGES? i.e. EXTERNAL_STAGE_CACHE or CBMEM_STAGE_CACHE don't need to select them, right?
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@264 PS17, Line 264: enable enables
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@266 PS17, Line 266: chiset chipset
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@272 PS17, Line 272: enable enables
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@279 PS17, Line 279: NO_RELOCATABLE_STAGES || This condition does not look completely right to me. I believe it should just be based on !HAVE_ACPI_RESUME. Same comment as above for STAGE_CACHE.
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc@... PS17, Line 378: CONFIG_STAGE_CACHE It just feels a bit twisted that ramstage being a rmodule is dependent on availability of stage cache. Should we still retain CONFIG_RELOCATABLE_RAMSTAGE?
https://review.coreboot.org/c/coreboot/+/33116/17/src/include/stage_cache.h File src/include/stage_cache.h:
https://review.coreboot.org/c/coreboot/+/33116/17/src/include/stage_cache.h@... PS17, Line 40: CONFIG_NO_STAGE_CACHE This should be updated as well.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES
This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. […]
If a platform does not support relocatable ramstage, but still wants to use stage cache to stash refcode/S3 data for example, would that be a valid use case?
Right now this is invalid usecase and platform will hang in S3 resume.
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc@... PS17, Line 378: CONFIG_STAGE_CACHE
It just feels a bit twisted that ramstage being a rmodule is dependent on availability of stage cach […]
we should make this !NO_RELOCATABLE_RAMSTAGE ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES
If a platform does not support relocatable ramstage, but still wants to use stage cache to stash […]
Interesting. Did you check where the hang comes from?
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc@... PS17, Line 378: CONFIG_STAGE_CACHE
we should make this !NO_RELOCATABLE_RAMSTAGE ?
That might keep things clear.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
I did not do full review here, just couple comments.
Leadership may have decided that ARCH_X86=y && NO_RELOCATABLE_RAMSTAGE=y is not allowed after October 2019 release.
Now, before that, once 4.10 release happens, I plan to rebase HAVE_ACPI_RESUME=y && NO_RELOCATABLE_RAMSTAGE=y patchset from 2018 (CB:26384 and friends). I am not sure if I get it approved, but despite multiple request I don't remember anyone sending logs of this feature actually working on the two amdfam10-15 boards.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES
Interesting. […]
i will share log, its throwing exception while running from stage cache in that case
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES
i will share log, its throwing exception while running from stage cache in that case
Chrome EC: clear events_b mask to 0x0000000021004000 top_of_ram = 0x9a000000 MTRR Range: Start=99000000 End=9a000000 (Size 1000000) MTRR Range: Start=ff000000 End=0 (Size 1000000) CPU Index 0 - APIC 0 Unexpected Exception:6 @ 08:00000028 - Halting Code: 0 eflags: 00010082 cr2: 00000000 eax: 005c0004 ebx: fef06f6c ecx: 00000012 edx: 99c2d0cc edi: 00000000 esi: fef06fd0 ebp: fef06ff8 esp: fef06f40
ffffffe8: 00 00 00 00 00 00 00 00 fffffff0: e9 0d 40 ff ff 66 90 66 fffffff8: 90 66 90 66 38 40 cf ff 00000000: d8 93 c2 99 5c 00 08 4e 00000008: 56 53 41 0c 00 20 b9 99 00000010: 10 82 04 00 5c 00 08 4f 00000018: 49 50 47 12 87 03 00 02 00000020: 12 8b 01 00 04 01 00 0e 00000028: ff ff ff ff ff ff ff ff 00000030: 0d 49 4e 54 33 34 42 42 00000038: 3a 30 30 00 12 86 01 00 00000040: 04 0a 03 01 0b 14 01 0d 00000048: 49 4e 54 33 34 42 42 3a 00000050: 30 30 00 53 54 55 56 57 00000058: 58 59 5a 5b 5c 5d 5e 5f 00000060: 60 61 62 63 64 65 66 67
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 17:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@244 PS17, Line 244: default !NO_RELOCATABLE_STAGES
Chrome EC: clear events_b mask to 0x0000000021004000 […]
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@248 PS17, Line 248: ramstage to be built as a : relocatable module
Is it right to mention ramstage here since you seem to be making it more of a generic relocatable st […]
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@258 PS17, Line 258: .
It would be good to mention that the platform is responsible for providing storage for this external […]
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@262 PS17, Line 262: select RELOCATABLE_MODULES
RELOCATABLE_MODULES can be set to true based on !NO_RELOCATABLE_STAGES? i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@264 PS17, Line 264: enable
enables
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@266 PS17, Line 266: chiset
chipset
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@272 PS17, Line 272: enable
enables
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/Kconfig@279 PS17, Line 279: NO_RELOCATABLE_STAGES ||
This condition does not look completely right to me. […]
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc File src/arch/x86/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/17/src/arch/x86/Makefile.inc@... PS17, Line 378: CONFIG_STAGE_CACHE
That might keep things clear.
Done
https://review.coreboot.org/c/coreboot/+/33116/17/src/include/stage_cache.h File src/include/stage_cache.h:
https://review.coreboot.org/c/coreboot/+/33116/17/src/include/stage_cache.h@... PS17, Line 40: CONFIG_NO_STAGE_CACHE
This should be updated as well.
Done
Hello Aaron Durbin, ron minnich, Arthur Heymans, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#18).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper
1. Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE (C) NO_STAGE_CACHE - Selecting (A) or (B) will enable CONFIG_STAGE_CACHE 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/security/memory/Kconfig M src/security/memory/memory_clear.c M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 36 files changed, 94 insertions(+), 77 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/18
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18: Code-Review-2
That variable "RELOCATABLE_RAMSTAGE" has some amount of conceptional significance over the stage caches. We have also used it as criteria for board removals in past discussion on the mailing list.
I am too much unhappy about the Kconfig just disappering like this, thus my -2 here now, until we have some more reviewer opinions. At least, can you keep the Kconfig as a placeholder and keep all the existing cases of CONFIG(RELOCATABLE_RAMSTAGE) unmodified?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18:
Patch Set 18: Code-Review-2
That variable "RELOCATABLE_RAMSTAGE" has some amount of conceptional significance over the stage caches. We have also used it as criteria for board removals in past discussion on the mailing list.
I am too much unhappy about the Kconfig just disappering like this, thus my -2 here now, until we have some more reviewer opinions. At least, can you keep the Kconfig as a placeholder and keep all the existing cases of CONFIG(RELOCATABLE_RAMSTAGE) unmodified?
yes, i can keep RELOCATABLE_RAMSTAGE KConfig as is but if i don't modify .inc and .c then what this CL will do.
Please review with an opinion what we like to achieve and share feedback if you think its not meeting your requirement.
I believe review comment has enough description (from Furquan, Arthur) about why we started cleaning this stage_cache logic. Today entire stage_cache logic is tied with RAMSTAGE which shouldn't be the case as i understand, stage_cache is something well beyond ramstage.
And kconfig is not just disappearing its been replaced with some code which might not meet your requirement, i want to understand that.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18:
Patch Set 18:
Patch Set 18: Code-Review-2
That variable "RELOCATABLE_RAMSTAGE" has some amount of conceptional significance over the stage caches. We have also used it as criteria for board removals in past discussion on the mailing list.
I am too much unhappy about the Kconfig just disappering like this, thus my -2 here now, until we have some more reviewer opinions. At least, can you keep the Kconfig as a placeholder and keep all the existing cases of CONFIG(RELOCATABLE_RAMSTAGE) unmodified?
yes, i can keep RELOCATABLE_RAMSTAGE KConfig as is but if i don't modify .inc and .c then what this CL will do.
Please review with an opinion what we like to achieve and share feedback if you think its not meeting your requirement.
I believe review comment has enough description (from Furquan, Arthur) about why we started cleaning this stage_cache logic. Today entire stage_cache logic is tied with RAMSTAGE which shouldn't be the case as i understand, stage_cache is something well beyond ramstage.
And kconfig is not just disappearing its been replaced with some code which might not meet your requirement, i want to understand that.
For simplicity, here is feedback from Furquan
Stage cache is used by 3 components: 1. Refcode --> Added/used by romstage and/or ramstage 2. Ramstage if it is relocatable --> Added/used by romstage or postcar (if it exists) 3. Postcar --> Added/used by romstage
Originally, stage cache started out with #1 and #2, but then it was added for postcar as well. Stage cache can exist in two locations: 1. External to CBMEM -- Requires SoC/mainboard to provide an area for this. I believe most SoCs put this in SMM sub-region 2. Internal to CBMEM -- Provided by cbmem_stage_cache.c
If stage cache exists: Out of the above 3 components, refcode and postcar always are added to stage cache (either internal or external). However, ramstage is added to stage cache (internal/external) dependent upon the RELOCATABLE_RAMSTAGE config.
However, the config option for relocatable ramstage seems to have been incorrectly applied to more than ramstage. In addition to that, it looks like FSP is providing the implementation of stage_cache_external_region which is nothing to do with FSP (I believe it got moved there because FSP was adding refcode to stage cache?).
All of this can be untangled by doing something like: 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
2. Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
3. Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
4. Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
5. Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself.
Thoughts?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18:
And kconfig is not just disappearing its been replaced with some code which might not meet your requirement, i want to understand that.
I am only critisizing the silent removal of 'RELOCATABLE_RAMSTAGE'. It is a pretty fundamental Kconfig. Sure, you are replacing it, but there is no mention of doing so in the commit message. There was some chance of this being merged just before 4.10 release, thus my -2. I don't have proper understanding about possible new stage cache requirements triggering this particular commit.
As a background info, I believe I am the one who brought most of pre-Haswell Intels and AGESA/binaryPI AMD CPUs all the way through STATIC_CBMEM, DYNAMIC_CBMEM, EARLY_CBMEM_INIT, RELOCATABLE_RAMSTAGE and finally POSTCAR_STAGE phases. I guess I should know, but is there something fundamentally wrong with the name 'RELOCATABLE_RAMSTAGE'? We have had it for maybe six years like that. And from day one, it has implied parts other than the ramstage are also relocatable.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18:
Patch Set 18:
And kconfig is not just disappearing its been replaced with some code which might not meet your requirement, i want to understand that.
I am only critisizing the silent removal of 'RELOCATABLE_RAMSTAGE'. It is a pretty fundamental Kconfig. Sure, you are replacing it, but there is no mention of doing so in the commit message. There was some chance of this being merged just before 4.10 release, thus my -2. I don't have proper understanding about possible new stage cache requirements triggering this particular commit.
As a background info, I believe I am the one who brought most of pre-Haswell Intels and AGESA/binaryPI AMD CPUs all the way through STATIC_CBMEM, DYNAMIC_CBMEM, EARLY_CBMEM_INIT, RELOCATABLE_RAMSTAGE and finally POSTCAR_STAGE phases. I guess I should know, but is there something fundamentally wrong with the name 'RELOCATABLE_RAMSTAGE'? We have had it for maybe six years like that. And from day one, it has implied parts other than the ramstage are also relocatable.
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18:
Patch Set 18:
Patch Set 18:
And kconfig is not just disappearing its been replaced with some code which might not meet your requirement, i want to understand that.
I am only critisizing the silent removal of 'RELOCATABLE_RAMSTAGE'. It is a pretty fundamental Kconfig. Sure, you are replacing it, but there is no mention of doing so in the commit message. There was some chance of this being merged just before 4.10 release, thus my -2. I don't have proper understanding about possible new stage cache requirements triggering this particular commit.
As a background info, I believe I am the one who brought most of pre-Haswell Intels and AGESA/binaryPI AMD CPUs all the way through STATIC_CBMEM, DYNAMIC_CBMEM, EARLY_CBMEM_INIT, RELOCATABLE_RAMSTAGE and finally POSTCAR_STAGE phases. I guess I should know, but is there something fundamentally wrong with the name 'RELOCATABLE_RAMSTAGE'? We have had it for maybe six years like that. And from day one, it has implied parts other than the ramstage are also relocatable.
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 18:
Patch Set 18:
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#19).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Rename RELOCATABLE_RAMSTAGE and CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM Kconfig as CBMEM_STAGE_CACHE and EXTERNAL_STAGE_CACHE 3. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/arch/x86/Makefile.inc M src/arch/x86/acpi_s3.c M src/arch/x86/gdt.c M src/arch/x86/memlayout.ld M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/drivers/intel/fsp2_0/Makefile.inc M src/include/stage_cache.h M src/lib/Makefile.inc M src/lib/prog_loaders.c M src/northbridge/amd/amdfam10/Kconfig M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/northbridge/via/vx900/Kconfig M src/security/memory/Kconfig M src/security/memory/memory_clear.c M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/fsp_baytrail/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 36 files changed, 86 insertions(+), 70 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/19
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 18:
Patch Set 18:
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
I have added required details to know about RELOCATABLE_RAMSTAGE renaming
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 18:
Patch Set 18:
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
I have added required details to know about RELOCATABLE_RAMSTAGE renaming
As mentioned before, my idea is to untangle stage cache from relocatable ramstage. But that doesn't mean we have to get rid of RELOCATABLE_RAMSTAGE or its uses. Hence I commented on your patchset #17 -- "This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make?" I think RELOCATABLE_RAMSTAGE could just live as it is right now. That is what I had collected in my comment:
" 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
2. Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
3. Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
4. Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
5. Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself."
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 18:
Patch Set 18:
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
I have added required details to know about RELOCATABLE_RAMSTAGE renaming
As mentioned before, my idea is to untangle stage cache from relocatable ramstage. But that doesn't mean we have to get rid of RELOCATABLE_RAMSTAGE or its uses. Hence I commented on your patchset #17 -- "This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make?" I think RELOCATABLE_RAMSTAGE could just live as it is right now. That is what I had collected in my comment:
" 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself."
Entire stage_cache concept exist today based on RELOCATABLE_RAMSTAGE config which is default enable if !NO_RELOCATABLE_RAMSTAGE. Specifically CB has include ext_stage_cache.c or cbmem_stage_cache.c based on RELOCATABLE_RAMSTAGE or CACHE_RELOCATED_OUTSIDE_CBMEM Kconfig ?
What we like to achieve is that, to create generic Kconfig as RELOCATABLE_STAGES and enable .rmod based on that, also enable stage_cache on based on additional Kconfig HAVE_EXT_STAGE_CACHE if its ext to cbmem.
RELOCATABLE_RAMSTAGE and RELOCATABLE_STAGES are going to be duplicate, isn't it ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 18:
Patch Set 18:
I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
I have added required details to know about RELOCATABLE_RAMSTAGE renaming
As mentioned before, my idea is to untangle stage cache from relocatable ramstage. But that doesn't mean we have to get rid of RELOCATABLE_RAMSTAGE or its uses. Hence I commented on your patchset #17 -- "This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make?" I think RELOCATABLE_RAMSTAGE could just live as it is right now. That is what I had collected in my comment:
" 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself."
Entire stage_cache concept exist today based on RELOCATABLE_RAMSTAGE config which is default enable if !NO_RELOCATABLE_RAMSTAGE. Specifically CB has include ext_stage_cache.c or cbmem_stage_cache.c based on RELOCATABLE_RAMSTAGE or CACHE_RELOCATED_OUTSIDE_CBMEM Kconfig ?
What we like to achieve is that, to create generic Kconfig as RELOCATABLE_STAGES and enable .rmod based on that, also enable stage_cache on based on additional Kconfig HAVE_EXT_STAGE_CACHE if its ext to cbmem.
RELOCATABLE_RAMSTAGE and RELOCATABLE_STAGES are going to be duplicate, isn't it ?
Is the intention to create RELOCATABLE_STAGES because of RAMPAYLOAD i.e. having no ramstage?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 18:
Patch Set 18:
> I can add more description in commit msg, will wait for Furquan's further review before new patchset.
Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
I have added required details to know about RELOCATABLE_RAMSTAGE renaming
As mentioned before, my idea is to untangle stage cache from relocatable ramstage. But that doesn't mean we have to get rid of RELOCATABLE_RAMSTAGE or its uses. Hence I commented on your patchset #17 -- "This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make?" I think RELOCATABLE_RAMSTAGE could just live as it is right now. That is what I had collected in my comment:
" 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself."
Entire stage_cache concept exist today based on RELOCATABLE_RAMSTAGE config which is default enable if !NO_RELOCATABLE_RAMSTAGE. Specifically CB has include ext_stage_cache.c or cbmem_stage_cache.c based on RELOCATABLE_RAMSTAGE or CACHE_RELOCATED_OUTSIDE_CBMEM Kconfig ?
What we like to achieve is that, to create generic Kconfig as RELOCATABLE_STAGES and enable .rmod based on that, also enable stage_cache on based on additional Kconfig HAVE_EXT_STAGE_CACHE if its ext to cbmem.
RELOCATABLE_RAMSTAGE and RELOCATABLE_STAGES are going to be duplicate, isn't it ?
Is the intention to create RELOCATABLE_STAGES because of RAMPAYLOAD i.e. having no ramstage?
Its also true that RELOCATABLE_STAGES might help RAMPAYLOAD WIP activity for sure. But i guess the idea generates from you, when you said need to create more generic layer.
Also i'm not able to understand why <--->_RAMSTAGE_<--> (CONFIG_CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) be a config to choose what needs to be complied for romstage and postcar stage like below ?
ramstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c romstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmme_stage_cache.c postcar-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
Rather shouldn't be generic enough without specifying any such stage name in Kconfig ?
I have tested this patch with different sets of board rather only Intel platform to verify that new implementation of stage_cache don't break any such assumption made in past.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 18:
> Patch Set 18: > > > I can add more description in commit msg, will wait for Furquan's further review before new patchset. > > Btw, i don't understand if i need to wait for Oct'19 to merge this changes ?
Definetly not. I can accept a commit renaming 'RELOCATABLE_RAMSTAGE' immediately after 4.10 release. That is, if there is agreement we want to rename it at all due to predicted removal in near future (if POSTCAR_STAGE=y becomes criteria in 2019).
I have added required details to know about RELOCATABLE_RAMSTAGE renaming
As mentioned before, my idea is to untangle stage cache from relocatable ramstage. But that doesn't mean we have to get rid of RELOCATABLE_RAMSTAGE or its uses. Hence I commented on your patchset #17 -- "This equates STAGE_CACHE with RELOCATABLE_STAGES/RELOCATABLE_RAMSTAGE. Is that really a good assumption to make?" I think RELOCATABLE_RAMSTAGE could just live as it is right now. That is what I had collected in my comment:
" 1. Define configs for location of stage cache i.e. EXTERNAL_STAGE_CACHE and CBMEM_STAGE_CACHE which are dependent only on STAGE_CACHE config and are mutually exclusive.
Since stage cache support is a property of the platform, it doesn't have to be dependent on things like relocatable ramstage i.e. Platform can have stage cache support (internal / external) independent of other properties. Thus, add appropriate stage_cache files depending upon EXTERNAL_STAGE_CACHE/CBMEM_STAGE_CACHE to romstage, postcar and ramstage. (Basically any stage that exists).
Romstage and ramstage can add refcode and postcar to stage cache dependent on the selection of STAGE_CACHE. stage_cache_* APIs will take care of adding those to the right location. Probably we can get rid of the weak functions for stage_cache_*.
Romstage or Postcar (if present) will add ramstage to stage cache only if RELOCATABLE_RAMSTAGE is selected and STAGE_CACHE is enabled by the platform. Location of stage cache will be taken care of by stage_cache_* APIs.
Finally stage_cache implementation in FSP should be moved to intel/common/block/ since it has nothing to do with the FSP itself."
Entire stage_cache concept exist today based on RELOCATABLE_RAMSTAGE config which is default enable if !NO_RELOCATABLE_RAMSTAGE. Specifically CB has include ext_stage_cache.c or cbmem_stage_cache.c based on RELOCATABLE_RAMSTAGE or CACHE_RELOCATED_OUTSIDE_CBMEM Kconfig ?
What we like to achieve is that, to create generic Kconfig as RELOCATABLE_STAGES and enable .rmod based on that, also enable stage_cache on based on additional Kconfig HAVE_EXT_STAGE_CACHE if its ext to cbmem.
RELOCATABLE_RAMSTAGE and RELOCATABLE_STAGES are going to be duplicate, isn't it ?
Is the intention to create RELOCATABLE_STAGES because of RAMPAYLOAD i.e. having no ramstage?
Its also true that RELOCATABLE_STAGES might help RAMPAYLOAD WIP activity for sure. But i guess the idea generates from you, when you said need to create more generic layer.
Also i'm not able to understand why <--->_RAMSTAGE_<--> (CONFIG_CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM) be a config to choose what needs to be complied for romstage and postcar stage like below ?
ramstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c romstage-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmme_stage_cache.c postcar-$(CONFIG_RELOCATABLE_RAMSTAGE) += cbmem_stage_cache.c
Rather shouldn't be generic enough without specifying any such stage name in Kconfig ?
I have tested this patch with different sets of board rather only Intel platform to verify that new implementation of stage_cache don't break any such assumption made in past.
@Furquan, anything on this CL ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
RELOCATABLE_RAMSTAGE and RELOCATABLE_STAGES are going to be duplicate, isn't it ?
If at the end of the day, this is the case, renaming is a simple sed-job worth a completely separate commit. Personally, I would neither -2 or +2 such commit. It is just too big a change to be hidden (even with the two lines in the commit message) inside a 'stage cache' rework.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
RELOCATABLE_RAMSTAGE and RELOCATABLE_STAGES are going to be duplicate, isn't it ?
If at the end of the day, this is the case, renaming is a simple sed-job worth a completely separate commit. Personally, I would neither -2 or +2 such commit. It is just too big a change to be hidden (even with the two lines in the commit message) inside a 'stage cache' rework.
the purpose is to not just limit relocation for ramstage (its not today for sure, it just that those Kconfig names are confusing and not clear enough) and make it generic along with concept of internal and external stage_cache
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/PCP6...
In 24 April 2019 coreboot leadership minutes, you find mention that "Relocatable ramstage (on x86)" would be requirement for 4.11 release for platforms to stay on upstream master branch. I feel we just cause confusion if we rename the variable now, but no -2/+2 from me should such commit appear.
Equally my -2 here shall disappear if you can avoid the renaming.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/PCP6...
In 24 April 2019 coreboot leadership minutes, you find mention that "Relocatable ramstage (on x86)" would be requirement for 4.11 release for platforms to stay on upstream master branch. I feel we just cause confusion if we rename the variable now, but no -2/+2 from me should such commit appear.
Equally my -2 here shall disappear if you can avoid the renaming.
I am totally fine with keeping "Relocatable ramstage" configs as is.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/PCP6...
In 24 April 2019 coreboot leadership minutes, you find mention that "Relocatable ramstage (on x86)" would be requirement for 4.11 release for platforms to stay on upstream master branch. I feel we just cause confusion if we rename the variable now, but no -2/+2 from me should such commit appear.
Equally my -2 here shall disappear if you can avoid the renaming.
I am totally fine with keeping "Relocatable ramstage" configs as is.
So lets clarify few things
1. RELOCATABLE_RAMSTAGE name rename as is 2. ext_stage_cache.c file includes in rom/post/ramstage based on users selection of HAVE_EXT_STAGE_CACHE config 3. cbmem_stage_cache.c include is user doesn't selects HAVE_EXT_STAGE_CACHE and NO_STAGE_CACHE that means stage_cache move into internal cbmem 4. Anyway NO_STAGE_CACHE is dependent on HAVE_ACPI_TABLE.
this patch today modifies #1 log and remaining it does as is. So, i will maintain #1 as mentioned above.
rest fine ? all agrees ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/PCP6...
In 24 April 2019 coreboot leadership minutes, you find mention that "Relocatable ramstage (on x86)" would be requirement for 4.11 release for platforms to stay on upstream master branch. I feel we just cause confusion if we rename the variable now, but no -2/+2 from me should such commit appear.
Equally my -2 here shall disappear if you can avoid the renaming.
I am totally fine with keeping "Relocatable ramstage" configs as is.
So lets clarify few things
- RELOCATABLE_RAMSTAGE name rename as is
- ext_stage_cache.c file includes in rom/post/ramstage based on users selection of HAVE_EXT_STAGE_CACHE config
- cbmem_stage_cache.c include is user doesn't selects HAVE_EXT_STAGE_CACHE and NO_STAGE_CACHE that means stage_cache move into internal cbmem
- Anyway NO_STAGE_CACHE is dependent on HAVE_ACPI_TABLE.
this patch today modifies #1 log and remaining it does as is. So, i will maintain #1 as mentioned above.
rest fine ? all agrees ?
sorry for typo 1. RELOCATABLE_RAMSTAGE name remain as is
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/PCP6...
In 24 April 2019 coreboot leadership minutes, you find mention that "Relocatable ramstage (on x86)" would be requirement for 4.11 release for platforms to stay on upstream master branch. I feel we just cause confusion if we rename the variable now, but no -2/+2 from me should such commit appear.
Equally my -2 here shall disappear if you can avoid the renaming.
I am totally fine with keeping "Relocatable ramstage" configs as is.
So lets clarify few things
- RELOCATABLE_RAMSTAGE name rename as is
- ext_stage_cache.c file includes in rom/post/ramstage based on users selection of HAVE_EXT_STAGE_CACHE config
- cbmem_stage_cache.c include is user doesn't selects HAVE_EXT_STAGE_CACHE and NO_STAGE_CACHE that means stage_cache move into internal cbmem
- Anyway NO_STAGE_CACHE is dependent on HAVE_ACPI_TABLE.
this patch today modifies #1 log and remaining it does as is. So, i will maintain #1 as mentioned above.
rest fine ? all agrees ?
sorry for typo
- RELOCATABLE_RAMSTAGE name remain as is
I think its better to have separate Kconfigs just so that the checks don't become too complicated.
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
USE_CBMEM_STAGE_CACHE --> defaults to y depending upon !NO_STAGE_CACHE && !USE_EXTERNAL_STAGE_CACHE. This can add cbmem_stage_cache.c to appropriate stages.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
Coming back to this. I think we can agree that renaming of RELOCATABLE_RAMSTAGE(if really required) should probably be done as a separate commit and this change can focus primarily on clean up for the stage cache Kconfigs. Does that make sense?
https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/message/PCP6...
In 24 April 2019 coreboot leadership minutes, you find mention that "Relocatable ramstage (on x86)" would be requirement for 4.11 release for platforms to stay on upstream master branch. I feel we just cause confusion if we rename the variable now, but no -2/+2 from me should such commit appear.
Equally my -2 here shall disappear if you can avoid the renaming.
I am totally fine with keeping "Relocatable ramstage" configs as is.
So lets clarify few things
- RELOCATABLE_RAMSTAGE name rename as is
- ext_stage_cache.c file includes in rom/post/ramstage based on users selection of HAVE_EXT_STAGE_CACHE config
- cbmem_stage_cache.c include is user doesn't selects HAVE_EXT_STAGE_CACHE and NO_STAGE_CACHE that means stage_cache move into internal cbmem
- Anyway NO_STAGE_CACHE is dependent on HAVE_ACPI_TABLE.
this patch today modifies #1 log and remaining it does as is. So, i will maintain #1 as mentioned above.
rest fine ? all agrees ?
sorry for typo
- RELOCATABLE_RAMSTAGE name remain as is
I think its better to have separate Kconfigs just so that the checks don't become too complicated.
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
USE_CBMEM_STAGE_CACHE --> defaults to y depending upon !NO_STAGE_CACHE && !USE_EXTERNAL_STAGE_CACHE. This can add cbmem_stage_cache.c to appropriate stages.
Agree Furquan, we make these changes and add cbmem_stage_cache.c and ext_stage_cache.c based on those kconfig
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
Only i issue i saw here with AMD, Gardenia board, where HAVE_ACPI_TABLE is not selected hence NO_STAGE_CACHE is enable and in same time, mainboard also selects HAVE_EXTERNAL_STAGE_CACHE. As per this mainboard expectation, it might need to make use of stage_cache hence I can't make USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
Only i issue i saw here with AMD, Gardenia board, where HAVE_ACPI_TABLE is not selected hence NO_STAGE_CACHE is enable and in same time, mainboard also selects HAVE_EXTERNAL_STAGE_CACHE. As per this mainboard expectation, it might need to make use of stage_cache hence I can't make USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE
That sounds like a bug in the board?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
Only i issue i saw here with AMD, Gardenia board, where HAVE_ACPI_TABLE is not selected hence NO_STAGE_CACHE is enable and in same time, mainboard also selects HAVE_EXTERNAL_STAGE_CACHE. As per this mainboard expectation, it might need to make use of stage_cache hence I can't make USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE
That sounds like a bug in the board?
yeah, it appears like that, as it expects stage_cache to get involved hence i'm getting un-definition error (USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
Only i issue i saw here with AMD, Gardenia board, where HAVE_ACPI_TABLE is not selected hence NO_STAGE_CACHE is enable and in same time, mainboard also selects HAVE_EXTERNAL_STAGE_CACHE. As per this mainboard expectation, it might need to make use of stage_cache hence I can't make USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE
That sounds like a bug in the board?
yeah, it appears like that, as it expects stage_cache to get involved hence i'm getting un-definition error (USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE)
from AMD, gardenia .config
CONFIG_RELOCATABLE_RAMSTAGE=y CONFIG_HAVE_EXTERNAL_STAGE_CACHE=y CONFIG_EXTERNAL_STAGE_CACHE=y CONFIG_RELOCATABLE_MODULES=y CONFIG_NO_STAGE_CACHE=y
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#20).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 75 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/20
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#21).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 73 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/21
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
Patch Set 18: Code-Review-2
That variable "RELOCATABLE_RAMSTAGE" has some amount of conceptional significance over the stage caches. We have also used it as criteria for board removals in past discussion on the mailing list.
I am too much unhappy about the Kconfig just disappering like this, thus my -2 here now, until we have some more reviewer opinions. At least, can you keep the Kconfig as a placeholder and keep all the existing cases of CONFIG(RELOCATABLE_RAMSTAGE) unmodified?
Can you please have a look now?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
(1 comment)
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
Only i issue i saw here with AMD, Gardenia board, where HAVE_ACPI_TABLE is not selected hence NO_STAGE_CACHE is enable and in same time, mainboard also selects HAVE_EXTERNAL_STAGE_CACHE. As per this mainboard expectation, it might need to make use of stage_cache hence I can't make USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE
That sounds like a bug in the board?
yeah, it appears like that, as it expects stage_cache to get involved hence i'm getting un-definition error (USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE)
from AMD, gardenia .config
CONFIG_RELOCATABLE_RAMSTAGE=y CONFIG_HAVE_EXTERNAL_STAGE_CACHE=y CONFIG_EXTERNAL_STAGE_CACHE=y CONFIG_RELOCATABLE_MODULES=y CONFIG_NO_STAGE_CACHE=y
In my opinion, that SoC should have: diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index 78b89e3025..73869eced1 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -60,7 +60,7 @@ config CPU_SPECIFIC_OPTIONS select C_ENVIRONMENT_BOOTBLOCK select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH - select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM + select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if HAVE_ACPI_RESUME select PARALLEL_MP select PARALLEL_MP_AP_WORK select HAVE_SMI_HANDLER
What do you think? Kyosti/Marshall might be able to provide more insight.
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@265 PS21, Line 265: romstage Is that really true?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
(1 comment)
Patch Set 21:
(1 comment)
Patch Set 19:
Patch Set 19:
Patch Set 19:
Patch Set 19:
NO_STAGE_CACHE --> already exists
USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE (can be selected by a mainboard/SoC that provides implementation of external stage cache).
Only i issue i saw here with AMD, Gardenia board, where HAVE_ACPI_TABLE is not selected hence NO_STAGE_CACHE is enable and in same time, mainboard also selects HAVE_EXTERNAL_STAGE_CACHE. As per this mainboard expectation, it might need to make use of stage_cache hence I can't make USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE
That sounds like a bug in the board?
yeah, it appears like that, as it expects stage_cache to get involved hence i'm getting un-definition error (USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE)
from AMD, gardenia .config
CONFIG_RELOCATABLE_RAMSTAGE=y CONFIG_HAVE_EXTERNAL_STAGE_CACHE=y CONFIG_EXTERNAL_STAGE_CACHE=y CONFIG_RELOCATABLE_MODULES=y CONFIG_NO_STAGE_CACHE=y
In my opinion, that SoC should have: diff --git a/src/soc/amd/stoneyridge/Kconfig b/src/soc/amd/stoneyridge/Kconfig index 78b89e3025..73869eced1 100644 --- a/src/soc/amd/stoneyridge/Kconfig +++ b/src/soc/amd/stoneyridge/Kconfig @@ -60,7 +60,7 @@ config CPU_SPECIFIC_OPTIONS select C_ENVIRONMENT_BOOTBLOCK select BOOT_DEVICE_SUPPORTS_WRITES if BOOT_DEVICE_SPI_FLASH select BOOT_DEVICE_SPI_FLASH_RW_NOMMAP_EARLY if BOOT_DEVICE_SPI_FLASH
select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
select CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM if HAVE_ACPI_RESUME select PARALLEL_MP select PARALLEL_MP_AP_WORK select HAVE_SMI_HANDLER
What do you think? Kyosti/Marshall might be able to provide more insight.
yes, agree. but i don't have platform to verify if it can boot without issue. @Kyosti/Marshall can help sharing there feedback. @Furquan, do you want me to push a CL for that as well ? as baseline and then modify this CL top of that with USE_EXTERNAL_STAGE_CACHE --> depends on !NO_STAGE_CACHE ?
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@265 PS21, Line 265: romstage
Is that really true?
no, we have to exclude romstage
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21: -Code-Review
Any STAGE_CACHE=y is pointless with HAVE_ACPI_RESUME=n, not just in amd/gardenia.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
Patch Set 21: -Code-Review
Any STAGE_CACHE=y is pointless with HAVE_ACPI_RESUME=n, not just in amd/gardenia.
Agree, i will submit another CL then to fix amd/gardenia where we are seeing issue now
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
(1 comment)
A lot less to review, if you just first renamed CACHE_RELOCATABLE_RAMSTAGE_OUTSIDE_CBMEM and adjusted the Makefile.inc files. The real contents of this commit appears to be in that src/Kconfig file only.
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) { Since this implies it's carved from TSEG/SMM, maybe the name EXTERNAL is not that good.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
(1 comment)
Patch Set 21:
(1 comment)
A lot less to review, if you just first renamed CACHE_RELOCATABLE_RAMSTAGE_OUTSIDE_CBMEM and adjusted the Makefile.inc files. The real contents of this commit appears to be in that src/Kconfig file only.
yes, you are right, i have just rename CACHE_RELOCATABLE_RAMSTAGE_OUTSIDE_CBMEM to USE_EXTERNAL_STAGE_CACHE isn't it ? and when user doesn't select this CACHE_RELOCATABLE_RAMSTAGE_OUTSIDE_CBMEM config naturally there will be an alternative option like USE_INTERNAL_STAGE_CACHE. i guess the change density might be same if i "just" rename the kconfig alone in one CL and in next CL, i have add concept of USE_INTERNAL_STAGE_CACHE.
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
Since this implies it's carved from TSEG/SMM, maybe the name EXTERNAL is not that good.
i guess USE_EXTERNAL_STAGE_CACHE kconfig has detailed help to know where it might like to store the stage cache, internal into cbmem or external to cbmem which could be TSEG region.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@259 PS21, Line 259: config USE_EXTERNAL_STAGE_CACHE I would call this simply 'TSEG_STAGE_CACHE' if that reflects accurately what we currently have.
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@268 PS21, Line 268: config USE_CBMEM_STAGE_CACHE Just 'CBMEM_STAGE_CACHE'. At least I find the prefix 'USE_' just useless.
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
i guess USE_EXTERNAL_STAGE_CACHE kconfig has detailed help to know where it might like to store the […]
The conditional here needs to evaluate true with TSEG only. From what I remember, all external stage caches are currently TSEG.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 21:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
The conditional here needs to evaluate true with TSEG only. […]
yes, true. So are you suggesting to call it TSEG_STAGE_CACHE rather calling EXTERNAL_STAGE_CACHE ?
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#22).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 73 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/22
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 22:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@256 PS22, Line 256: is can be? USE is for doing it while this is indicating support.
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@270 PS22, Line 270: !NO_STAGE_CACHE Indicating no stage cache implies the use of a stage cache?
https://review.coreboot.org/c/coreboot/+/33116/22/src/cpu/intel/haswell/Make... File src/cpu/intel/haswell/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/22/src/cpu/intel/haswell/Make... PS22, Line 13: y Why aren't these 'y's using $(CONFIG_USE_EXTERNAL_STAGE_CACHE) instead of the guard?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@270 PS22, Line 270: !NO_STAGE_CACHE
Indicating no stage cache implies the use of a stage cache?
USE_EXTERNAL_STAGE_CACHE if HAVE_EXTERNAL_STAGE_CACHE and !NO_STAGE_CACHE USE_CBMEM_STAGE_CACHE if !NO_STAGE_CACHE and !USE_EXTERNAL_STAGE_CACHE
Indicating no stage cache implies the use of a stage cache, but stage cache can be between external and internal. User can choose external stage cache based on HAVE_EXTERNAL_STAGE_CACHE but if doesn't specify anything from mb then default USE_CBMEM_STAGE_CACHE
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@270 PS22, Line 270: !NO_STAGE_CACHE
USE_EXTERNAL_STAGE_CACHE if HAVE_EXTERNAL_STAGE_CACHE and !NO_STAGE_CACHE […]
I missed the double negative.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 22:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@270 PS22, Line 270: !NO_STAGE_CACHE
I missed the double negative.
Got it, Aaron if you have little time today, can you please help to look into this CL https://review.coreboot.org/c/coreboot/+/34368.
I'm seeing some basic issue with stage_cache implementation in some board. i have captured my observation there in comments. may be you can help to confirm
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#23).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 74 insertions(+), 45 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/23
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 23:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@256 PS22, Line 256: is
can be? USE is for doing it while this is indicating support.
Done
https://review.coreboot.org/c/coreboot/+/33116/22/src/cpu/intel/haswell/Make... File src/cpu/intel/haswell/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/22/src/cpu/intel/haswell/Make... PS22, Line 13: y
Why aren't these 'y's using $(CONFIG_USE_EXTERNAL_STAGE_CACHE) instead of the guard?
Done
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#24).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 62 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/24
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 24: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/24/src/include/stage_cache.h File src/include/stage_cache.h:
https://review.coreboot.org/c/coreboot/+/33116/24/src/include/stage_cache.h@... PS24, Line 35: CONFIG(USE_CBMEM_STAGE_CACHE) || CONFIG(USE_EXTERNAL_STAGE_CACHE) I believe once the dependencies are resolved, you can just do if !CONFIG(NO_STAGE_CACHE) here?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 24:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/24/src/include/stage_cache.h File src/include/stage_cache.h:
https://review.coreboot.org/c/coreboot/+/33116/24/src/include/stage_cache.h@... PS24, Line 35: CONFIG(USE_CBMEM_STAGE_CACHE) || CONFIG(USE_EXTERNAL_STAGE_CACHE)
I believe once the dependencies are resolved, you can just do if !CONFIG(NO_STAGE_CACHE) here?
yes, thats better
Kyösti Mälkki has uploaded a new patch set (#25) to the change originally created by Subrata Banik. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 62 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/25
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 25:
(10 comments)
https://review.coreboot.org/c/coreboot/+/33116/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33116/1//COMMIT_MSG@7 PS1, Line 7: Kconfig: Make CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM dependent on ARCH_RAMSTAGE_X86_32
why? That's not detailed here. […]
Ack
https://review.coreboot.org/c/coreboot/+/33116/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/33116/4//COMMIT_MSG@7 PS4, Line 7: Set default value
if we don't have ramstage enable then does it make sense to have a config (CACHE_RELOCATED_RAMSTAGE_ […]
Ack
https://review.coreboot.org/c/coreboot/+/33116/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/3/src/Kconfig@281 PS3, Line 281: config CACHE_RELOCATED_RAMSTAGE_OUTSIDE_CBMEM
i will add default here then
Ack
https://review.coreboot.org/c/coreboot/+/33116/3/src/Kconfig@1214 PS3, Line 1214: ARCH_X86
true, so far only x86 platform has selected this config
Ack
https://review.coreboot.org/c/coreboot/+/33116/7/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/7/src/Kconfig@252 PS7, Line 252: default n if !ENABLE_STAGE_RAMSTAGE
can you please look at newer patchset, i have put explicit dependency from soc select
Ack
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@259 PS21, Line 259: config USE_EXTERNAL_STAGE_CACHE
I would call this simply 'TSEG_STAGE_CACHE' if that reflects accurately what we currently have.
Ack
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@265 PS21, Line 265: romstage
no, we have to exclude romstage
Ack
https://review.coreboot.org/c/coreboot/+/33116/21/src/Kconfig@268 PS21, Line 268: config USE_CBMEM_STAGE_CACHE
Just 'CBMEM_STAGE_CACHE'. At least I find the prefix 'USE_' just useless.
Ack
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/22/src/Kconfig@270 PS22, Line 270: !NO_STAGE_CACHE
Got it, Aaron if you have little time today, can you please help to look into this CL https://review […]
Ack
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
yes, true. So are you suggesting to call it TSEG_STAGE_CACHE rather calling […]
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 25:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/24/src/include/stage_cache.h File src/include/stage_cache.h:
https://review.coreboot.org/c/coreboot/+/33116/24/src/include/stage_cache.h@... PS24, Line 35: CONFIG(USE_CBMEM_STAGE_CACHE) || CONFIG(USE_EXTERNAL_STAGE_CACHE)
yes, thats better
Done
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#26).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 62 insertions(+), 44 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/26
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
Ack
Subrata, IMHO this is not addressed yet, but wait for others to comment about the suggested Kconfig name change to make this part clean.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
Subrata, IMHO this is not addressed yet,
oh yes, sorry for marking Ack.
but wait for others to comment about the suggested Kconfig name change to make this part clean.
yes, better we wait for Furquan to take a further look on naming. But in my opinion internal and external stage cache goes very well (at least in naming side)
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 26: Code-Review+1
(2 comments)
Overall LGTM.
https://review.coreboot.org/c/coreboot/+/33116/26/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/26/src/Kconfig@265 PS26, Line 265: Platform : can relocate postcar(if enable), ramstage in an area specified by the : board and/or chipset. Why do external and cbmem stage cache have different text for what can be added to the stage cache?
https://review.coreboot.org/c/coreboot/+/33116/26/src/lib/Makefile.inc File src/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/26/src/lib/Makefile.inc@180 PS26, Line 180: y nit: If you change the y here and below with CONFIG_USE_EXTERNAL_STAGE_CACHE, then you won't need the ifeq...else...endif
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 26:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
The conditional here needs to evaluate true with TSEG only. From what I remember, all external stage caches are currently TSEG.
Kyosti -- do you think having TSEG_STAGE_CACHE would make it clear? Or are you implying there should be a check here or in Kconfigs for SMM_TSEG?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#27).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) USE_EXTERNAL_STAGE_CACHE (B) USE_CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 64 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/27
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33116/26/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/26/src/Kconfig@265 PS26, Line 265: Platform : can relocate postcar(if enable), ramstage in an area specified by the : board and/or chipset.
Why do external and cbmem stage cache have different text for what can be added to the stage cache?
Done
https://review.coreboot.org/c/coreboot/+/33116/26/src/lib/Makefile.inc File src/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/33116/26/src/lib/Makefile.inc@180 PS26, Line 180: y
nit: If you change the y here and below with CONFIG_USE_EXTERNAL_STAGE_CACHE, then you won't need th […]
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
The conditional here needs to evaluate true with TSEG only. […]
If the implementation places cache outside TSEG, it is wrong to carve out SMM_RESERVED_SIZE here based on USE_EXTERNAL_STAGE_CACHE. Code here would be fine if we evaluated CONFIG(TSEG_STAGE_CACHE) instead as it would imply the reservation needs to be made.
Also:
* I think CBMEM vs TSEG is better symmetry in naming than CBMEM vs "somewhere else" * We may want to deprecate CBMEM as stage cache as a security measure. From what I know CBMEM is mutable from OS (gaining root priviledges on /dev/mem might be enough on some cases?) * Some platforms have configurable TSEG size, we should assert at buildtime there is room for cache. I would be willing to always enable max TSEG regions for all platforms though.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
- I think CBMEM vs TSEG is better symmetry in naming than CBMEM vs "somewhere else"
SG.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
- We may want to deprecate CBMEM as stage cache as a security measure. From what I know CBMEM is mutable from OS (gaining root priviledges on /dev/mem might be enough on some cases?)
Um, what? I don't think all platforms have TSEG. That's a first. If you are concerned about security, I'd rather check the cache contents cryptogra- phically. TSEG seems much more like a best-effort solution, than secure. We'll never know if there is not a weird combination of mapping options somewhere in these chips that is exploitable.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33116/27/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/27/src/Kconfig@260 PS27, Line 260: config USE_EXTERNAL_STAGE_CACHE nit: I know we are not consistent about the naming when it comes to HAVE_/HAS_. I feel the USE_ prefix is just superfluous and the existing ones should be removed.
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
- We may want to deprecate CBMEM as stage cache as a security measure. […]
My first impression was only AGESA remains with CBMEM stage cache and there are other motivations to transform those to TSEG. Last time I tried, I did not get very far with attempts on VBOOT on AGESA/binaryPI platforms and I completely agree about the superiority of cryptographic checking.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
My first impression was only AGESA remains with CBMEM stage cache and there are other motivations to […]
just to summarize. Should i make it TSEG from external stage cache? but as Nico has said and i also have same understanding that all platform does have TSEG. is that meaningful to do right now ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
just to summarize. […]
I had a more thorough look. Looks like I would revert 75% of this change with my alternative approach so I have little reasons to rush this change at all. We'll see in a day or two what it looks like.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 27:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
I had a more thorough look. […]
you should feel free to pick your task, bt i don't see any reason to hold this CL for any further day if other reviews already give +1 many times
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#28).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) EXTERNAL_STAGE_CACHE (B) CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 64 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/28
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 28:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33116/27/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33116/27/src/Kconfig@260 PS27, Line 260: config USE_EXTERNAL_STAGE_CACHE
nit: I know we are not consistent about the naming when it comes to HAVE_/HAS_. […]
removed USE_ now
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Arthur Heymans, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, ron minnich, Huang Jin, Philipp Deppenwiese, Nico Huber, Damien Zammit, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33116
to look at the new patch set (#29).
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Kconfig: Make stage cache kconfig selection proper
This patch makes stage cache kconfig proper:
1. Create generic stage cache options as external or internal stage cache for any relocatable stage - Define kconfig options as below for platform to select (A) TSEG_STAGE_CACHE (B) CBMEM_STAGE_CACHE - Option (A) and (B) are mutually exclusive 2. Modify .inc and .c as applicable with those configs.
Change-Id: I45e894ad335a4661cc7916b3768e1614a038b31c Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/Kconfig M src/cpu/intel/haswell/Makefile.inc M src/cpu/intel/model_2065x/Kconfig M src/cpu/intel/model_206ax/Kconfig M src/cpu/intel/model_206ax/Makefile.inc M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/stage_cache.h M src/lib/Makefile.inc M src/northbridge/intel/gm45/Kconfig M src/northbridge/intel/gm45/Makefile.inc M src/northbridge/intel/haswell/Kconfig M src/northbridge/intel/i945/Kconfig M src/northbridge/intel/i945/Makefile.inc M src/northbridge/intel/pineview/Kconfig M src/northbridge/intel/pineview/Makefile.inc M src/northbridge/intel/x4x/Kconfig M src/northbridge/intel/x4x/Makefile.inc M src/soc/amd/picasso/Kconfig M src/soc/amd/stoneyridge/Kconfig M src/soc/intel/apollolake/Kconfig M src/soc/intel/braswell/Kconfig M src/soc/intel/broadwell/Kconfig M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig M src/soc/intel/skylake/Kconfig 25 files changed, 63 insertions(+), 49 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/16/33116/29
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 29:
(1 comment)
added cbmem and tseg stage cache to get feedback as discussed
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... File src/cpu/intel/smm/gen1/smmrelocate.c:
https://review.coreboot.org/c/coreboot/+/33116/21/src/cpu/intel/smm/gen1/smm... PS21, Line 124: if (CONFIG(USE_EXTERNAL_STAGE_CACHE)) {
you should feel free to pick your task, bt i don't see any reason to hold this CL for any further da […]
Ack
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Patch Set 29:
This: 25 files changed, 63 insertions(+), 49 deletions(-) CB:34664 19 files changed, 30 insertions(+), 31 deletions(-)
Off-topic, *shrug*, just my five cents
Often the commits going less back-and-forth, not introducing redundant Kconfig options and with better argumentation in general get picked for merge.
Sometimes reviewers (at least I) get tired of scrolling through 30 odd patchsets or 100+ comments in a seemingly simple commit that was supposed to be a simple rename. I try to defer myself from doing +2 reviews while frustrated on the process, just to avoid errors and the need for reverts. If I see little value in the past review commments, I am not worried about upsetting someone by starting it from scratch with a fresh Change-Id.
Most of the time, arguments like "But I need this merged now" and "We are in a hurry" will just backfire with bikeshedding. Even more so today with the enforced ACR.
Last, and I have stated this somewhere before, there should be a 3-form application to fill for adding any Kconfig variables. That's how I feel about attacking a non-problem in Kconfig by adding yet another new variable.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33116 )
Change subject: Kconfig: Make stage cache kconfig selection proper ......................................................................
Abandoned