Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29669 )
Change subject: cpu/intel/sandybridge: Add `hyper_threading` option ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/K... File src/cpu/intel/model_206ax/Kconfig:
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/K... PS8, Line 39: CPU has support for Hyper-Threading This option is seriously confusing. You want to select this only if you want to control HT with a CMOS option.
This also depends on CMOS settings.
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/a... File src/cpu/intel/model_206ax/acpi.c:
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/a... PS8, Line 29: msr_t msr; Shouldn't this be on a separate patch?
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/b... File src/cpu/intel/model_206ax/bootblock.c:
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/b... PS8, Line 55: /* See if flex ratio is already set to nominal TDP ratio */ Why is this gone?
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/b... PS8, Line 18: #include <arch/io.h> Maybe move these conditional includes below, to distinguish between unconditional and conditional inclusions?
https://review.coreboot.org/c/coreboot/+/29669/8/src/cpu/intel/model_206ax/b... PS8, Line 79: Bit 0 is ht disable I'm not sure of what read_option returns if it can't find cmos options. If this is wired so that it defaults to HT enable, then the preprocessor can be dropped.
https://review.coreboot.org/c/coreboot/+/29669/8/src/mainboard/hp/2570p/Kcon... File src/mainboard/hp/2570p/Kconfig:
https://review.coreboot.org/c/coreboot/+/29669/8/src/mainboard/hp/2570p/Kcon... PS8, Line 27: select CPU_INTEL_HT Why do you add a prompt to this Kconfig option, if it then gets selected anyway?
Note that this will result in weird things if not using cmos for settings.
-
Angel Pons (Code Review)