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:
(41 comments)
https://review.coreboot.org/c/coreboot/+/22604/60//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/22604/60//COMMIT_MSG@9 PS60, Line 9: CPUs Netburst CPUs hangs
Netburst CPUs hang …
Done
https://review.coreboot.org/c/coreboot/+/22604/60//COMMIT_MSG@12 PS60, Line 12:
Tested how?
Done
https://review.coreboot.org/c/coreboot/+/22604/42/src/cpu/intel/common/fsb.c File src/cpu/intel/common/fsb.c:
https://review.coreboot.org/c/coreboot/+/22604/42/src/cpu/intel/common/fsb.c... PS42, Line 45: printk(BIOS_WARNING, : "Warning: No supported FSB frequency. Assuming 200MHz\n");
Ooops lol
Ack
https://review.coreboot.org/c/coreboot/+/22604/42/src/cpu/intel/common/fsb.c... PS42, Line 48: case 0xf: /* Netburst */
Possible switch case/default not preceded by break or fallthrough comment
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/common/fsb.c File src/cpu/intel/common/fsb.c:
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/common/fsb.c... PS44, Line 1: /*
Please do the moving of fetching the FSB in a different patch
Ack
https://review.coreboot.org/c/coreboot/+/22604/4/src/cpu/intel/speedstep/acp... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/4/src/cpu/intel/speedstep/acp... PS4, Line 60: }
Done
Ack
https://review.coreboot.org/c/coreboot/+/22604/6/src/cpu/intel/speedstep/acp... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/6/src/cpu/intel/speedstep/acp... PS6, Line 143: if (!(((cpuid_eax(1) >> 8) & 0xf) == 0xf)) { : num_cstates = get_cst_entries(&cstates); : speedstep_gen_pstates(&pstates);
Try to figure out why those hang instead of simply ommiting creating pstates.
Ack
https://review.coreboot.org/c/coreboot/+/22604/6/src/cpu/intel/speedstep/acp... PS6, Line 166: if (!(((cpuid_eax(1) >> 8) & 0xf) == 0xf)) { : gen_pstate_entries(&pstates, cpuID, : cores_per_package, coordination); : : /* Generate c-state entries. */ : if (num_cstates > 0) : acpigen_write_CST_package( : cstates, num_cstates); : }
same
Ack
https://review.coreboot.org/c/coreboot/+/22604/7/src/cpu/intel/speedstep/acp... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/7/src/cpu/intel/speedstep/acp... PS7, Line 56: (c.x86) == 0xf && (c.x86_model) >= 2
Please create a method for that, named `is_intel_desktop()` (no idea what model greater or equal two […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/ac... PS22, Line 50: get_Netburst_fsb
we normally don't use upper case letters in function names except for abbreviations (DDR, FSB, ... […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/22/src/cpu/intel/speedstep/ac... PS22, Line 63: struct cpuinfo_x86 c;
this is all pretty crowded. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/ac... PS23, Line 87: static int get_fsb(void)
This function is for CPUs by intel. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/32/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/32/src/cpu/intel/speedstep/ac... PS32, Line 87: get_fsb
I'll try, but seems that cpu/x86/lapic/lapic_timer.c get_fsb() will not return the same thing.
Ack
https://review.coreboot.org/c/coreboot/+/22604/47/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/47/src/cpu/intel/speedstep/ac... PS47, Line 57: get_ia32_fsb
also check the return value for negative values.
Ack
https://review.coreboot.org/c/coreboot/+/22604/47/src/cpu/intel/speedstep/ac... PS47, Line 57: 3
I don't like this magic ...
Ack
https://review.coreboot.org/c/coreboot/+/22604/51/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/51/src/cpu/intel/speedstep/ac... PS51, Line 49: */
Copy comment next to new get_ia32_fsb_x3()
Ack
https://review.coreboot.org/c/coreboot/+/22604/51/src/cpu/intel/speedstep/ac... PS51, Line 57: const int fsb3 = 3 * get_ia32_fsb();
Use new get_ia32_fsb_x3() here.
Ack
https://review.coreboot.org/c/coreboot/+/22604/54/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/54/src/cpu/intel/speedstep/ac... PS54, Line 59: printk(BIOS_ERR, "FSB not supported or not found\n");
To retain previous behaviour, use fsb3==600 unless you have arguments to do otherwise.
Ack
https://review.coreboot.org/c/coreboot/+/22604/66/src/cpu/intel/speedstep/ac... File src/cpu/intel/speedstep/acpi.c:
https://review.coreboot.org/c/coreboot/+/22604/66/src/cpu/intel/speedstep/ac... PS66, Line 57: int fsb3 = get_ia32_fsb_x3();
This could be a separate change, with argument MSR_FSB_FREQ was not available with NetBurst?
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... PS23, Line 44: !(((cpuid_eax(1) >> 8) & 0xf) == 0xf)
Ok, I will. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... PS23, Line 45: if (((rdmsr(MSR_EXTENDED_CONFIG).lo >> 27) & 3) == 3) {/*supported
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... PS23, Line 46: and enabled bits */
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... PS23, Line 48: params->slfm = SPEEDSTEP_STATE_FROM_MSR(msr.lo, state_mask);
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... PS23, Line 74: !(((cpuid_eax(1) >> 8) & 0xf) == 0xf)
same here. […]
Ack
https://review.coreboot.org/c/coreboot/+/22604/23/src/cpu/intel/speedstep/sp... PS23, Line 80: params->turbo = SPEEDSTEP_STATE_FROM_MSR(msr.hi, state_mask);
line over 80 characters
Ack
https://review.coreboot.org/c/coreboot/+/22604/32/src/cpu/intel/speedstep/sp... File src/cpu/intel/speedstep/speedstep.c:
https://review.coreboot.org/c/coreboot/+/22604/32/src/cpu/intel/speedstep/sp... PS32, Line 34: const uint16_t cpu_id = (cpuid_eax(1) >> 4) & 0xffff;
The code would look better with 2 variables. x86_model and x86_family.
Ack
https://review.coreboot.org/c/coreboot/+/22604/32/src/cpu/intel/speedstep/sp... PS32, Line 70: cpu_id == 0x006e || cpu_id == 0x00f6
You are right, F4x have the ratio in IA32_PREF_STATUS. […]
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 */
Ack
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
Ack
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 79: msr_platform_info_supported
Ack
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 81: MSR_FSB_CLOCK_VCC
Ack
Ack
https://review.coreboot.org/c/coreboot/+/22604/44/src/cpu/intel/speedstep/sp... PS44, Line 84: !
Ack
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 */
Ack
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)))) {
Ack
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; : }
Ack
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)))) {
Ack
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;
Ack
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;
Ack
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;
Ack
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:
Ack
Ack
https://review.coreboot.org/c/coreboot/+/22604/45/src/cpu/x86/lapic/apic_tim... File src/cpu/x86/lapic/apic_timer.c:
https://review.coreboot.org/c/coreboot/+/22604/45/src/cpu/x86/lapic/apic_tim... PS45, Line 48: car_set_var(g_timer_fsb, get_ia32_fsb()); : return 0;
return 0 if g_timer_fsb is set to a sane value, return -1 else
Ack
-
HAOUAS Elyes (Code Review)