Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45927 )
Change subject: nb/intel/*/memmap.c: Use `postcar_frame_add_cbmem_top` ......................................................................
nb/intel/*/memmap.c: Use `postcar_frame_add_cbmem_top`
Not all cases can be covered by this function.
Change-Id: I15ab4b73adf6af3b870be8394aac017754238d8f Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/gm45/memmap.c M src/northbridge/intel/i945/memmap.c M src/northbridge/intel/ironlake/memmap.c M src/northbridge/intel/pineview/memmap.c M src/northbridge/intel/sandybridge/memmap.c M src/northbridge/intel/x4x/memmap.c 6 files changed, 6 insertions(+), 20 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/45927/1
diff --git a/src/northbridge/intel/gm45/memmap.c b/src/northbridge/intel/gm45/memmap.c index da5e0ed..acc4199 100644 --- a/src/northbridge/intel/gm45/memmap.c +++ b/src/northbridge/intel/gm45/memmap.c @@ -117,11 +117,8 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram; - /* Cache 8 MiB region below the top of RAM to cover CBMEM */ - top_of_ram = (uintptr_t)cbmem_top(); - postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 8 * MiB, MTRR_TYPE_WRBACK); + postcar_frame_add_cbmem_top(pcf, 8 * MiB, 8 * MiB);
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf); diff --git a/src/northbridge/intel/i945/memmap.c b/src/northbridge/intel/i945/memmap.c index af32f39..e2f64a8 100644 --- a/src/northbridge/intel/i945/memmap.c +++ b/src/northbridge/intel/i945/memmap.c @@ -83,11 +83,8 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram; - /* Cache 8 MiB region below the top of RAM to cover CBMEM */ - top_of_ram = (uintptr_t)cbmem_top(); - postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 8 * MiB, MTRR_TYPE_WRBACK); + postcar_frame_add_cbmem_top(pcf, 8 * MiB, 8 * MiB);
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf); diff --git a/src/northbridge/intel/ironlake/memmap.c b/src/northbridge/intel/ironlake/memmap.c index 6771a20..e922c4e 100644 --- a/src/northbridge/intel/ironlake/memmap.c +++ b/src/northbridge/intel/ironlake/memmap.c @@ -35,10 +35,8 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), 8 * MiB); - /* Cache 8 MiB below the top of RAM */ - postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 8 * MiB, MTRR_TYPE_WRBACK); + postcar_frame_add_cbmem_top(pcf, 8 * MiB, 8 * MiB);
/* Cache TSEG region */ postcar_enable_tseg_cache(pcf); diff --git a/src/northbridge/intel/pineview/memmap.c b/src/northbridge/intel/pineview/memmap.c index d26ae01..12366fa 100644 --- a/src/northbridge/intel/pineview/memmap.c +++ b/src/northbridge/intel/pineview/memmap.c @@ -124,11 +124,8 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram; - /* Cache 8 MiB region below the top of RAM to cover CBMEM */ - top_of_ram = (uintptr_t)cbmem_top(); - postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 8 * MiB, MTRR_TYPE_WRBACK); + postcar_frame_add_cbmem_top(pcf, 8 * MiB, 8 * MiB);
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf); diff --git a/src/northbridge/intel/sandybridge/memmap.c b/src/northbridge/intel/sandybridge/memmap.c index eaf146d..519fb95 100644 --- a/src/northbridge/intel/sandybridge/memmap.c +++ b/src/northbridge/intel/sandybridge/memmap.c @@ -46,7 +46,7 @@ * be 8MiB aligned. Set this area as cacheable so it can be used later * for ramstage before setting up the entire RAM as cacheable. */ - postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 8 * MiB, MTRR_TYPE_WRBACK); + postcar_frame_add_cbmem_top(pcf, 8 * MiB, 1);
/* * Cache the TSEG region using regular MTRRs. This is only useful diff --git a/src/northbridge/intel/x4x/memmap.c b/src/northbridge/intel/x4x/memmap.c index 0c62fdc..28d4eb3 100644 --- a/src/northbridge/intel/x4x/memmap.c +++ b/src/northbridge/intel/x4x/memmap.c @@ -122,11 +122,8 @@
void fill_postcar_frame(struct postcar_frame *pcf) { - uintptr_t top_of_ram; - /* Cache 8 MiB region below the top of RAM to cover CBMEM */ - top_of_ram = (uintptr_t)cbmem_top(); - postcar_frame_add_mtrr(pcf, top_of_ram - 8 * MiB, 8 * MiB, MTRR_TYPE_WRBACK); + postcar_frame_add_cbmem_top(pcf, 8 * MiB, 8 * MiB);
/* Cache the TSEG region */ postcar_enable_tseg_cache(pcf);
Masanori Ogino has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45927 )
Change subject: nb/intel/*/memmap.c: Use `postcar_frame_add_cbmem_top` ......................................................................
Patch Set 2:
(2 comments)
This is nice de-duplication work, but I have some minor comments.
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/memmap.c:
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... PS2, Line 41: uintptr_t top_of_ram = (uintptr_t)cbmem_top(); This variable can be eliminated now.
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... PS2, Line 49: postcar_frame_add_cbmem_top(pcf, 8 * MiB, 1); Is the change of alignment from 8 MiB to 1 byte here intentional?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45927 )
Change subject: nb/intel/*/memmap.c: Use `postcar_frame_add_cbmem_top` ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/memmap.c:
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... PS2, Line 41: uintptr_t top_of_ram = (uintptr_t)cbmem_top();
This variable can be eliminated now.
Thanks for noticing. Will move to /dev/null
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... PS2, Line 49: postcar_frame_add_cbmem_top(pcf, 8 * MiB, 1);
Is the change of alignment from 8 MiB to 1 byte here intentional?
There's no change of alignment. The original code did not align `top_of_ram` at all. Given that TSEG needs to be aligned to 8 MiB because of SMRRs, this should make no difference. I'll inject a change forcing 8 MiB alignment before this one.
Masanori Ogino has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45927 )
Change subject: nb/intel/*/memmap.c: Use `postcar_frame_add_cbmem_top` ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
Thank you for your prompt answers, Angel! Feel free to mark my comments as resolved when the patchset is revised.
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/memmap.c:
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... PS2, Line 41: uintptr_t top_of_ram = (uintptr_t)cbmem_top();
Thanks for noticing. […]
Ack
https://review.coreboot.org/c/coreboot/+/45927/2/src/northbridge/intel/sandy... PS2, Line 49: postcar_frame_add_cbmem_top(pcf, 8 * MiB, 1);
There's no change of alignment. The original code did not align `top_of_ram` at all. […]
I see. Thank you for your explanation.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/45927 )
Change subject: nb/intel/*/memmap.c: Use `postcar_frame_add_cbmem_top` ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Masanori Ogino (Code Review)