Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38664 )
Change subject: mb/intel/glkrvp: Simplify FMAP file ......................................................................
mb/intel/glkrvp: Simplify FMAP file
Tested with BUILD_TIMELESS, no changes.
Change-Id: Iac1d1e16be5db7bfbadc5929057cc2d93b5cd876 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/mainboard/intel/glkrvp/chromeos.fmd 1 file changed, 31 insertions(+), 31 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/38664/1
diff --git a/src/mainboard/intel/glkrvp/chromeos.fmd b/src/mainboard/intel/glkrvp/chromeos.fmd index 8f3c63a..cfde802 100644 --- a/src/mainboard/intel/glkrvp/chromeos.fmd +++ b/src/mainboard/intel/glkrvp/chromeos.fmd @@ -1,41 +1,41 @@ FLASH 16M { - WP_RO@0x0 0x400000 { - SI_DESC@0x0 0x1000 - IFWI@0x1000 0x1ff000 - RO_VPD(PRESERVE)@0x200000 0x4000 - RO_SECTION@0x204000 0x1fc000 { - FMAP@0x0 0x800 - RO_FRID@0x800 0x40 - RO_FRID_PAD@0x840 0x7c0 - COREBOOT(CBFS)@0x1000 0x1ab000 - GBB@0x1ac000 0x40000 - RO_UNUSED@0x1ec000 0x10000 + WP_RO 0x400000 { + SI_DESC 0x1000 + IFWI 0x1ff000 + RO_VPD(PRESERVE) 0x4000 + RO_SECTION 0x1fc000 { + FMAP 0x800 + RO_FRID 0x40 + RO_FRID_PAD 0x7c0 + COREBOOT(CBFS) + GBB 0x40000 + RO_UNUSED 0x10000 } } - MISC_RW@0x400000 0x4a000 { - UNIFIED_MRC_CACHE@0x0 0x31000 { - RECOVERY_MRC_CACHE@0x0 0x10000 - RW_MRC_CACHE@0x10000 0x20000 - RW_VAR_MRC_CACHE@0x30000 0x1000 + MISC_RW 0x4a000 { + UNIFIED_MRC_CACHE 0x31000 { + RECOVERY_MRC_CACHE 0x10000 + RW_MRC_CACHE 0x20000 + RW_VAR_MRC_CACHE 0x1000 } - RW_ELOG(PRESERVE)@0x31000 0x4000 - RW_SHARED@0x35000 0x4000 { - SHARED_DATA@0x0 0x2000 - VBLOCK_DEV@0x2000 0x2000 + RW_ELOG(PRESERVE) 0x4000 + RW_SHARED 0x4000 { + SHARED_DATA 0x2000 + VBLOCK_DEV 0x2000 } - RW_VPD(PRESERVE)@0x39000 0x2000 - FPF_STATUS@0x3B000 0x1000 - TMP_UNUSED_HOLE@0x3C000 0xE000 + RW_VPD(PRESERVE) 0x2000 + FPF_STATUS 0x1000 + TMP_UNUSED_HOLE 0xe000 } - RW_SECTION_A@0x44a000 0x477800 { - VBLOCK_A@0x0 0x10000 - FW_MAIN_A(CBFS)@0x10000 0x4677c0 - RW_FWID_A@0x4777c0 0x40 + RW_SECTION_A 0x477800 { + VBLOCK_A 0x10000 + FW_MAIN_A(CBFS) + RW_FWID_A 0x40 } - RW_SECTION_B@0x8c1800 0x477800 { - VBLOCK_B@0x0 0x10000 - FW_MAIN_B(CBFS)@0x10000 0x4677c0 - RW_FWID_B@0x4777c0 0x40 + RW_SECTION_B 0x477800 { + VBLOCK_B 0x10000 + FW_MAIN_B(CBFS) + RW_FWID_B 0x40 } RW_NVRAM(PRESERVE)@0xd39000 0x6000 SMMSTORE(PRESERVE)@0xd40000 0x40000
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38664 )
Change subject: mb/intel/glkrvp: Simplify FMAP file ......................................................................
Patch Set 1:
I don't necessarily understand the impetus for this change. While it happens to be the case that the regions here are continuous, there's no guarantee that will always be the case. And removing the start address for each region just makes it harder to calculate the absolute position of any given region if needed, and even moreso when we get to 3 and 4 layers of nesting.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38664 )
Change subject: mb/intel/glkrvp: Simplify FMAP file ......................................................................
Patch Set 1:
(6 comments)
Patch Set 1:
I don't necessarily understand the impetus for this change.
First of all, nobody seems to use this board. As it is one of the few GLK examples, I tried to make the fmap simpler so that it is easier to adapt for another board.
While it happens to be the case that the regions here are continuous, there's no guarantee that will always be the case.
What do you mean? I would believe fmaps do not change very often, even less for an Intel reference board. Also, other fmaps in the tree use this simpler style.
Moreover, if there are any changes to it, only about half of the numbers need to be changed (because about half of them are gone with this change). It's also much less error-prone: what if one makes a mistake while calculating the new positions?
And removing the start address for each region just makes it harder to calculate the absolute position of any given region if needed, and even moreso when we get to 3 and 4 layers of nesting.
I beg to differ. Sure, you would have to calculate the absolute position (it's not explicit anymore) but it's just a matter of adding hex numbers together. Also, as there's only one hex number now, it's much harder to take the wrong number by mistake.
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 9: PAD PAD.
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 10: COREBOOT(CBFS) This will expand and use all available space inside RO_SECTION. If we are to remove the size of that section, then it will expand and use all the space of WP_RO.
Regions before this one will get compacted at the beginning of the region with constraints, and the regions after this will get compacted at the end.
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 15: 0x4a000 Everything here has a fixed size, so the region sizes might as well be omitted
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 32: FW_MAIN_A(CBFS) This makes RW_SECTION_A be completely filled.
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 37: FW_MAIN_B(CBFS) Same story here.
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 39: } And I stopped here because there's gaps.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38664 )
Change subject: mb/intel/glkrvp: Simplify FMAP file ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... File src/mainboard/intel/glkrvp/chromeos.fmd:
https://review.coreboot.org/c/coreboot/+/38664/1/src/mainboard/intel/glkrvp/... PS1, Line 26: RW_VPD(PRESERVE) 0x2000 Let's calculate the absolute position of this. Before, it was like this:
[RW_VPD pos in FLASH] = [MISC_RW pos in FLASH] + [RW_VPD pos in MISC_RW]
pos = 0x400000 + 0x39000 = 0x439000
Now, we do a similar thing:
[RW_VPD pos in FLASH] = [WP_RO size] + [UNIFIED_MRC_CACHE size] + [RW_ELOG size] + [RW_SHARED size]
pos = 0x400000 + 0x31000 + 0x4000 + 0x4000 = 0x439000
I mean, it's not much more complicated. If you have an hex calculator (or Google), it's a piece of cake.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38664 )
Change subject: mb/intel/glkrvp: Simplify FMAP file ......................................................................
Abandoned
Nobody likes it
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)