Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43408 )
Change subject: mb/google/zork: add dptc support ......................................................................
Patch Set 11:
(7 comments)
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... File src/mainboard/google/zork/variants/baseboard/include/baseboard/acpi/dptc.asl:
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 5: #define DPTC_THERMAL_CONTROL_LIMIT 0x3 : #define DPTC_SUSTAINED_POWER_LIMIT 0x5 : #define DPTC_FAST_PPT_LIMIT 0x6 : #define DPTC_SLOW_PPT_LIMIT 0x7 : #define DPTC_SLOW_PPT_TIME_CONSTANT 0x8 : #define DPTC_PROCHOT_L 0x9 : #define DPTC_VRM_CURRENT_LIMIT 0xb : #define DPTC_VRM_MAXIMUM_CURRENT_LIMIT 0xc : #define DPTC_VRM_SOC_CURRENT_LIMIT 0xe : Are these register offsets? Or actual limit values?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 26: POF1 These are just empty functions. Do you intend to add more changes later?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 33: Sample call What does this mean?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 34: POFX Where does this get called from?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 40: Store(0x07, SSZE) Having comments about what each of these statements is doing would be helpful.
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 42: 4.8w Does this apply to all variants using all types of processors? Picasso, Dali, Pollock?
https://review.coreboot.org/c/coreboot/+/43408/11/src/mainboard/google/zork/... PS11, Line 43: ALIB Shouldn't the call to ALIB be done from SoC code? Ideally, I think we should generate the code using runtime SSDT generator and using mainboard devicetree configs to generate the code.