Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45794 )
Change subject: soc/intel/common/block/pmc: Add PMC API for low power programming ......................................................................
Patch Set 2: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/pmclib.h:
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... PS2, Line 236: void pmc_ignore_xtal_shutdown(void); Is clocking the same across all SoCs? I'm not sure which platforms need this.
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... PS2, Line 33: config PMC_LOW_POWER_MODE_PROGRAM Do we need this Kconfig option?
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/pmclib.c:
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... PS2, Line 709: write8(pmcbase + PCH_PWRM_ACPI_TMR_CTL, : read8(pmcbase + PCH_PWRM_ACPI_TMR_CTL) | (1 << 1)); This can be a single line:
setbits8(pmcbase + PCH_PWRM_ACPI_TMR_CTL, (1 << 1));
It would be nice to have a macro for (1 << 1), too
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... PS2, Line 717: write32(pmcbase + CPPMVRIC, read32(pmcbase + CPPMVRIC) | XTALSDQDIS); Same here:
setbits32(pmcbase + CPPMVRIC, XTALSDQDIS);
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... PS2, Line 719: // nit: other comments are C-style