Patrick Rudolph 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 1:
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.
I tried to follow the discussion, but I don't get where the CAR_NEM is being used. The choice only consists of FSP-T and CAR_NEM_ENHANCED, doesn't it?
Also enhanced NEM needs documentation. Google doesn't give any results for that and I don't understand the Kconfig help text.