Patch Set 6:
(2 comments)
You already have several TODOs left behind like bootblock-y += cache_as_ram.S
Not in this patch.
Right. I have little reason to be happy about CB:32413 being submitted with the amount of review it had. Sure, there were +2's given... Please file bugs on every TODO you leave behind so they eventually get assigned to someone and you don't forget about them.
MD: We've started capturing ideas for ways to streamline the bootflow once the system is fully functional, and ultimately I'd hope to pull this particular change back out once it's no longer useful.
Which is sort of admitting that this entire change is one big TODO.
The TODOs are indicators of what will require some pretty substantial work, and the intent to do so. I don't mind rewording it, however "todo" is a fairly common search term.
To include car.ld for this concept seems invalid to me. Also, we have ENV_CACHE_AS_RAM that should be adjusted accordingly and you should have CACHE_AS_RAM=n in your .config.
In my opinion, there are many things that are less than ideal with this initial approach. coreboot makes lots of assumptions for how an x86 behaves and they don't fit well with how Family 17h comes out of reset. When challenged with both porting new technology (which intends to rely on reference code that's not yet functional) and into a codebase that has no established paradigm for how it operates...
It has been known since like 2015 that these platforms will start romstage from DRAM. The on-going review will resolve how big a mess you will be allowed to (temporarily) leave behind in common directories until more optimal solution is available.
From what I know, amd/picasso is largely result of pair-programming and as such, I classify Martin acking Marshall's work as self-approval and vice-versa. I would appreciate if you did not submit code to arch/x86 without 'substantial' review votes and defer yourselves from self-approvals. Otherwise it becomes an insult towars the transparency of the review process itself.
> That's how we arrived at this approach, i.e. to make it functional first but with absolute minimal disruption to the non-amd directories. Otherwise, more stuff breaks than you can imagine by only looking at these patches. Once it's fully functional, then the changes required to make picasso a good port will be more understandable.
I am fine with the approach. Just that I don't see the need to modify and include arch/x86/assembly_entry.S in your romstage at all.
The alternative was to modify coreboot first before pushing picasso. However that seems like a much more difficult argument for a unique processor where no reviewers would be allowed to know how it works. I don't think people would be willing to consider those types of x86 patches without understanding the premise.Last time someone approved this from AMD contractors, it took 5 years or more to have them addressed. And it was not AMD contractor who sorted it out.
I try not to get upset over things I can't change.
Things you put in coreboot proper are things you can change. Trouble is when you try land TODOs in common code and they become PITA to everyone else.
To view, visit change 32414. To unsubscribe, or for help writing mail filters, visit settings.