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 25:
(4 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. […]
This MSR is consistent between family 15h and 17h for these fields, so i left these here intentionally.
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.
I was using clang-format --style=file to help with some of the formatting here. Might need a format rule update?
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 […]
Moved out of the loop, but in order to make this const, i will have to modify the helper function. This function is utilized by a ton of other intel files right now. If I go down that road, i recommend doing it in a separate effort. Let me know if that is ok.
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 26: 0x3E
Shouldn't this be 0x2C?
The next reserved ranged starts at 0x3f. After some internal discussion on a different issue, it was decided to accept the widest interpretation of this section.