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 20:
(18 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
Ack. […]
I double checked against 15h BKDG and this is uniform between family 15h and 17h
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 37: 7
Bits 2:0 correspond to states 0 - 7 which makes it a total of 8. […]
Done
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
Thanks, that makes sense to me, i'll move it over.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/include/cpu/amd/msr.h@... PS19, Line 69: SERIAL_VID_DECODE_VALUE
Sure.
Done
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 think it's just a miss in the doc. The definitions I have been using have been out of the PPR. […]
Done
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 259: for
In that case, the macros being added to common cpu/amd/msr.h is not correct. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 29: #include <cpu/amd/msr.h>
NP.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 183: (core_freq_div >= PSTATE_DEF_LO_FREQ_DIV_MIN) : && (core_freq_div <= PSTATE_DEF_LO_FREQ_DIV_MAX
I don't think all values between this min and max are really valid. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: 1550000L
Will do.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: voltage_in_uvolts
Agree, they inverted the unit shift direction from micro to mili
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10
Sorry, I don't understand the comment. […]
I went ahead and moved the 1000 up instead of first dividing by 10 and then by 100, like agesa had it.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default
For Family 17h this cannot be 0x11 as this is a 2-bit field.
Meant to type 11, rather than 0x11. I've changed the default case to 3.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: *thread_count
I don't understand the significance of setting the thread count here. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: ? 2 : 1
I can change it to that. i took some liberty, knowing each core can only run in 1T or 2T.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 260: if (pstate_enable)
Thanks. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 342: cstate_info
Will do
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 343: 0
Will do.
Done
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 381: 2
Sure.
Done