Michał Żygowski has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
nb/amd/{agesa,pi}/acpi: include thermal zone
According to BKDGs these northbridges should support the K10 compatible temperature sensors.
Signed-off-by: Michał Żygowski michal.zygowski@3mdeb.com Change-Id: Icbdf44508085964452d74e084b133f1baa39e1a8 --- M src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl M src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl M src/northbridge/amd/pi/00630F01/acpi/northbridge.asl M src/northbridge/amd/pi/00730F01/acpi/northbridge.asl 4 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/38755/1
diff --git a/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl b/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl index c360da6..aabfed3 100644 --- a/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl +++ b/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl @@ -93,3 +93,8 @@ Return (PS7) /* PIC Mode */ } /* end _PRT */ } /* end PBR7 */ + +Device (K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +} diff --git a/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl b/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl index ce889c7..6e8f5b6 100644 --- a/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl +++ b/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl @@ -93,3 +93,8 @@ Return (PS8) /* PIC Mode */ } /* end _PRT */ } /* end PBR8 */ + +Device (K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +} diff --git a/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl b/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl index ffe1367..975260a 100644 --- a/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl +++ b/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl @@ -63,3 +63,8 @@ Return (PS3) /* PIC Mode */ } /* end _PRT */ } /* end PBR3 */ + +Device (K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +} diff --git a/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl b/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl index ce889c7..6e8f5b6 100644 --- a/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl +++ b/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl @@ -93,3 +93,8 @@ Return (PS8) /* PIC Mode */ } /* end _PRT */ } /* end PBR8 */ + +Device (K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +}
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
I am still a bit unsure which platforms use Socket F/AM2+, which report unreliable readings according to erratum #319
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1: Code-Review+1
I can confirm the register compatibility. But... reading thermal_mixin.asl, that code doesn't seem to make much sense. The ThermalZone is only reported as present if HTC (hardware temperature control) is enable, so far so good. But then the limit that would trigger HTC is used as _CRT (immediate shut- down) and (-10K) as _HOT (better power down now). Unless the HTC limit register is configured very wrong, this would force the OS to shut down rather early.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
I can confirm the register compatibility. But... reading thermal_mixin.asl, that code doesn't seem to make much sense. The ThermalZone is only reported as present if HTC (hardware temperature control) is enable, so far so good. But then the limit that would trigger HTC is used as _CRT (immediate shut- down) and (-10K) as _HOT (better power down now). Unless the HTC limit register is configured very wrong, this would force the OS to shut down rather early.
On apu2 it worked well, but I agree that an incorrectly programmed limit can cause trouble. I will look for the bits that enable HTC and program the limit (probably in AGESA) if they exist. If not will add a preceding patch that ensures the correct programming of these bits.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review+1
I can confirm the register compatibility. But... reading thermal_mixin.asl, that code doesn't seem to make much sense. The ThermalZone is only reported as present if HTC (hardware temperature control) is enable, so far so good. But then the limit that would trigger HTC is used as _CRT (immediate shut- down) and (-10K) as _HOT (better power down now). Unless the HTC limit register is configured very wrong, this would force the OS to shut down rather early.
On apu2 it worked well, but I agree that an incorrectly programmed limit can cause trouble. I will look for the bits that enable HTC and program the limit (probably in AGESA) if they exist. If not will add a preceding patch that ensures the correct programming of these bits.
For family 14h HtcEn is set if the limit is not 0: src/vendorcode/amd/agesa/f14/Proc/CPU/Family/0x14/cpuF14SoftwareThermal.c:108
For family 15h tn limit is set via IDS (I think we do not use them): src/vendorcode/amd/agesa/f15tn/Proc/IDS/Family/0x15/TN/IdsF15TnAllService.c:127 HtcEn is set to enabled if limti is not 0 or IDS is set to enable HTC: src/vendorcode/amd/agesa/f15tn/Proc/CPU/Family/0x15/TN/cpuF15TnHtc.c:162 src/vendorcode/amd/agesa/f15tn/Proc/IDS/Family/0x15/TN/IdsF15TnAllService.c:100
For family 16h kb is the same as for family 15h: src/vendorcode/amd/agesa/f16kb/Proc/CPU/Family/0x16/KB/F16KbHtc.c:142 src/vendorcode/amd/agesa/f16kb/Proc/IDS/Family/0x16/KB/IdsF16KbAllService.c:101
BinaryPI is unknown (at least 00730F01 does enable it correctly).
I will explicitly enable the limit and HTC in common bootpath for all these chipsets.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Sorry, there's some misunderstanding. I didn't doubt that the HTC feature is configured correctly.
I doubt that the thermal_mix.asl code is actually useful with correctly configured HTC. If I read it correctly, it would prevent HTC to become active (powers down long before that would happen).
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Patch Set 1:
Sorry, there's some misunderstanding. I didn't doubt that the HTC feature is configured correctly.
I doubt that the thermal_mix.asl code is actually useful with correctly configured HTC. If I read it correctly, it would prevent HTC to become active (powers down long before that would happen).
Okay, so are you referring to this line? Return (Subtract (_CRT, K10TEMP_HOT_OFFSET))
Or I am still missing your point (or something obvious)?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Sorry, there's some misunderstanding. I didn't doubt that the HTC feature is configured correctly.
I doubt that the thermal_mix.asl code is actually useful with correctly configured HTC. If I read it correctly, it would prevent HTC to become active (powers down long before that would happen).
Okay, so are you referring to this line? Return (Subtract (_CRT, K10TEMP_HOT_OFFSET))
Or I am still missing your point (or something obvious)?
That too. But also that it is using TLMT (the limit when HTC should trigger, if I understand the BWG correctly) in _CRT. ACPI says, the OS should go into S4 at _HOT and immediately shutdown at _CRT.
I don't know AMD very well. Just when I read the BWG and ACPI spec, it seems to me that TLMT would rather match _PSV.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Sorry, there's some misunderstanding. I didn't doubt that the HTC feature is configured correctly.
I doubt that the thermal_mix.asl code is actually useful with correctly configured HTC. If I read it correctly, it would prevent HTC to become active (powers down long before that would happen).
Okay, so are you referring to this line? Return (Subtract (_CRT, K10TEMP_HOT_OFFSET))
Or I am still missing your point (or something obvious)?
That too. But also that it is using TLMT (the limit when HTC should trigger, if I understand the BWG correctly) in _CRT. ACPI says, the OS should go into S4 at _HOT and immediately shutdown at _CRT.
I don't know AMD very well. Just when I read the BWG and ACPI spec, it seems to me that TLMT would rather match _PSV.
It's not a big deal, I guess, as long as the machine is actively cooled. But on a passively cooled system, where the thermal budget is fully exhausted, I fear this patch could make the OS shut down too early.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 1:
Patch Set 1:
Sorry, there's some misunderstanding. I didn't doubt that the HTC feature is configured correctly.
I doubt that the thermal_mix.asl code is actually useful with correctly configured HTC. If I read it correctly, it would prevent HTC to become active (powers down long before that would happen).
Okay, so are you referring to this line? Return (Subtract (_CRT, K10TEMP_HOT_OFFSET))
Or I am still missing your point (or something obvious)?
That too. But also that it is using TLMT (the limit when HTC should trigger, if I understand the BWG correctly) in _CRT. ACPI says, the OS should go into S4 at _HOT and immediately shutdown at _CRT.
I don't know AMD very well. Just when I read the BWG and ACPI spec, it seems to me that TLMT would rather match _PSV.
HTC will trigger when the current temperature exceeds the TLMT, which results in performance drop and PROCHOT assertion. So it is assuming that the processor will keep running, not transitioning to S4. Indeed this is not well written, I haven't figured that out at the beginning. So _HOT should be the TLMT+<some arbitrary value, let's say 5 or 10 degrees>. And TLMT should be decreased in such a case.
Thank you for pointing this out.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 3:
Nico, corrected the behavior in the preceding patch CB:39697 Please have a look
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 3: Code-Review+2
Nico, corrected the behavior in the preceding patch CB:39697 Please have a look
Thanks for fixing this. Did you have a chance to test some actual behavior? I know for most processors it's hard to push them to their limit, so I don't think it has to be tested.
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 3:
Patch Set 3: Code-Review+2
Nico, corrected the behavior in the preceding patch CB:39697 Please have a look
Thanks for fixing this. Did you have a chance to test some actual behavior? I know for most processors it's hard to push them to their limit, so I don't think it has to be tested.
I was able to hit _CRT only on Summer during heavy regression tests, where the platform was not in the case. The temperature got to 115 C and got shut down immediately. Now it will be close to impossible I guess.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 3:
(2 comments)
Please move `northbridge/amd/agesa/family14/acpi/thermal_mixin.asl` to a common location.
Commit d5e6618a (amd/fam10: Add k10temp ACPI thermal zone mixin., CB:10617) also has the comment below.
It should not be used on boards for which errata 319 (The thermal sensor of Socket F/AM2+ processors may be unreliable) is applicable. AM3 and later should be fine.
https://review.coreboot.org/c/coreboot/+/38755/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38755/3//COMMIT_MSG@11 PS3, Line 11: Tested how?
https://review.coreboot.org/c/coreboot/+/38755/3/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38755/3/src/northbridge/amd/agesa/f... PS3, Line 96: Device (K10M) { Please use the consistent style in this file without spaces.
Hello build bot (Jenkins), Nico Huber, Patrick Georgi, Paul Menzel,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38755
to look at the new patch set (#4).
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
nb/amd/{agesa,pi}/acpi: include thermal zone
According to BKDGs these northbridges should support the K10 compatible temperature sensors.
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: Icbdf44508085964452d74e084b133f1baa39e1a8 --- M src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl M src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl M src/northbridge/amd/pi/00630F01/acpi/northbridge.asl M src/northbridge/amd/pi/00730F01/acpi/northbridge.asl 4 files changed, 20 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/38755/4
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38755/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38755/3//COMMIT_MSG@11 PS3, Line 11:
Tested how?
Done
https://review.coreboot.org/c/coreboot/+/38755/3/src/northbridge/amd/agesa/f... File src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38755/3/src/northbridge/amd/agesa/f... PS3, Line 96: Device (K10M) {
Please use the consistent style in this file without spaces.
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 4: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 4:
Please move `northbridge/amd/agesa/family14/acpi/thermal_mixin.asl` to a common location.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 4: Code-Review+2
Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 4:
Patch Set 4:
Please move `northbridge/amd/agesa/family14/acpi/thermal_mixin.asl` to a common location.
Will do in subsequent patch
Michał Żygowski has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
nb/amd/{agesa,pi}/acpi: include thermal zone
According to BKDGs these northbridges should support the K10 compatible temperature sensors.
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: Icbdf44508085964452d74e084b133f1baa39e1a8 Reviewed-on: https://review.coreboot.org/c/coreboot/+/38755 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/family15tn/acpi/northbridge.asl M src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl M src/northbridge/amd/pi/00630F01/acpi/northbridge.asl M src/northbridge/amd/pi/00730F01/acpi/northbridge.asl 4 files changed, 20 insertions(+), 0 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/family15tn/acpi/northbridge.asl b/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl index ce3715e..40df891 100644 --- a/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl +++ b/src/northbridge/amd/agesa/family15tn/acpi/northbridge.asl @@ -92,3 +92,8 @@ Return (PS7) /* PIC Mode */ } /* end _PRT */ } /* end PBR7 */ + +Device(K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +} diff --git a/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl b/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl index cdfab58..34bf33a 100644 --- a/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl +++ b/src/northbridge/amd/agesa/family16kb/acpi/northbridge.asl @@ -92,3 +92,8 @@ Return (PS8) /* PIC Mode */ } /* end _PRT */ } /* end PBR8 */ + +Device(K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +} diff --git a/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl b/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl index 621efb5..cdb4063 100644 --- a/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl +++ b/src/northbridge/amd/pi/00630F01/acpi/northbridge.asl @@ -62,3 +62,8 @@ Return (PS3) /* PIC Mode */ } /* end _PRT */ } /* end PBR3 */ + +Device(K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +} diff --git a/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl b/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl index cdfab58..34bf33a 100644 --- a/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl +++ b/src/northbridge/amd/pi/00730F01/acpi/northbridge.asl @@ -92,3 +92,8 @@ Return (PS8) /* PIC Mode */ } /* end _PRT */ } /* end PBR8 */ + +Device(K10M) { + Name (_ADR, 0x00180003) + #include <northbridge/amd/agesa/family14/acpi/thermal_mixin.asl> +}
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38755 )
Change subject: nb/amd/{agesa,pi}/acpi: include thermal zone ......................................................................
Patch Set 5:
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/1625 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1624 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1623 Non-emulation targets: HP_COMPAQ_8200_ELITE_SFF_PC using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1627 HP_COMPAQ_8200_ELITE_SFF_PC using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1626
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Michał Żygowski (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)