Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38221 )
Change subject: soc/amd/picasso: Drop forked copy of SMBus source ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Is this patch blocking anything important? I see a big patch chain of clean-up patches that have almost all been merged, so it seems progress is being made here. Why can't the common code be merged first with this as part of the chain?
I had to re-arrange to get them move forwards. I gave this a couple weeks of silence waiting for amd/picasso to be rebased so some of my local work is not avaiable in gerrit.
Marshall, as per the Gerrit guidelines, [1] I have to ask that you please remove your -2.
What part of that are you citing? Marshall has outlined the plan for resolving this and +2'd CB:38222. In his -2 he made a reasonable suggestion to merge the common code first before dropping this code, as to avoid breaking platform development that is underway.
The dependency of amd/picasso on SMBus has not been pinpointed to me. Marshall argued it is needed for IOAPIC-enablement due the PCI dev.fn involved, but this on its own sounds wrong even for amd/stoneyridge where it was forked from. I have not investigated this argument any deeper.
The guidelines also say "Don’t submit patches that you know will break other platforms", though it's murky in this case since Picasso appears to be broken in its current state. If it were orphaned or obsolete hardware then the answer could simply be to remove it entirely. But since it's being actively developed, that is another matter.
I fail to see both the dependency on SMBus messages and any predictable timeline on amd/picasso code merges. What I clearly see, is there will be naming collision between source files that are currently not build-tested and newly introduced common headerfiles. Either my tree or Marshall's tree will get broken. If my tree lands on master first, and Marshall merges his changes without fresh rebase, master will be broken.
While we're citing guidelines, the ad hominem attacks used in some of the later comments definitely fails to meet the threshold for respectful conduct.
Well yes, my frustration came through in those comments, apologies for that. Maybe coreboot leadership instead can discuss with Martin and Marshall, if they have had any intentions of ever sending the board or if it was merely a scheme to get free work done from community side. Reminds me of past promises AMD made to Google about scrubbing amd/stoneyridge after first Chromebooks hit shelves that motivated me to contribute _a lot_ on amd/stoneyridge review. The fault is at AMD policies for sure, but I am pointing at the messengers and I feel very much justified in doing so.
Cleaning up APIs is certainly necessary and Kyösti's efforts here are appreciated. AFAICT nobody is against this patch, the only resistance is due to timing of platform development currently underway.
I gave this a couple weeks of silence waiting for amd/picasso rebase, and got response that the concept of amd/picasso bootblock-verstage-romstage transitions was going to be re-architected. I am actually pleased about this happening, my first impression from the brief comments I received is that after their December decisions they are going to take an approach I had suggested in my review comments earlier, maybe in June already, I did not track down the dates. I can only assess the new situation when their source changes land, so I may be mistaken about this.
Kyosti, you are stepping WAY over the line here. You need to follow the code of conduct and the guidelines for gerrit. If you need to vent, choose a more professional way to do it.
I never "PROMISED" you anything. I don't control AMD. Both Marshall and I are WORKING on getting you a CRB. It's gone through AMD legal and I believe it has been okayed, but with the attitude displayed here, I see no reason that ANYONE would want to work with you in any fashion.
That's all I'm saying here.