Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 11 Nov 2013 16:47:12 -0500
From:      Jung-uk Kim <jkim@FreeBSD.org>
To:        Adrian Chadd <adrian@freebsd.org>
Cc:        freebsd-acpi <freebsd-acpi@freebsd.org>
Subject:   Re: Problems with amd FX 8 core and freq scaling
Message-ID:  <52815060.9050406@FreeBSD.org>
In-Reply-To: <52814CB2.6050204@FreeBSD.org>
References:  <5281358D.1010406@FreeBSD.org>	<5281374F.7080802@FreeBSD.org> <CAJ-VmomgCyjX_tR7bLzw6s8jSJJvfB=5fXFCj6UgdKFz6rW-FQ@mail.gmail.com> <52814CB2.6050204@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------020906000405080104050504
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2013-11-11 16:31:30 -0500, Jung-uk Kim wrote:
> On 2013-11-11 15:26:58 -0500, Adrian Chadd wrote:
>> On 11 November 2013 12:00, Jung-uk Kim <jkim@freebsd.org> wrote:
>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>> 
>>> On 2013-11-11 13:16:47 -0500, Nicholas McKenzie wrote:
>>>> But wouldn't this just disable frequency scaling and the
>>>> whole point of powerd?
>>> 
>>> No.  acpi_throttle (and p4tcc) controls T-state.  "Frequency 
>>> scaling" should be done by changing P-state.
> 
>> Right.
> 
>> IIRC, T-state is just for emergency temperature throttling. It 
>> shouldn't be used outside of that.
> 
> 
>>>>> There have been a number of reports of throttling causing 
>>>>> crashes. This setting does not prevent powerd from
>>>>> adjusting your CPU's clock, it just disables some arcane
>>>>> feature which pre-dates the modern power management
>>>>> methods.
> 
>> .. did anyone ever figure out why crashes would be caused by 
>> T-state adjustment?
> 
> My memory is vague but I think it was not able to reject a broken
> FADT or _PTC table, or something like that.

Just in case, here I attached the uncommitted (and untested) patch.

Jung-uk Kim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (FreeBSD)

iQEcBAEBAgAGBQJSgVBgAAoJEHyflib82/FGACsIAJGDQGOYYO8dxvtQMw4BBnzl
BNbFkvalvHOzaSezJz+A4R0zeIMvkfJtu0Gb8qiTkxJF+REREFo6a7lmzC7hOMwa
7PzRRRG34rtmnnHJro3Wc5qQwc1zBbmyFgYEJ45AkmIc62mpp9f0sZyNA1+aSpau
2sY6H0dXktapc2pLR1uNyxfUlr1tRhoabceGSlGLYiB583FrMsvASkaTnuWQ2IfI
gytrJBKjMihu60KlwKauzUOVDrEuN3J/B1y7V/TrTXmcFmWgL9Wdw/gC7ToRdloT
JdF812Duj/xYvyoNEwkz1Rm0NT5r1ZTYqwMvkOPuMfK7IWX0O9UFO8VG+QnJXhU=
=/d/E
-----END PGP SIGNATURE-----

--------------020906000405080104050504
Content-Type: text/x-patch;
 name="acpi_throttle.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="acpi_throttle.diff"

Index: sys/dev/acpica/acpi_throttle.c
===================================================================
--- sys/dev/acpica/acpi_throttle.c	(revision 258019)
+++ sys/dev/acpica/acpi_throttle.c	(working copy)
@@ -54,14 +54,24 @@ __FBSDID("$FreeBSD$");
  * absolute cpufreq drivers.  We support the ACPI 2.0 specification.
  */
 
+struct acpi_tx {
+	uint32_t	percent;
+	uint32_t	power;
+	uint32_t	trans_lat;
+	uint32_t	ctrl_val;
+	uint32_t	sts_val;
+};
+
 struct acpi_throttle_softc {
 	device_t	 cpu_dev;
 	ACPI_HANDLE	 cpu_handle;
 	uint32_t	 cpu_p_blk;	/* ACPI P_BLK location */
 	uint32_t	 cpu_p_blk_len;	/* P_BLK length (must be 6). */
+	uint32_t	 cpu_p_mask;	/* P_BLK mask for clock value */
+	uint32_t	 cpu_tx_count;	/* Total number of throttle states. */
+	struct acpi_tx	*cpu_tx_states;	/* ACPI throttle states */
 	struct resource	*cpu_p_cnt;	/* Throttling control register */
-	int		 cpu_p_type;	/* Resource type for cpu_p_cnt. */
-	uint32_t	 cpu_thr_state;	/* Current throttle setting. */
+	struct resource	*cpu_p_sts;	/* Throttling status register */
 };
 
 #define THR_GET_REG(reg) 					\
