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 19:
(5 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 17: 0xFF
This should be 0x3. It is a 2-bit field. (Reference: doc#25481, rev 2. […]
I found this one in the PPR to be [15:8] doc# 55570.
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7
Looking at the way this is being used, this should be 8. […]
In the PPR, which i used for a lot of this code cur [2:0] in PstateCtrl. I inferred max state being 7 from this.
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 52: #define PSTATE_DEF_HI_ENABLE_SHIFT 31 : #define PSTATE_DEF_HI_ENABLE_MASK (0x1 << PSTATE_DEF_HI_ENABLE_SHIFT) : #define PSTATE_DEF_LO_CUR_DIV_SHIFT 30 : #define PSTATE_DEF_LO_CUR_DIV_MASK (0x3 << PSTATE_DEF_LO_CUR_DIV_SHIFT) : #define PSTATE_DEF_LO_CUR_VAL_SHIFT 22 : #define PSTATE_DEF_LO_CUR_VAL_MASK (0xFF << PSTATE_DEF_LO_CUR_VAL_SHIFT) : #define PSTATE_DEF_LO_CORE_VID_SHIFT 14 : #define PSTATE_DEF_LO_CORE_VID_MASK (0xFF << PSTATE_DEF_LO_CORE_VID_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_SHIFT 8 : #define PSTATE_DEF_LO_FREQ_DIV_MASK (0x3F << PSTATE_DEF_LO_FREQ_DIV_SHIFT) : #define PSTATE_DEF_LO_FREQ_DIV_MIN 0x8 : #define PSTATE_DEF_LO_FREQ_DIV_MAX 0x3E : #define PSTATE_DEF_LO_FREQ_MUL_SHIFT 0 : #define PSTATE_DEF_LO_FREQ_MUL_MASK (0xFF << PSTATE_DEF_LO_FREQ_MUL_SHIFT) : #define PSTATE_DEF_LO_CORE_FREQ_BASE 25
Based on what Marshall mentioned on the other patchset, this seems to be specific to Family 17h. […]
Thanks, that makes sense to me, i'll move it over.
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 69: SERIAL_VID_DECODE_VALUE
probably say _MICROVOLTS to indicate the unit.
Sure.
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
I am looking at System Programmer's Manual Volume 2 Figure 17-1 (https://www.amd. […]
I think it's just a miss in the doc. The definitions I have been using have been out of the PPR. In https://devhub.amd.com/download/processor-programming-reference-ppr-for-amd-... it has the definition that i used.