Marshall Dawson 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 11:
(5 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:
I will submit cleanup in a follow up review and set them all to 2 spaces.
Since you need to open this patch up one more time, can we go ahead and get that changed?
https://review.coreboot.org/c/coreboot/+/45340/10/src/include/cpu/amd/msr.h@... PS10, Line 66: CORE_VOLTAGE_INCREMENT_UV
msr?
No, it's a value to translate between serial VID codes and actual voltage. Jason, maybe go with a different name? But also, I'd add a comment that this is defined in the Serial VID Interface 2.0 spec (#48022, NDA only).
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); :
Why are these all declared at the top? I don't think that is required.
I agree. I'd like to see them come out.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 259: for
Isn't all of this logic common to AMD platforms? Can this be put under soc/amd/common or some other […]
I'm getting ready to throw a temper tantrum! No, I just checked the PPR against the Family 15h BKDG and the registers have been redefined...
It's worth commonizing IMO but we'll want some additional smarts in the source to know what Family we're on (and without needing to wait for cpuid). Maybe that's good prepwork for the next device.
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?