Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
soc/intel/{cnl, icl}: Allocate 64KB as FSP heap
TEST=Build and boot CML-Hatch
With this CL No "Smashed stack detected in romstage" msg in serial log.
Change-Id: Icc39cdb71f427189186222ae53b8881cbbd5e0bc Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/35237/1
diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig index d949fff..ac100e2 100644 --- a/src/soc/intel/cannonlake/Kconfig +++ b/src/soc/intel/cannonlake/Kconfig @@ -125,6 +125,13 @@ The amount of anticipated stack usage in CAR by bootblock and other stages.
+config DCACHE_BSP_HEAP_SIZE + hex + depends on FSP_USES_CB_STACK + default 0x10000 + help + The amount of anticipated heap usage in CAR by FSP. + config IFD_CHIPSET string default "cnl" diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index 1bd478c..78bd133 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -76,6 +76,13 @@ The amount of anticipated stack usage in CAR by bootblock and other stages.
+config DCACHE_BSP_HEAP_SIZE + hex + depends on FSP_USES_CB_STACK + default 0x10000 + help + The amount of anticipated heap usage in CAR by FSP. + config IFD_CHIPSET string default "icl"
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35237/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35237/1//COMMIT_MSG@7 PS1, Line 7: 64KB as FSP heap How was this size determined?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... PS1, Line 122: default 0x20000 if FSP_USES_CB_STACK Shouldn't this be reduced now?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... PS1, Line 122: default 0x20000 if FSP_USES_CB_STACK
Shouldn't this be reduced now?
If 64KB was used for the HOB heap, then shouldn't this be 0x10000 now? Or should we change this to 96KB (160KB - 64KB) ?
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... PS1, Line 131: 0x10000 64KB worth of HOBs?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35237/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35237/1//COMMIT_MSG@7 PS1, Line 7: 64KB as FSP heap
How was this size determined?
it will be captured in next FSP integration guide release. and FSP team will update the requirement from now onwards
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... PS1, Line 122: default 0x20000 if FSP_USES_CB_STACK
If 64KB was used for the HOB heap, then shouldn't this be 0x10000 now? Or should we change this to […]
No Aaron/Tim. Stack requirement remain same ~128KB and HOB heap requirement is ~64KB. earlier we were actually passing ~128KB to stack and heap both using same arch->upd = stack_size. Now with next integration guide things will be call out correctly.
https://review.coreboot.org/c/coreboot/+/35237/1/src/soc/intel/cannonlake/Kc... PS1, Line 131: 0x10000
64KB worth of HOBs?
integration guide will call that out, its combination of HOB and some other features. As i understood from CML FSP document will specify the same size
Hello Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Aamir Bohra, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35237
to look at the new patch set (#2).
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
soc/intel/{cnl, icl}: Allocate 64KB as FSP heap
For CML & ICL, FSP requires at least heap = 0x10000 and stack = 0x20000.
TEST=Build and boot CML-Hatch and ICL.
With this CL No "Smashed stack detected in romstage" msg in serial log.
Change-Id: Icc39cdb71f427189186222ae53b8881cbbd5e0bc Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/35237/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35237/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35237/1//COMMIT_MSG@7 PS1, Line 7: 64KB as FSP heap
it will be captured in next FSP integration guide release. […]
Done
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35237/2/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/2/src/soc/intel/cannonlake/Kc... PS2, Line 128: FSP_DCACHE_HEAP_SIZE DCACHE_FSP_HEAP_SIZE instead?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35237/2/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/2/src/soc/intel/cannonlake/Kc... PS2, Line 128: FSP_DCACHE_HEAP_SIZE
DCACHE_FSP_HEAP_SIZE instead?
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig#28 check this
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Abandoned
clubbed with 35165
Subrata Banik has restored this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Restored
Hello Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Aamir Bohra, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35237
to look at the new patch set (#4).
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
soc/intel/{cnl, icl}: Allocate 64KB as FSP heap
For CML & ICL, FSP requires at least heap = 0x10000 and stack = 0x20000.
BUG=b:140268415 TEST=Build and boot CML-Hatch and ICL.
Change-Id: Icc39cdb71f427189186222ae53b8881cbbd5e0bc Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 14 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/35237/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 4: Code-Review+1
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/cannonlake/Kc... PS4, Line 133: The amount of anticipated heap usage in CAR by FSP. Can we add details here about where the size requirement is coming from i.e. doc# if any? Or at least the integration guide name?
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/icelake/Kconf... PS4, Line 84: The amount of anticipated heap usage in CAR by Same here?
Hello Aaron Durbin, Patrick Rudolph, Nathaniel L Desimone, Aamir Bohra, Tim Wawrzynczak, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35237
to look at the new patch set (#5).
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
soc/intel/{cnl, icl}: Allocate 64KB as FSP heap
For CML & ICL, FSP requires at least heap = 0x10000 and stack = 0x20000. Refer to FSP integration guide to know the exact FSP requirement.
BUG=b:140268415 TEST=Build and boot CML-Hatch and ICL.
Change-Id: Icc39cdb71f427189186222ae53b8881cbbd5e0bc Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/cannonlake/Kconfig M src/soc/intel/icelake/Kconfig 2 files changed, 18 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/35237/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/cannonlake/Kc... PS4, Line 133: The amount of anticipated heap usage in CAR by FSP.
Can we add details here about where the size requirement is coming from i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/35237/4/src/soc/intel/icelake/Kconf... PS4, Line 84: The amount of anticipated heap usage in CAR by
Same here?
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 5: Code-Review+1
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 5: Code-Review+2
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Abandoned
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 5:
Why was this abandoned? Piggy backing off of default?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35237 )
Change subject: soc/intel/{cnl, icl}: Allocate 64KB as FSP heap ......................................................................
Patch Set 5:
Patch Set 5:
Why was this abandoned? Piggy backing off of default?
Furquan has mentioned to clubbed into CB:35301 to ensure no brokenness
-
Aamir Bohra (Code Review) -
Aaron Durbin (Code Review)
-
Furquan Shaikh (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)