Aaron Durbin 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:
(5 comments)
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.
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?
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). Please keep the symbol leaking to a minimum and only expose what is necessary.
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 library. Or am I not understanding how all of this is specific picasso?
https://review.coreboot.org/c/coreboot/+/45340/7/src/soc/amd/picasso/acpi.c@... PS7, Line 317: 8 sizeof(bytes)