Change in coreboot[master]: nb/amd/agesa/family14: Use generic allocation functions for northbridge

[Nico Huber (Code Review)]

Attention is currently required from: Michał Żygowski, Kyösti Mälkki, Piotr Król.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52934 )

Change subject: nb/amd/agesa/family14: Use generic allocation functions for northbridge
......................................................................

Patch Set 4:

(1 comment)

Patchset:

PS2:
> > > It hurts me to see how deep in debts this code is and that the
> > > community has to constantly pay interest for it. Please do not
> > > make things worse by making the review harder than necessary.
> > > Every change should be reasoned about in the commit message
> > > before review starts. If reviewers constantly have to ask, why
> > > this and that, that's a waste of time. If the code should be
> > > kept upstream in the long run, unexplained changes and shallow
> > > reviews only add additional debts. If nobody wants to invest
> > > into the code, better make plans to maintain it on another
> > > branch.
>
> We never get so negative feedback in such style. For external entities trying to join coreboot development this would not sound welcoming.

Sorry if this came across too rude. I don't see you as somebody who
still needs to be welcomed, rather as integrated into the community.
The more I know what people can do and the more I respect them, the
more I expect from them. The burden of this code is not your doing,
AFAIK. I did not mean to say that. But it's you asking to keep the
burden around. Such a request should not come in the form of
neglectful commits.
> We would understand part of comment if Michał or 3mdeb would continuously provide low quality code, but AFAIK this is not the case here. We would like to understand and learn by examples how we can avoid such comments in future.

It's not about the code quality. Maybe the quality of the
commit as a whole. Commit messages are very important for long
term maintenance, sometimes more than the current state of the
code. If somebody wonders later why the code does something in
a certain way, the commit message is the first place they'll
look at. It seemed to me that details/reasons about the code
changes were known but withheld from the rest of the community.
That's not nice in such a situation where, I assume, you ask
to keep this platform maintained on the master branch in the
last minute. The burden of this port is very big already,
empty commit messages add to it.
> Also we would appreciate precise explanation about interest community constantly have to pay, since we feel part of community and also would like to stop paying that interest, if possible.

tl;dr This code was never properly integrated into coreboot.
Don't make plans for such code to maintain it for 10 years,
unless you are prepared to pay 10 times.

Looking at the state of the code, it seems the original invest-
ment into the family14 port was not well placed. There are
signs of copy-pasting things until it happened to work all
over the place. This very change removes some sophisticated
coreboot code in favor of code that uses a foreign API for
no reason in a file called `fixme.c`?!? Is it really hard to
see that there are debts? that this port was never properly
integrated into coreboot?

Native coreboot ports are easy to maintain. See Intel ports,
for instance. We still support systems from the late 1990s
without any trouble. In case of these AMD platforms, that
AGESA is used already makes things harder. But the shim code
around it is also not of good quality. Whenever I look into
a mainboard port with AGESA I don't see coreboot code in
the mainboard dir. I see AGESA boilerplate. There's a lot
of things done (copy-pasted) per board that should be handled
in the chipset code.

Long story short, I don't think coreboot ports based on
vendorcode are suited for long-term maintenance on a common
branch. The intention of silicon vendors is to provide some-
thing that works at a particular moment and they'll move on
to the next platform very quickly. That's ok for consumer
products, not so much for embedded ones.

Integrating vendorcode may save some time in the beginning
(maybe 50%?) but from then on maintenance is probably 10
times more expensive compared to what is possible with a
native coreboot port. It's not just you fixing things to
avoid deprecation, it's also a lot of work to review everything.
Plus the occasional fix when something breaks unexpectedly.
When I do bigger changes in coreboot I have to look into
AGESA ports about 20 times more often than into native
coreboot ports. Even though there are fewer of them!

What is missing is an initial investment into a proper
coreboot port. Maybe it can be a well designed AGESA shim.
But a native port would be much better, IMHO. I don't know
AGESA and how hard it is to integrate, but a native port
may even be less expensive. I do understand that this may
look infeasible. But that's mostly because of the poor
quality of existing code. It seems nobody knows what the
code actually has to do on an AMD platform, and AGESA
makes it look like it's a lot. I'd bet it's much easier than it
looks like (writing the code; figuring out what really needs
to be done is harder). If we'd had a high-quality fam14 port,
it would also have been much cheaper to extend it to newer
platforms. It's a vicious circle; without anyone ever
investing into a good foundation, we'll stay busy picking
up the shards of fallen shingles. The sooner we break the
cycle, the less costs we'll have overall.

Also, the poor initial integration causes a deprecation warning
with every other coreboot release. At least it feels like this.
And the deprecation is always only avoided last minute. This
creates stressful situations not only for authors but also
for reviewers. Everybody thinks the debt will be paid back
(removal from the master branch) and then suddenly somebody
steps up to claim interest instead.

(I'm still happy to see old platforms supported. But even
if we stay in the vicious circle, things could be done with
less friction.)
> Last sentence worries me because business-wise it means arbitrary resource requests under the threat of exile.

I have no idea why people fear branches that much. IMHO, they
can help to advance coreboot faster whilst keeping stability
for existing platforms. For code of the given quality, a branch
is much less expensive. Also, there is no exile. If you'd maintain
a stable branch, people would still help you. Maybe even more
happily because there's something stable.

You wouldn't get any big new feature (like vboot mentioned on
the ML) for free. But that's a matter of balancing your own
and others investments into the code base.
> It will effectively cause wiping out any old hardware, even one which is still in wide use.

Like mentioned above, we still support late '90s hardware
without any trouble. It's not `old hardware` that is causing
issues, it's low-quality platform ports. I pity you, that you
build some long-term business on top of low-quality code. But
that's a burden you chose. If you had asked me if this code
can be maintained for 10 years, I'd have said no (or "better
re-write it now" or something like that, which would have been
cheaper in the long run).