Change in coreboot[master]: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold be...
Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better ......................................................................
nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Change the behavior of temperature limit to act as a passive cooling threshold. Make the passive cooling threshold a reference value for critical and hot temperature with 5 degrees step.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Ife64c3aab76f8e125493ecc8183a6e87fb012e3b --- M src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39697/1
diff --git a/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl b/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl index 1f877cc..714fb45 100644 --- a/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl +++ b/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl @@ -35,7 +35,7 @@ */
#ifndef K10TEMP_HOT_OFFSET -# define K10TEMP_HOT_OFFSET 100 +# define K10TEMP_HOT_OFFSET 50 #endif
#define K10TEMP_KELVIN_OFFSET 2732 @@ -71,7 +71,12 @@ Return (Add (Local0, K10TEMP_KELVIN_OFFSET)) }
- Method (_CRT) { /* Critical temp in tenths degree Kelvin. */ + /* + * TLMT indicates threshold where HTC become active. That is the + * processor will limit P-State and power consumption in order to + * cool down. + */ + Method (_PSV) { Multiply (TLMT, 10, Local0) ShiftRight (Local0, 1, Local0) Add (Local0, K10TEMP_TLIMIT_OFFSET, Local0) @@ -79,6 +84,10 @@ }
Method (_HOT) { /* Hot temp in tenths degree Kelvin. */ - Return (Subtract (_CRT, K10TEMP_HOT_OFFSET)) + Return (Add (_PSV, K10TEMP_HOT_OFFSET)) + } + + Method (_CRT) { /* Critical temp in tenths degree Kelvin. */ + Return (Add (_HOT, K10TEMP_HOT_OFFSET)) } }
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39697/1/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl:
https://review.coreboot.org/c/coreboot/+/39697/1/src/northbridge/amd/agesa/f... PS1, Line 74: /* trailing whitespace
https://review.coreboot.org/c/coreboot/+/39697/1/src/northbridge/amd/agesa/f... PS1, Line 75: * TLMT indicates threshold where HTC become active. That is the trailing whitespace
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39697
to look at the new patch set (#2).
Change subject: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better ......................................................................
nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Change the behavior of temperature limit to act as a passive cooling threshold. Make the passive cooling threshold a reference value for critical and hot temperature with 5 degrees step.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Ife64c3aab76f8e125493ecc8183a6e87fb012e3b --- M src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39697/2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG@7 PS2, Line 7: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better nb/amd/agesa/family14: Improve HTC threshold handling
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG@9 PS2, Line 9: BKDGs Which version?
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG@18 PS2, Line 18: Tested how?
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl:
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... PS2, Line 77: * cool down. Please use 96 character line length limit.
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... PS2, Line 79: Method (_PSV) { Add comment as below about *Passive Temp*?
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39697
to look at the new patch set (#3).
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
nb/amd/agesa/family14: Improve HTC threshold handling
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Change the behavior of temperature limit to act as a passive cooling threshold. Make the passive cooling threshold a reference value for critical and hot temperature with 5 degrees step.
TEST=boot FreeBSD on PC Engines apu2 and check the thermal zone temperature using sysctl
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Ife64c3aab76f8e125493ecc8183a6e87fb012e3b --- M src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl 1 file changed, 12 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39697/3
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG@7 PS2, Line 7: nb/amd/agesa/family14/acpi/thermal_mixin.asl: handle HTC threshold better
nb/amd/agesa/family14: Improve HTC threshold handling
Done
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG@9 PS2, Line 9: BKDGs
Which version?
Each BKDG for each family describes the temperature limit. No point to list all so I did a shortcut using plural.
https://review.coreboot.org/c/coreboot/+/39697/2//COMMIT_MSG@18 PS2, Line 18:
Tested how?
Tested only Thermal Zone temperature reading as I have no means to test _HOT and _CRT triggering.
Hello build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39697
to look at the new patch set (#4).
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
nb/amd/agesa/family14: Improve HTC threshold handling
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Change the behavior of temperature limit to act as a passive cooling threshold. Make the passive cooling threshold a reference value for critical and hot temperature with 5 degrees step.
TEST=boot FreeBSD on PC Engines apu2 and check the thermal zone temperature using sysctl
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Ife64c3aab76f8e125493ecc8183a6e87fb012e3b --- M src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl 1 file changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39697/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl:
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... PS2, Line 77: * cool down.
Please use 96 character line length limit.
Done
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... PS2, Line 79: Method (_PSV) {
Add comment as below about *Passive Temp*?
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl:
https://review.coreboot.org/c/coreboot/+/39697/2/src/northbridge/amd/agesa/f... PS2, Line 77: * cool down.
Done
WTF? this was way better readable before. There is no rule to make lines longer, only to break them before 96 chars.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG@9 PS4, Line 9: According to BKDGs HTC temperature limit field indicates the threshold I think these lines are a bit too long, the commit message looks weird in Gerrit. It should be reflowed:
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Hello build bot (Jenkins), Nico Huber, Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39697
to look at the new patch set (#5).
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
nb/amd/agesa/family14: Improve HTC threshold handling
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Change the behavior of temperature limit to act as a passive cooling threshold. Make the passive cooling threshold a reference value for critical and hot temperature with 5 degrees step.
TEST=boot FreeBSD on PC Engines apu2 and check the thermal zone temperature using sysctl
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Ife64c3aab76f8e125493ecc8183a6e87fb012e3b --- M src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl 1 file changed, 11 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/97/39697/5
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG@9 PS4, Line 9: According to BKDGs HTC temperature limit field indicates the threshold
I think these lines are a bit too long, the commit message looks weird in Gerrit. […]
You're right. Wrapping at 75 is a bad idea.
Michał Żygowski has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
nb/amd/agesa/family14: Improve HTC threshold handling
According to BKDGs HTC temperature limit field indicates the threshold where HTC becomes active. HTC active state means that processor is limiting its power consumption and maximum P-State. Using this threshold as _CRT is incorrect, since HTC active is designed to prevent overheating, not causing immediate shutdown.
Change the behavior of temperature limit to act as a passive cooling threshold. Make the passive cooling threshold a reference value for critical and hot temperature with 5 degrees step.
TEST=boot FreeBSD on PC Engines apu2 and check the thermal zone temperature using sysctl
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Ife64c3aab76f8e125493ecc8183a6e87fb012e3b Reviewed-on: https://review.coreboot.org/c/coreboot/+/39697 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl 1 file changed, 11 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl b/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl index 1f877cc..3c692ce 100644 --- a/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl +++ b/src/northbridge/amd/agesa/family14/acpi/thermal_mixin.asl @@ -35,7 +35,7 @@ */
#ifndef K10TEMP_HOT_OFFSET -# define K10TEMP_HOT_OFFSET 100 +# define K10TEMP_HOT_OFFSET 50 #endif
#define K10TEMP_KELVIN_OFFSET 2732 @@ -71,7 +71,11 @@ Return (Add (Local0, K10TEMP_KELVIN_OFFSET)) }
- Method (_CRT) { /* Critical temp in tenths degree Kelvin. */ + /* + * TLMT indicates threshold where HTC become active. That is the processor will limit + * P-State and power consumption in order to cool down. + */ + Method (_PSV) { /* Passive temp in tenths degree Kelvin. */ Multiply (TLMT, 10, Local0) ShiftRight (Local0, 1, Local0) Add (Local0, K10TEMP_TLIMIT_OFFSET, Local0) @@ -79,6 +83,10 @@ }
Method (_HOT) { /* Hot temp in tenths degree Kelvin. */ - Return (Subtract (_CRT, K10TEMP_HOT_OFFSET)) + Return (Add (_PSV, K10TEMP_HOT_OFFSET)) + } + + Method (_CRT) { /* Critical temp in tenths degree Kelvin. */ + Return (Add (_HOT, K10TEMP_HOT_OFFSET)) } }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG@9 PS4, Line 9: According to BKDGs HTC temperature limit field indicates the threshold
You're right. Wrapping at 75 is a bad idea.
It is actually 72 characters, AFAIUI
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39697/4//COMMIT_MSG@9 PS4, Line 9: According to BKDGs HTC temperature limit field indicates the threshold
It is actually 72 characters, AFAIUI
Okay, I was just following point 3 from here: https://www.coreboot.org/Git#Commit_messages Which states max 75. https://doc.coreboot.org/tutorial/part2.html also doesn't state the max liens in commit message. I guess we have found a hole in our docs. I may update the tutorial then.
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39697 )
Change subject: nb/amd/agesa/family14: Improve HTC threshold handling ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 5/0/5 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1620 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1619 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1618 Non-emulation targets: HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1622 HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1621
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
build bot (Jenkins) (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)