[coreboot-gerrit] Change in coreboot[master]: soc/intel: cnl,icl,elh,jsl,tgl: enable C6DRAM

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45417 )

Change subject: soc/intel: cnl,icl,elh,jsl,tgl: enable C6DRAM
......................................................................

Patch Set 6:
> > > What about adding a Kconfig (default n) instead?
> >
> > How about removing the option entirely? We already have
> > enough unused options ;)
>
> Huh? Remove an option just because some ucode breaks it? This is a power-saving option we definitely should *not* remove entirely.

It was unused, hence we would effectively not remove any functionality.
And yes, it's a power-saving option. That means without some business
backup, it will likely never be really tested. People start testing and
say "it boots". But this option has no effect on booting at all. If
you wanted to test this on a single unit with a single ucode revision,
I'd suggest that you use this unit for a few years without making any
more changes just to see if it is stable ;) If it trips once per year
and somebody would sell 1,000 units, that would mean roughly 3 units
trip per day. So really, we don't have the means to test this properly.

If you can find some documentation about when it is safe to use, it
would be a completely different story, IMHO. But until then, why present
an option that would need a warning in the help text?