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 19:
(10 comments)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c File src/soc/amd/picasso/acpi.c:
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 29: #include <cpu/amd/msr.h>
nit: Can you please put this above in alphabetic order before cpu/x86/smm. […]
NP.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: voltage_in_uvolts
BTW, from PPR 55570 Rev 3.14, section 2.1.6.1.1.3. […]
Agree, they inverted the unit shift direction from micro to mili
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 216: 1550000L
I think it would be good to have a macro for this.
Will do.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 220: 10
Shouldn't this be 1000 to convert the power to mW?
I can factor in the 10*100. I kept this format as I ported over the way Agesa does the calculation.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 231: default
The only other value that divisor can be is 3. PPR does not say anything about it being invalid. […]
I believe so. I had to go back to Stoney BKDG to see the values. 0x11 is a reserved value.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 253: ? 2 : 1
Shouldn't this be: […]
I can change it to that. i took some liberty, knowing each core can only run in 1T or 2T.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 260: if (pstate_enable)
nit: If you add a early continue here, then the block below wouldn't have to be aligned an extra tab […]
Thanks. I agree that's easier to read :)
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 342: cstate_info
Same here as mentioned below about perf_ctrl and perf_sts: […]
Will do
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 343: 0
Instead of setting these to 0, can't we just initialize them right away? […]
Will do.
https://review.coreboot.org/c/coreboot/+/45340/19/src/soc/amd/picasso/acpi.c... PS19, Line 381: 2
ARRAY_SIZE(cstate_info)
Sure.