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:
(1 comment)
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.
That's not true. It's clearly visible from the log of the comments that you are only looking for reasons not to remove it.
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 ?
You asked me to change it, I did. You are getting even more annoying nevertheless. I'm not sure what to do. I never judge the history of code, never judge the persons who wrote, I only judge the current state of the code. And the current state is insane (for an open-source project). I don't know why you always have to make it personal.
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34166/2//COMMIT_MSG@7 PS2, Line 7: arrogant
Kindly be diplomatic which submitting CL, i don;t know if CB has any COC. Such adjectives can be avoided easily in work environment.
You asked me to find *better* words, so I checked the review of the original change and came to the conclusion that the current defaults were chosen because of arrogance. Then checked a dictionary to make sure that it's a less offensive word. So I changed it.