Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32725
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/include/arch/acpi.h M src/console/post.c M src/drivers/uart/uart8250io.c M src/drivers/uart/uart8250mem.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/bootmode.c M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c 15 files changed, 27 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/1
diff --git a/src/arch/x86/include/arch/acpi.h b/src/arch/x86/include/arch/acpi.h index 79feaad..26fa5da 100644 --- a/src/arch/x86/include/arch/acpi.h +++ b/src/arch/x86/include/arch/acpi.h @@ -838,7 +838,7 @@ unsigned long (*acpi_fill_ivrs)(acpi_ivrs_t *ivrs_struct, unsigned long current));
-#if ENV_RAMSTAGE && !defined(__SIMPLE_DEVICE__) +#if ENV_PAYLOAD_LOADER && !defined(__SIMPLE_DEVICE__) void acpi_create_hpet(acpi_hpet_t *hpet); unsigned long acpi_write_hpet(struct device *device, unsigned long start, acpi_rsdp_t *rsdp); diff --git a/src/console/post.c b/src/console/post.c index 236aa8c..dd39aad 100644 --- a/src/console/post.c +++ b/src/console/post.c @@ -44,7 +44,7 @@
DECLARE_SPIN_LOCK(cmos_post_lock)
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER void cmos_post_log(void) { u8 code = 0; @@ -125,7 +125,7 @@ post_log_extra(0); } #endif /* CONFIG_CMOS_POST_EXTRA */ -#endif /* ENV_RAMSTAGE */ +#endif /* ENV_PAYLOAD_LOADER */
static void cmos_post_code(u8 value) { diff --git a/src/drivers/uart/uart8250io.c b/src/drivers/uart/uart8250io.c index 62671e2..85c01a4 100644 --- a/src/drivers/uart/uart8250io.c +++ b/src/drivers/uart/uart8250io.c @@ -124,7 +124,7 @@ uart8250_tx_flush(uart_platform_base(idx)); }
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER void uart_fill_lb(void *data) { struct lb_serial serial; diff --git a/src/drivers/uart/uart8250mem.c b/src/drivers/uart/uart8250mem.c index c3dff6a..bc9717e 100644 --- a/src/drivers/uart/uart8250mem.c +++ b/src/drivers/uart/uart8250mem.c @@ -147,7 +147,7 @@ uart8250_mem_tx_flush(base); }
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER void uart_fill_lb(void *data) { struct lb_serial serial; diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 273a80d..1e6207f 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -41,7 +41,8 @@
/* Default is that currently ramstage, smm, and rmodules have a heap. */ #ifndef ARCH_STAGE_HAS_HEAP_SECTION -#define ARCH_STAGE_HAS_HEAP_SECTION (ENV_RAMSTAGE || ENV_SMM || ENV_RMODULE) +#define ARCH_STAGE_HAS_HEAP_SECTION (ENV_PAYLOAD_LOADER || ENV_SMM || \ + ENV_RMODULE) #endif
#define STR(x) #x diff --git a/src/include/rules.h b/src/include/rules.h index ce968f0..801a7fe 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -272,7 +272,7 @@ * be built with simple device model. */
-#if (defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR) +#if (defined(__PRE_RAM__) || ENV_SMM || !ENV_PAYLOAD_LOADER) #define __SIMPLE_DEVICE__ #endif
diff --git a/src/include/stddef.h b/src/include/stddef.h index 993d5f0..cd29e31 100644 --- a/src/include/stddef.h +++ b/src/include/stddef.h @@ -21,9 +21,11 @@
#define NULL ((void *)0)
-/* The devicetree data structures are only mutable in ramstage. All other - stages have a constant devicetree. */ -#if !ENV_RAMSTAGE +/* + * The devicetree data structures are only mutable in ENV_PAYLOAD_LOADER + * All other stages have a constant devicetree. + */ +#if !ENV_PAYLOAD_LOADER #define DEVTREE_EARLY 1 #else #define DEVTREE_EARLY 0 diff --git a/src/lib/bootmode.c b/src/lib/bootmode.c index 18f6d5d..93f317d 100644 --- a/src/lib/bootmode.c +++ b/src/lib/bootmode.c @@ -17,7 +17,7 @@ #include <bootmode.h> #include <vendorcode/google/chromeos/chromeos.h>
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER static int gfx_init_done = -1;
int gfx_get_init_done(void) diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 1a67ad5..b1b63f6 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -307,7 +307,7 @@ imd_region_used(cbmem_get_imd(), baseptr, size); }
-#if ENV_RAMSTAGE || (CONFIG(EARLY_CBMEM_LIST) \ +#if ENV_PAYLOAD_LOADER || (CONFIG(EARLY_CBMEM_LIST) \ && (ENV_POSTCAR || ENV_ROMSTAGE)) /* * -fdata-sections doesn't work so well on read only strings. They all diff --git a/src/lib/memrange.c b/src/lib/memrange.c index 96d7524..79a1b0e 100644 --- a/src/lib/memrange.c +++ b/src/lib/memrange.c @@ -47,7 +47,7 @@ range_entry_unlink(&ranges->free_list, r); return r; } - if (ENV_RAMSTAGE) + if (ENV_PAYLOAD_LOADER) return malloc(sizeof(struct range_entry)); return NULL; } diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 3b77712..b73a61a 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -165,7 +165,7 @@ die("Ramstage was not loaded!\n"); }
-#ifdef __RAMSTAGE__ // gc-sections should take care of this +#if ENV_PAYLOAD_LOADER // gc-sections should take care of this
static struct prog global_payload = PROG_INIT(PROG_PAYLOAD, CONFIG_CBFS_PREFIX "/payload"); diff --git a/src/lib/program.ld b/src/lib/program.ld index 851aa75..66421d1 100644 --- a/src/lib/program.ld +++ b/src/lib/program.ld @@ -55,7 +55,7 @@ KEEP(*(.rsbe_init)); _ersbe_init_begin = .;
-#if ENV_RAMSTAGE +#if ENV_PAYLOAD_LOADER . = ALIGN(ARCH_POINTER_ALIGN_SIZE); _pci_drivers = .; KEEP(*(.rodata.pci_driver)); diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index b6330fa..cf882b8 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -48,7 +48,7 @@ #endif
/* The cache location will sit in BSS when in ramstage. */ -#define TIMESTAMP_CACHE_IN_BSS ENV_RAMSTAGE +#define TIMESTAMP_CACHE_IN_BSS ENV_PAYLOAD_LOADER
#define HAS_CBMEM (ENV_ROMSTAGE || ENV_RAMSTAGE || ENV_POSTCAR)
@@ -113,8 +113,11 @@ * based x86 platforms. */ static int timestamp_should_run(void) { - /* Only check boot_cpu() in other stages than ramstage on x86. */ - if ((!ENV_RAMSTAGE && CONFIG(ARCH_X86)) && !boot_cpu()) + /* + * Only check boot_cpu() in other stages than + * ENV_PAYLOAD_LOADER on x86. + */ + if ((!ENV_PAYLOAD_LOADER && CONFIG(ARCH_X86)) && !boot_cpu()) return 0;
return 1; @@ -300,7 +303,7 @@ ts_cbmem_table->base_time = ts_cache_table->base_time;
/* Seed the timestamp tick frequency in ramstage. */ - if (ENV_RAMSTAGE) + if (ENV_PAYLOAD_LOADER) ts_cbmem_table->tick_freq_mhz = timestamp_tick_freq_mhz();
/* Cache no longer required. */ diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index 455b13c..0628aae 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -250,7 +250,7 @@ bios_size = ALIGN_UP(bios_size, alignment); base = 4ULL*GiB - bios_size;
- if (ENV_RAMSTAGE) { + if (ENV_PAYLOAD_LOADER) { mtrr_use_temp_range(base, bios_size, type); } else { int mtrr = get_free_var_mtrr(); diff --git a/src/soc/intel/common/block/i2c/i2c.c b/src/soc/intel/common/block/i2c/i2c.c index a99dfea..d551c51 100644 --- a/src/soc/intel/common/block/i2c/i2c.c +++ b/src/soc/intel/common/block/i2c/i2c.c @@ -47,7 +47,7 @@ return EARLY_I2C_BASE(bus); }
-#if !ENV_RAMSTAGE +#if !ENV_PAYLOAD_LOADER static int lpss_i2c_early_init_bus(unsigned int bus) { const struct dw_i2c_bus_config *config;
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 1: Code-Review+1
it seems reasonable to me but we definitely need a few more eyes on it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2:
it seems reasonable to me but we definitely need a few more eyes on it.
I have added reviewer as well. i will wait for others review ?
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2: Code-Review+2
I did another pass on it. It looks fine.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2:
(9 comments)
Is there no need to update these:
https://review.coreboot.org/cgit/coreboot.git/tree/src/arch/x86/cbmem.c?id=r...
https://review.coreboot.org/cgit/coreboot.git/tree/src/drivers/elog/elog.c?i...
https://review.coreboot.org/#/c/32725/2/src/arch/x86/include/arch/acpi.h File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/#/c/32725/2/src/arch/x86/include/arch/acpi.h@841 PS2, Line 841: #if ENV_PAYLOAD_LOADER && !defined(__SIMPLE_DEVICE__) I don't think there is a need to guard the declarations here.
https://review.coreboot.org/#/c/32725/2/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/32725/2/src/include/rules.h@265 PS2, Line 265: post-CAR always build with simple device model This should be updated to reflect the change being done.
https://review.coreboot.org/#/c/32725/2/src/include/rules.h@267 PS2, Line 267: Currently there's : * no known requirement that devicetree would be needed during that stage. This is not accurate anymore.
https://review.coreboot.org/#/c/32725/2/src/include/rules.h@275 PS2, Line 275: defined(__PRE_RAM__) || ENV_SMM Aren't checks for __PRE_RAM__ and ENV_SMM redundant now since !ENV_PAYLOAD_LOADER would always be true for those stages?
https://review.coreboot.org/#/c/32725/2/src/lib/bootmode.c File src/lib/bootmode.c:
https://review.coreboot.org/#/c/32725/2/src/lib/bootmode.c@20 PS2, Line 20: #if ENV_PAYLOAD_LOADER Same here. It should be possible to avoid this and just let the linker get rid of it for stages that don't use it.
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@50 PS2, Line 50: ramstage Need to update the comment.
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@55 PS2, Line 55: ramstage this too.
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@226 PS2, Line 226: ENV_RAMSTAGE Shouldn't this be updated as well?
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@305 PS2, Line 305: ramstage Update comment too.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2: -Code-Review
(1 comment)
Furquan, thanks for that review, it was great.
https://review.coreboot.org/#/c/32725/2/src/arch/x86/include/arch/acpi.h File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/#/c/32725/2/src/arch/x86/include/arch/acpi.h@841 PS2, Line 841: #if ENV_PAYLOAD_LOADER && !defined(__SIMPLE_DEVICE__)
I don't think there is a need to guard the declarations here.
I agree. I've gotten confused as to coreboot practice around protos. There should be no need to guard protos but the guards are lots of places. One advantage of such guards is that you will get an error at compile time -- the protos are not defined.Perhaps it is considered less confusing if you get the error at compile time and not link time?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32725/2/src/arch/x86/include/arch/acpi.h File src/arch/x86/include/arch/acpi.h:
https://review.coreboot.org/#/c/32725/2/src/arch/x86/include/arch/acpi.h@841 PS2, Line 841: #if ENV_PAYLOAD_LOADER && !defined(__SIMPLE_DEVICE__)
I agree. I've gotten confused as to coreboot practice around protos. […]
can you please review this CL: https://review.coreboot.org/#/c/coreboot/+/32764/
i have removed those unnecessary ENV_RAMSTAGE guards
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32725/2/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/32725/2/src/include/rules.h@275 PS2, Line 275: defined(__PRE_RAM__) || ENV_SMM
Aren't checks for __PRE_RAM__ and ENV_SMM redundant now since !ENV_PAYLOAD_LOADER would always be tr […]
can you please review this CL: https://review.coreboot.org/#/c/coreboot/+/32765/
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#3).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/cbmem.c M src/arch/x86/exception.c M src/arch/x86/include/arch/exception.h M src/console/console.c M src/console/init.c M src/cpu/intel/microcode/microcode.c M src/cpu/x86/pae/pgtbl.c M src/drivers/elog/elog.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/drivers/net/ne2k.c M src/drivers/usb/ehci_debug.c M src/drivers/usb/pci_ehci.c M src/include/memlayout.h M src/include/stddef.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/program.ld M src/lib/timestamp.c M src/mainboard/google/dragonegg/chromeos.c M src/mainboard/google/eve/chromeos.c M src/mainboard/google/fizz/chromeos.c M src/mainboard/google/glados/chromeos.c M src/mainboard/google/poppy/chromeos.c M src/mainboard/intel/cannonlake_rvp/chromeos.c M src/mainboard/intel/coffeelake_rvp/chromeos.c M src/mainboard/intel/icelake_rvp/chromeos.c M src/mainboard/intel/kblrvp/chromeos.c M src/mainboard/intel/kunimitsu/chromeos.c M src/soc/intel/cannonlake/lpc.c M src/soc/intel/common/block/cse/cse.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c M src/soc/intel/common/block/uart/uart.c M src/soc/intel/icelake/lpc.c M src/soc/intel/skylake/pmc.c 37 files changed, 71 insertions(+), 62 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 2:
(7 comments)
https://review.coreboot.org/#/c/32725/2/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/32725/2/src/include/rules.h@265 PS2, Line 265: post-CAR always build with simple device model
This should be updated to reflect the change being done.
Done
https://review.coreboot.org/#/c/32725/2/src/include/rules.h@267 PS2, Line 267: Currently there's : * no known requirement that devicetree would be needed during that stage.
This is not accurate anymore.
Done
https://review.coreboot.org/#/c/32725/2/src/lib/bootmode.c File src/lib/bootmode.c:
https://review.coreboot.org/#/c/32725/2/src/lib/bootmode.c@20 PS2, Line 20: #if ENV_PAYLOAD_LOADER
Same here. […]
Done
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c File src/lib/timestamp.c:
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@50 PS2, Line 50: ramstage
Need to update the comment.
Done
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@55 PS2, Line 55: ramstage
this too.
Done
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@226 PS2, Line 226: ENV_RAMSTAGE
Shouldn't this be updated as well?
Done
https://review.coreboot.org/#/c/32725/2/src/lib/timestamp.c@305 PS2, Line 305: ramstage
Update comment too.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 4:
(7 comments)
It looks like you have replaced all occurrences of ENV_RAMSTAGE with ENV_PAYLOAD_LOADER. But, is the intent to really enable all the different blocks for postcar when it is the payload loader? e.g. pci drivers? microcode loader? others? How is the ramstage different than postcar in that case?
https://review.coreboot.org/#/c/32725/4/src/arch/x86/exception.c File src/arch/x86/exception.c:
PS4: Is there a plan to enable this for postcar? Sorry, I am not very clear at this point what all blocks postcar would include from ramstage.
https://review.coreboot.org/#/c/32725/4/src/arch/x86/exception.c@597 PS4, Line 597: /* This global is for src/cpu/x86/lapic/secondary.S usage which is only : used during ramstage. */ Same question as the file comment. Do you plan to do this for postcar?
https://review.coreboot.org/#/c/32725/4/src/console/console.c File src/console/console.c:
https://review.coreboot.org/#/c/32725/4/src/console/console.c@82 PS4, Line 82: GDB_STUB Do you plan to enable GDB stub for postcar too?
https://review.coreboot.org/#/c/32725/4/src/cpu/intel/microcode/microcode.c File src/cpu/intel/microcode/microcode.c:
PS4: Is there a plan to enable this for postcar?
https://review.coreboot.org/#/c/32725/4/src/drivers/net/ne2k.c File src/drivers/net/ne2k.c:
https://review.coreboot.org/#/c/32725/4/src/drivers/net/ne2k.c@323 PS4, Line 323: device_operations I thought you were planning to skip pci resource allocation for postcar? Is that not correct?
https://review.coreboot.org/#/c/32725/4/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/#/c/32725/4/src/lib/program.ld@58 PS4, Line 58: ENV_PAYLOAD_LOADER All the pci drivers are included in postcar?
https://review.coreboot.org/#/c/32725/4/src/mainboard/google/fizz/chromeos.c File src/mainboard/google/fizz/chromeos.c:
PS4: Do we really need to change all mainboards even if they won't enable loading from postcar?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 4:
(7 comments)
It looks like you have replaced all occurrences of ENV_RAMSTAGE with ENV_PAYLOAD_LOADER.
[Subrata] Move majority of stuffs because in future in anyone wish to call those functionally as well for an example GDB. but to enable CONFIG_RAMPAYLOAD we will have dedicated CL to just include required files in postcar stage.
But, is the intent to really enable all the different blocks for postcar when it is the payload loader? e.g. pci drivers? microcode loader? others? How is the ramstage different than postcar in that case?
[Subrata] No, we won't include those in Makefile.inc while selecting CONFIG_RAMPAYLOAD sample CL: https://review.coreboot.org/#/c/coreboot/+/32675/
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 4:
(7 comments)
It looks like you have replaced all occurrences of ENV_RAMSTAGE with ENV_PAYLOAD_LOADER.
[Subrata] Move majority of stuffs because in future in anyone wish to call those functionally as well for an example GDB. but to enable CONFIG_RAMPAYLOAD we will have dedicated CL to just include required files in postcar stage.
But, is the intent to really enable all the different blocks for postcar when it is the payload loader? e.g. pci drivers? microcode loader? others? How is the ramstage different than postcar in that case?
[Subrata] No, we won't include those in Makefile.inc while selecting CONFIG_RAMPAYLOAD sample CL: https://review.coreboot.org/#/c/coreboot/+/32675/
let me only keep files intended for postcar for now, we can increase list later of required. patchset 3 has those file list
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#5).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/arch/x86/cbmem.c M src/console/init.c M src/drivers/elog/elog.c M src/drivers/intel/fsp2_0/include/fsp/info_header.h M src/include/memlayout.h M src/include/stddef.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 13 files changed, 32 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 4:
(7 comments)
https://review.coreboot.org/#/c/32725/4/src/arch/x86/exception.c File src/arch/x86/exception.c:
PS4:
Is there a plan to enable this for postcar? Sorry, I am not very clear at this point what all blocks […]
removed for now, we will see later if required
https://review.coreboot.org/#/c/32725/4/src/arch/x86/exception.c@597 PS4, Line 597: /* This global is for src/cpu/x86/lapic/secondary.S usage which is only : used during ramstage. */
Same question as the file comment. […]
-same
https://review.coreboot.org/#/c/32725/4/src/console/console.c File src/console/console.c:
https://review.coreboot.org/#/c/32725/4/src/console/console.c@82 PS4, Line 82: GDB_STUB
Do you plan to enable GDB stub for postcar too?
-same
https://review.coreboot.org/#/c/32725/4/src/cpu/intel/microcode/microcode.c File src/cpu/intel/microcode/microcode.c:
PS4:
Is there a plan to enable this for postcar?
-same
https://review.coreboot.org/#/c/32725/4/src/drivers/net/ne2k.c File src/drivers/net/ne2k.c:
https://review.coreboot.org/#/c/32725/4/src/drivers/net/ne2k.c@323 PS4, Line 323: device_operations
I thought you were planning to skip pci resource allocation for postcar? Is that not correct?
yes you are right
https://review.coreboot.org/#/c/32725/4/src/lib/program.ld File src/lib/program.ld:
https://review.coreboot.org/#/c/32725/4/src/lib/program.ld@58 PS4, Line 58: ENV_PAYLOAD_LOADER
All the pci drivers are included in postcar?
Done
https://review.coreboot.org/#/c/32725/4/src/mainboard/google/fizz/chromeos.c File src/mainboard/google/fizz/chromeos.c:
PS4:
Do we really need to change all mainboards even if they won't enable loading from postcar?
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 6:
(4 comments)
Ok.. I aborted the review midway because I did not exactly understand what this is about.
Nevertheless, I think it's too controversial to merge this before 4.10 is out if you plan to introduce a completely new stage to replace ramstage in your builds.
https://review.coreboot.org/#/c/32725/6/src/arch/x86/cbmem.c File src/arch/x86/cbmem.c:
https://review.coreboot.org/#/c/32725/6/src/arch/x86/cbmem.c@25 PS6, Line 25: if (ENV_PAYLOAD_LOADER && cbmem_top_backup != NULL) ENV_RAMSTAGE was used here because otherwise this creates BSS section for romstage (not allowed for platforms with CAR migration).
https://review.coreboot.org/#/c/32725/6/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/32725/6/src/console/init.c@89 PS6, Line 89: if (CONFIG(EARLY_PCI_BRIDGE) && !ENV_PAYLOAD_LOADER) Nothing to do with payload loader. We don't want SMM console init to mess up PCI bridge configurations. We assume ramstage does not have to redo it (it's called early).
https://review.coreboot.org/#/c/32725/6/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/#/c/32725/6/src/drivers/elog/elog.c@863 PS6, Line 863: ramstage_elog_add_boot_count(); Not sure about this, at least the called function name conflicts with the condition you use now, you probably want some elog entry added though.
https://review.coreboot.org/#/c/32725/6/src/drivers/intel/fsp2_0/include/fsp... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/#/c/32725/6/src/drivers/intel/fsp2_0/include/fsp... PS6, Line 47: #if ENV_PAYLOAD_LOADER Why the guard in the first place?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/#/c/32725/6/src/arch/x86/cbmem.c File src/arch/x86/cbmem.c:
https://review.coreboot.org/#/c/32725/6/src/arch/x86/cbmem.c@25 PS6, Line 25: if (ENV_PAYLOAD_LOADER && cbmem_top_backup != NULL)
ENV_RAMSTAGE was used here because otherwise this creates BSS section for romstage (not allowed for […]
reverted
https://review.coreboot.org/#/c/32725/6/src/console/init.c File src/console/init.c:
https://review.coreboot.org/#/c/32725/6/src/console/init.c@89 PS6, Line 89: if (CONFIG(EARLY_PCI_BRIDGE) && !ENV_PAYLOAD_LOADER)
Nothing to do with payload loader. […]
reverted
https://review.coreboot.org/#/c/32725/6/src/drivers/elog/elog.c File src/drivers/elog/elog.c:
https://review.coreboot.org/#/c/32725/6/src/drivers/elog/elog.c@863 PS6, Line 863: ramstage_elog_add_boot_count();
Not sure about this, at least the called function name conflicts with the condition you use now, you […]
i have removed ramstage_ prefix
https://review.coreboot.org/#/c/32725/6/src/drivers/intel/fsp2_0/include/fsp... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/#/c/32725/6/src/drivers/intel/fsp2_0/include/fsp... PS6, Line 47: #if ENV_PAYLOAD_LOADER
Why the guard in the first place?
removed in another CL
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#7).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/drivers/elog/elog.c M src/include/memlayout.h M src/include/stddef.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 10 files changed, 28 insertions(+), 19 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/7
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
(2 comments)
reviewers: please be aware that this change should only relate to correctly distinguishing those areas relating to being a payload loader vs. those areas related to being the ramstage.
that's the goal anyway. This has some utility outside the idea of rampayloads, namely: what activities relate to being a payloader vs. being a ramstage?
https://review.coreboot.org/#/c/32725/7/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/32725/7/src/include/memlayout.h@42 PS7, Line 42: /* Default is that currently ramstage, smm, and rmodules have a heap. */ fix this comment.
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h@25 PS7, Line 25: * The devicetree data structures are only mutable in ENV_PAYLOAD_LOADER. In the case of RAMPAYLOAD, where we run in (e.g.) postcar, can we use a constant device tree? Given that the goal is a simple boot environment, need it be mutable?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h@25 PS7, Line 25: * The devicetree data structures are only mutable in ENV_PAYLOAD_LOADER.
In the case of RAMPAYLOAD, where we run in (e.g. […]
got it, so you suggest to keep this file untouched as we might not use mutable devicetree structure in rampayload ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
Patch Set 7:
(2 comments)
reviewers: please be aware that this change should only relate to correctly distinguishing those areas relating to being a payload loader vs. those areas related to being the ramstage.
that's the goal anyway. This has some utility outside the idea of rampayloads, namely: what activities relate to being a payloader vs. being a ramstage?
I wonder how much of this coreboot-lite could be accomplished by just providing a different boot state flow in ramstage? AFAICS, pretty much everything about resource allocator and tables creations is pulled in from execution passing through particular states.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
Patch Set 7:
(2 comments)
reviewers: please be aware that this change should only relate to
correctly distinguishing those areas relating to being a payload loader vs. those areas related to being the ramstage.
that's the goal anyway. This has some utility outside the idea of
rampayloads, namely: what activities relate to being a payloader vs. being a ramstage?
I wonder how much of this coreboot-lite could be accomplished by just providing a different boot state flow in ramstage? AFAICS, pretty much everything about resource allocator and tables creations is pulled in from execution passing through particular states.
May be you can take a look at POC CL: https://review.coreboot.org/#/c/coreboot/+/30985/
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
May be you can take a look at POC CL: https://review.coreboot.org/#/c/coreboot/+/30985/
Bad link?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
May be you can take a look at POC CL: https://review.coreboot.org/#/c/coreboot/+/30985/
Bad link?
its not BAD link :(
are you referring at verified -1 ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
Patch Set 7:
May be you can take a look at POC CL: https://review.coreboot.org/#/c/coreboot/+/30985/
Bad link?
its not BAD link :(
are you referring at verified -1 ?
Gerrit throws 404 not found. What's the ChangeId ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
Patch Set 7:
May be you can take a look at POC CL: https://review.coreboot.org/#/c/coreboot/+/30985/
Bad link?
its not BAD link :(
are you referring at verified -1 ?
Gerrit throws 404 not found. What's the ChangeId ?
Somehow it was marked private, removed private now
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/#/c/32725/7/src/include/memlayout.h File src/include/memlayout.h:
https://review.coreboot.org/#/c/32725/7/src/include/memlayout.h@42 PS7, Line 42: /* Default is that currently ramstage, smm, and rmodules have a heap. */
fix this comment.
Done
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h@25 PS7, Line 25: * The devicetree data structures are only mutable in ENV_PAYLOAD_LOADER.
In the case of RAMPAYLOAD, where we run in (e.g. […]
Done
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#8).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/drivers/elog/elog.c M src/include/memlayout.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 9 files changed, 27 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/8
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#9).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/drivers/elog/elog.c M src/include/memlayout.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 9 files changed, 20 insertions(+), 13 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/9
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#10).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/drivers/elog/elog.c M src/include/memlayout.h M src/include/stddef.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 10 files changed, 21 insertions(+), 14 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/10
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h File src/include/stddef.h:
https://review.coreboot.org/#/c/32725/7/src/include/stddef.h@25 PS7, Line 25: * The devicetree data structures are only mutable in ENV_PAYLOAD_LOADER.
Done
looks like const devicetree won't work in this model because acpi table creation function pretty much designed for !const devicetree structure. if we wish to make use of const devicetree in acpi table creation then the amount of code changes will be very high.
Hello Aaron Durbin, Patrick Rudolph, ron minnich, Julius Werner, build bot (Jenkins), Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32725
to look at the new patch set (#11).
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/drivers/elog/elog.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 11 files changed, 22 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/25/32725/11
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 11: Code-Review+1
So this is looking good to me, I will +1 for now and absent objections from more qualified reviewers I'll +1 later. I hope some of you get a chance to look. Thanks again.
ron minnich has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 11: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER
This patch relying on new rule, ENV_PAYLOAD_LOADER which is set to ENV_RAMSTAGE.
This approach will help to add future optimization (rampayload) in coreboot flow if required.
Change-Id: Ib54ece7b9e5f281f8a092dc6f38c07406edfa5fa Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32725 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: ron minnich rminnich@gmail.com --- M src/drivers/elog/elog.c M src/include/memlayout.h M src/include/rules.h M src/include/stddef.h M src/lib/imd_cbmem.c M src/lib/memrange.c M src/lib/prog_loaders.c M src/lib/timestamp.c M src/soc/intel/common/block/fast_spi/fast_spi.c M src/soc/intel/common/block/i2c/i2c.c M src/soc/intel/common/block/lpc/lpc_lib.c 11 files changed, 22 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified ron minnich: Looks good to me, approved
diff --git a/src/drivers/elog/elog.c b/src/drivers/elog/elog.c index e41b55b..9ac2d3c 100644 --- a/src/drivers/elog/elog.c +++ b/src/drivers/elog/elog.c @@ -859,7 +859,7 @@ " shrink size %d\n", region_device_sz(&es->nv_dev), es->full_threshold, es->shrink_size);
- if (ENV_RAMSTAGE) + if (ENV_PAYLOAD_LOADER) elog_add_boot_count(); return 0; } diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 273a80d..1ed87b6 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -39,9 +39,13 @@ #define ARCH_STAGE_HAS_BSS_SECTION 1 #endif
-/* Default is that currently ramstage, smm, and rmodules have a heap. */ +/* + * Default is that currently ENV_PAYLOAD_LOADER enable stage, smm, + * and rmodules have a heap. + */ #ifndef ARCH_STAGE_HAS_HEAP_SECTION -#define ARCH_STAGE_HAS_HEAP_SECTION (ENV_RAMSTAGE || ENV_SMM || ENV_RMODULE) +#define ARCH_STAGE_HAS_HEAP_SECTION (ENV_PAYLOAD_LOADER || ENV_SMM || \ + ENV_RMODULE) #endif
#define STR(x) #x diff --git a/src/include/rules.h b/src/include/rules.h index fcb827d..d8f6e74 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -277,7 +277,7 @@ * be built with simple device model. */
-#if (defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR) +#if (defined(__PRE_RAM__) || ENV_SMM || !ENV_PAYLOAD_LOADER) #define __SIMPLE_DEVICE__ #endif
diff --git a/src/include/stddef.h b/src/include/stddef.h index 993d5f0..7cae2e6 100644 --- a/src/include/stddef.h +++ b/src/include/stddef.h @@ -23,7 +23,7 @@
/* The devicetree data structures are only mutable in ramstage. All other stages have a constant devicetree. */ -#if !ENV_RAMSTAGE +#if !ENV_PAYLOAD_LOADER #define DEVTREE_EARLY 1 #else #define DEVTREE_EARLY 0 diff --git a/src/lib/imd_cbmem.c b/src/lib/imd_cbmem.c index 1a67ad5..b1b63f6 100644 --- a/src/lib/imd_cbmem.c +++ b/src/lib/imd_cbmem.c @@ -307,7 +307,7 @@ imd_region_used(cbmem_get_imd(), baseptr, size); }
-#if ENV_RAMSTAGE || (CONFIG(EARLY_CBMEM_LIST) \ +#if ENV_PAYLOAD_LOADER || (CONFIG(EARLY_CBMEM_LIST) \ && (ENV_POSTCAR || ENV_ROMSTAGE)) /* * -fdata-sections doesn't work so well on read only strings. They all diff --git a/src/lib/memrange.c b/src/lib/memrange.c index 96d7524..79a1b0e 100644 --- a/src/lib/memrange.c +++ b/src/lib/memrange.c @@ -47,7 +47,7 @@ range_entry_unlink(&ranges->free_list, r); return r; } - if (ENV_RAMSTAGE) + if (ENV_PAYLOAD_LOADER) return malloc(sizeof(struct range_entry)); return NULL; } diff --git a/src/lib/prog_loaders.c b/src/lib/prog_loaders.c index 81ec2ec..dfabd31 100644 --- a/src/lib/prog_loaders.c +++ b/src/lib/prog_loaders.c @@ -160,7 +160,7 @@ die_with_post_code(POST_INVALID_ROM, "Ramstage was not loaded!\n"); }
-#ifdef __RAMSTAGE__ // gc-sections should take care of this +#if ENV_PAYLOAD_LOADER // gc-sections should take care of this
static struct prog global_payload = PROG_INIT(PROG_PAYLOAD, CONFIG_CBFS_PREFIX "/payload"); diff --git a/src/lib/timestamp.c b/src/lib/timestamp.c index 38d0212..adacf6b 100644 --- a/src/lib/timestamp.c +++ b/src/lib/timestamp.c @@ -116,8 +116,11 @@ * based x86 platforms. */ static int timestamp_should_run(void) { - /* Only check boot_cpu() in other stages than ramstage on x86. */ - if ((!ENV_RAMSTAGE && CONFIG(ARCH_X86)) && !boot_cpu()) + /* + * Only check boot_cpu() in other stages than + * ENV_PAYLOAD_LOADER on x86. + */ + if ((!ENV_PAYLOAD_LOADER && CONFIG(ARCH_X86)) && !boot_cpu()) return 0;
return 1; @@ -302,8 +305,8 @@ if (ts_cbmem_table->base_time == 0) ts_cbmem_table->base_time = ts_cache_table->base_time;
- /* Seed the timestamp tick frequency in ramstage. */ - if (ENV_RAMSTAGE) + /* Seed the timestamp tick frequency in ENV_PAYLOAD_LOADER. */ + if (ENV_PAYLOAD_LOADER) ts_cbmem_table->tick_freq_mhz = timestamp_tick_freq_mhz();
/* Cache no longer required. */ diff --git a/src/soc/intel/common/block/fast_spi/fast_spi.c b/src/soc/intel/common/block/fast_spi/fast_spi.c index 58e7db7..e40b844 100644 --- a/src/soc/intel/common/block/fast_spi/fast_spi.c +++ b/src/soc/intel/common/block/fast_spi/fast_spi.c @@ -249,7 +249,7 @@ bios_size = ALIGN_UP(bios_size, alignment); base = 4ULL*GiB - bios_size;
- if (ENV_RAMSTAGE) { + if (ENV_PAYLOAD_LOADER) { mtrr_use_temp_range(base, bios_size, type); } else { int mtrr = get_free_var_mtrr(); diff --git a/src/soc/intel/common/block/i2c/i2c.c b/src/soc/intel/common/block/i2c/i2c.c index a99dfea..d551c51 100644 --- a/src/soc/intel/common/block/i2c/i2c.c +++ b/src/soc/intel/common/block/i2c/i2c.c @@ -47,7 +47,7 @@ return EARLY_I2C_BASE(bus); }
-#if !ENV_RAMSTAGE +#if !ENV_PAYLOAD_LOADER static int lpss_i2c_early_init_bus(unsigned int bus) { const struct dw_i2c_bus_config *config; diff --git a/src/soc/intel/common/block/lpc/lpc_lib.c b/src/soc/intel/common/block/lpc/lpc_lib.c index c67c435..b31c799 100644 --- a/src/soc/intel/common/block/lpc/lpc_lib.c +++ b/src/soc/intel/common/block/lpc/lpc_lib.c @@ -289,7 +289,7 @@ soc_get_gen_io_dec_range(dev, gen_io_dec); lpc_set_gen_decode_range(gen_io_dec); soc_setup_dmi_pcr_io_dec(gen_io_dec); - if (ENV_RAMSTAGE) + if (ENV_PAYLOAD_LOADER) pch_lpc_interrupt_init(); }
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 12:
So, what's the plan? Will there eventually be a board with 'select HAVE_RAMPAYLOAD' that enables some of this. Some parts of this work is good, previous stuff in Kconfig adding "RAMPAYLOAD and HAVE_RAMPAYLOAD" maybe not so much.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32725 )
Change subject: Replace ENV_RAMSTAGE with ENV_PAYLOAD_LOADER ......................................................................
Patch Set 12:
Patch Set 12:
So, what's the plan? Will there eventually be a board with 'select HAVE_RAMPAYLOAD' that enables some of this. Some parts of this work is good, previous stuff in Kconfig adding "RAMPAYLOAD and HAVE_RAMPAYLOAD" maybe not so much.
yes, we were targeting RVPs/CRBs for such POC work and WIP patch will help to achieve the same. Ron will talk more about this work in coming OSFC
-
Furquan Shaikh (Code Review) -
Kyösti Mälkki (Code Review)
-
ron minnich (Code Review)
-
Subrata Banik (Code Review)