Subrata Banik 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:
Patch Set 2:
(1 comment)
Patch Set 1:
Patch Set 1:
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 ? what if i change that order? how do one know that order is important, we have seen some issue in past where NEM alone is not enough for small LLC cache hence we added enhanced NEM option.
Hi Subrata,
I did not know Kconfig choices default to the first option in them, and this is why I asked before giving a +2. I know, it may not look obvious, so maybe it's worth debating if we should explicitly state default values for choices, but I would discuss this somewhere else: this is a Kconfig style question, and if we agree on a rule we would need to change all the Kconfig choices in a new CL anyway.
Maybe it is a good idea to bring up the topic on the mailing list?
I'm totally okay to remove FSP_CAR support if no one is using. i was just advocating that ppl should know NEM_ENHANCE is something what should be default, as nico said if its first choice then automatically NEM_ENHANCE be default.
My -2 is just for commit msg. i guess we all know what needs to write when we think something been added in past and not required now. Do i really need to be that explicit ?