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
[-- Attachment #1 --]
-----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-----
[-- Attachment #2 --]
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?52815060.9050406>