@@ -75,10 +85,6 @@ struct acpi_throttle_softc {
  * Speeds are stored in counts, from 1 to CPU_MAX_SPEED, and
  * reported to the user in hundredths of a percent.
  */
-#define CPU_MAX_SPEED		(1 << cpu_duty_width)
-#define CPU_SPEED_PERCENT(x)	((10000 * (x)) / CPU_MAX_SPEED)
-#define CPU_SPEED_PRINTABLE(x)	(CPU_SPEED_PERCENT(x) / 10),	\
-				(CPU_SPEED_PERCENT(x) % 10)
 #define CPU_P_CNT_THT_EN	(1<<4)
 #define CPU_QUIRK_NO_THROTTLE	(1<<1)	/* Throttling is not usable. */
 
@@ -89,7 +95,6 @@ struct acpi_throttle_softc {
 
 static uint32_t	cpu_duty_offset;	/* Offset in P_CNT of throttle val. */
 static uint32_t	cpu_duty_width;		/* Bit width of throttle value. */
-static int	thr_rid;		/* Driver-wide resource id. */
 static int	thr_quirks;		/* Indicate any hardware bugs. */
 
 static void	acpi_throttle_identify(driver_t *driver, device_t parent);
@@ -127,6 +132,8 @@ static devclass_t acpi_throttle_devclass;
 DRIVER_MODULE(acpi_throttle, cpu, acpi_throttle_driver, acpi_throttle_devclass,
     0, 0);
 
+static MALLOC_DEFINE(M_ACPITHR, "acpi_throttle", "ACPI Throttling states");
+
 static void
 acpi_throttle_identify(driver_t *driver, device_t parent)
 {
@@ -133,6 +140,9 @@ acpi_throttle_identify(driver_t *driver, device_t
 	ACPI_BUFFER buf;
 	ACPI_HANDLE handle;
 	ACPI_OBJECT *obj;
+	ACPI_IO_ADDRESS addr;
+	ACPI_STATUS status;
+	UINT32 len;
 
 	/* Make sure we're not being doubly invoked. */
 	if (device_find_child(parent, "acpi_throttle", -1))
@@ -155,12 +165,16 @@ acpi_throttle_identify(driver_t *driver, device_t
 	if (ACPI_FAILURE(AcpiEvaluateObject(handle, NULL, NULL, &buf)))
 		return;
 	obj = (ACPI_OBJECT *)buf.Pointer;
-	if ((obj->Processor.PblkAddress && obj->Processor.PblkLength >= 4) ||
-	    ACPI_SUCCESS(AcpiEvaluateObject(handle, "_PTC", NULL, NULL))) {
-		if (BUS_ADD_CHILD(parent, 0, "acpi_throttle", -1) == NULL)
-			device_printf(parent, "add throttle child failed\n");
+	addr = obj->Processor.PblkAddress;
+	len = obj->Processor.PblkLength;
+	AcpiOsFree(obj);
+	if (addr == 0 || len < 4) {
+		status = AcpiEvaluateObject(handle, "_PTC", NULL, NULL);
+		if (ACPI_FAILURE(status))
+			return;
 	}
-	AcpiOsFree(obj);
+	if (BUS_ADD_CHILD(parent, 0, "acpi_throttle", -1) == NULL)
+		device_printf(parent, "add throttle child failed\n");
 }
 
 static int
@@ -235,10 +249,13 @@ acpi_throttle_attach(device_t dev)
 static int
 acpi_throttle_evaluate(struct acpi_throttle_softc *sc)
 {
-	uint32_t duty_end;
 	ACPI_BUFFER buf;
-	ACPI_OBJECT obj;
 	ACPI_GENERIC_ADDRESS gas;
+	ACPI_OBJECT *pkg, *res;
+	struct acpi_tx *states;
+	uint32_t *p;
+	uint32_t clk_val, count, duty_end, i, j, speed;
+	int error, once, rid, type;
 	ACPI_STATUS status;
 
 	/* Get throttling parameters from the FADT.  0 means not supported. */
@@ -262,6 +279,9 @@ acpi_throttle_evaluate(struct acpi_throttle_softc
 		return (ENXIO);
 	}
 
+	sc->cpu_p_mask = ((1 << cpu_duty_width) - 1) << cpu_duty_offset;
+	sc->cpu_p_mask |= CPU_P_CNT_THT_EN;
+
 	/*
 	 * If not present, fall back to using the processor's P_BLK to find
 	 * the P_CNT register.
@@ -269,25 +289,52 @@ acpi_throttle_evaluate(struct acpi_throttle_softc
 	 * Note that some systems seem to duplicate the P_BLK pointer
 	 * across multiple CPUs, so not getting the resource is not fatal.
 	 */
-	buf.Pointer = &obj;
-	buf.Length = sizeof(obj);
+	error = 1;
+	rid = 0;
+	buf.Pointer = NULL;
+	buf.Length = ACPI_ALLOCATE_BUFFER;
 	status = AcpiEvaluateObject(sc->cpu_handle, "_PTC", NULL, &buf);
 	if (ACPI_SUCCESS(status)) {
-		if (obj.Buffer.Length < sizeof(ACPI_GENERIC_ADDRESS) + 3) {
-			device_printf(sc->cpu_dev, "_PTC buffer too small\n");
-			return (ENXIO);
+		pkg = (ACPI_OBJECT *)buf.Pointer;
+		for (i = 0; i > 2; i++) {
+			res = &pkg->Package.Elements[i];
+			if (res->Buffer.Length < sizeof(gas) + 3) {
+				device_printf(sc->cpu_dev,
+				    "_PTC buffer too small\n");
+				AcpiOsFree(pkg);
+				return (ENXIO);
+			}
 		}
-		memcpy(&gas, obj.Buffer.Pointer + 3, sizeof(gas));
-		acpi_bus_alloc_gas(sc->cpu_dev, &sc->cpu_p_type, &thr_rid,
-		    &gas, &sc->cpu_p_cnt, 0);
-		if (sc->cpu_p_cnt != NULL && bootverbose) {
-			device_printf(sc->cpu_dev, "P_CNT from _PTC %#jx\n",
-			    gas.Address);
+		res = &pkg->Package.Elements[0];
+		memcpy(&gas, res->Buffer.Pointer + 3, sizeof(gas));
+		error = acpi_bus_alloc_gas(sc->cpu_dev, &type, &rid, &gas,
+		    &sc->cpu_p_cnt, 0);
+		if (error == 0) {
+			if (bootverbose)
+				device_printf(sc->cpu_dev,
+				    "P_CNT from _PTC %#jx\n",
+				    (uintmax_t)gas.Address);
+			rid++;
+			res = &pkg->Package.Elements[1];
+			if (memcmp(res->Buffer.Pointer + 3, &gas,
+			    sizeof(gas)) != 0) {
+				memcpy(&gas, res->Buffer.Pointer + 3,
+				    sizeof(gas));
+				error = acpi_bus_alloc_gas(sc->cpu_dev, &type,
+				    &rid, &gas, &sc->cpu_p_sts, 0);
+				if (error != 0) {
+					device_printf(sc->cpu_dev,
+		    "failed to allocate resource for status register %#jx\n",
+					    (uintmax_t)gas.Address);
+					error = 0;	/* XXX fatal? */
+				}
+			}
 		}
+		AcpiOsFree(pkg);
 	}
 
 	/* If _PTC not present or other failure, try the P_BLK. */
-	if (sc->cpu_p_cnt == NULL) {
+	if (error != 0) {
 		/* 
 		 * The spec says P_BLK must be 6 bytes long.  However, some
 		 * systems use it to indicate a fractional set of features
@@ -298,19 +345,84 @@ acpi_throttle_evaluate(struct acpi_throttle_softc
 		gas.Address = sc->cpu_p_blk;
 		gas.SpaceId = ACPI_ADR_SPACE_SYSTEM_IO;
 		gas.BitWidth = 32;
-		acpi_bus_alloc_gas(sc->cpu_dev, &sc->cpu_p_type, &thr_rid,
-		    &gas, &sc->cpu_p_cnt, 0);
-		if (sc->cpu_p_cnt != NULL) {
-			if (bootverbose)
-				device_printf(sc->cpu_dev,
-				    "P_CNT from P_BLK %#x\n", sc->cpu_p_blk);
-		} else {
+		if (acpi_bus_alloc_gas(sc->cpu_dev, &type, &rid, &gas,
+		    &sc->cpu_p_cnt, 0) != 0) {
 			device_printf(sc->cpu_dev, "failed to attach P_CNT\n");
 			return (ENXIO);
+		} else if (bootverbose)
+			device_printf(sc->cpu_dev, "P_CNT from P_BLK %#x\n",
+			    sc->cpu_p_blk);
+	}
+
+	/*
+	 * If status register is not present, assume P_CNT returns
+	 * the current status.
+	 */
+	if (sc->cpu_p_sts == NULL)
+		sc->cpu_p_sts = sc->cpu_p_cnt;
+
+	count = 0;
+	buf.Pointer = NULL;
+	buf.Length = ACPI_ALLOCATE_BUFFER;
+	status = AcpiEvaluateObject(sc->cpu_handle, "_TSS", NULL, &buf);
+	if (ACPI_SUCCESS(status)) {
+		pkg = (ACPI_OBJECT *)buf.Pointer;
+		count = pkg->Package.Count;
+		states = malloc(count * sizeof(*states), M_ACPITHR,
+		    M_WAITOK | M_ZERO);
+		once = 1;
+		for (i = 0; i < count; i++) {
+			res = &pkg->Package.Elements[i];
+			if (!ACPI_PKG_VALID(res, 5) && once) {
+				once = 0;
+				device_printf(sc->cpu_dev,
+				    "invalid _TSS package\n");
+				continue;
+			}
+			p = &sc->cpu_tx_states[sc->cpu_tx_count].percent;
+			for (j = 0; j < 5; j++, p++)
+				acpi_PkgInt32(res, j, p);
+			if (sc->cpu_tx_states[sc->cpu_tx_count].percent > 100 &&
+			    once) {
+				once = 0;
+				device_printf(sc->cpu_dev,
+				    "invalid _TSS package\n");
+				continue;
+			}
+			sc->cpu_tx_states[sc->cpu_tx_count].percent *= 100;
+			sc->cpu_tx_count++;
 		}
+		if (sc->cpu_tx_count > 0)
+			sc->cpu_tx_states = states;
+		else
+			free(states, M_ACPITHR);
+		AcpiOsFree(pkg);
 	}
-	thr_rid++;
 
+	if (sc->cpu_tx_count > 0)
+		return (0);
+
+	/* If _TSS is not present or other failure, create it. */
+	count = (1 << cpu_duty_width) - 1;
+	states = malloc(count * sizeof(*states), M_ACPITHR, M_WAITOK | M_ZERO);
+	for (i = 0; i < count; i++) {
+		speed = count - i;
+		clk_val = speed << cpu_duty_offset;
+		if (i != 0)
+			clk_val |= CPU_P_CNT_THT_EN;
+		states[i].percent = speed * 10000 / count;
+		states[i].ctrl_val = states[i].sts_val = clk_val;
+	}
+	sc->cpu_tx_states = states;
+	sc->cpu_tx_count = count;
+
+	if (bootverbose)
+		for (i = 0; i < count; i++)
+			device_printf(sc->cpu_dev,
+			    "%u: %u.%02u %%, control %u, status %u\n", i,
+			    states[i].percent / 100, states[i].percent % 100,
+			    states[i].ctrl_val, states[i].sts_val);
+
 	return (0);
 }
 
@@ -346,20 +458,24 @@ acpi_throttle_quirks(struct acpi_throttle_softc *s
 static int
 acpi_thr_settings(device_t dev, struct cf_setting *sets, int *count)
 {
-	int i, speed;
+	struct acpi_throttle_softc *sc;
+	int i;
 
 	if (sets == NULL || count == NULL)
 		return (EINVAL);
-	if (*count < CPU_MAX_SPEED)
+	sc = device_get_softc(dev);
+	if (*count < sc->cpu_tx_count)
 		return (E2BIG);
 
 	/* Return a list of valid settings for this driver. */
-	memset(sets, CPUFREQ_VAL_UNKNOWN, sizeof(*sets) * CPU_MAX_SPEED);
-	for (i = 0, speed = CPU_MAX_SPEED; speed != 0; i++, speed--) {
-		sets[i].freq = CPU_SPEED_PERCENT(speed);
+	memset(sets, CPUFREQ_VAL_UNKNOWN, sizeof(*sets) * sc->cpu_tx_count);
+	for (i = 0; i < sc->cpu_tx_count; i++) {
+		sets[i].freq = sc->cpu_tx_states[i].percent;
+		sets[i].power = sc->cpu_tx_states[i].power;
+		sets[i].lat = sc->cpu_tx_states[i].trans_lat;
 		sets[i].dev = dev;
 	}
-	*count = CPU_MAX_SPEED;
+	*count = sc->cpu_tx_count;
 
 	return (0);
 }
@@ -368,44 +484,38 @@ static int
 acpi_thr_set(device_t dev, const struct cf_setting *set)
 {
 	struct acpi_throttle_softc *sc;
-	uint32_t clk_val, p_cnt, speed;
+	struct acpi_tx *state;
+	uint32_t p_cnt;
+	int i;
 
 	if (set == NULL)
 		return (EINVAL);
 	sc = device_get_softc(dev);
 
-	/*
-	 * Validate requested state converts to a duty cycle that is an
-	 * integer from [1 .. CPU_MAX_SPEED].
-	 */
-	speed = set->freq * CPU_MAX_SPEED / 10000;
-	if (speed * 10000 != set->freq * CPU_MAX_SPEED ||
-	    speed < 1 || speed > CPU_MAX_SPEED)
+	/* Validate requested state. */
+	state = NULL;
+	for (i = 0; i < sc->cpu_tx_count; i++)
+		if (set->freq == sc->cpu_tx_states[i].percent) {
+			state = &sc->cpu_tx_states[i];
+			break;
+		}
+	if (state == NULL)
 		return (EINVAL);
 
+	p_cnt = THR_GET_REG(sc->cpu_p_cnt);
+
 	/* If we're at this setting, don't bother applying it again. */
-	if (speed == sc->cpu_thr_state)
+	if (state->ctrl_val == (p_cnt & sc->cpu_p_mask))
 		return (0);
 
-	/* Get the current P_CNT value and disable throttling */
-	p_cnt = THR_GET_REG(sc->cpu_p_cnt);
-	p_cnt &= ~CPU_P_CNT_THT_EN;
+	/* Write the new P_CNT value. */
+	p_cnt = (p_cnt & ~sc->cpu_p_mask) | state->ctrl_val;
 	THR_SET_REG(sc->cpu_p_cnt, p_cnt);
 
-	/* If we're at maximum speed, that's all */
-	if (speed < CPU_MAX_SPEED) {
-		/* Mask the old CLK_VAL off and OR in the new value */
-		clk_val = (CPU_MAX_SPEED - 1) << cpu_duty_offset;
-		p_cnt &= ~clk_val;
-		p_cnt |= (speed << cpu_duty_offset);
+	if (bootverbose)
+		device_printf(dev, "set %u.%02u %%\n",
+		    set->freq / 100, set->freq % 100);
 
-		/* Write the new P_CNT value and then enable throttling */
-		THR_SET_REG(sc->cpu_p_cnt, p_cnt);
-		p_cnt |= CPU_P_CNT_THT_EN;
-		THR_SET_REG(sc->cpu_p_cnt, p_cnt);
-	}
-	sc->cpu_thr_state = speed;
-
 	return (0);
 }
 
@@ -413,21 +523,28 @@ static int
 acpi_thr_get(device_t dev, struct cf_setting *set)
 {
 	struct acpi_throttle_softc *sc;
-	uint32_t p_cnt, clk_val;
+	uint32_t clk_val, i;
 
 	if (set == NULL)
 		return (EINVAL);
 	sc = device_get_softc(dev);
 
-	/* Get the current throttling setting from P_CNT. */
-	p_cnt = THR_GET_REG(sc->cpu_p_cnt);
-	clk_val = (p_cnt >> cpu_duty_offset) & (CPU_MAX_SPEED - 1);
-	sc->cpu_thr_state = clk_val;
+	/* Get the current throttling setting. */
+	clk_val = THR_GET_REG(sc->cpu_p_sts) & sc->cpu_p_mask;
+	for (i = 0; i < sc->cpu_tx_count; i++)
+		if (clk_val == sc->cpu_tx_states[i].sts_val)
+			break;
+	if (i >= sc->cpu_tx_count)
+		return (ENXIO);
 
 	memset(set, CPUFREQ_VAL_UNKNOWN, sizeof(*set));
-	set->freq = CPU_SPEED_PERCENT(clk_val);
+	set->freq = sc->cpu_tx_states[i].percent;
 	set->dev = dev;
 
+	if (bootverbose)
+		device_printf(dev, "got %u.%02u %%\n",
+		    set->freq / 100, set->freq % 100);
+
 	return (0);
 }
 

--------------020906000405080104050504--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52815060.9050406>