Change in ...coreboot[master]: soc/intel/../../cpu: Remove redundent enable/disable functions
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32282
Change subject: soc/intel/../../cpu: Remove redundent enable/disable functions ......................................................................
soc/intel/../../cpu: Remove redundent enable/disable functions
This patch removes multiple enable/disable function definitions and make use of single function with argument to know feature status (enable/disable).
Change-Id: I502cd2497b07e9de062df453ecbb9c11df692f5a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/cpu.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/icelake/cpu.c 6 files changed, 34 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/32282/1
diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c index 8f1d933..488b02e 100644 --- a/src/soc/intel/apollolake/cpu.c +++ b/src/soc/intel/apollolake/cpu.c @@ -94,10 +94,12 @@ /* Set Max Non-Turbo ratio if RAPL is disabled. */ if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) { cpu_set_p_state_to_max_non_turbo_ratio(); - cpu_disable_eist(); + /* Disable speed step */ + cpu_set_eist(FALSE); } else if (CONFIG(APL_SET_MIN_CLOCK_RATIO)) { cpu_set_p_state_to_min_clock_ratio(); - cpu_disable_eist(); + /* Disable speed step */ + cpu_set_eist(FALSE); } }
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 47fbc0d..e8c27d8 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -197,10 +197,10 @@ }
/* Enable burst mode */ - cpu_enable_burst_mode(); + cpu_burst_mode(TRUE);
/* Enable speed step. */ - cpu_enable_eist(); + cpu_set_eist(TRUE);
/* Set P-State ratio */ cpu_set_p_state_to_turbo_ratio(); diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 7c06d25..01386dd 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -269,10 +269,8 @@ msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ - if (conf && conf->eist_enable) - cpu_enable_eist(); - else - cpu_disable_eist(); + /* Set EIST status */ + cpu_set_eist(conf->eist_enable); wrmsr(IA32_MISC_ENABLE, msr);
/* Disable Thermal interrupts */ diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 9964f2b..b0061cc 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -185,50 +185,36 @@ }
/* - * Enable Burst mode. + * Program CPU Burst mode + * TRUE = Enable Burst mode. + * FALSE = Disable Burst mode. */ -void cpu_enable_burst_mode(void) +void cpu_burst_mode(bool burst_mode_status) { msr_t msr;
msr = rdmsr(IA32_MISC_ENABLE); - msr.hi &= ~BURST_MODE_DISABLE; + if (burst_mode_status) + msr.hi &= ~BURST_MODE_DISABLE; + else + msr.hi |= BURST_MODE_DISABLE; wrmsr(IA32_MISC_ENABLE, msr); }
/* - * Disable Burst mode. + * Program Enhanced Intel Speed Step Technology + * TRUE = Enable EIST. + * FALSE = Disable EIST. */ -void cpu_disable_burst_mode(void) +void cpu_set_eist(bool eist_status) { msr_t msr;
msr = rdmsr(IA32_MISC_ENABLE); - msr.hi |= BURST_MODE_DISABLE; - wrmsr(IA32_MISC_ENABLE, msr); -} - -/* - * Enable Intel Enhanced Speed Step Technology. - */ -void cpu_enable_eist(void) -{ - msr_t msr; - - msr = rdmsr(IA32_MISC_ENABLE); - msr.lo |= (1 << 16); /* Enhanced SpeedStep Enable */ - wrmsr(IA32_MISC_ENABLE, msr); -} - -/* - * Disable Intel Enhanced Speed Step Technology. - */ -void cpu_disable_eist(void) -{ - msr_t msr; - - msr = rdmsr(IA32_MISC_ENABLE); - msr.lo &= ~(1 << 16); /* Enhanced SpeedStep Disable */ + if (eist_status) + msr.lo |= (1 << 16); + else + msr.lo &= ~(1 << 16); wrmsr(IA32_MISC_ENABLE, msr); }
diff --git a/src/soc/intel/common/block/include/intelblocks/cpulib.h b/src/soc/intel/common/block/include/intelblocks/cpulib.h index 5cea96e..23d9909 100644 --- a/src/soc/intel/common/block/include/intelblocks/cpulib.h +++ b/src/soc/intel/common/block/include/intelblocks/cpulib.h @@ -97,24 +97,18 @@ int cpu_get_burst_mode_state(void);
/* - * Enable Burst mode. + * Program CPU Burst mode + * TRUE = Enable Burst mode. + * FALSE = Disable Burst mode. */ -void cpu_enable_burst_mode(void); +void cpu_burst_mode(bool burst_mode_status);
/* - * Disable Burst mode. + * Program Enhanced Intel Speed Step Technology + * TRUE = Enable EIST. + * FALSE = Disable EIST. */ -void cpu_disable_burst_mode(void); - -/* - * Enable Intel Enhanced Speed Step Technology. - */ -void cpu_enable_eist(void); - -/* - * Disable Intel Enhanced Speed Step Technology. - */ -void cpu_disable_eist(void); +void cpu_set_eist(bool eist_status);
/* * Set Bit 6 (ENABLE_IA_UNTRUSTED_MODE) of MSR 0x120 diff --git a/src/soc/intel/icelake/cpu.c b/src/soc/intel/icelake/cpu.c index f4ebacf..62bcff6 100644 --- a/src/soc/intel/icelake/cpu.c +++ b/src/soc/intel/icelake/cpu.c @@ -73,10 +73,8 @@ msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ - if (conf->eist_enable) - cpu_enable_eist(); - else - cpu_disable_eist(); + /* Set EIST status */ + cpu_set_eist(conf->eist_enable); wrmsr(IA32_MISC_ENABLE, msr);
/* Disable Thermal interrupts */
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/../../cpu: Remove redundent enable/disable functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32282/1/src/soc/intel/apollolake/cpu.c File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/#/c/32282/1/src/soc/intel/apollolake/cpu.c@98 PS1, Line 98: FALSE we tend to use the lower case version. (see also elsewhere in this patch)
Hello Aaron Durbin, Patrick Rudolph, Aamir Bohra, Rizwan Qureshi, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32282
to look at the new patch set (#2).
Change subject: soc/intel/../../cpu: Remove redundent enable/disable functions ......................................................................
soc/intel/../../cpu: Remove redundent enable/disable functions
This patch removes multiple enable/disable function definitions and make use of single function with argument to know feature status (enable/disable).
Change-Id: I502cd2497b07e9de062df453ecbb9c11df692f5a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/cpu.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/icelake/cpu.c 6 files changed, 34 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/32282/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/../../cpu: Remove redundent enable/disable functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/32282/1/src/soc/intel/apollolake/cpu.c File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/#/c/32282/1/src/soc/intel/apollolake/cpu.c@98 PS1, Line 98: FALSE
we tend to use the lower case version. […]
Done
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/../../cpu: Remove redundent enable/disable functions ......................................................................
Patch Set 2: Code-Review+1
(2 comments)
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/cpu/cpuli... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/cpu/cpuli... PS2, Line 189: TRUE leftover
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/include/i... PS2, Line 101: TRUE leftover
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/../../cpu: Remove redundent enable/disable functions ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/cpu/cpuli... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/cpu/cpuli... PS2, Line 189: TRUE
leftover
Done
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/#/c/32282/2/src/soc/intel/common/block/include/i... PS2, Line 101: TRUE
leftover
Done
Hello Aaron Durbin, Patrick Rudolph, Aamir Bohra, Rizwan Qureshi, Duncan Laurie, build bot (Jenkins), Lijian Zhao, Patrick Georgi, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32282
to look at the new patch set (#3).
Change subject: soc/intel/cpulib: Remove redundent enable/disable functions ......................................................................
soc/intel/cpulib: Remove redundent enable/disable functions
This patch removes multiple enable/disable function definitions and make use of single function with argument to know feature status (enable/disable).
Change-Id: I502cd2497b07e9de062df453ecbb9c11df692f5a Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/cpu.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/icelake/cpu.c 6 files changed, 34 insertions(+), 56 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/32282/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/cpulib: Remove redundent enable/disable functions ......................................................................
Patch Set 3: Code-Review+1
Lijian Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/cpulib: Remove redundent enable/disable functions ......................................................................
Patch Set 3: Code-Review+2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/cpulib: Remove redundent enable/disable functions ......................................................................
Patch Set 3: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32282 )
Change subject: soc/intel/cpulib: Remove redundent enable/disable functions ......................................................................
soc/intel/cpulib: Remove redundent enable/disable functions
This patch removes multiple enable/disable function definitions and make use of single function with argument to know feature status (enable/disable).
Change-Id: I502cd2497b07e9de062df453ecbb9c11df692f5a Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/32282 Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Lijian Zhao lijian.zhao@intel.com Reviewed-by: Duncan Laurie dlaurie@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/apollolake/cpu.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/cannonlake/cpu.c M src/soc/intel/common/block/cpu/cpulib.c M src/soc/intel/common/block/include/intelblocks/cpulib.h M src/soc/intel/icelake/cpu.c 6 files changed, 34 insertions(+), 56 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Lijian Zhao: Looks good to me, approved Arthur Heymans: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c index 8f1d933..11d15e4 100644 --- a/src/soc/intel/apollolake/cpu.c +++ b/src/soc/intel/apollolake/cpu.c @@ -94,10 +94,12 @@ /* Set Max Non-Turbo ratio if RAPL is disabled. */ if (CONFIG(APL_SKIP_SET_POWER_LIMITS)) { cpu_set_p_state_to_max_non_turbo_ratio(); - cpu_disable_eist(); + /* Disable speed step */ + cpu_set_eist(false); } else if (CONFIG(APL_SET_MIN_CLOCK_RATIO)) { cpu_set_p_state_to_min_clock_ratio(); - cpu_disable_eist(); + /* Disable speed step */ + cpu_set_eist(false); } }
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 47fbc0d..c976ac2 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -197,10 +197,10 @@ }
/* Enable burst mode */ - cpu_enable_burst_mode(); + cpu_burst_mode(true);
/* Enable speed step. */ - cpu_enable_eist(); + cpu_set_eist(true);
/* Set P-State ratio */ cpu_set_p_state_to_turbo_ratio(); diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c index 7c06d25..01386dd 100644 --- a/src/soc/intel/cannonlake/cpu.c +++ b/src/soc/intel/cannonlake/cpu.c @@ -269,10 +269,8 @@ msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ - if (conf && conf->eist_enable) - cpu_enable_eist(); - else - cpu_disable_eist(); + /* Set EIST status */ + cpu_set_eist(conf->eist_enable); wrmsr(IA32_MISC_ENABLE, msr);
/* Disable Thermal interrupts */ diff --git a/src/soc/intel/common/block/cpu/cpulib.c b/src/soc/intel/common/block/cpu/cpulib.c index 9964f2b..a7f89ba 100644 --- a/src/soc/intel/common/block/cpu/cpulib.c +++ b/src/soc/intel/common/block/cpu/cpulib.c @@ -185,50 +185,36 @@ }
/* - * Enable Burst mode. + * Program CPU Burst mode + * true = Enable Burst mode. + * false = Disable Burst mode. */ -void cpu_enable_burst_mode(void) +void cpu_burst_mode(bool burst_mode_status) { msr_t msr;
msr = rdmsr(IA32_MISC_ENABLE); - msr.hi &= ~BURST_MODE_DISABLE; + if (burst_mode_status) + msr.hi &= ~BURST_MODE_DISABLE; + else + msr.hi |= BURST_MODE_DISABLE; wrmsr(IA32_MISC_ENABLE, msr); }
/* - * Disable Burst mode. + * Program Enhanced Intel Speed Step Technology + * true = Enable EIST. + * false = Disable EIST. */ -void cpu_disable_burst_mode(void) +void cpu_set_eist(bool eist_status) { msr_t msr;
msr = rdmsr(IA32_MISC_ENABLE); - msr.hi |= BURST_MODE_DISABLE; - wrmsr(IA32_MISC_ENABLE, msr); -} - -/* - * Enable Intel Enhanced Speed Step Technology. - */ -void cpu_enable_eist(void) -{ - msr_t msr; - - msr = rdmsr(IA32_MISC_ENABLE); - msr.lo |= (1 << 16); /* Enhanced SpeedStep Enable */ - wrmsr(IA32_MISC_ENABLE, msr); -} - -/* - * Disable Intel Enhanced Speed Step Technology. - */ -void cpu_disable_eist(void) -{ - msr_t msr; - - msr = rdmsr(IA32_MISC_ENABLE); - msr.lo &= ~(1 << 16); /* Enhanced SpeedStep Disable */ + if (eist_status) + msr.lo |= (1 << 16); + else + msr.lo &= ~(1 << 16); wrmsr(IA32_MISC_ENABLE, msr); }
diff --git a/src/soc/intel/common/block/include/intelblocks/cpulib.h b/src/soc/intel/common/block/include/intelblocks/cpulib.h index 5cea96e..70ad253 100644 --- a/src/soc/intel/common/block/include/intelblocks/cpulib.h +++ b/src/soc/intel/common/block/include/intelblocks/cpulib.h @@ -97,24 +97,18 @@ int cpu_get_burst_mode_state(void);
/* - * Enable Burst mode. + * Program CPU Burst mode + * true = Enable Burst mode. + * false = Disable Burst mode. */ -void cpu_enable_burst_mode(void); +void cpu_burst_mode(bool burst_mode_status);
/* - * Disable Burst mode. + * Program Enhanced Intel Speed Step Technology + * true = Enable EIST. + * false = Disable EIST. */ -void cpu_disable_burst_mode(void); - -/* - * Enable Intel Enhanced Speed Step Technology. - */ -void cpu_enable_eist(void); - -/* - * Disable Intel Enhanced Speed Step Technology. - */ -void cpu_disable_eist(void); +void cpu_set_eist(bool eist_status);
/* * Set Bit 6 (ENABLE_IA_UNTRUSTED_MODE) of MSR 0x120 diff --git a/src/soc/intel/icelake/cpu.c b/src/soc/intel/icelake/cpu.c index f4ebacf..62bcff6 100644 --- a/src/soc/intel/icelake/cpu.c +++ b/src/soc/intel/icelake/cpu.c @@ -73,10 +73,8 @@ msr = rdmsr(IA32_MISC_ENABLE); msr.lo |= (1 << 0); /* Fast String enable */ msr.lo |= (1 << 3); /* TM1/TM2/EMTTM enable */ - if (conf->eist_enable) - cpu_enable_eist(); - else - cpu_disable_eist(); + /* Set EIST status */ + cpu_set_eist(conf->eist_enable); wrmsr(IA32_MISC_ENABLE, msr);
/* Disable Thermal interrupts */
-
Arthur Heymans (Code Review) -
Duncan Laurie (Code Review)
-
Lijian Zhao (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)