HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/22604 )
Change subject: cpu/intel/speedstep: Add Netburst ......................................................................
Patch Set 77:
(24 comments)
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/sp... PS22, Line 45: if (((rdmsr(MSR_EXTENDED_CONFIG).lo >> 27) & 3) == 3) {/*supported
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/sp... PS22, Line 46: and enabled bits */
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/sp... PS22, Line 48: params->slfm = SPEEDSTEP_STATE_FROM_MSR(msr.lo, state_mask);
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/sp... PS22, Line 80: params->turbo = SPEEDSTEP_STATE_FROM_MSR(msr.hi, state_mask);
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/42/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/42/src/cpu/intel/speedstep/sp... PS42, Line 111: /* Merom */
The code is correct but it's easy to make errors with switch statements so I'd add a comment that Me […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 42: (c.x86 == 0xf) ? 0 : 1
just change it to c.x86 == 0x6 or c. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 79: msr_platform_info_supported
the MSR at 0xce is MSR_PLATFORM_INFO
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 81: MSR_FSB_CLOCK_VCC
Please rename MSR_FSB_CLOCK_VCC in this case
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 84: !
use tab
Ack
https://review.coreboot.org/c/coreboot/+/22604/46/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/46/src/cpu/intel/speedstep/sp... PS46, Line 42: ==
!=
Ack
https://review.coreboot.org/c/coreboot/+/22604/54/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/54/src/cpu/intel/speedstep/sp... PS54, Line 110: } /* Merom fallthrough */
do you want me to add "case 0xf" ? (it falls through)
Ack
https://review.coreboot.org/c/coreboot/+/22604/61/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/61/src/cpu/intel/speedstep/sp... PS61, Line 84: !(rdmsr(IA32_MISC_ENABLE).hi & (1 << (38 - 32)))) {
you don't want this on the same indentation level as the code.
Ack
https://review.coreboot.org/c/coreboot/+/22604/61/src/cpu/intel/speedstep/sp... PS61, Line 95: switch (c.x86) { : case 0x6: : switch (c.x86_model) { : case 0xe: : /* Yonah */ : params->min.power = SPEEDSTEP_MIN_POWER_YONAH; : params->max.power = SPEEDSTEP_MAX_POWER_YONAH; : break; : case 0x17: : /* Penryn */ : params->slfm.power = SPEEDSTEP_SLFM_POWER_PENRYN; : params->min.power = SPEEDSTEP_MIN_POWER_PENRYN; : params->max.power = SPEEDSTEP_MAX_POWER_PENRYN; : params->turbo.power = SPEEDSTEP_MAX_POWER_PENRYN; : break; : } /* Merom fallthrough */ : default: : /* Use Merom values by default (as before). */ : params->slfm.power = SPEEDSTEP_SLFM_POWER_MEROM; : params->min.power = SPEEDSTEP_MIN_POWER_MEROM; : params->max.power = SPEEDSTEP_MAX_POWER_MEROM; : params->turbo.power = SPEEDSTEP_MAX_POWER_MEROM; : break; : }
it is likely more readable if you set the default and then override it for select CPU's
Ack
https://review.coreboot.org/c/coreboot/+/22604/64/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/64/src/cpu/intel/speedstep/sp... PS64, Line 84: !(rdmsr(IA32_MISC_ENABLE).hi & (1 << (38 - 32)))) {
Done
Ack
https://review.coreboot.org/c/coreboot/+/22604/66/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/66/src/cpu/intel/speedstep/sp... PS66, Line 102: break;
Hmmm.. I don't like nested 'switch' statements that much. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/67/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/67/src/cpu/intel/speedstep/sp... PS67, Line 118: }
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/22604/67/src/cpu/intel/speedstep/sp... PS67, Line 118: }
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/22604/68/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/68/src/cpu/intel/speedstep/sp... PS68, Line 118: }
please, no spaces at the start of a line
Ack
https://review.coreboot.org/c/coreboot/+/22604/68/src/cpu/intel/speedstep/sp... PS68, Line 118: }
code indent should use tabs where possible
Ack
https://review.coreboot.org/c/coreboot/+/22604/69/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/69/src/cpu/intel/speedstep/sp... PS69, Line 42: u8 msr_extended_config_supported = c.x86 != 0xf;
const bool
Ack
https://review.coreboot.org/c/coreboot/+/22604/70/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/70/src/cpu/intel/speedstep/sp... PS70, Line 113: params->slfm.power = SPEEDSTEP_SLFM_POWER_MEROM;
Wrong indent. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/71/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/71/src/cpu/intel/speedstep/sp... PS71, Line 110: default:
Families other than 0x06 will no longer have these set. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/77/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/77/src/cpu/intel/speedstep/sp... PS77, Line 52: SPEEDSTEP_STATE_FROM_MSR(msr.lo, state_mask);
This now fits in 96 characters.
Ack
https://review.coreboot.org/c/coreboot/+/22604/77/src/cpu/intel/speedstep/sp... PS77, Line 93: if (c.x86 == 6 && c.x86_model == 0xe) {
Why is the if-else statement better than the switch?
Please see CL66
-
HAOUAS Elyes (Code Review)