Hello Arthur Heymans,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/37497
to review the following change.
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
fmap: Make FMAP_CACHE mandatory if it is configured in
Now that we have a CONFIG_NO_FMAP_CACHE to completely configure out the pre-RAM FMAP cache code, there's no point in allowing the region to be optional anymore. This patch makes the section required by the linker. If a board doesn't want to provide it, it has to select NO_FMAP_CACHE.
Adding FMAP_CACHE regions to a couple more targets that I think can use them but I don't know anything about... please yell if one of these is a bad idea and I should mark them NO_FMAP_CACHE instead.
Change-Id: Ic7d47772ab3abfa7e3a66815c3739d0af071abc2 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/cpu/ti/am335x/memlayout.ld M src/lib/fmap.c M src/mainboard/emulation/qemu-aarch64/memlayout.ld M src/mainboard/emulation/qemu-armv7/memlayout.ld M src/mainboard/emulation/qemu-power8/memlayout.ld M src/mainboard/emulation/qemu-riscv/memlayout.ld M src/mainboard/emulation/spike-riscv/memlayout.ld M src/soc/cavium/cn81xx/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/sifive/fu540/include/soc/memlayout.ld 10 files changed, 14 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37497/1
diff --git a/src/cpu/ti/am335x/memlayout.ld b/src/cpu/ti/am335x/memlayout.ld index 0de86f1..f69a315 100644 --- a/src/cpu/ti/am335x/memlayout.ld +++ b/src/cpu/ti/am335x/memlayout.ld @@ -19,7 +19,8 @@ { DRAM_START(0x40000000) BOOTBLOCK(0x402f0400, 20K) - ROMSTAGE(0x402f5400, 90K) + ROMSTAGE(0x402f5400, 88K) + FMAP_CACHE(0x4030b400, 2K) STACK(0x4030be00, 4K) RAMSTAGE(0x80200000, 192K)
diff --git a/src/lib/fmap.c b/src/lib/fmap.c index af26152..9d2b4e7 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -36,8 +36,6 @@ printk(__VA_ARGS__); \ } while (0)
-DECLARE_OPTIONAL_REGION(fmap_cache); - uint64_t get_fmap_flash_offset(void) { return FMAP_OFFSET; @@ -71,14 +69,6 @@ return; }
- if (REGION_SIZE(fmap_cache) == 0) { - /* If you see this you should add FMAP_CACHE() to your memlayout - (or select NO_FMAP_CACHE if you can't afford the 2K). */ - print_once(BIOS_ERR, - "ERROR: FMAP_CACHE enabled but no region provided!\n"); - return; - } - struct fmap *fmap = (struct fmap *)_fmap_cache; if (!ENV_BOOTBLOCK) { /* NOTE: This assumes that for all platforms running this code, diff --git a/src/mainboard/emulation/qemu-aarch64/memlayout.ld b/src/mainboard/emulation/qemu-aarch64/memlayout.ld index 0b52d31..aba4205 100644 --- a/src/mainboard/emulation/qemu-aarch64/memlayout.ld +++ b/src/mainboard/emulation/qemu-aarch64/memlayout.ld @@ -24,7 +24,8 @@
DRAM_START(0x40000000) BOOTBLOCK(0x60010000, 64K) - STACK(0x60020000, 64K) + STACK(0x60020000, 62K) + FMAP_CACHE(0x6002F800, 2K) ROMSTAGE(0x60030000, 128K) RAMSTAGE(0x60070000, 16M)
diff --git a/src/mainboard/emulation/qemu-armv7/memlayout.ld b/src/mainboard/emulation/qemu-armv7/memlayout.ld index 776e051..2b33cb3 100644 --- a/src/mainboard/emulation/qemu-armv7/memlayout.ld +++ b/src/mainboard/emulation/qemu-armv7/memlayout.ld @@ -42,6 +42,7 @@ /* TODO: does this thing emulate SRAM? */
BOOTBLOCK(0x00000, 64K) + FMAP_CACHE(0x10000, 2K)
DRAM_START(0x60000000) STACK(0x60000000, 64K) diff --git a/src/mainboard/emulation/qemu-power8/memlayout.ld b/src/mainboard/emulation/qemu-power8/memlayout.ld index da0f4a5..c22d3e4 100644 --- a/src/mainboard/emulation/qemu-power8/memlayout.ld +++ b/src/mainboard/emulation/qemu-power8/memlayout.ld @@ -26,5 +26,6 @@ ROMSTAGE(0x20000, 128K) STACK(0x40000, 0x3ff00) PRERAM_CBMEM_CONSOLE(0x80000, 8K) + FMAP_CACHE(0x82000, 2K) RAMSTAGE(0x100000, 16M) } diff --git a/src/mainboard/emulation/qemu-riscv/memlayout.ld b/src/mainboard/emulation/qemu-riscv/memlayout.ld index b29bc14..e53df38 100644 --- a/src/mainboard/emulation/qemu-riscv/memlayout.ld +++ b/src/mainboard/emulation/qemu-riscv/memlayout.ld @@ -37,6 +37,7 @@ REGION(opensbi, STAGES_START, 128K, 4K) #endif PRERAM_CBMEM_CONSOLE(STAGES_START + 128K, 8K) + FMAP_CACHE(STAGES_START + 136K, 2K) RAMSTAGE(STAGES_START + 200K, 16M) STACK(STAGES_START + 200K + 16M, 4K) } diff --git a/src/mainboard/emulation/spike-riscv/memlayout.ld b/src/mainboard/emulation/spike-riscv/memlayout.ld index bae414f..b6e4d9d 100644 --- a/src/mainboard/emulation/spike-riscv/memlayout.ld +++ b/src/mainboard/emulation/spike-riscv/memlayout.ld @@ -24,7 +24,8 @@ DRAM_START(START) BOOTBLOCK(START, 64K) STACK(START + 8M, 4K) - /* hole at (START + 8M + 4K, 60K) */ + FMAP_CACHE(START + 8M + 4K, 2K) + /* hole at (START + 8M + 6K, 58K) */ ROMSTAGE(START + 8M + 64K, 128K) PRERAM_CBMEM_CONSOLE(START + 8M + 192k, 8K) RAMSTAGE(START + 8M + 200K, 256K) diff --git a/src/soc/cavium/cn81xx/include/soc/memlayout.ld b/src/soc/cavium/cn81xx/include/soc/memlayout.ld index e4e3490..1a0eb15 100644 --- a/src/soc/cavium/cn81xx/include/soc/memlayout.ld +++ b/src/soc/cavium/cn81xx/include/soc/memlayout.ld @@ -31,7 +31,8 @@
STACK(BOOTROM_OFFSET, 16K) TIMESTAMP(BOOTROM_OFFSET + 0x4000, 4K) - PRERAM_CBFS_CACHE(BOOTROM_OFFSET + 0x6000, 8K) + PRERAM_CBFS_CACHE(BOOTROM_OFFSET + 0x6000, 6K) + FMAP_CACHE(BOOTROM_OFFSET + 0x7800, 2K) PRERAM_CBMEM_CONSOLE(BOOTROM_OFFSET + 0x8000, 8K) BOOTBLOCK(BOOTROM_OFFSET + 0x20000, 64K) VBOOT2_WORK(BOOTROM_OFFSET + 0x30000, 12K) diff --git a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld index b0d2d43..c3a3b4c 100644 --- a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld +++ b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld @@ -57,7 +57,8 @@ TIMESTAMP(0x14836000, 1K) PRERAM_CBMEM_CONSOLE(0x14836400, 32K) PRERAM_CBFS_CACHE(0x1483E400, 70K) - REGION(bsram_unused, 0x1484FC00, 0x9E300, 0x100) + FMAP_CACHE(0x1484FC00, 2K) + REGION(bsram_unused, 0x14850400, 0x9DB00, 0x100) REGION(ddr_information, 0x148EDF00, 256, 256) REGION(limits_cfg, 0x148EE000, 4K, 4K) REGION(qclib_serial_log, 0x148EF000, 4K, 4K) diff --git a/src/soc/sifive/fu540/include/soc/memlayout.ld b/src/soc/sifive/fu540/include/soc/memlayout.ld index df30ede..46c559c 100644 --- a/src/soc/sifive/fu540/include/soc/memlayout.ld +++ b/src/soc/sifive/fu540/include/soc/memlayout.ld @@ -27,6 +27,7 @@ BOOTBLOCK(FU540_L2LIM, 64K) CAR_STACK(FU540_L2LIM + 64K, 20K) PRERAM_CBMEM_CONSOLE(FU540_L2LIM + 84K, 8K) + FMAP_CACHE(FU540_L2LIM + 92K, 2K) ROMSTAGE(FU540_L2LIM + 128K, 128K) PRERAM_CBFS_CACHE(FU540_L2LIM + 256K, 128K) L2LIM_END(FU540_L2LIM + 2M)
Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37497 )
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
Patch Set 1:
Hmm. I don't know about FMAP_CACHE so I really can't comment on this right now. I assume its documented somewhere?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37497 )
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
Patch Set 1:
Hmm. I don't know about FMAP_CACHE so I really can't comment on this right now. I assume its documented somewhere?
Not much. Original CL is CB:36657.
Basically, I'm just asking if your platform can spare the extra 2K I'm carving out here and if I can rely on that staying available from the bootblock through to romstage. If so, you probably want it because it may safe a few ms of boot time.
Marty E. Plummer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37497 )
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
Patch Set 1: Code-Review+1
Patch Set 1:
Not much. Original CL is CB:36657.
Basically, I'm just asking if your platform can spare the extra 2K I'm carving out here and if I can rely on that staying available from the bootblock through to romstage. If so, you probably want it because it may safe a few ms of boot time.
Seems resonable to me. 2k is cheap.
Hello ron minnich, Marty E. Plummer, Arthur Heymans, Patrick Rudolph, build bot (Jenkins), Philipp Hug,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37497
to look at the new patch set (#2).
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
fmap: Make FMAP_CACHE mandatory if it is configured in
Now that we have a CONFIG_NO_FMAP_CACHE to completely configure out the pre-RAM FMAP cache code, there's no point in allowing the region to be optional anymore. This patch makes the section required by the linker. If a board doesn't want to provide it, it has to select NO_FMAP_CACHE.
Adding FMAP_CACHE regions to a couple more targets that I think can use them but I don't know anything about... please yell if one of these is a bad idea and I should mark them NO_FMAP_CACHE instead.
Change-Id: Ic7d47772ab3abfa7e3a66815c3739d0af071abc2 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/cpu/ti/am335x/memlayout.ld M src/include/memlayout.h M src/lib/fmap.c M src/mainboard/emulation/qemu-aarch64/memlayout.ld M src/mainboard/emulation/qemu-armv7/memlayout.ld M src/mainboard/emulation/qemu-power8/memlayout.ld M src/mainboard/emulation/qemu-riscv/memlayout.ld M src/mainboard/emulation/spike-riscv/memlayout.ld M src/soc/cavium/cn81xx/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/sifive/fu540/include/soc/memlayout.ld 11 files changed, 15 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/37497/2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37497 )
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37497 )
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37497 )
Change subject: fmap: Make FMAP_CACHE mandatory if it is configured in ......................................................................
fmap: Make FMAP_CACHE mandatory if it is configured in
Now that we have a CONFIG_NO_FMAP_CACHE to completely configure out the pre-RAM FMAP cache code, there's no point in allowing the region to be optional anymore. This patch makes the section required by the linker. If a board doesn't want to provide it, it has to select NO_FMAP_CACHE.
Adding FMAP_CACHE regions to a couple more targets that I think can use them but I don't know anything about... please yell if one of these is a bad idea and I should mark them NO_FMAP_CACHE instead.
Change-Id: Ic7d47772ab3abfa7e3a66815c3739d0af071abc2 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/37497 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/cpu/ti/am335x/memlayout.ld M src/include/memlayout.h M src/lib/fmap.c M src/mainboard/emulation/qemu-aarch64/memlayout.ld M src/mainboard/emulation/qemu-armv7/memlayout.ld M src/mainboard/emulation/qemu-power8/memlayout.ld M src/mainboard/emulation/qemu-riscv/memlayout.ld M src/mainboard/emulation/spike-riscv/memlayout.ld M src/soc/cavium/cn81xx/include/soc/memlayout.ld M src/soc/qualcomm/sdm845/include/soc/memlayout.ld M src/soc/sifive/fu540/include/soc/memlayout.ld 11 files changed, 15 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/cpu/ti/am335x/memlayout.ld b/src/cpu/ti/am335x/memlayout.ld index 0de86f1..f69a315 100644 --- a/src/cpu/ti/am335x/memlayout.ld +++ b/src/cpu/ti/am335x/memlayout.ld @@ -19,7 +19,8 @@ { DRAM_START(0x40000000) BOOTBLOCK(0x402f0400, 20K) - ROMSTAGE(0x402f5400, 90K) + ROMSTAGE(0x402f5400, 88K) + FMAP_CACHE(0x4030b400, 2K) STACK(0x4030be00, 4K) RAMSTAGE(0x80200000, 192K)
diff --git a/src/include/memlayout.h b/src/include/memlayout.h index 56dfb6a..e3aeec6 100644 --- a/src/include/memlayout.h +++ b/src/include/memlayout.h @@ -78,7 +78,7 @@
#define FMAP_CACHE(addr, sz) \ REGION(fmap_cache, addr, sz, 4) \ - _ = ASSERT(sz == 0 || sz >= FMAP_SIZE, \ + _ = ASSERT(sz >= FMAP_SIZE, \ STR(FMAP does not fit in FMAP_CACHE! (sz < FMAP_SIZE)));
#if ENV_ROMSTAGE_OR_BEFORE diff --git a/src/lib/fmap.c b/src/lib/fmap.c index af26152..9d2b4e7 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -36,8 +36,6 @@ printk(__VA_ARGS__); \ } while (0)
-DECLARE_OPTIONAL_REGION(fmap_cache); - uint64_t get_fmap_flash_offset(void) { return FMAP_OFFSET; @@ -71,14 +69,6 @@ return; }
- if (REGION_SIZE(fmap_cache) == 0) { - /* If you see this you should add FMAP_CACHE() to your memlayout - (or select NO_FMAP_CACHE if you can't afford the 2K). */ - print_once(BIOS_ERR, - "ERROR: FMAP_CACHE enabled but no region provided!\n"); - return; - } - struct fmap *fmap = (struct fmap *)_fmap_cache; if (!ENV_BOOTBLOCK) { /* NOTE: This assumes that for all platforms running this code, diff --git a/src/mainboard/emulation/qemu-aarch64/memlayout.ld b/src/mainboard/emulation/qemu-aarch64/memlayout.ld index 0b52d31..aba4205 100644 --- a/src/mainboard/emulation/qemu-aarch64/memlayout.ld +++ b/src/mainboard/emulation/qemu-aarch64/memlayout.ld @@ -24,7 +24,8 @@
DRAM_START(0x40000000) BOOTBLOCK(0x60010000, 64K) - STACK(0x60020000, 64K) + STACK(0x60020000, 62K) + FMAP_CACHE(0x6002F800, 2K) ROMSTAGE(0x60030000, 128K) RAMSTAGE(0x60070000, 16M)
diff --git a/src/mainboard/emulation/qemu-armv7/memlayout.ld b/src/mainboard/emulation/qemu-armv7/memlayout.ld index 776e051..2b33cb3 100644 --- a/src/mainboard/emulation/qemu-armv7/memlayout.ld +++ b/src/mainboard/emulation/qemu-armv7/memlayout.ld @@ -42,6 +42,7 @@ /* TODO: does this thing emulate SRAM? */
BOOTBLOCK(0x00000, 64K) + FMAP_CACHE(0x10000, 2K)
DRAM_START(0x60000000) STACK(0x60000000, 64K) diff --git a/src/mainboard/emulation/qemu-power8/memlayout.ld b/src/mainboard/emulation/qemu-power8/memlayout.ld index da0f4a5..c22d3e4 100644 --- a/src/mainboard/emulation/qemu-power8/memlayout.ld +++ b/src/mainboard/emulation/qemu-power8/memlayout.ld @@ -26,5 +26,6 @@ ROMSTAGE(0x20000, 128K) STACK(0x40000, 0x3ff00) PRERAM_CBMEM_CONSOLE(0x80000, 8K) + FMAP_CACHE(0x82000, 2K) RAMSTAGE(0x100000, 16M) } diff --git a/src/mainboard/emulation/qemu-riscv/memlayout.ld b/src/mainboard/emulation/qemu-riscv/memlayout.ld index b29bc14..e53df38 100644 --- a/src/mainboard/emulation/qemu-riscv/memlayout.ld +++ b/src/mainboard/emulation/qemu-riscv/memlayout.ld @@ -37,6 +37,7 @@ REGION(opensbi, STAGES_START, 128K, 4K) #endif PRERAM_CBMEM_CONSOLE(STAGES_START + 128K, 8K) + FMAP_CACHE(STAGES_START + 136K, 2K) RAMSTAGE(STAGES_START + 200K, 16M) STACK(STAGES_START + 200K + 16M, 4K) } diff --git a/src/mainboard/emulation/spike-riscv/memlayout.ld b/src/mainboard/emulation/spike-riscv/memlayout.ld index bae414f..b6e4d9d 100644 --- a/src/mainboard/emulation/spike-riscv/memlayout.ld +++ b/src/mainboard/emulation/spike-riscv/memlayout.ld @@ -24,7 +24,8 @@ DRAM_START(START) BOOTBLOCK(START, 64K) STACK(START + 8M, 4K) - /* hole at (START + 8M + 4K, 60K) */ + FMAP_CACHE(START + 8M + 4K, 2K) + /* hole at (START + 8M + 6K, 58K) */ ROMSTAGE(START + 8M + 64K, 128K) PRERAM_CBMEM_CONSOLE(START + 8M + 192k, 8K) RAMSTAGE(START + 8M + 200K, 256K) diff --git a/src/soc/cavium/cn81xx/include/soc/memlayout.ld b/src/soc/cavium/cn81xx/include/soc/memlayout.ld index e4e3490..1a0eb15 100644 --- a/src/soc/cavium/cn81xx/include/soc/memlayout.ld +++ b/src/soc/cavium/cn81xx/include/soc/memlayout.ld @@ -31,7 +31,8 @@
STACK(BOOTROM_OFFSET, 16K) TIMESTAMP(BOOTROM_OFFSET + 0x4000, 4K) - PRERAM_CBFS_CACHE(BOOTROM_OFFSET + 0x6000, 8K) + PRERAM_CBFS_CACHE(BOOTROM_OFFSET + 0x6000, 6K) + FMAP_CACHE(BOOTROM_OFFSET + 0x7800, 2K) PRERAM_CBMEM_CONSOLE(BOOTROM_OFFSET + 0x8000, 8K) BOOTBLOCK(BOOTROM_OFFSET + 0x20000, 64K) VBOOT2_WORK(BOOTROM_OFFSET + 0x30000, 12K) diff --git a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld index b0d2d43..c3a3b4c 100644 --- a/src/soc/qualcomm/sdm845/include/soc/memlayout.ld +++ b/src/soc/qualcomm/sdm845/include/soc/memlayout.ld @@ -57,7 +57,8 @@ TIMESTAMP(0x14836000, 1K) PRERAM_CBMEM_CONSOLE(0x14836400, 32K) PRERAM_CBFS_CACHE(0x1483E400, 70K) - REGION(bsram_unused, 0x1484FC00, 0x9E300, 0x100) + FMAP_CACHE(0x1484FC00, 2K) + REGION(bsram_unused, 0x14850400, 0x9DB00, 0x100) REGION(ddr_information, 0x148EDF00, 256, 256) REGION(limits_cfg, 0x148EE000, 4K, 4K) REGION(qclib_serial_log, 0x148EF000, 4K, 4K) diff --git a/src/soc/sifive/fu540/include/soc/memlayout.ld b/src/soc/sifive/fu540/include/soc/memlayout.ld index df30ede..46c559c 100644 --- a/src/soc/sifive/fu540/include/soc/memlayout.ld +++ b/src/soc/sifive/fu540/include/soc/memlayout.ld @@ -27,6 +27,7 @@ BOOTBLOCK(FU540_L2LIM, 64K) CAR_STACK(FU540_L2LIM + 64K, 20K) PRERAM_CBMEM_CONSOLE(FU540_L2LIM + 84K, 8K) + FMAP_CACHE(FU540_L2LIM + 92K, 2K) ROMSTAGE(FU540_L2LIM + 128K, 128K) PRERAM_CBFS_CACHE(FU540_L2LIM + 256K, 128K) L2LIM_END(FU540_L2LIM + 2M)
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Marty E. Plummer (Code Review)
-
Patrick Georgi (Code Review)