John Su has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
mb/google/drallion: Update IA VR TDC current limit
Since Drallion is run CML U platform use baseline CPU and follow b:148912093#comment1.
Change IA TDC value from 58w to 48w.
BUG=b:148912093 BRANCH=None TEST=build coreboot and checked IA_TDC from TAT tool.
Change-Id: I5a80fb4fd0a9bd9bc343c1c0c7b47bb947fca226 Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/drallion/ramstage.c 1 file changed, 22 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/38718/1
diff --git a/src/mainboard/google/drallion/ramstage.c b/src/mainboard/google/drallion/ramstage.c index 3855045..40a0faf 100644 --- a/src/mainboard/google/drallion/ramstage.c +++ b/src/mainboard/google/drallion/ramstage.c @@ -21,11 +21,33 @@ #include <soc/ramstage.h> #include <variant/gpio.h> #include <vendorcode/google/chromeos/chromeos.h> +#include <device/pci_ids.h> +#include <device/pci_ops.h>
#define VPD_KEY_SYSTEM_SERIAL "serial_number" #define VPD_KEY_MAINBOARD_SERIAL "mlb_serial_number" #define VPD_SERIAL_LEN 64
+/* Thermal Design Current current limit. 0-4095 in 1/8A units. 1000 = 125A */ +#define VR_CFG_TDC(i) (uint16_t)((i) * 8) + +void mainboard_silicon_init_params(FSP_S_CONFIG *params) +{ + config_t *cfg = config_of_soc(); + static uint16_t mch_id = 0; + + if (!mch_id) { + struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT); + mch_id = dev ? pci_read_config16(dev, PCI_DEVICE_ID) : 0xffff; + } + /* IA TDC current limit is 48A for baseline U42 and U62 */ + if ((mch_id == PCI_DEVICE_ID_INTEL_CML_ULT) || (mch_id == PCI_DEVICE_ID_INTEL_CML_ULT_6_2)) { + if (cfg->cpu_pl2_4_cfg == baseline) + /* TdcPowerLimit[1] = IA TDC current limit */ + params->TdcPowerLimit[1] = VR_CFG_TDC(48); + } +} + const char *smbios_system_serial_number(void) { static char serial[VPD_SERIAL_LEN];
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 44: if ((mch_id == PCI_DEVICE_ID_INTEL_CML_ULT) || (mch_id == PCI_DEVICE_ID_INTEL_CML_ULT_6_2)) { line over 96 characters
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 36: config_t *cfg = config_of_soc(); please use a function like mainboard_config_vr etc.. to put those in it.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 44: if ((mch_id == PCI_DEVICE_ID_INTEL_CML_ULT) || (mch_id == PCI_DEVICE_ID_INTEL_CML_ULT_6_2)) { You can use !=ULT_2_2.
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 3:
This change is ready for review.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 3: Code-Review+2
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG@10 PS3, Line 10: and follow b:148912093#comment1. follow power team request.
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 3:
(2 comments)
fe
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG@10 PS3, Line 10: and follow b:148912093#comment1.
follow power team request.
I guess what you want to say is Dralion HW design is baseline so need to set TDC to 48A for U42, U62?
https://review.coreboot.org/c/coreboot/+/38718/3/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/3/src/mainboard/google/dralli... PS3, Line 42: params->TdcPowerLimit[1] = VR_CFG_TDC(48); Are you sure TdcEnable is set? Also, you have plt2 set in dev tree already, why do you still need to set TDC? Did you observe the current over Dralion HW design?
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 3:
Add Thejaswani for Dralion
Hello Thejaswani Putta, EricR Lai, Edward O'Callaghan, Kane Chen, Mathew King, Duncan Laurie, Marx Wang, build bot (Jenkins), Jamie Chen, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38718
to look at the new patch set (#4).
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
mb/google/drallion: Update IA VR TDC current limit
Since Drallion is run CML U platform use baseline CPU and follow power team request.
Change IA TDC value from 58w to 48w.
BUG=b:148912093 BRANCH=None TEST=build coreboot and checked IA_TDC from TAT tool.
Change-Id: I5a80fb4fd0a9bd9bc343c1c0c7b47bb947fca226 Signed-off-by: John Su john_su@compal.corp-partner.google.com --- M src/mainboard/google/drallion/ramstage.c 1 file changed, 21 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/18/38718/4
John Su has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 4:
(7 comments)
Patch Set 3:
(2 comments)
fe
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG@10 PS3, Line 10: and follow b:148912093#comment1.
I guess what you want to say is Dralion HW design is baseline so need to set TDC to 48A for U42, U62 […]
Yes.
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 36: config_t *cfg = config_of_soc();
please use a function like mainboard_config_vr etc.. to put those in it.
Done
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 39: if (!mch_id) {
"if" is not require, and remove static above, we only program once. […]
Done
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 44: if ((mch_id == PCI_DEVICE_ID_INTEL_CML_ULT) || (mch_id == PCI_DEVICE_ID_INTEL_CML_ULT_6_2)) {
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 44: if ((mch_id == PCI_DEVICE_ID_INTEL_CML_ULT) || (mch_id == PCI_DEVICE_ID_INTEL_CML_ULT_6_2)) {
You can use !=ULT_2_2.
Done
https://review.coreboot.org/c/coreboot/+/38718/1/src/mainboard/google/dralli... PS1, Line 45: if (cfg->cpu_pl2_4_cfg == baseline)
always true not need. you can remove it.
Done
https://review.coreboot.org/c/coreboot/+/38718/3/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/3/src/mainboard/google/dralli... PS3, Line 42: params->TdcPowerLimit[1] = VR_CFG_TDC(48);
Are you sure TdcEnable is set? […]
I check FSP log. CPU_POWER_MGMT_VR_CONFIG : TdcEnable[0] : 0x1 TdcEnable is set.
Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38718/3//COMMIT_MSG@10 PS3, Line 10: and follow b:148912093#comment1.
Yes.
Pls fix the commit msg. it's not clear. You can probably say. Dralion HW is using baseline config, so this commit sets baseline IA TDC limit for U42/U62 accordingly.
Thanks.
https://review.coreboot.org/c/coreboot/+/38718/3/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/3/src/mainboard/google/dralli... PS3, Line 42: params->TdcPowerLimit[1] = VR_CFG_TDC(48);
I check FSP log. […]
i made some comments in the issue tracker according to your log.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 4: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38718/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38718/4//COMMIT_MSG@7 PS4, Line 7: Update Decrease
https://review.coreboot.org/c/coreboot/+/38718/4//COMMIT_MSG@10 PS4, Line 10: and follow power team request. Please reflow for 75 characters per line.
https://review.coreboot.org/c/coreboot/+/38718/4//COMMIT_MSG@12 PS4, Line 12: 58w 58W
https://review.coreboot.org/c/coreboot/+/38718/4//COMMIT_MSG@12 PS4, Line 12: 48w 48W
https://review.coreboot.org/c/coreboot/+/38718/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/4/src/mainboard/google/dralli... PS4, Line 40: mch_id Do you need this variable? Just put `pci_read_config16()` here.
EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38718/4/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/38718/4/src/mainboard/google/dralli... PS4, Line 40: mch_id
Do you need this variable? Just put `pci_read_config16()` here.
hmm, if we use one time, it's not needed. We can add back if we need it.
John Su has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38718 )
Change subject: mb/google/drallion: Update IA VR TDC current limit ......................................................................
Abandoned
-
build bot (Jenkins) (Code Review) -
EricR Lai (Code Review)
-
John Su (Code Review)
-
Kane Chen (Code Review)
-
Paul Menzel (Code Review)