Change in coreboot[master]: soc/intel/common/block/pmc: Add PMC API for low power programming
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45794 )
Change subject: soc/intel/common/block/pmc: Add PMC API for low power programming ......................................................................
soc/intel/common/block/pmc: Add PMC API for low power programming
List of changes: 1. Create Kconfig to select pmc low power program by SoC 2. Add API to make ACPI timer disable 3. Add API to ignore XTAL shutdown for SLP_S0# assertion
Change-Id: I017ddc772f02ccba889d316319ab3d5626b80ba5 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/include/intelblocks/pmclib.h M src/soc/intel/common/block/pmc/Kconfig M src/soc/intel/common/block/pmc/pmclib.c 3 files changed, 29 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/45794/1
diff --git a/src/soc/intel/common/block/include/intelblocks/pmclib.h b/src/soc/intel/common/block/include/intelblocks/pmclib.h index a339fb1..fa063f2 100644 --- a/src/soc/intel/common/block/include/intelblocks/pmclib.h +++ b/src/soc/intel/common/block/include/intelblocks/pmclib.h @@ -229,4 +229,10 @@ uint8_t get_pm_pwr_cyc_dur(uint8_t slp_s4_min_assert, uint8_t slp_s3_min_assert, uint8_t slp_a_min_assert, uint8_t pm_pwr_cyc_dur);
+/* Disabling ACPI PM timer to ensure switches off TCO and necessary of XTAL OSC shutdown */ +void pmc_disable_acpi_timer(void); + +/* Disable XTAL shutdown qualification for low power idle. */ +void pmc_ignore_xtal_shutdown(void); + #endif /* SOC_INTEL_COMMON_BLOCK_PMCLIB_H */ diff --git a/src/soc/intel/common/block/pmc/Kconfig b/src/soc/intel/common/block/pmc/Kconfig index 3aa0da8..ce41b23 100644 --- a/src/soc/intel/common/block/pmc/Kconfig +++ b/src/soc/intel/common/block/pmc/Kconfig @@ -29,3 +29,9 @@ and lock register is located under PMC BASE at offset ETR. Note that the reset register is still at 0xCF9 this only controls the enable and lock feature. + +config PMC_LOW_POWER_MODE_PROGRAM + bool + help + Enable this for PMC devices to perform registers programming + to ensure low power in active idle scenario. diff --git a/src/soc/intel/common/block/pmc/pmclib.c b/src/soc/intel/common/block/pmc/pmclib.c index ad9c4fe..6e9a1f2 100644 --- a/src/soc/intel/common/block/pmc/pmclib.c +++ b/src/soc/intel/common/block/pmc/pmclib.c @@ -700,3 +700,20 @@
return PCH_PM_PWR_CYC_DUR; } + +#if CONFIG(PMC_LOW_POWER_MODE_PROGRAM) +void pmc_disable_acpi_timer(void) +{ + uint8_t *pmcbase = pmc_mmio_regs(); + + write8(pmcbase + PCH_PWRM_ACPI_TMR_CTL, + read8(pmcbase + PCH_PWRM_ACPI_TMR_CTL) | (1 << 1)); +} + +void pmc_ignore_xtal_shutdown(void) +{ + uint8_t *pmcbase = pmc_mmio_regs(); + + write32(pmcbase + CPPMVRIC, read32(pmcbase + CPPMVRIC) | XTALSDQDIS); +} +#endif // PMC_LOW_POWER_MODE_PROGRAM
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
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
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45794
to look at the new patch set (#3).
Change subject: soc/intel/common/block/pmc: Add PMC API for low power programming ......................................................................
soc/intel/common/block/pmc: Add PMC API for low power programming
List of changes: 1. Create Kconfig to select pmc low power program by SoC 2. Add API to make ACPI timer disable 3. Add API to ignore XTAL shutdown for SLP_S0# assertion
Change-Id: I017ddc772f02ccba889d316319ab3d5626b80ba5 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/include/intelblocks/pmclib.h M src/soc/intel/common/block/pmc/Kconfig M src/soc/intel/common/block/pmc/pmclib.c 3 files changed, 28 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/45794/3
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 5:
Angel/Furquan/Tim: request you to review this CL
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 5: Code-Review+2
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 5:
Patch Set 5:
Angel/Furquan/Tim: request you to review this CL
Thanks Angel
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45794 )
Change subject: soc/intel/common/block/pmc: Add PMC API for low power programming ......................................................................
soc/intel/common/block/pmc: Add PMC API for low power programming
List of changes: 1. Create Kconfig to select pmc low power program by SoC 2. Add API to make ACPI timer disable 3. Add API to ignore XTAL shutdown for SLP_S0# assertion
Change-Id: I017ddc772f02ccba889d316319ab3d5626b80ba5 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/45794 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/include/intelblocks/pmclib.h M src/soc/intel/common/block/pmc/Kconfig M src/soc/intel/common/block/pmc/pmclib.c 3 files changed, 28 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/common/block/include/intelblocks/pmclib.h b/src/soc/intel/common/block/include/intelblocks/pmclib.h index a339fb1..fa063f2 100644 --- a/src/soc/intel/common/block/include/intelblocks/pmclib.h +++ b/src/soc/intel/common/block/include/intelblocks/pmclib.h @@ -229,4 +229,10 @@ uint8_t get_pm_pwr_cyc_dur(uint8_t slp_s4_min_assert, uint8_t slp_s3_min_assert, uint8_t slp_a_min_assert, uint8_t pm_pwr_cyc_dur);
+/* Disabling ACPI PM timer to ensure switches off TCO and necessary of XTAL OSC shutdown */ +void pmc_disable_acpi_timer(void); + +/* Disable XTAL shutdown qualification for low power idle. */ +void pmc_ignore_xtal_shutdown(void); + #endif /* SOC_INTEL_COMMON_BLOCK_PMCLIB_H */ diff --git a/src/soc/intel/common/block/pmc/Kconfig b/src/soc/intel/common/block/pmc/Kconfig index 3aa0da8..ce41b23 100644 --- a/src/soc/intel/common/block/pmc/Kconfig +++ b/src/soc/intel/common/block/pmc/Kconfig @@ -29,3 +29,9 @@ and lock register is located under PMC BASE at offset ETR. Note that the reset register is still at 0xCF9 this only controls the enable and lock feature. + +config PMC_LOW_POWER_MODE_PROGRAM + bool + help + Enable this for PMC devices to perform registers programming + to ensure low power in active idle scenario. diff --git a/src/soc/intel/common/block/pmc/pmclib.c b/src/soc/intel/common/block/pmc/pmclib.c index ad9c4fe..8825bbe 100644 --- a/src/soc/intel/common/block/pmc/pmclib.c +++ b/src/soc/intel/common/block/pmc/pmclib.c @@ -700,3 +700,19 @@
return PCH_PM_PWR_CYC_DUR; } + +#if CONFIG(PMC_LOW_POWER_MODE_PROGRAM) +void pmc_disable_acpi_timer(void) +{ + uint8_t *pmcbase = pmc_mmio_regs(); + + setbits8(pmcbase + PCH_PWRM_ACPI_TMR_CTL, ACPI_TIM_DIS); +} + +void pmc_ignore_xtal_shutdown(void) +{ + uint8_t *pmcbase = pmc_mmio_regs(); + + setbits8(pmcbase + CPPMVRIC, XTALSDQDIS); +} +#endif /* PMC_LOW_POWER_MODE_PROGRAM */
Michael Niewöhner 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. : this is a lie
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
this is a lie
sorry didn't get you. system won't enter into s0ix if we don't bypass XTAL shutdown also keeping PCH PM timer enable has higher power penalty
Michael Niewöhner 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
sorry didn't get you. […]
I should have commented way clearer :-) actually this Kconfig does not cause code to be run but to be built and linked.
Ack on the XTAL and PM timer, however, fsp does parts of that already. I'm working on some changes for that, but it'll take some ... hours maybe or a bit less
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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
I should have commented way clearer :-)
no worries. 😊
actually this Kconfig does not cause code to be run but to be built and linked.
yup, agree.
Ack on the XTAL and PM timer, however, fsp does parts of that already. I'm working on some changes for that, but it'll take some ... hours maybe or a bit less
Yes, that was chain effect that we asked FSP to ensure they have those programming as well as part of FSP as well. But i would like to keep things in more control in coreboot because in some platform if FSP fails to do so, coreboot still has means to handle it. i meant for early SoC platform with newer FSP
Michael Niewöhner 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
I should have commented way clearer :-) […]
well, that's imo not the right way. either let fsp do or cb but not both. I'd appreciate the latter
Nico Huber 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
if FSP fails to do so, coreboot still has means to handle it. i meant for early SoC platform with newer FSP
Sorry to jump in here so rudely, but I have to say this:
Expecting FSP to fail is no excuse to make a mess in coreboot.
If you want a library of things coreboot could do "for early SoC platform with newer FSP", please do it outside of the production code. It's ok to have that in the coreboot tree, IMO, but why clutter all of coreboot just to be prepared for a new, faulty, FSP? It probably costs the coreboot community hundreds of thousands of dollars (plus the same in spare time of volunteers) to maintain this mess.
And the XTAL topic is a huge mess: The commit that introduced it is not clear about the effects of the change (e.g. it tested things after the commit, but doesn't mention what was before). We have in the past weeks spend days(!), accumulated, to figure out the details just to eventually find out that the bit is always set by FSP unconditionally. Such things need to be documented!!!! please start with the explanation why the bit (XTALSDQDIS) doesn't default to 1 after reset. Why does it exist? What are the potential downside of setting it?
Also, why does the bit need options in coreboot if it can be set uncon- ditionally?
If Intel doesn't have the resources for proper documentation, please keep coreboot as small and dull as possible! No unnecessary options, please, no unnecessary `if`, especially no `#if`.
Michael Niewöhner 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... File src/soc/intel/common/block/pmc/Kconfig:
https://review.coreboot.org/c/coreboot/+/45794/6/src/soc/intel/common/block/... PS6, Line 36: Enable this for PMC devices to perform registers programming : to ensure low power in active idle scenario. :
if FSP fails to do so, coreboot still has means to handle it. […]
see https://review.coreboot.org/q/topic:%22intel_pm_acpi_timer%22
-
Angel Pons (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Subrata Banik (Code Review)