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 16:
(25 comments)
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13:
Please only one space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 13:
Why the two spaces? If you are going to change the spacing, can you do that in a different CL? It ma […]
I completely misunderstood previous feedback from Haouas. I have fixed now.
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 15:
also here, only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 18: MC4_MISC0 0x00000413 : #define MC4_MISC1 0xC0000408 : #define MC4_MISC2 0xC0000409 : #define FS_Base 0xC0000100 : #define
please keep using only 1 space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 24: NB_CFG_MSR 0xC001001f : #define FidVidStatus 0xC0010042 : #define MC1_CTL_MASK 0xC0010045 : #define MC4_CTL_MASK 0xC0010048 : #define MSR_INTPEND 0xC0010055 : #define
please keep using only one space here
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 33:
one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 34:
2 white spaces if PS_MAX_VAL_SHIFT is to be used with PS_LIM_REG
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 39:
only 1 space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 45: PS_STS_REG 0xC0010063 : #define PSTATE_0_MSR 0xC0010064 : #define PSTATE_1_MSR 0xC0010065 : #define PSTATE_2_MSR 0xC0010066 : #define PSTATE_3_MSR 0xC0010067 : #define
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 69: MSR_PATCH_LOADER 0xC0010020 : : #define MSR_COFVID_STS 0xC0010071 : #define
only 1
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 75: e OSVW_ID_Length 0xC0010140 : #define OSVW_Status 0xC0010141 : : #define SMM_BASE_MSR 0xC0010111 : #define SMM_ADDR_MSR 0xC0010112 : #define SMM_MASK_MSR 0xC0010113
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/include/cpu/amd/msr.h@... PS13, Line 84: fine CPU_ID_FEATURES_MSR 0xC0011004 : #define CPU_ID_EXT_FEATURES_MSR 0xC0011005 : #define CPU_ID_HYPER_EXT_FEATURES 0xC001100d : #define LOGICAL_CPUS_NUM_MSR 0xC001100d : #define LS_CFG_MSR 0xC0011020 : #define IC_CFG_MSR 0xC0011021 : #define DC_CFG_MSR 0xC0011022 : #define BU_CFG_MSR 0xC0011023 : #define FP_CFG_MSR 0xC0011028 : #define DE_CFG_MSR 0xC0011029 : #define BU_CFG2_MSR 0xC001102A : #define BU_CFG3_MSR 0xC001102B : #define EX_CFG_MSR 0xC001102C : #define LS_CFG2_MSR 0xC001102D : #define IBS_OP_DATA3_MSR 0xC0011037 : #define S3_RESUME_EIP_MSR 0xC00110E0 : : #define M
only one space
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 232: undefined
Should we print an error?
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 239: size_t
Can you add a comment describing the return value.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 239: static size_t get_pstate_info(struct acpi_sw_pstate *pstate_values, : struct acpi_xpss_sw_pstate *pstate_xpss_values, uint32_t *max_pstate, : uint32_t *thread_count) : {
clang-format
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 293: fill_cst_objects
Maybe add a comment that you expect 2 cstate_infos to be passed in. […]
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 298: cstate_base_address = rdmsr(MSR_CSTATE_ADDRESS).lo & MSR_CSTATE_ADDRESS_MASK;
Move this below the first cstate_info, since it's not used until the second.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 335: MAX_PSTATES * sizeof(struct acpi_sw_pstate)
sizeof(pstate_values), or in the declaration you could do `= {{0}};` and avoid calling memset.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 338: max_pstate
Doesn't look like you ever use this.
Removed this hold over from earlier versions of the flow.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 347:
Can you move the proc_blk_len logic into this part of the loop? This way all the logic is right next […]
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 355: perf_ctrl
Can you declare the variables in the loop and avoid the memset in the function? […]
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 358: &pstate_values[0]
Use `pstate_values`, no need to deref.
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 360: &pstate_xpss_values[0]
same
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: &cstate_info[0]
same
Done
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: fill_cst_objects(&cstate_info[0]);
Does this always populate the same values? I would pull it out of the loop.
Done