Subrata Banik 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:
(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.
Starting from SKL i have seen this on all platform (exception is APL)
CLK freq is different might be but disqualification is required as i do see in chrome platform
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?
this register sets not present in APL EDS and PMC driver is common across hence to avoid APL giving error due to register undefined, we need this Kconfig guard in pmclib.c
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: […]
Ack
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: […]
Ack
https://review.coreboot.org/c/coreboot/+/45794/2/src/soc/intel/common/block/... PS2, Line 719: //
nit: other comments are C-style
Ack