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>