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 7:
(8 comments)
https://review.coreboot.org/c/coreboot/+/45340/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45340/5//COMMIT_MSG@7 PS5, Line 7: /
You can drop the leading slash
Ack
https://review.coreboot.org/c/coreboot/+/45340/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45340/7//COMMIT_MSG@7 PS7, Line 7: soc/amd/picasso
no leading slash. just the relative path.
Ack
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/common/block/in... File src/soc/amd/common/block/include/amdblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/common/block/in... PS7, Line 37: } sw_pstate_values_t;
Can we please not add typedefs?
Ack
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 362: 0x414
This should probably be ACPI_CPU_CONTROL.
Doh! I meant to replace this with the right value, vs what i took from the agesa tables. Changing to Core::X86::Msr::CStateBaseAddr[CstateAddr] + 1
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 376: thread_count
nit, this is threads per core and not total thread count, right?
Ack
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 170: void write_pct_object(void)
I don't think all these functions need to be global (i.e. outside of the compilation unit). […]
Good point. Will make these static.
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 310: acpigen_write_package(0x08);
It seems that the format of the AML could live in helpers while the values would be passed to the li […]
That's fair. I will make these more generic and toss them in with the rest of the helpers in acpigen.c
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 317: 8
sizeof(bytes)
Ack