Hello Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/47585
to review the following change.
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
intel/d945gclf: Disable bootblock console.
This mainboard has less than 20 bytes of space left in its bootblock, hindering development. I don't know anything about it or if/how it could be optimized effectively, so according to our long-standing tradition of what to do when random boards run out of space, let's disable the bootblock console (this frees up over 4KB of the total 16).
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I620c13eab53c3326a4f4660b63ed1dd0fc81f563 --- M src/mainboard/intel/d945gclf/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/47585/1
diff --git a/src/mainboard/intel/d945gclf/Kconfig b/src/mainboard/intel/d945gclf/Kconfig index 06e1a0c..8f5bd32 100644 --- a/src/mainboard/intel/d945gclf/Kconfig +++ b/src/mainboard/intel/d945gclf/Kconfig @@ -18,6 +18,7 @@ select BOARD_ROMSIZE_KB_512 select MAINBOARD_HAS_NATIVE_VGA_INIT select INTEL_GMA_HAVE_VBT + select NO_BOOTBLOCK_CONSOLE
config MAINBOARD_DIR string
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: intel/d945gclf: Disable bootblock console. Nit: Please remove the dot/period at the end.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: d945gclf Have you considered raising the bootblock size in cpu/intel/socket_441 instead? I can ask someone to test such a change.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
I recovered topic:x86-bootblock. The previous valuable gerrit commentary is however gone now.
Said topic branch drops static bootblock size on x86.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: d945gclf
Have you considered raising the bootblock size in cpu/intel/socket_441 instead? I can ask someone to […]
Sorry, I just don't have any idea about how this platform works or what is safe to do on it. I can bump C_ENV_BOOTBLOCK_SIZE for that socket to 8K if you want, but I'll have to take your word on it that it's okay to do so. (If this needs any more complicated work than that, I'd prefer to push a patch like this to unblock other patches and leave the cleanup to people who know and can test the platform.)
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: d945gclf
Sorry, I just don't have any idea about how this platform works or what is safe to do on it. […]
Architectural limit is 64 KiB and you probably planned to increase it to 0x8000 for 32 KiB. It (x86) should no longer have the log2 size requirement for C_ENV_BOOTBLOCK_SIZE.
Someone may come and complain their Tianocore payload no longer fits in flash, but I welcome him/her to assist and complete the topic:x86-bootblock then.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: d945gclf
Sorry, I just don't have any idea about how this platform works or what is safe to do on it. I can bump C_ENV_BOOTBLOCK_SIZE for that socket to 8K if you want, but I'll have to take your word on it that it's okay to do so. (If this needs any more complicated work than that, I'd prefer to push a patch like this to unblock other patches and leave the cleanup to people who know and can test the platform.)
It's safe for that platform (socket_441, ich7) to increase C_ENV_BOOTBLOCK_SIZE (preferably power of 2 and <= 64K). ATM only intel/d945gclf uses that socket. I have this board but I'm very sure that an increased bootblock size will work fine.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/d945gclf: Disable bootblock console. ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: d945gclf
Sorry, I just don't have any idea about how this platform works or what is safe to do on it. […]
TianoCore is usually over 512 KiB, so it didn't really fit to begin with. I'd simply remove the C_ENV_BOOTBLOCK_SIZE value from socket_441 (IIRC the northbridge already has a reasonable default).
Hello build bot (Jenkins), Angel Pons, Arthur Heymans, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47585
to look at the new patch set (#2).
Change subject: intel/socket_441: Increase bootblock size ......................................................................
intel/socket_441: Increase bootblock size
One mainboard using this socket has less than 20 bytes of space left in its bootblock, hindering development. Double the bootblock size to solve the problem.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I620c13eab53c3326a4f4660b63ed1dd0fc81f563 --- M src/cpu/intel/socket_441/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/85/47585/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/socket_441: Increase bootblock size ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: d945gclf
TianoCore is usually over 512 KiB, so it didn't really fit to begin with. […]
Right, sorry, I meant 0x8000, not 8K. Done.
https://review.coreboot.org/c/coreboot/+/47585/1//COMMIT_MSG@7 PS1, Line 7: intel/d945gclf: Disable bootblock console.
Nit: Please remove the dot/period at the end.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/socket_441: Increase bootblock size ......................................................................
Patch Set 2: Code-Review+2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/socket_441: Increase bootblock size ......................................................................
Patch Set 2: Code-Review+1
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/socket_441: Increase bootblock size ......................................................................
Patch Set 2: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47585 )
Change subject: intel/socket_441: Increase bootblock size ......................................................................
intel/socket_441: Increase bootblock size
One mainboard using this socket has less than 20 bytes of space left in its bootblock, hindering development. Double the bootblock size to solve the problem.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I620c13eab53c3326a4f4660b63ed1dd0fc81f563 Reviewed-on: https://review.coreboot.org/c/coreboot/+/47585 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-by: Arthur Heymans arthur@aheymans.xyz Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/cpu/intel/socket_441/Kconfig 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Kyösti Mälkki: Looks good to me, but someone else must approve Arthur Heymans: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/cpu/intel/socket_441/Kconfig b/src/cpu/intel/socket_441/Kconfig index af43f72..7a303af 100644 --- a/src/cpu/intel/socket_441/Kconfig +++ b/src/cpu/intel/socket_441/Kconfig @@ -12,7 +12,7 @@
config C_ENV_BOOTBLOCK_SIZE hex - default 0x4000 + default 0x8000
config DCACHE_RAM_BASE hex
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Felix Held (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Paul Menzel (Code Review)