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 10:
(7 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 34: 7 Isn't this 16 since the field is 4 bits wide?
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/Kconfig... PS7, Line 345: AMD recommends the ACPI _PSD object to be configured to cause : cores to transition between p-states independently. A vendor may : choose to generate _PSD object to allow cores to transition together. Please align the help text similar to other configs i.e. 1 tab and 2 spaces.
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.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 256: 0x3 Define a macro for this. Also, this is 4-bit field.
https://review.coreboot.org/c/coreboot/+/45340/10/src/soc/amd/picasso/acpi.c... PS10, Line 257: : space around :
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 amd common directory?
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?