Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35165 )
Change subject: arch/x86: Reserve some CAR memory for FSP single stack requirement ......................................................................
Patch Set 4:
(10 comments)
Patch Set 4:
Just curious, why would we want to make it explicit entry in car.ld when using .bss should achieve just the same?
Isn't that more meaningful now to know why we are reserving that block in car.ld?
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35165/4//COMMIT_MSG@27 PS4, Line 27: stack
heap
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld File src/arch/x86/car.ld:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@96 PS4, Line 96: CONFIG_C_ENVIRONMENT_BOOTBLOCK
Yeah, having this space should be independent.
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/car.ld@102 PS4, Line 102: CONFIG_DCACHE_BSP_HEAP_SIZE
Put 'FSP' in the name since it has only to do w/ FSP. Same for the symbols -- add 'fsp' in there.
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... File src/arch/x86/include/arch/symbols.h:
https://review.coreboot.org/c/coreboot/+/35165/4/src/arch/x86/include/arch/s... PS4, Line 64: _car_heap_start - _car_heap_end
Isn't this inverted?
my bad
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig File src/cpu/Kconfig:
https://review.coreboot.org/c/coreboot/+/35165/4/src/cpu/Kconfig@28 PS4, Line 28: depends on FSP_USES_CB_STACK
maybe FSP_DCACHE_HEAP_SIZE ?
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 164: setup_fsp_stack_frame
Doing this in setup_fsp_stack_frame() makes it confusing. […]
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 178: stack
heap?
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 188: FSP stack base
Agreed, it shouldn't be called out specifically, as it's no longer separate.
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 189: vboot
I understand the intent here to add vboot to provide a complete picture of the memory layout. […]
Done
https://review.coreboot.org/c/coreboot/+/35165/4/src/drivers/intel/fsp2_0/me... PS4, Line 196: arch_upd->StackSize = CONFIG_DCACHE_BSP_HEAP_SIZE;
Please use symbol macros for size.
Done