Shaunak Saha has posted comments on this change. ( https://review.coreboot.org/19349 )
Change subject: [WIP]soc/intel/common/block: Add Intel PMC support
......................................................................
Patch Set 4:
(13 comments)
https://review.coreboot.org/#/c/19349/1//COMMIT_MSG
Commit Message:
PS1, Line 7: [WIP]
> remove this
Done
https://review.coreboot.org/#/c/19349/2/src/soc/intel/common/block/include/…
File src/soc/intel/common/block/include/intelblocks/pmc.h:
PS2, Line 4: * Copyright 2017 Intel Corporation.
> i guess one (C) i missing
Done
PS2, Line 27: void pmc_enable_pm1_control(uint32_t mask);
: void pmc_disable_pm1_control(uint32_t mask);
> can we have single function with bool argument as enable/disable
Will fix.
https://review.coreboot.org/#/c/19349/3/src/soc/intel/common/block/include/…
File src/soc/intel/common/block/include/intelblocks/pmc.h:
Line 18:
> #include the headers that provide the types you use in this header.
Done
Line 19: /* SMI */
> Provide some documentation for these functions. If they are returning a val
Done
Line 36: void pmc_enable_gpe(uint32_t mask);
> something to think about. GPE block is typically longer than a 32-bit value
Agreed. Will try to name these in more meaningful way and add comments.
https://review.coreboot.org/#/c/19349/1/src/soc/intel/common/block/pmc/Kcon…
File src/soc/intel/common/block/pmc/Kconfig:
PS1, Line 4: PMC
> please describe what PMC stands for
Done
https://review.coreboot.org/#/c/19349/1/src/soc/intel/common/block/pmc/pmc.c
File src/soc/intel/common/block/pmc/pmc.c:
PS1, Line 31:
> use tabs
Done
https://review.coreboot.org/#/c/19349/3/src/soc/intel/common/block/pmc/pmc.c
File src/soc/intel/common/block/pmc/pmc.c:
Line 44: }
> This whole file is not consistent with the proper indention of using tabs.
Done
PS3, Line 49: #define SMI_STS_DEF
: #include <soc/bit_def.h>
> What is this? Just provide a function that provides the array and its size.
OK. Will implement the function way. I did it this way so that the arrays are replaced by proper SOC bit defines during compile time.
Line 85: sts |= (1 << FAKE_PM1_SMI_STS);
> This is specific to apollolake's brokenness. It shouldn't be in "common" wi
Agreed. Will fix.
PS3, Line 140: #define PM1_STS_DEF
: #include <soc/bit_def.h>
> same as above
Done
PS3, Line 232: #define GPE_STS_DEF
: #include <soc/bit_def.h>
> ditto
Done
--
To view, visit https://review.coreboot.org/19349
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3d96fc23a98c30e8ea0969a7be09d217eeaa889
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Shaunak Saha <shaunak.saha(a)intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik(a)intel.com>
Gerrit-Reviewer: Brandon Breitenstein <brandon.breitenstein(a)intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams(a)intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Shaunak Saha <shaunak.saha(a)intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes
Werner Zeh has submitted this change and it was merged. ( https://review.coreboot.org/19537 )
Change subject: fsp_broadwell_de: Switch CPU to high frequency mode
......................................................................
fsp_broadwell_de: Switch CPU to high frequency mode
According to Yang York the FSP is responsible for switching the CPU into
high frequency mode (HFM). For an unknown reason this is not done for the
BSP on my platform though the APs are switched properly.
This code switches the CPU into HFM which makes sure that all cores are in
high frequency mode before payload is started.
It should not harm the operation even if FSP was successful in switching
to HFM.
Change-Id: I91baf538511747d1692a8b6b359df5c3a8d56848
Signed-off-by: Werner Zeh <werner.zeh(a)siemens.com>
Reviewed-on: https://review.coreboot.org/19537
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Aaron Durbin <adurbin(a)chromium.org>
---
M src/soc/intel/fsp_broadwell_de/cpu.c
M src/soc/intel/fsp_broadwell_de/include/soc/msr.h
2 files changed, 32 insertions(+), 0 deletions(-)
Approvals:
Aaron Durbin: Looks good to me, approved
build bot (Jenkins): Verified
diff --git a/src/soc/intel/fsp_broadwell_de/cpu.c b/src/soc/intel/fsp_broadwell_de/cpu.c
index 9d7fe98..68825a3 100644
--- a/src/soc/intel/fsp_broadwell_de/cpu.c
+++ b/src/soc/intel/fsp_broadwell_de/cpu.c
@@ -65,8 +65,38 @@
*parallel = 1;
}
+static int cpu_config_tdp_levels(void)
+{
+ msr_t platform_info;
+
+ /* Bits 34:33 indicate how many levels are supported. */
+ platform_info = rdmsr(MSR_PLATFORM_INFO);
+ return (platform_info.hi >> 1) & 3;
+}
+
+static void set_max_ratio(void)
+{
+ msr_t msr, perf_ctl;
+
+ perf_ctl.hi = 0;
+
+ /* Check for configurable TDP option. */
+ if (cpu_config_tdp_levels()) {
+ /* Set to nominal TDP ratio. */
+ msr = rdmsr(MSR_CONFIG_TDP_NOMINAL);
+ perf_ctl.lo = (msr.lo & 0xff) << 8;
+ } else {
+ /* Platform Info Bits 15:8 give max ratio. */
+ msr = rdmsr(MSR_PLATFORM_INFO);
+ perf_ctl.lo = msr.lo & 0xff00;
+ }
+ wrmsr(IA32_PERF_CTL, perf_ctl);
+}
+
static void post_mp_init(void)
{
+ /* Set Max Ratio */
+ set_max_ratio();
/* Now that all APs have been relocated as well as the BSP let SMIs
start flowing. */
southbridge_smm_enable_smi();
diff --git a/src/soc/intel/fsp_broadwell_de/include/soc/msr.h b/src/soc/intel/fsp_broadwell_de/include/soc/msr.h
index f5ea34c..6b87061 100644
--- a/src/soc/intel/fsp_broadwell_de/include/soc/msr.h
+++ b/src/soc/intel/fsp_broadwell_de/include/soc/msr.h
@@ -21,10 +21,12 @@
#define MSR_IA32_PLATFORM_ID 0x17
#define MSR_CORE_THREAD_COUNT 0x35
#define MSR_PLATFORM_INFO 0xce
+#define IA32_PERF_CTL 0x199
#define MSR_TURBO_RATIO_LIMIT 0x1ad
#define MSR_IA32_MC0_STATUS 0x400
#define MSR_PKG_POWER_SKU_UNIT 0x606
#define MSR_PKG_POWER_LIMIT 0x610
+#define MSR_CONFIG_TDP_NOMINAL 0x648
#define SMM_MCA_CAP_MSR 0x17d
#define SMM_CPU_SVRSTR_BIT 57
--
To view, visit https://review.coreboot.org/19537
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I91baf538511747d1692a8b6b359df5c3a8d56848
Gerrit-PatchSet: 2
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Reviewer: York Yang <york.yang(a)intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/18946 )
Change subject: cr50: check if the new image needs to be enabled and act on it
......................................................................
Patch Set 9: Code-Review+1
I verified this to work with a 0.0.19 cr50 code on a reef. There were a few conflicts to resolve to cherry pick it into the reef branch, nothing major.
--
To view, visit https://review.coreboot.org/18946
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I45fd6058c03f32ff8edccd56ca2aa5359d9b21b1
Gerrit-PatchSet: 9
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Duncan Laurie <dlaurie(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Vadim Bendebury has posted comments on this change. ( https://review.coreboot.org/19573 )
Change subject: ec/google/chromeec: provide reboot function
......................................................................
Patch Set 3: Code-Review+1
I verified this to work with a 0.0.19 cr50 code on a reef.
--
To view, visit https://review.coreboot.org/19573
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1458bd7119b0df626a043ff3806c15ffb5446c9a
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Aaron Durbin <adurbin(a)chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan(a)google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Vadim Bendebury <vbendeb(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: No
Caesar Wang has posted comments on this change. ( https://review.coreboot.org/19557 )
Change subject: rockchip: rk3399: enable DPLL SSC for DDR EMI test on bob
......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/#/c/19557/3//COMMIT_MSG
Commit Message:
PS3, Line 7: rockchip: rk3399
> Please use `rockchip/rk3399` as prefix.
Done
PS3, Line 9: Modulator(SSMOD)
> Please add a space before the *(*.
Done
https://review.coreboot.org/#/c/19557/3/src/soc/rockchip/rk3399/Kconfig
File src/soc/rockchip/rk3399/Kconfig:
Line 28: bool
> I think it might make sense to make this user configurable, so let's add an
Done
Line 29: default y if BOARD_GOOGLE_BOB
> No board references in this file, please. Instead just have 'default n' her
Okay, done.
https://review.coreboot.org/#/c/19557/3/src/soc/rockchip/rk3399/clock.c
File src/soc/rockchip/rk3399/clock.c:
Line 344: #if (IS_ENABLED(CONFIG_RK3399_SPREAD_SPECTRUM_DDR))
> This function doesn't need to be guarded. The compiler will eliminate it au
Done
PS3, Line 351: /* Need to acquire ~30kHZ which is the target modulation frequency.
: * The modulation frequency ~ 30kHz= OSC_HZ/revdiv/128/divval
: * (the 128 is the number points in the query table).
: */
> Please use the style
done
Line 375: * SSMOD SPREADAMP value 8 appears to mitigate EMI on boards tested.
> nit: Can you say how many Hz of spread "8" will do exactly? (Or if it's dep
done
Line 632: /* configure DPLL SSC */
> nit: slightly pointless comment, the function name already tells you this m
done
Line 633: rkclk_set_dpllssc(&dpll_cfg);
> Please use a C if, not the preprocessor.
done.
I'm worrying about that rkclk_set_dpllssc() put here is too early.
Maybe we need put it in src/soc/rockchip/rk3399/sdram.c.
Since I found the reboot hang sdram_init...
Starting SDRAM initialization...
PLL at 00000000ff760040: fbdiv=116, refdiv=1, postdiv1=3, postdiv2=1, vco=2784000 khz, output=928000 khz
<hang>
..
I try to add the debug log, this issue is gone. that's weird.
@@ -346,12 +346,13 @@ static void rkclk_set_dpllssc(struct pll_div *dpll_cfg)
{
u32 divval;
+ printk(BIOS_INFO,"%s:refdiv=%d====\n",__func__, dpll_cfg->refdiv);
/*
* Need to acquire ~30kHZ which is the target modulation frequency.
--
To view, visit https://review.coreboot.org/19557
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I75461d4235bcf55324e6664a1220754e770b4786
Gerrit-PatchSet: 3
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Caesar Wang <wxt(a)rock-chips.com>
Gerrit-Reviewer: Julius Werner <jwerner(a)chromium.org>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: Philip Chen <philipchen(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-HasComments: Yes