Raul Rangel 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 13:
(14 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: Why the two spaces? If you are going to change the spacing, can you do that in a different CL? It makes it hard to see what actually changed.
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?
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.
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
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. Or you could split the function into two.
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.
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.
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.
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 to each other and I don't have to back trace while reading the code.
if (cpu == 0 ) { /* BSP values for _SB.Pxxx */ proc_blk_len = 6; proc_blk_addr = ACPI_GPE0_BLK; } else { /* AP values for _SB.Pxxx */ proc_blk_addr = 0; proc_blk_len = 0; }
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?
acpi_addr_t perf_ctrl = {0}, perf_sts = {0};
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.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 360: &pstate_xpss_values[0] same
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.
https://review.coreboot.org/c/coreboot/+/45340/13/src/soc/amd/picasso/acpi.c... PS13, Line 370: &cstate_info[0] same