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.