Change in ...coreboot[master]: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 ...
Sumeet R Pawnikar has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33147
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal Shutdown when S0ix is enabled.
BUG=133345634 BRANCH=None TEST=Verified Thermal Device (B0: D20: F2) TSPM offset 0x1c [LTT (8:0)] value is 0xFA on Hatch.
Change-Id: Ib20fae04080b28c6105e5a187cc5d7a55b48d709 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb 1 file changed, 3 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/33147/1
diff --git a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb index 9d10cac..e5f4651 100644 --- a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb @@ -47,6 +47,9 @@ # Unlock GPIO pads register "PchUnlockGpioPads" = "1"
+ # PCH Trip Temperature in degree C + register "pch_trip_temp" = "75" + register "PchPmSlpS3MinAssert" = "2" # 50ms register "PchPmSlpS4MinAssert" = "1" # 1s register "PchPmSlpSusMinAssert" = "1" # 500ms
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
Why is 75 a good value for this mainboard?
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem place under thermal device enable thermal device
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
place under thermal device […]
Sorry, did not follow your point here. Do you mean to put this below entry here instead of at line 50 as per above code? register "pch_trip_temp" = "75"
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
Sorry, did not follow your point here. […]
correct and enable the thermal device
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
correct […]
Do you see any benefit/advantage of putting here rather than above as global ?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
Do you see any benefit/advantage of putting here rather than above as global ?
It's used to program a register in the corresponding PCI device. If you wouldn't reuse "chip soc/intel/cannonlake" you would have to place it here anyway. Besides that there's no benefit with the current code.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
It's used to program a register in the corresponding PCI device. […]
We are using this register under chip soc/intel/cannonlake. So, I would prefer to use this above (at the same place) compared to here, if there are no other benefit.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 75 degree C ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
We are using this register under chip soc/intel/cannonlake. […]
If the param needs to be moved here, it will need another pci device driver for the thermal device which we do not already add. Not sure if adding a driver for it really gains us much.
Hello Aaron Durbin, Rajat Jain, Paul Fagerburg, Patrick Rudolph, Tim Wawrzynczak, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33147
to look at the new patch set (#2).
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 77 degree C ......................................................................
mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 77 degree C
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal Shutdown when S0ix is enabled.
BUG=133345634 BRANCH=None TEST=Verified Thermal Device (B0: D20: F2) TSPM offset 0x1c [LTT (8:0)] value is 0xFE on Hatch.
Change-Id: Ib20fae04080b28c6105e5a187cc5d7a55b48d709 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/33147/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 77 degree C ......................................................................
Patch Set 2: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 77 degree C ......................................................................
Patch Set 2: Code-Review+2
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/baseboard: Set PCH Thermal Trip value to 77 degree C ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
If the param needs to be moved here, it will need another pci device driver for the thermal device w […]
Done
Hello Aaron Durbin, Rajat Jain, Paul Fagerburg, Patrick Rudolph, Tim Wawrzynczak, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33147
to look at the new patch set (#3).
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal Shutdown when S0ix is enabled.
BUG=133345634 BRANCH=None TEST=Verified Thermal Device (B0: D20: F2) TSPM offset 0x1c [LTT (8:0)] value is 0xFE on Hatch.
Change-Id: Ib20fae04080b28c6105e5a187cc5d7a55b48d709 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/mainboard/google/hatch/variants/hatch/overridetree.cb 2 files changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/33147/3
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 3: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 3:
Build error:
i386-elf-ld.bfd: /cb-build/coreboot-gerrit.0/GOOGLE_ATLAS/ramstage/soc/intel/common/pch/thermal/thermal.o: in function `pch_thermal_configuration': /home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/intel/common/pch/thermal/thermal.c:80: multiple definition of `pch_thermal_configuration'; /cb-build/coreboot-gerrit.0/GOOGLE_ATLAS/ramstage/soc/intel/skylake/thermal.o:/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/intel/skylake/thermal.c:85: first defined here
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 3:
Patch Set 3:
Build error:
i386-elf-ld.bfd: /cb-build/coreboot-gerrit.0/GOOGLE_ATLAS/ramstage/soc/intel/common/pch/thermal/thermal.o: in function `pch_thermal_configuration':/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/intel/common/pch/thermal/thermal.c:80: multiple definition of `pch_thermal_configuration'; /cb-build/coreboot-gerrit.0/GOOGLE_ATLAS/ramstage/soc/intel/skylake/thermal.o:/home/coreboot/slave-root/workspace/coreboot-gerrit/src/soc/intel/skylake/thermal.c:85: first defined here
We have identified this issue, will be submitting new patch set.
Hello Aaron Durbin, Rajat Jain, Paul Fagerburg, Patrick Rudolph, Tim Wawrzynczak, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33147
to look at the new patch set (#4).
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal Shutdown when S0ix is enabled.
BUG=133345634 BRANCH=None TEST=Verified Thermal Device (B0: D20: F2) TSPM offset 0x1c [LTT (8:0)] value is 0xFE on Hatch.
Change-Id: Ib20fae04080b28c6105e5a187cc5d7a55b48d709 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb M src/mainboard/google/hatch/variants/hatch/overridetree.cb 2 files changed, 3 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/33147/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on why we need to enable this device just for LTT programming ? this can be achieved using temp bar. btw, there is no thermal PCI device where ops will work.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
why we need to enable this device just for LTT programming ? this can be achieved using temp bar. […]
wrong, we should never operate on devices that are marked off. Ideally those devices are disabled using FD bits and might not decode anything valid.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
wrong, we should never operate on devices that are marked off. […]
i have replied to your concern already in original CL. It might be still okay to skip PCI enumeration of some devices in BIOS space because kernel has capability to handle pci enumeration. IMHO, bios should only allocated BAR for devices that it plan to use in bios space and if we have any fixed resource guideline in SoC like PMC, ACPI BAR to protect from OS to override.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
i have replied to your concern already in original CL. […]
That is entirely dependent on the kernel that will be booted. Not all kernels are the same or have that ability. I don't think it's correct to assume consistent behavior across different kernels/payloads.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
That is entirely dependent on the kernel that will be booted. […]
"bios should only allocated BAR for devices that it plan to use in bios space ..." yes, that's why it need to be "on", as LTT programming is done in "bios space".
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
"bios should only allocated BAR for devices that it plan to use in bios space ..." yes, that's why it need to be "on", as LTT programming is done in "bios space".
no, thats not the point, LTT is just a register and BIOS for say won't use this thermal device. its not the TPM, Touch or Track pad device that BIOS might need to deal with and provide resources to OS.
LTT programming can even done in bootblock :) for that will i do pci enumeration in bootblock? :)
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
"bios should only allocated BAR for devices that it plan to use in bios space ..." […]
if the bios deals with Touchpad devices (USB devices), it has to deal with thermal devices for sure. BTW: Accessing registers is actually *using* a device.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
if the bios deals with Touchpad devices (USB devices), it has to deal with thermal devices for sure. […]
For bootblock we use tempbar and that does end up adding device specific code to program the BARs. Also, typically it is done that way since there are very few devices which need to be accessed early on before device enumeration is done.
For ramstage, we already have common code to deal with resource allocation and so we do not need to carry device specific code every where to program temporary BARs and setup access. In my opinion, it makes sense to enable the right device in devicetree and let the resource allocator provide the required resources rather than having to calculate a BAR manually.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/4/src/mainboard/google/hatch/... PS4, Line 57: .pch_thermal_trip = 77, Shouldn't this be part of baseboard since it applies to all Hatch variants?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
can u copy the same trip value for all hatch variants ?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77, This seems to apply to all hatch variant, so can you move this to baseboard/devicetree.cb instead?
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77,
This seems to apply to all hatch variant, so can you move this to baseboard/devicetree. […]
Currently here the “common_soc_config” structure has the generic config settings that are done under the "chip soc/intel/cannonlake" in this file “src/mainboard/google/hatch/variants/hatch/overridetree.cb”. Also, there is no common SoC related config changes available in “baseboard/devicetree.cb” file.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77,
Currently here the “common_soc_config” structure has the generic config settings that are done under […]
Oops, my bad. If it's applicable to all hatch variants, then please update helios, kindred, and kohaku as well.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77,
Oops, my bad. […]
Sure, will do that.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77,
Sure, will do that.
You can add this to baseboard/devicetree.cb:
register "common_soc_config.pch_thermal_trip" = "77" and that should take effect for all variants.
One word of caution: If a variant wants to override this value, they will have to use the same format i.e register "ommon_soc_config.pch_thermal_trip" = "xy" instead of putting it under register "common_soc_config". This is because of the way overrides are currently applied in sconfig.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77,
You can add this to baseboard/devicetree.cb: […]
We have done this change and it works fine. Thanks Furquan for this suggestion. For caution, will add below as comment NOTE.
+ # NOTE: if any variant wants to override this value, use the same format + # as register "common_soc_config.pch_thermal_trip" = "value", instead of + # putting it under register "common_soc_config" in overridetree.cb file. + register "common_soc_config.pch_thermal_trip" = "77"
Hello Aaron Durbin, Rajat Jain, Paul Fagerburg, Subrata Banik, Patrick Rudolph, Tim Wawrzynczak, Duncan Laurie, Shelley Chen, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33147
to look at the new patch set (#7).
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal Shutdown when S0ix is enabled.
BUG=133345634 BRANCH=None TEST=Verified Thermal Device (B0: D20: F2) TSPM offset 0x1c [LTT (8:0)] value is 0xFE on Hatch.
Change-Id: Ib20fae04080b28c6105e5a187cc5d7a55b48d709 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb 1 file changed, 6 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/33147/7
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 7: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 7: Code-Review+2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 7: Code-Review+2
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/3/src/mainboard/google/hatch/... PS3, Line 191: on
For bootblock we use tempbar and that does end up adding device specific code to program the BARs. […]
Done
https://review.coreboot.org/c/coreboot/+/33147/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/4/src/mainboard/google/hatch/... PS4, Line 57: .pch_thermal_trip = 77,
Shouldn't this be part of baseboard since it applies to all Hatch variants?
Thanks for this input. This is addressed in patch set 7.
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/1/src/mainboard/google/hatch/... PS1, Line 114: device pci 12.0 off end # Thermal Subsystem
Done
Ack
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/hatch/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/33147/6/src/mainboard/google/hatch/... PS6, Line 57: .pch_thermal_trip = 77,
We have done this change and it works fine. Thanks Furquan for this suggestion. […]
Done
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33147 )
Change subject: mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C ......................................................................
mb/google/hatch/variants/hatch: Set PCH Thermal Threshold value to 77 deg C
PMC logic shuts down the PCH thermal sensor when CPU is in a C-state and DTS Temp <= Low Temp Threshold (LTT) in case of Dynamic Thermal Shutdown when S0ix is enabled.
BUG=133345634 BRANCH=None TEST=Verified Thermal Device (B0: D20: F2) TSPM offset 0x1c [LTT (8:0)] value is 0xFE on Hatch.
Change-Id: Ib20fae04080b28c6105e5a187cc5d7a55b48d709 Signed-off-by: Sumeet Pawnikar sumeet.r.pawnikar@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33147 Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Subrata Banik subrata.banik@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/hatch/variants/baseboard/devicetree.cb 1 file changed, 6 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Subrata Banik: Looks good to me, approved Tim Wawrzynczak: Looks good to me, approved
diff --git a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb index 6f4adc4..14630a4 100644 --- a/src/mainboard/google/hatch/variants/baseboard/devicetree.cb +++ b/src/mainboard/google/hatch/variants/baseboard/devicetree.cb @@ -47,6 +47,11 @@ # Unlock GPIO pads register "PchUnlockGpioPads" = "1"
+ # NOTE: if any variant wants to override this value, use the same format + # as register "common_soc_config.pch_thermal_trip" = "value", instead of + # putting it under register "common_soc_config" in overridetree.cb file. + register "common_soc_config.pch_thermal_trip" = "77" + # VR Settings Configuration for 4 Domains #+----------------+-------+-------+-------+-------+ #| Domain/Setting | SA | IA | GTUS | GTS | @@ -194,7 +199,7 @@ device pci 02.0 on end # Integrated Graphics Device device pci 04.0 off end # SA Thermal device device pci 05.0 off end # SA IPU - device pci 12.0 off end # Thermal Subsystem + device pci 12.0 on end # Thermal Subsystem device pci 12.5 off end # UFS SCS device pci 12.6 off end # GSPI #2 device pci 14.0 on
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Martin Roth (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Fagerburg (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Sumeet R Pawnikar (Code Review)
-
Tim Wawrzynczak (Code Review)