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 24:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/include/cpu/amd/msr.h@... PS24, Line 36: #define PS_LIM_MAX_VAL_MASK (0x7 << PS_MAX_VAL_SHFT) : #define MAX_PSTATES 8 Should these also be added to picasso/.../msr.h?
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 318: .ctype = 1, : .latency = 1, : .power = 0, : .resource = { : .space_id = ACPI_ADDRESS_SPACE_FIXED, : .bit_width = 2, : .bit_offset = 2, : .addrl = 0, : .addrh = 0, : }, : } Needs one less tab.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 330: .ctype = 2, : .latency = 400, : .power = 0, : .resource = { : .space_id = ACPI_ADDRESS_SPACE_IO, : .bit_width = 8, : .bit_offset = 0, : .addrl = 0, /* Address will be initialized later : from MSR_CSTATE_ADDRESS */ : .addrh = 0, : .access_size = ACPI_ACCESS_SIZE_BYTE_ACCESS, : }, : }, Same here.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/acpi.c... PS24, Line 379: cstate_info[1].resource.addrl = : (rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK) + 1; Why does this need to be done in the loop? I believe it is the same for all CPUs? I think you can do this above before cstate_info: uint32_t cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK;
and use cstate_base_address in cstate_info. That will also allow you to make it const.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... File src/soc/amd/picasso/include/soc/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 3: /* This file applies to AMD64 products. : * The definitions come from the AMD64 Programmers Manual vol2 : * Revision 3.30 and/or the device's PPR. This is not really correct. The definitions here are very Family 17h specific and coming from the PPR. That is the reason why they cannot be added to common cpu/amd/msr.h
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 11: #include <cpu/amd/msr.h> Why is this included here? This is required by picasso/acpi.c and should be included directly in that file.
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 26: 0x3E Shouldn't this be 0x2C?
https://review.coreboot.org/c/coreboot/+/45340/24/src/soc/amd/picasso/includ... PS24, Line 32: #define SERIAL_VID_DECODE_MICROVOLTS 6250 : #define SERIAL_VID_MAX_MICROVOLTS 1550000L Is this also Family 17h specific?