Marshall Dawson 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:
(7 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
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h File src/include/cpu/amd/msr.h:
https://review.coreboot.org/c/coreboot/+/45340/5/src/include/cpu/amd/msr.h@6... PS5, Line 60: 0x30 There's some relevant discussion in https://review.coreboot.org/c/coreboot/+/45055/4/src/soc/amd/picasso/tsc_fre.... I would tend more toward how the register is said to behave, and less toward what its documented max value is.
It might be useful to implement a single function for determining the frequency defined in a p-state register that can be use throughout.
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/Kconfig File src/soc/amd/picasso/Kconfig:
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/Kconfig... PS5, Line 347: Help text is indented by two spaces following the tab.
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 343: 3 curious why 3. It looks like we're only doing 2.
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.
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?
https://review.coreboot.org/c/coreboot/+/45340/5/src/soc/amd/picasso/acpi.c@... PS5, Line 383: if (cpu == 0) { There would be no harm in making this look like Intel vs. doing the comparison on each loop: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...