Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35029 )
Change subject: soc/intel/{cnl, dnv, icl, skl}: Make top_of_ram align ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/romstage.c:
https://review.coreboot.org/c/coreboot/+/35029/1/src/soc/intel/cannonlake/ro... PS1, Line 162: top_of_ram = ALIGN_DOWN((uintptr_t)cbmem_top(), top_of_ram_size);
So.. if cbmem_top() was at 31 MiB, your WB cache region now covers 0 MiB to 16MiB ? […]
It does what the comment says, at least 8 MiB below, at most 8 MiB above cbmem_top() will be WB. The alignment is to 8 MiB but size is 16 MiB. And this approach is only allowed because postcar_frame_add_mtrr() will split this to multiple MTRRs when necessary.
That fsp1_1/car.c is once again a good sample where figuring out whether alignment requirements are met is difficult, because cbmem_top() implementations live in completely different directories. And it is the same platform_enter_postcar() for multiple different implementations of cbmem_top() too. That's why the approach of just having CBMEM and TSEG covered *somewhere* within the WB region would be so nice and easy.