Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45340 )
Change subject: soc/amd/picasso: Generate ACPI pstate and cstate objects in cb ......................................................................
Patch Set 19:
(19 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 17: 0xFF This should be 0x3. It is a 2-bit field. (Reference: doc#25481, rev 2.35)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7 Looking at the way this is being used, this should be 8. This macro defines the maximum number of P-states that are supported and it is 0-7 i.e. total 8 (at least for family 17h)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 52: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE 25 Based on what Marshall mentioned on the other patchset, this seems to be specific to Family 17h. In that case, they should not be added to common cpu/amd/msr.h file and instead moved to soc/amd/picasso/include/msr.h
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 69: SERIAL_VID_DECODE_VALUE probably say _MICROVOLTS to indicate the unit.
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 34: 7
max and current are 3 bits [6:4]maxval [2:0] curpstate in 0xC0010061
I am looking at System Programmer's Manual Volume 2 Figure 17-1 (https://www.amd.com/system/files/TechDocs/24593.pdf) where it says: CurPstateLimite bits 3:0 PstateMaxVal bits 7:4
Or am I looking at the wrong thing?
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 259: for
I'm getting ready to throw a temper tantrum! No, I just checked the PPR against the Family 15h BKDG […]
In that case, the macros being added to common cpu/amd/msr.h is not correct. Those should be moved to soc/amd/picasso/include/soc/msr.h
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 29: #include <cpu/amd/msr.h> nit: Can you please put this above in alphabetic order before cpu/x86/smm.h
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 183: (core_freq_div >= PSTATE_DEF_LO_FREQ_DIV_MIN) : && (core_freq_div <= PSTATE_DEF_LO_FREQ_DIV_MAX I don't think all values between this min and max are really valid. Example: I see 0x1B, 0x1D and a lot others marked as reserved?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 186: 200 * core_freq_mul) / (core_freq_div I understand that it's the same, but probably better to represent it like:
(PSTATE_DEF_LO_CORE_FREQ_BASE * core_freq_mul) / (core_freq_div / 8);
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: voltage_in_uvolts BTW, from PPR 55570 Rev 3.14, section 2.1.6.1.1.3.2, it looks like power is calculated as CpuVid[7:0] * IddValue * 1000. But, here you are looking at SVID decode table to actually calculate the voltage. I believe this is just a miss in the PPR?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: 1550000L I think it would be good to have a macro for this.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10 Shouldn't this be 1000 to convert the power to mW?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default The only other value that divisor can be is 3. PPR does not say anything about it being invalid. Again, just a miss in the PPR?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: *thread_count I don't understand the significance of setting the thread count here. It is not really used in this function. So, why can't this be done within the calling function?
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: ? 2 : 1 Shouldn't this be: ((cpuid_ebx(CPUID_EBX_CORE_ID) & CPUID_EBX_THREADS_MASK) >> CPUID_EBX_THREADS_SHIFT) + 1
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 260: if (pstate_enable) nit: If you add a early continue here, then the block below wouldn't have to be aligned an extra tab.
i.e. if (!pstate_enable) continue;
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 342: cstate_info Same here as mentioned below about perf_ctrl and perf_sts:
uint32_t cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK; static const acpi_cstate_t cstate_info[] = { [0] = { .ctype = 1, .latency = 1, .power = 0, .resource = { .space_id = ACPI_ADDRESS_SPACE_FIXED, .bit_width = 2, .bit_offset = 2, .addrl = 0, .addrh = 0, }, }, [1] = { .ctype = 2, .latency = 400, .power = 0, .resource = { .space_id = ACPI_ADDRESS_SPACE_IO, .bit_width = 8, .bit_offset = 0, .addrl = cstate_base_address + 1, .addrh = 0, .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, }, }, };
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 343: 0 Instead of setting these to 0, can't we just initialize them right away?
static const acpi_addr_t perf_ctrl = { .space_id = ACPI_ADDRESS_SPACE_FIXED, .bit_width = 64, .addrl = PS_CTL_REG, };
static const acpi_addr_t perf_sts = { .space_id = ACPI_ADDRESS_SPACE_FIXED, .bit_width = 64, .addrl = PS_STS_REG, };
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 381: 2 ARRAY_SIZE(cstate_info)