Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36136 )
Change subject: soc/intel: skl,cnl,icl: rely on TOLUM as cbmem_top returned by FSP ......................................................................
Patch Set 13:
please don't make this change, this is exactly opposite the direction what we decided in past that we shouldn't relying much on FSP in memory snap shot creation and make design where FSP and bootloader have transparent memory design. please look at original CL when it got merged.
What is trying to be achieved here is reducing the coupling between coreboot and FSP for the memory layout. The original patches were attempting to closely track FSP's behavior.
I think below assumption doesn't hold good for CNL onwards platform, thats the reason we had decided to drop the FSP dependency in memory layout design
/* Lookup the FSP_BOOTLOADER_TOLUM_HOB */ if (fsp_find_bootloader_tolum(&tolum)) die("9.3: FSP_BOOTLOADER_TOLUM_HOB missing!\n");
Could you please elaborate on what doesn't hold? Why does this not work?
its 3 year old discussion when we had seen issue with consuming FSP hob to cbmem memory top. Because FSP TOLUM base calculation was different in SKL and CNL (BOOTLOADER TOLUM base and FSP Reserve memory base order) if i'm not wrong.
Top of that Aaron, its you who took stand that why do i relying on FSP to tell me what is my memory start and CB is master not FSP. Then we started looking at transparent memory layout design and come up with this CLs. Now if you are okay with original idea then from SOC side we don't have much objection except a request to don't break other platform with just SKL assumptions.
I recall, Subrata. I still hold the opinion (and wrote in a comment elsewhere) that I prefer to understand and know the memory map. However, I think I'm in the minority on this, and people are wanting to just rely upon what FSP spits out. If it allows people to utilize coreboot more in many different environments and the memory map tracking is preventing that then I'm ok with this approach. I do think we lose visibility and understanding by going this route, though.
Just want to say some words why I support Michael's changes. I was confused originally, when I learned that our cbmem_top() calculation just mimics FSP calculations. Back then, we didn't know any spare scratchpad register nor did we want to use low memory, so I assumed it was the only solution. And it worked for a while. But now and then we discovered that our calculations didn't match FSP's in some corner cases, that was fixed... now it's 2019 and it seems we still don't even got Skylake right. I assume mostly because nobody can review the changes due to lack of documentation and unreadable reference code. Well, that's what we get when we try to maintain an inter- face that doesn't exist.
I agree that there is benefit in having the calculations in coreboot. And honestly, I like it when some coding pattern forces people to leak infor- mation :) But looking at all the working hours wasted to try to keep core- boot's memmaps in sync with FSP, all the reviewing and bikeshedding, the bug hunting when something wasn't perfectly aligned with FSP... I think we just failed. We have to embrace easier, less expensive solutions if we want coreboot to flourish.
So why not try Michaels approach. At least to have a reliable code base for once. If we discover that we really miss some information later (something not easily read from a register like TSEG base or from a HOB like PRMRR base), we'll have all we need in the Git history.
About FSP_BOOTLOADER_TOLUM_HOB, if that fails with newer FSP binaries, well, then that's a violation of the FSP spec. I don't say we shouldn't test these patches. But if we see that it fails, the binaries will have to be fixed. And I wonder, if anybody really discovered a related bug already, why wasn't that reported/fixed?