Jason Glenesk 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 12:
(9 comments)
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 14:
Since you need to open this patch up one more time, can we go ahead and get that changed?
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 66: CORE_VOLTAGE_INCREMENT_UV
No, it's a value to translate between serial VID codes and actual voltage. […]
Done
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 31: static uint32_t get_pstate_core_freq(msr_t pstate_def); : static uint32_t get_pstate_core_power(msr_t pstate_def); : static int get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, int *max_pstate, : uint32_t *thread_count); : static void fill_pct_objects(acpi_addr_t *perf_ctrl, acpi_addr_t *perf_sts); : static void fill_cst_objects(acpi_cstate_t *cstate_info); :
I agree. I'd like to see them come out.
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 256: 0x3
This field is 3 bits [6:4]. I can define a macro for a mask and then shift.
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 257: :
space around :
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 297: perf_sts->bit_width = 0x40;
What about the register address?
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 310: 0
ACPI_ACCESS_SIZE_UNDEFINED
Done
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 322: 1
ACPI_ACCESS_SIZE_BYTE_ACCESS
Done
https://review.coreboot.org/c/coreboot/+/45340/11/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/11/src/soc/amd/picasso/acpi.c... PS11, Line 268: latency
transition_latency?
Done