Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT.
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/1
diff --git a/src/drivers/intel/dptf/dptf.c b/src/drivers/intel/dptf/dptf.c index bf9e98a..e3e4efd 100644 --- a/src/drivers/intel/dptf/dptf.c +++ b/src/drivers/intel/dptf/dptf.c @@ -5,6 +5,27 @@ #include <device/device.h> #include "chip.h"
+enum { + /* Generic DPTF participants have a PTYP field */ + DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER = 0xB, + DPTF_GENERIC_PARTICIPANT_TYPE_TSR = 0x3, +#ifndef CONFIG_DPTF_CPU_ADDR + CONFIG_DPTF_CPU_ADDR = 0x00040000ul, +#endif +}; + +#define DEFAULT_CHARGER_NAME "Battery Charger" + +/* + * Helper method to determine if a device is "used" (called out anywhere as a source or a target + * of any policies, and therefore should be included in the ACPI tables. + */ +static bool is_participant_used(const struct drivers_intel_dptf_config *config, + enum dptf_participant participant) +{ + return false; +} + static const char *dptf_acpi_name(const struct device *dev) { return "DPTF"; @@ -25,10 +46,22 @@ static void dptf_inject_dsdt(const struct device *dev) { const struct drivers_intel_dptf_config *config; + enum dptf_participant p; + char name[5]; + int uid; + int id;
config = dev->chip_info; acpigen_write_scope("\_SB");
+ /* DPTF CPU device */ + if (is_participant_used(config, DPTF_CPU)) { + acpigen_write_device("TCPU"); + acpigen_write_name_integer("_ADR", CONFIG_DPTF_CPU_ADDR); + dptf_write_STA(); + acpigen_pop_len(); /* Device */ + } + /* Toplevel DPTF device */ acpigen_write_device("DPTF"); acpigen_write_name("_HID"); @@ -36,6 +69,43 @@ acpigen_write_name_integer("_UID", 0); dptf_write_STA();
+ /* Fan device */ + if (is_participant_used(config, DPTF_FAN)) { + acpigen_write_device("TFN1"); + acpigen_write_name("_HID"); + acpigen_emit_eisaid("INT3404"); + acpigen_write_name_integer("_UID", 0); + dptf_write_STA(); + acpigen_pop_len(); /* Device */ + } + + /* Charger */ + if (is_participant_used(config, DPTF_CHARGER)) { + acpigen_write_device("TCHG"); + acpigen_write_name("_HID"); + acpigen_emit_eisaid("INT3403"); + acpigen_write_name_integer("_UID", 0); + acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER); + acpigen_write_name_string("_STR", DEFAULT_CHARGER_NAME); + dptf_write_STA(); + acpigen_pop_len(); /* Device */ + } + + /* TSR0 - TSR3 */ + for (p = DPTF_TSR0, uid = 1, id = 0; p <= DPTF_TSR3; ++p, ++uid, ++id) { + if (is_participant_used(config, p)) { + snprintf(name, sizeof(name), "TSR%1d", id); + acpigen_write_device(name); + acpigen_write_name("_HID"); + acpigen_emit_eisaid("INT3403"); + acpigen_write_name_integer("TMPI", id); + acpigen_write_name_integer("_UID", uid); + acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_TSR); + dptf_write_STA(); + acpigen_pop_len(); /* Device */ + } + } + acpigen_pop_len(); /* DPTF Device */ acpigen_pop_len(); /* Scope */ }
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#3).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT.
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/3
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 12: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif Given that this enum is defined for Participant Type, can we keep the address outside?
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 60: CONFIG_DPTF_CPU_ADDR Since we are using the System Thermal Agent device, can we use it's PCI BDF to construct this address?
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 101: acpigen_write_name_integer("TMPI", id); What is this id? I am not able to find any reference to this Id in the DPTF document that I am looking at - 541817 rev 1.3.13
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 76: INT3404 for tigerlake these EISAID seems to be changed as per https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2...
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 86: INT3403 for tigerlake these EISAID seems to be changed as per https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2...
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 100: INT3403 for tigerlake these EISAID seems to be changed as per https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 60: CONFIG_DPTF_CPU_ADDR
Since we are using the System Thermal Agent device, can we use it's PCI BDF to construct this addres […]
I don't think that's true for all SoCs though, for APL/GLK the address was 0x0000_0001 (DPTF_CPU_ADDR)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 76: INT3404
for tigerlake these EISAID seems to be changed as per https://chromium-review.googlesource. […]
Is there a new DPTF spec that shows this information?
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 101: acpigen_write_name_integer("TMPI", id);
What is this id? I am not able to find any reference to this Id in the DPTF document that I am looki […]
It was included in the previous implementation, and seemed to be only used in the vendor-specific functions (though because it's generated, TMPI can become obsolete, I just wasn't sure if it was strictly necessary to be exported or not).
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#5).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT.
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/5
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/6/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/6/src/drivers/intel/dptf/dptf... PS6, Line 73: "INT3404" also out of scope for this commit: we should create src/include/device/hid_ids.h (to match pci_ids and mipi_ids) for all these various HIDs sprinkled throughout the codebase.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 76: INT3404
Is there a new DPTF spec that shows this information?
Also will this still work with our open-source DPTF implementation? Is it expecting the old HIDs?
https://review.coreboot.org/c/coreboot/+/41884/6/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/6/src/drivers/intel/dptf/dptf... PS6, Line 73: "INT3404"
also out of scope for this commit: we should create src/include/device/hid_ids. […]
+1.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#7).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 83 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/7/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/7/src/drivers/intel/dptf/dptf... PS7, Line 104: for (p = DPTF_TEMP_SENSOR_0, uid = 1, id = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++uid, ++id) { line over 96 characters
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#8).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 85 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/8
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 12: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif
Given that this enum is defined for Participant Type, can we keep the address outside?
This is the default for DPTF, for all SoCs I've seen except the Core line (APL, etc.) which seem to use 0x1. I don't think it needs to be exported from here, only this module should need to use the address.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 23: DPTF_GENERIC_PARTICIPANT_TYPE_TSR = 0x3, What about CPU(0x0) and Fan(0x4)?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 24: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif Isn't this the PCI device address for the CPU thermal device? Can we instead derive this from the struct device *dev? Are the thermal devices being added under this PCI device?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 68: acpigen_write_name_integer("_ADR", CONFIG_DPTF_CPU_ADDR); acpigen_write_ADR()?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 69: is_participant_used(config, DPTF_CPU) Is it a valid scenario to have DPTF_CPU as unused?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 92: acpigen_write_device("TCHG"); : acpigen_write_name("_HID"); : acpigen_emit_eisaid("INT3403"); : acpigen_write_name_integer("_UID", 0); : acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER); : acpigen_write_name_string("_STR", DEFAULT_CHARGER_NAME); : acpigen_write_STA(is_participant_used(config, DPTF_CHARGER) ? : ACPI_STATUS_DEVICE_ALL_ON : : ACPI_STATUS_DEVICE_ALL_OFF); Should this sequence be converted to a helper function -- dptf_write_generic_participant? It can be used for TCHG as well as TSR. Probably for TFN as well?
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 104: 1 I believe this is starting from 1 because TCHG above uses UID 0? It might be a good idea to ensure that this uid account for all INT3403 devices so that if a new device gets added, we don't have to manually do the math.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 112: acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_TSR); Why no "_STR" object?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 23: DPTF_GENERIC_PARTICIPANT_TYPE_TSR = 0x3,
What about CPU(0x0) and Fan(0x4)?
The CPU and Fan are not exposed as 'Generic' devices in this implementation, they have their own Device types (PTYP). I don't know why the devices are exposed like that.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 24: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif
Isn't this the PCI device address for the CPU thermal device?
Yes.
Can we instead derive this from the struct device *dev?
Well, PCI_DEV(SA_DEV_DPTF) returns 0x20000, so I'm not sure how this ADDR is related to the PCI address, unless it's just intended to be PCI_DEV(...) << 1 ? but I don't get why.
Also, for Atom SoCs, this address is supposed to be 0x1 (see APL) 😞
Are the thermal devices being added under this PCI device?
Nope, they're under _SB.DPTF
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 68: acpigen_write_name_integer("_ADR", CONFIG_DPTF_CPU_ADDR);
acpigen_write_ADR()?
Ack
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 69: is_participant_used(config, DPTF_CPU)
Is it a valid scenario to have DPTF_CPU as unused?
I think you could. If you had the fans all configured as only cooling down TSRs and the passive controls only on TCHG & TSRs. Whether it's a good idea or not is maybe a different question 😊
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 92: acpigen_write_device("TCHG"); : acpigen_write_name("_HID"); : acpigen_emit_eisaid("INT3403"); : acpigen_write_name_integer("_UID", 0); : acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER); : acpigen_write_name_string("_STR", DEFAULT_CHARGER_NAME); : acpigen_write_STA(is_participant_used(config, DPTF_CHARGER) ? : ACPI_STATUS_DEVICE_ALL_ON : : ACPI_STATUS_DEVICE_ALL_OFF);
Should this sequence be converted to a helper function -- dptf_write_generic_participant? It can be […]
Good call.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 104: 1
I believe this is starting from 1 because TCHG above uses UID 0? It might be a good idea to ensure t […]
Sure, I will try to make that more explicit. your suggestion of dptf_write_generic_participant above would make this simpler.
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 112: acpigen_write_name_integer("PTYP", DPTF_GENERIC_PARTICIPANT_TYPE_TSR);
Why no "_STR" object?
It's added in a later patchset, with a devicetree option.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 24: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif
Isn't this the PCI device address for the CPU thermal device? […]
nm, forgot about acpigen_write_ADR_pci_device
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 77: acpigen_emit_eisaid("INT3400"); Are we following the DPTF spec or DTT spec. The latest DTT spec(doc # 626708) says the _HID as INTC1040 for whereas the old DPTF spec says INT3400
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 77: acpigen_emit_eisaid("INT3400");
Are we following the DPTF spec or DTT spec. […]
I've been trying to decide whether or not to backport this to older platforms, so that we could completely get rid of all of the DPTF ASL files, but I count 100 'dptf.asl' files in src/mainboard/, so that may not be worthwhile and we should just stick with the new _HID values. If I needed to support both old & new then there would have to be a way to select which set of _HID values to use, but if I'm not backporting, it's easy 😊
However, our current implementation of acpigen_emit_eisaid only supports the 7-character format for EISA IDs. So I need to figure out what iasl is doing with the 8-character ones and add a function for that first... it's on my list of things to do !
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#9).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 111 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/9
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 77: acpigen_emit_eisaid("INT3400");
I've been trying to decide whether or not to backport this to older platforms, so that we could comp […]
Whoops, they're not even EISAIDs anymore, somehow my eyes glazed over that. So no worries, next patchset awaits.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 9:
(5 comments)
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 12: #ifndef CONFIG_DPTF_CPU_ADDR : CONFIG_DPTF_CPU_ADDR = 0x00040000ul, : #endif
This is the default for DPTF, for all SoCs I've seen except the Core line (APL, etc. […]
Ack
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 76: INT3404
Also will this still work with our open-source DPTF implementation? Is it expecting the old HIDs?
Ack
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 86: INT3403
for tigerlake these EISAID seems to be changed as per https://chromium-review.googlesource. […]
Ack
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 100: INT3403
for tigerlake these EISAID seems to be changed as per https://chromium-review.googlesource. […]
Ack
https://review.coreboot.org/c/coreboot/+/41884/4/src/drivers/intel/dptf/dptf... PS4, Line 101: acpigen_write_name_integer("TMPI", id);
It was included in the previous implementation, and seemed to be only used in the vendor-specific fu […]
Ack
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#10).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/dptf.c 1 file changed, 111 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/10
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... PS10, Line 95: /* The CPU device gets an _ADR that matches the ACPI PCI address for 00:04.00 */ : parent = dev && dev->bus ? dev->bus->dev : NULL; : if (!parent || parent->path.type != DEVICE_PATH_PCI) { : printk(BIOS_ERR, "%s: DPTF objects must live under 00:04.0 PCI device\n", : __func__); : return; : } acpigen_write_ADR_pci_device asserts that the device is of type PCI. Is this check to avoid that?
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... PS10, Line 125: acpigen_write_device("TFN1"); It is required only if active cooling is desired. I haven't seen a use case of small-core platforms employing active cooling. Small core platforms use passive cooling only. Do we want to make it configurable?
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... PS10, Line 125: acpigen_write_device("TFN1");
It is required only if active cooling is desired. […]
So the way this implementation works is during DSDT injection, it writes out a minimal device definition (including a _STA that will indicate it's unavailable if it's not used in any policy). This makes things easier in later patches when we're writing helper methods, we can just assume that those devices exist and we won't have any unhandled references. It does add a small amount of bloat to DSDT if devices are unused. All of the configurable stuff (i.e, policies) are written into SSDT.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... PS10, Line 125: acpigen_write_device("TFN1");
So the way this implementation works is during DSDT injection, it writes out a minimal device defini […]
Ack
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... PS10, Line 95: /* The CPU device gets an _ADR that matches the ACPI PCI address for 00:04.00 */ : parent = dev && dev->bus ? dev->bus->dev : NULL; : if (!parent || parent->path.type != DEVICE_PATH_PCI) { : printk(BIOS_ERR, "%s: DPTF objects must live under 00:04.0 PCI device\n", : __func__); : return; : }
acpigen_write_ADR_pci_device asserts that the device is of type PCI. […]
Also to make sure that the `chip drivers/intel/dptf` is placed under at least a PCI device 😊
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/10/src/drivers/intel/dptf/dpt... PS10, Line 95: /* The CPU device gets an _ADR that matches the ACPI PCI address for 00:04.00 */ : parent = dev && dev->bus ? dev->bus->dev : NULL; : if (!parent || parent->path.type != DEVICE_PATH_PCI) { : printk(BIOS_ERR, "%s: DPTF objects must live under 00:04.0 PCI device\n", : __func__); : return; : }
Also to make sure that the `chip drivers/intel/dptf` is placed under at least a PCI device 😊
Ack
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 77: acpigen_emit_eisaid("INT3400");
I've been trying to decide whether or not to backport this to older platforms, so that we could comp […]
It seems new ACPI Device IDs for an exmaple INTC10xx are for Tiger Lake platform only AFAIK. We might need support for both INT34xx (7-character EISA format) and INTC10xx (8-character EISA format).
https://review.coreboot.org/c/coreboot/+/41884/11/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/11/src/drivers/intel/dptf/dpt... PS11, Line 27: #define DPTF_DPTF_DEVICE_HID "INTC1040" These new ACPI Device IDs "INTC10xx" are being used for only TigerLake AFAIK at this point. In this case what about the old 7-char ESID ?
Also, can we move these defines and enums in dptf.h header file if possible. I did not go through complete series of these patches, if its already done. Thanks.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/8/src/drivers/intel/dptf/dptf... PS8, Line 77: acpigen_emit_eisaid("INT3400");
It seems new ACPI Device IDs for an exmaple INTC10xx are for Tiger Lake platform only AFAIK. […]
Ok, I can make that a configurable option, whether to use old- or new-style _HID.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/11/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/11/src/drivers/intel/dptf/dpt... PS11, Line 27: #define DPTF_DPTF_DEVICE_HID "INTC1040"
These new ACPI Device IDs "INTC10xx" are being used for only TigerLake AFAIK at this point. […]
I will add a config option to choose between EISAID (7-char) and the new ones (8-char).
Also the intent is that these _HIDs should not need to be used anywhere else. This file writes the Device objects into the DSDT itself, so no other file/module should need access to the _HID values.
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#12).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/Kconfig M src/drivers/intel/dptf/dptf.c 2 files changed, 140 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/12
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41884/12/src/drivers/intel/dptf/dpt... File src/drivers/intel/dptf/dptf.c:
https://review.coreboot.org/c/coreboot/+/41884/12/src/drivers/intel/dptf/dpt... PS12, Line 140: if (CONFIG(DPTF_USE_EISA_HID)) { suspect code indent for conditional statements (16, 16)
Hello build bot (Jenkins), Furquan Shaikh, Duncan Laurie, Sumeet R Pawnikar, Subrata Banik, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41884
to look at the new patch set (#13).
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/dptf/Kconfig M src/drivers/intel/dptf/dptf.c 2 files changed, 140 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/41884/13
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 13: Code-Review+1
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 13:
friendly ping, it's been a few weeks 😊
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
Patch Set 13: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41884 )
Change subject: drivers/intel/dptf: Add current participant Devices to DSDT ......................................................................
drivers/intel/dptf: Add current participant Devices to DSDT
In this DPTF implementation, the participant device objects are written into the DSDT with only minimal Names attached (_HID/_ADR, _STA, _UID, PTYP, and _STR). All other Methods & Names will be written into the SSDT. If a device is not used in any policy, then its _STA is set to return 0 ("off").
BUG=b:143539650 TEST=Compiles.
Change-Id: Ief69a57adce9ee0b19056ce6a11ed8a5b51b3f87 Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/41884 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Sumeet R Pawnikar sumeet.r.pawnikar@intel.com Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M src/drivers/intel/dptf/Kconfig M src/drivers/intel/dptf/dptf.c 2 files changed, 140 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Sumeet R Pawnikar: Looks good to me, but someone else must approve
diff --git a/src/drivers/intel/dptf/Kconfig b/src/drivers/intel/dptf/Kconfig index 7db335a..880b32a 100644 --- a/src/drivers/intel/dptf/Kconfig +++ b/src/drivers/intel/dptf/Kconfig @@ -5,3 +5,12 @@ help When enabled, entries in the devicetree are used to generate Intel DPTF Tables at runtime in the SSDT. + +config DPTF_USE_EISA_HID + bool "If selected, use 'old' 7 character EISA IDs for DPTF _HID" + depends on DRIVERS_INTEL_DPTF + default n + help + When selected, all DPTF devices will use the "old" style of + _HIDs, which are 7-character EISA IDs. Otherwise, it will use + the "new" style, which are regular 8-character _HIDs. diff --git a/src/drivers/intel/dptf/dptf.c b/src/drivers/intel/dptf/dptf.c index f168375..060864a 100644 --- a/src/drivers/intel/dptf/dptf.c +++ b/src/drivers/intel/dptf/dptf.c @@ -5,6 +5,44 @@ #include <device/device.h> #include "chip.h"
+enum dptf_participant { + DPTF_NONE, + DPTF_CPU, + DPTF_CHARGER, + DPTF_FAN, + DPTF_TEMP_SENSOR_0, + DPTF_TEMP_SENSOR_1, + DPTF_TEMP_SENSOR_2, + DPTF_TEMP_SENSOR_3, + DPTF_PARTICIPANT_COUNT, +}; + +/* Generic DPTF participants have a PTYP field to distinguish them */ +enum dptf_generic_participant_type { + DPTF_GENERIC_PARTICIPANT_TYPE_TSR = 0x3, + DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER = 0xB, +}; + +#define DEFAULT_CHARGER_STR "Battery Charger" + +#define DPTF_DEVICE_HID_EISAID "INT3400" +#define GENERIC_HID_EISAID "INT3403" +#define FAN_HID_EISAID "INT3404" + +#define DPTF_DEVICE_HID "INTC1040" +#define GENERIC_HID "INTC1043" +#define FAN_HID "INTC1044" + +/* + * Helper method to determine if a device is "used" (called out anywhere as a source or a target + * of any policies, and therefore should be included in the ACPI tables. + */ +static bool is_participant_used(const struct drivers_intel_dptf_config *config, + enum dptf_participant participant) +{ + return false; +} + static const char *dptf_acpi_name(const struct device *dev) { return "DPTF"; @@ -13,25 +51,112 @@ /* Add custom tables and methods to SSDT */ static void dptf_fill_ssdt(const struct device *dev) { - struct drivers_intel_dptf_config *config = dev->chip_info; + struct drivers_intel_dptf_config *config = config_of(dev);
printk(BIOS_INFO, "\_SB.DPTF: %s at %s\n", dev->chip_ops->name, dev_path(dev)); }
+static int get_STA_value(const struct drivers_intel_dptf_config *config, + enum dptf_participant participant) +{ + return is_participant_used(config, participant) ? + ACPI_STATUS_DEVICE_ALL_ON : + ACPI_STATUS_DEVICE_ALL_OFF; +} + +static void dptf_write_generic_participant(const char *name, + enum dptf_generic_participant_type ptype, + const char *str, int sta_val) +{ + /* Auto-incrementing UID for generic participants */ + static int generic_uid = 0; + + acpigen_write_device(name); + + if (CONFIG(DPTF_USE_EISA_HID)) { + acpigen_write_name("_HID"); + acpigen_emit_eisaid(GENERIC_HID_EISAID); + } else { + acpigen_write_name_string("_HID", GENERIC_HID); + } + + acpigen_write_name_integer("_UID", generic_uid++); + acpigen_write_STA(sta_val); + + if (str) + acpigen_write_name_string("_STR", str); + + acpigen_write_name_integer("PTYP", ptype); + + acpigen_pop_len(); /* Device */ +} + /* Add static definitions of DPTF devices into the DSDT */ static void dptf_inject_dsdt(const struct device *dev) { const struct drivers_intel_dptf_config *config; + enum dptf_participant participant; + struct device *parent; + char name[5]; + int i;
- config = dev->chip_info; + /* The CPU device gets an _ADR that matches the ACPI PCI address for 00:04.00 */ + parent = dev && dev->bus ? dev->bus->dev : NULL; + if (!parent || parent->path.type != DEVICE_PATH_PCI) { + printk(BIOS_ERR, "%s: DPTF objects must live under 00:04.0 PCI device\n", + __func__); + return; + } + + config = config_of(dev); acpigen_write_scope("\_SB");
- /* Toplevel DPTF device */ + /* DPTF CPU device - _SB.TCPU */ + acpigen_write_device("TCPU"); + acpigen_write_ADR_pci_device(parent); + acpigen_write_STA(get_STA_value(config, DPTF_CPU)); + acpigen_pop_len(); /* Device */ + + /* Toplevel DPTF device - _SB.DPTF*/ acpigen_write_device(acpi_device_name(dev)); - acpigen_write_name("_HID"); - acpigen_emit_eisaid("INT3400"); + if (CONFIG(DPTF_USE_EISA_HID)) { + acpigen_write_name("_HID"); + acpigen_emit_eisaid(DPTF_DEVICE_HID_EISAID); + } else { + acpigen_write_name_string("_HID", DPTF_DEVICE_HID); + } + acpigen_write_name_integer("_UID", 0); - dptf_write_STA(); + acpigen_write_STA(ACPI_STATUS_DEVICE_ALL_ON); + + /* + * The following devices live underneath _SB.DPTF: + * - Fan, _SB.DPTF.TFN1 + * - Charger, _SB.DPTF.TCHG + * - Temperature Sensors, _SB.DPTF.TSRn + */ + + acpigen_write_device("TFN1"); + if (CONFIG(DPTF_USE_EISA_HID)) { + acpigen_write_name("_HID"); + acpigen_emit_eisaid(FAN_HID_EISAID); + } else { + acpigen_write_name_string("_HID", FAN_HID); + } + + acpigen_write_name_integer("_UID", 0); + acpigen_write_STA(get_STA_value(config, DPTF_FAN)); + acpigen_pop_len(); /* Device */ + + dptf_write_generic_participant("TCHG", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER, + DEFAULT_CHARGER_STR, get_STA_value(config, + DPTF_CHARGER)); + + for (i = 0, participant = DPTF_TEMP_SENSOR_0; i < 4; ++i, ++participant) { + snprintf(name, sizeof(name), "TSR%1d", i); + dptf_write_generic_participant(name, DPTF_GENERIC_PARTICIPANT_TYPE_TSR, + NULL, get_STA_value(config, participant)); + }
acpigen_pop_len(); /* DPTF Device */ acpigen_pop_len(); /* Scope */