Attention is currently required from: YH Lin, Tarun Tuli, Subrata Banik, Paul Menzel, Nick Vaccaro.
Joey Peng has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74727 )
Change subject: soc/intel/alderlake: Add option to disable C1E ......................................................................
Patch Set 7:
(7 comments)
This change is ready for review.
Commit Message:
https://review.coreboot.org/c/coreboot/+/74727/comment/741266b6_b5d18774 PS6, Line 7: mb/google/brya/var/taeko:Disable
Please add a space after the colon (:).
Fixed in relation chain CL.
Patchset:
PS7:
is this really a chromeos specific change? […]
Hi Subrata,
Since taeko has ADL SKUs and RPL SKUs, we would like to keep C1E enabled on ADL SKUs.
From FspSUpd.dsc in rplfsp, I saw that C1E is enabled by default, so I think currently it is enabled on all RPL projects.
File src/mainboard/google/brya/variants/taeko/variant.c:
https://review.coreboot.org/c/coreboot/+/74727/comment/ef4d28ec_dddfebc5 PS6, Line 17: //Disable C1E for RPL CPU
Please add a space.
Fixed in relation chain CL.
https://review.coreboot.org/c/coreboot/+/74727/comment/3ab6e344_9d129f48 PS6, Line 21: Disabling
Disable […]
Fixed in relation chain CL.
https://review.coreboot.org/c/coreboot/+/74727/comment/29a61286_80f7a7ce PS6, Line 23: else if (cpu_id == CPUID_ALDERLAKE_R0) {
else should follow close brace '}' […]
Fixed in relation chain CL.
https://review.coreboot.org/c/coreboot/+/74727/comment/04f191a0_48ddfbd8 PS6, Line 24: config->c1e = 1;
Is there a default value? Should it be set explicitly?
The default value is 1. We added it here to make sure it doesn't set to 0 when it shouldn't.
File src/soc/intel/alderlake/chip.h:
https://review.coreboot.org/c/coreboot/+/74727/comment/3b64ae63_4646ae7d PS6, Line 685: /* Enable or Disable C1E : * Default is set to 1. : * Set to 0 in order to disable C1E : */
Please use the recommended style for multiline comments.
Done
-
Joey Peng (Code Review)