Change in coreboot[master]: dptf: Fix scope of TCPU device - coreboot-gerrit

16 Jul 2020

Tim Wawrzynczak has uploaded this change for review. ( 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
---
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(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/29/43529/1

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,
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/43529
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ia4922b4dc6544d79d44d39e6ad18c6ab9fee0fd7
Gerrit-Change-Number: 43529
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Wawrzynczak twawrzynczak@chromium.org
Gerrit-Reviewer: Patrick Rudolph siro@das-labor.org
Gerrit-MessageType: newchange



    