Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43529 )
Change subject: dptf: Fix scope of TCPU device ......................................................................
dptf: Fix scope of TCPU device
In the initial DPTF refactor, the scope of the TCPU device was incorrectly set as _SB, instead of _SB.PCI0. However, because of the way that the acpi_inject_dsdt() callback currently works (it injects contents before the dsdt.aml file), the Scope where the TCPU device lives (_SB.PCI0) doesn't exist yet. Therefore, to avoid playing games with *when* things are defined in the DSDT, switch to defining all of the DPTF devices in the SSDT.
Signed-off-by: Tim Wawrzynczak twawrzynczak@chromium.org Change-Id: Ia4922b4dc6544d79d44d39e6ad18c6ab9fee0fd7 Reviewed-on: https://review.coreboot.org/c/coreboot/+/43529 Reviewed-by: Duncan Laurie dlaurie@chromium.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/acpi/acpigen_dptf.c M src/drivers/intel/dptf/dptf.c M src/include/acpi/acpigen_dptf.h 3 files changed, 149 insertions(+), 111 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/acpi/acpigen_dptf.c b/src/acpi/acpigen_dptf.c index c884926..0c44b8f 100644 --- a/src/acpi/acpigen_dptf.c +++ b/src/acpi/acpigen_dptf.c @@ -3,9 +3,6 @@ #include <acpi/acpigen.h> #include <acpi/acpigen_dptf.h>
-/* Hardcoded paths */ -#define TOPLEVEL_DPTF_SCOPE "\_SB.DPTF" - /* Defaults */ #define DEFAULT_RAW_UNIT "ma"
@@ -81,14 +78,27 @@ static char scope[16];
if (participant == DPTF_CPU) - snprintf(scope, sizeof(scope), "\_SB.%s", namestring_of(participant)); + snprintf(scope, sizeof(scope), TCPU_SCOPE ".%s", namestring_of(participant)); else - snprintf(scope, sizeof(scope), TOPLEVEL_DPTF_SCOPE ".%s", + snprintf(scope, sizeof(scope), DPTF_DEVICE_PATH ".%s", namestring_of(participant));
return scope; }
+/* + * Most of the DPTF participants are underneath the _SB.DPTF scope, so we can just get away + * with using the simple namestring for references, but the TCPU has a different scope, so + * either an absolute or relative path must be used instead. + */ +static const char *path_of(enum dptf_participant participant) +{ + if (participant == DPTF_CPU) + return scope_of(participant); + else + return namestring_of(participant); +} + /* Write out scope of a participant */ void dptf_write_scope(enum dptf_participant participant) { @@ -111,7 +121,7 @@ if (!max_count || policies[0].target == DPTF_NONE) return;
- acpigen_write_scope(TOPLEVEL_DPTF_SCOPE); + acpigen_write_scope(DPTF_DEVICE_PATH); acpigen_write_method("_ART", 0);
/* Return this package */ @@ -133,8 +143,8 @@
/* Source, Target, Percent, Fan % for each of _AC0 ... _AC9 */ acpigen_write_package(13); - acpigen_emit_namestring(namestring_of(DPTF_FAN)); - acpigen_emit_namestring(namestring_of(policies[i].target)); + acpigen_emit_namestring(path_of(DPTF_FAN)); + acpigen_emit_namestring(path_of(policies[i].target)); acpigen_write_integer(DEFAULT_IF_0(policies[i].weight, DEFAULT_WEIGHT));
/* Write out fan %; corresponds with target's _ACx methods */ @@ -206,7 +216,7 @@ if (!max_count || policies[0].source == DPTF_NONE) return;
- acpigen_write_scope(TOPLEVEL_DPTF_SCOPE); + acpigen_write_scope(DPTF_DEVICE_PATH);
/* * A _TRT Revision (TRTR) of 1 means that the 'Priority' field is an arbitrary priority @@ -234,8 +244,8 @@ acpigen_write_package(8);
/* Source, Target, Priority, Sampling Period */ - acpigen_emit_namestring(namestring_of(policies[i].source)); - acpigen_emit_namestring(namestring_of(policies[i].target)); + acpigen_emit_namestring(path_of(policies[i].source)); + acpigen_emit_namestring(path_of(policies[i].target)); acpigen_write_integer(DEFAULT_IF_0(policies[i].priority, DEFAULT_PRIORITY)); acpigen_write_integer(to_acpi_time(policies[i].period));
@@ -458,7 +468,7 @@ if (!pkg_count) return;
- acpigen_write_scope(TOPLEVEL_DPTF_SCOPE); + acpigen_write_scope(DPTF_DEVICE_PATH); acpigen_write_name("IDSP"); acpigen_write_package(pkg_count);
diff --git a/src/drivers/intel/dptf/dptf.c b/src/drivers/intel/dptf/dptf.c index 1e4063e..4bc6b08 100644 --- a/src/drivers/intel/dptf/dptf.c +++ b/src/drivers/intel/dptf/dptf.c @@ -58,52 +58,6 @@ return "DPTF"; }
-/* Add custom tables and methods to SSDT */ -static void dptf_fill_ssdt(const struct device *dev) -{ - struct drivers_intel_dptf_config *config = config_of(dev); - enum dptf_participant p; - bool tsr_en[DPTF_MAX_TSR] = {false}; - int i; - - for (p = DPTF_TEMP_SENSOR_0, i = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++i) - tsr_en[i] = is_participant_used(config, p); - - /* Policies */ - dptf_write_enabled_policies(config->policies.active, DPTF_MAX_ACTIVE_POLICIES, - config->policies.passive, DPTF_MAX_PASSIVE_POLICIES, - config->policies.critical, DPTF_MAX_CRITICAL_POLICIES); - - dptf_write_active_policies(config->policies.active, - DPTF_MAX_ACTIVE_POLICIES); - - dptf_write_passive_policies(config->policies.passive, - DPTF_MAX_PASSIVE_POLICIES); - - dptf_write_critical_policies(config->policies.critical, - DPTF_MAX_CRITICAL_POLICIES); - - /* Controls */ - dptf_write_charger_perf(config->controls.charger_perf, DPTF_MAX_CHARGER_PERF_STATES); - dptf_write_fan_perf(config->controls.fan_perf, DPTF_MAX_FAN_PERF_STATES); - dptf_write_power_limits(&config->controls.power_limits); - - /* Fan options */ - dptf_write_fan_options(config->options.fan.fine_grained_control, - config->options.fan.step_size, - config->options.fan.low_speed_notify); - - /* TSR options */ - for (p = DPTF_TEMP_SENSOR_0, i = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++i) { - if (tsr_en[i]) { - dptf_write_tsr_hysteresis(config->options.tsr[i].hysteresis); - dptf_write_STR(config->options.tsr[i].desc); - } - } - - 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) { @@ -112,6 +66,7 @@ ACPI_STATUS_DEVICE_ALL_OFF; }
+/* Devices with GENERIC _HID (distinguished by PTYP) */ static void dptf_write_generic_participant(const char *name, enum dptf_generic_participant_type ptype, const char *str, int sta_val) @@ -120,13 +75,11 @@ static int generic_uid = 0;
acpigen_write_device(name); - - if (CONFIG(DPTF_USE_EISA_HID)) { - acpigen_write_name("_HID"); + acpigen_write_name("_HID"); + if (CONFIG(DPTF_USE_EISA_HID)) acpigen_emit_eisaid(GENERIC_HID_EISAID); - } else { - acpigen_write_name_string("_HID", GENERIC_HID); - } + else + acpigen_write_string(GENERIC_HID);
acpigen_write_name_integer("_UID", generic_uid++); acpigen_write_STA(sta_val); @@ -139,62 +92,40 @@ acpigen_pop_len(); /* Device */ }
-/* Add static definitions of DPTF devices into the DSDT */ -static void dptf_inject_dsdt(const struct device *dev) +/* _SB.PCI0.TCPU */ +static void write_tcpu(const struct device *pci_dev, + const struct drivers_intel_dptf_config *config) { - const struct drivers_intel_dptf_config *config; - enum dptf_participant participant; - struct device *parent; - char name[5]; - int i; - - /* 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"); - - /* DPTF CPU device - _SB.TCPU */ + /* DPTF CPU device - _SB.PCI0.TCPU */ + acpigen_write_scope(TCPU_SCOPE); acpigen_write_device("TCPU"); - acpigen_write_ADR_pci_device(parent); + acpigen_write_ADR_pci_device(pci_dev); acpigen_write_STA(get_STA_value(config, DPTF_CPU)); acpigen_pop_len(); /* Device */ + acpigen_pop_len(); /* TCPU Scope */ +}
- /* Toplevel DPTF device - _SB.DPTF*/ - acpigen_write_device(acpi_device_name(dev)); - 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); - 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 - */ - +/* _SB.DPTF.TFN1 */ +static void write_fan(const struct drivers_intel_dptf_config *config) +{ acpigen_write_device("TFN1"); - if (CONFIG(DPTF_USE_EISA_HID)) { - acpigen_write_name("_HID"); + acpigen_write_name("_HID"); + if (CONFIG(DPTF_USE_EISA_HID)) acpigen_emit_eisaid(FAN_HID_EISAID); - } else { - acpigen_write_name_string("_HID", FAN_HID); - } + else + acpigen_write_string(FAN_HID);
acpigen_write_name_integer("_UID", 0); acpigen_write_STA(get_STA_value(config, DPTF_FAN)); acpigen_pop_len(); /* Device */ +} + +/* _SB.DPTF.xxxx */ +static void write_generic_devices(const struct drivers_intel_dptf_config *config) +{ + enum dptf_participant participant; + char name[ACPI_NAME_BUFFER_SIZE]; + int i;
dptf_write_generic_participant("TCHG", DPTF_GENERIC_PARTICIPANT_TYPE_CHARGER, DEFAULT_CHARGER_STR, get_STA_value(config, @@ -205,17 +136,110 @@ dptf_write_generic_participant(name, DPTF_GENERIC_PARTICIPANT_TYPE_TSR, NULL, get_STA_value(config, participant)); } +}
- acpigen_pop_len(); /* DPTF Device */ +/* _SB.DPTF - note: leaves the Scope open for child devices*/ +static void write_open_dptf_device(const struct device *dev) +{ + acpigen_write_scope("\_SB"); + acpigen_write_device(acpi_device_name(dev)); + acpigen_write_name("_HID"); + if (CONFIG(DPTF_USE_EISA_HID)) + acpigen_emit_eisaid(DPTF_DEVICE_HID_EISAID); + else + acpigen_write_string(DPTF_DEVICE_HID); + + acpigen_write_name_integer("_UID", 0); + acpigen_write_STA(ACPI_STATUS_DEVICE_ALL_ON); +} + +/* Add minimal definitions of DPTF devices into the SSDT */ +static void write_device_definitions(const struct device *dev) +{ + const struct drivers_intel_dptf_config *config; + struct device *parent; + + /* 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); + write_tcpu(parent, config); + write_open_dptf_device(dev); + write_fan(config); + write_generic_devices(config); + + acpigen_pop_len(); /* DPTF Device (write_open_dptf_device) */ acpigen_pop_len(); /* Scope */ }
+/* Emites policy definitions for each policy type */ +static void write_policies(const struct drivers_intel_dptf_config *config) +{ + dptf_write_enabled_policies(config->policies.active, DPTF_MAX_ACTIVE_POLICIES, + config->policies.passive, DPTF_MAX_PASSIVE_POLICIES, + config->policies.critical, DPTF_MAX_CRITICAL_POLICIES); + + dptf_write_active_policies(config->policies.active, + DPTF_MAX_ACTIVE_POLICIES); + + dptf_write_passive_policies(config->policies.passive, + DPTF_MAX_PASSIVE_POLICIES); + + dptf_write_critical_policies(config->policies.critical, + DPTF_MAX_CRITICAL_POLICIES); +} + +/* Writes other static tables that are used by DPTF */ +static void write_controls(const struct drivers_intel_dptf_config *config) +{ + dptf_write_charger_perf(config->controls.charger_perf, DPTF_MAX_CHARGER_PERF_STATES); + dptf_write_fan_perf(config->controls.fan_perf, DPTF_MAX_FAN_PERF_STATES); + dptf_write_power_limits(&config->controls.power_limits); +} + +/* Options to control the behavior of devices */ +static void write_options(const struct drivers_intel_dptf_config *config) +{ + enum dptf_participant p; + int i; + + /* Fan options */ + dptf_write_fan_options(config->options.fan.fine_grained_control, + config->options.fan.step_size, + config->options.fan.low_speed_notify); + + /* TSR options */ + for (p = DPTF_TEMP_SENSOR_0, i = 0; p <= DPTF_TEMP_SENSOR_3; ++p, ++i) { + if (is_participant_used(config, p)) { + dptf_write_tsr_hysteresis(config->options.tsr[i].hysteresis); + dptf_write_STR(config->options.tsr[i].desc); + } + } +} + +/* Add custom tables and methods to SSDT */ +static void dptf_fill_ssdt(const struct device *dev) +{ + struct drivers_intel_dptf_config *config = config_of(dev); + + write_device_definitions(dev); + write_policies(config); + write_controls(config); + write_options(config); + + printk(BIOS_INFO, DPTF_DEVICE_PATH ": %s at %s\n", dev->chip_ops->name, dev_path(dev)); +} + static struct device_operations dptf_ops = { .read_resources = noop_read_resources, .set_resources = noop_set_resources, .acpi_name = dptf_acpi_name, .acpi_fill_ssdt = dptf_fill_ssdt, - .acpi_inject_dsdt = dptf_inject_dsdt, };
static void dptf_enable_dev(struct device *dev) diff --git a/src/include/acpi/acpigen_dptf.h b/src/include/acpi/acpigen_dptf.h index 89b64f3..1790df7 100644 --- a/src/include/acpi/acpigen_dptf.h +++ b/src/include/acpi/acpigen_dptf.h @@ -9,6 +9,10 @@ /* A common idiom is to use a default value if none is provided (i.e., == 0) */ #define DEFAULT_IF_0(thing, default_) ((thing) ? (thing) : (default_))
+/* Hardcoded paths */ +#define DPTF_DEVICE_PATH "\_SB.DPTF" +#define TCPU_SCOPE "\_SB.PCI0" + /* List of available participants (i.e., they can participate in policies) */ enum dptf_participant { DPTF_NONE,