Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34166 )
Change subject: soc/intel/{cnl,icl}: Drop arrogant defaults for CAR implementation ......................................................................
Patch Set 2:
I don't follow. This patch already sets `USE_CANNONLAKE_ CAR_NEM_ENHANCED` as default (just implicitly because the first entry of a choice is always the default). If you want me to add an unnecessary but explicit `default USE_CANNONLAKE_CAR_NEM_ENHANCED`, let me know.
However, in that case I would also like to make it a con- vention. General Kconfig style shouldn't be part of this review. Somebody would have to write a lint test to check for choices without explicit default and we should discuss if that is what we want for the project.
`default USE_CANNONLAKE_CAR_NEM_ENHANCED` this is what i meant just to make sure we are running with pre-validated configuration and noone need to debug some unnecessary hang issue
Adding that line wouldn't change anything. Hence can't make anything sure. Please read what you quoted above, then provide a sound answer.
so you are telling USE_CANNONLAKE_CAR_NEM_ENHANCED would be default choice anyway just because its first option in choice list ?
Yes.
what if i change that order?
Same as if you would change a `default` line, I couldn't stop you.
how do one know that order is important
I would assume that any reviewer giving +2 or -2 is responsible enough to make themselves familiar with the language they are reviewing before doing so. I know there are exceptions, irre- sponsible reviewers that I would rather not have in the Reviewers group. But I was told to play nice with them.
, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Well, I won't pity you. I've been telling that FSP-T integration is a bad idea and causes only friction all along.