Date: Fri, 22 Feb 2008 17:56:49 +0200 From: Andriy Gapon <avg@icyb.net.ua> To: Nate Lawson <nate@root.org> Cc: freebsd-acpi@freebsd.org Subject: Re: acpi_throttle: quirk based on pci info Message-ID: <47BEF0C1.5090800@icyb.net.ua> In-Reply-To: <47BB2D1F.8000008@icyb.net.ua> References: <47B96989.6070008@icyb.net.ua> <98F7E48C-1CBF-494D-8411-D80E25247214@FreeBSD.org> <47BAF97A.80405@icyb.net.ua> <47BAFB27.1050509@root.org> <47BB2D1F.8000008@icyb.net.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------030404020107030602050006 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit on 19/02/2008 21:25 Andriy Gapon said the following: >> Andriy Gapon wrote: >>>> On Feb 18, 2008, at 11:18 AM, Andriy Gapon wrote: >>>> >>>>> While looking for something else I accidentally noticed that >>>>> acpi_throttle has one quirk for some early revisions of PIIX4 chipset >>>>> and the quirk is enabled based on PCI info. >>>>> I have a newer revision of PIIX4 so the quirk is not applicable in >>>>> my case. >>>>> >>>>> Nevertheless I noticed that acpi_throttle is initialized before PCI >>>>> bus >>>>> driver, so when it calls pci_find_device() it always returns NULL and >>>>> quirk is not applied. At least this is what I see in dmesg on my >>>>> machine. [snip] >>> I.e. no identify method for acpi bus. >>> Additional device/driver for pci bus. >>> pci probe method checks for duplicates and adds the acpi device as a >>> child to acpi bus. >>> pci probe method sets quirks based on pci info. >>> pci probe method runs device_probe_and_attach on the acpi device. >>> as a consequence acpi probe and attach (for successful probe) are executed. [snip] > I have a bit of a practical interest in it: I have a system based on > PIIX4E (as you already know). Its ACPI FADT specifies duty offset of 1 > and duty width of 1. So, according to FADT only 50% throttling is available. > On the other hand, chipset specification updates for PIIX4E and PIIX4M > say that PCNTRL (P_CNT) register has 3 bits (at offset 1 indeed) for > controlling throttling in 12.5% steps. > So it seems, that acpi_throttle controls only the lowest bit of the > three; moreover, it seems that BIOS assigns some initial value to those > bits. So when throttling is active those bits do not have 001 value > corresponding to 87.5% duty cycle, but have some XX1 value and that > value seems to be 111 (12.5% duty cycle), because the system becomes > extremely slow. > So, as experiment, I hardcoded duty width to 3 and everything works > great - I have 8 throttling steps and system performance seems to really > correspond to expected duty levels. > So now I am thinking about writing a proper patch for this (positive) quirk. Please find attached a patch that makes sure that acpi_thermal is initialized after PCI bus is available, so that it is possible to decide about hardware quirks based on PCI information. The code uses the same approach as ichss does. The patch is tested to work. NOTE: This patch in contaminated! It contains code that forces throttle duty width to be 3 bits for PIIX4E and PIIX4M. This is in accordance with the chipsets specifications. Some motherboard/bios vendors lie about this value in FADT. If not accepted, this quirk can be easily stripped from the patch as its code is contained in distinct hunks. > Reference: > http://www.intel.com/design/chipsets/specupdt/29773817.pdf > -- Andriy Gapon --------------030404020107030602050006 Content-Type: text/x-patch; name="acpi_throttle-piix4-width.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="acpi_throttle-piix4-width.patch" --- acpi_throttle.c.orig 2008-02-21 21:08:28.000000000 +0200 +++ acpi_throttle.c 2008-02-21 21:16:25.000000000 +0200 @@ -80,18 +80,21 @@ (CPU_SPEED_PERCENT(x) % 10) #define CPU_P_CNT_THT_EN (1<<4) #define CPU_QUIRK_NO_THROTTLE (1<<1) /* Throttling is not usable. */ +#define CPU_QUIRK_PIIX4_WIDTH (1<<2) #define PCI_VENDOR_INTEL 0x8086 #define PCI_DEVICE_82371AB_3 0x7113 /* PIIX4 chipset for quirks. */ #define PCI_REVISION_A_STEP 0 #define PCI_REVISION_B_STEP 1 +#define PCI_REVISION_4E 2 +#define PCI_REVISION_4M 3 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); +static int acpi_throttle_pci_probe(device_t dev); static int acpi_throttle_probe(device_t dev); static int acpi_throttle_attach(device_t dev); static int acpi_throttle_evaluate(struct acpi_throttle_softc *sc); @@ -104,7 +107,6 @@ static device_method_t acpi_throttle_methods[] = { /* Device interface */ - DEVMETHOD(device_identify, acpi_throttle_identify), DEVMETHOD(device_probe, acpi_throttle_probe), DEVMETHOD(device_attach, acpi_throttle_attach), @@ -125,25 +127,46 @@ static devclass_t acpi_throttle_devclass; DRIVER_MODULE(acpi_throttle, cpu, acpi_throttle_driver, acpi_throttle_devclass, 0, 0); +MODULE_DEPEND(acpi_throttle, acpi, 1, 1, 1); -static void -acpi_throttle_identify(driver_t *driver, device_t parent) +static device_method_t acpi_throttle_pci_methods[] = { + DEVMETHOD(device_probe, acpi_throttle_pci_probe), + {0, 0} +}; + +static driver_t acpi_throttle_pci_driver = { + "acpi_throttle_pci", + acpi_throttle_pci_methods, + 0 +}; + +static devclass_t acpi_throttle_pci_devclass; +DRIVER_MODULE(acpi_throttle_pci, pci, acpi_throttle_pci_driver, + acpi_throttle_pci_devclass, 0, 0); + +static int +acpi_throttle_pci_probe(device_t dev) { ACPI_BUFFER buf; ACPI_HANDLE handle; ACPI_OBJECT *obj; + device_t parent; + device_t child; + + parent = devclass_get_device(devclass_find("acpi"), 0); + KASSERT(parent != NULL, ("acpi parent is NULL")); /* Make sure we're not being doubly invoked. */ if (device_find_child(parent, "acpi_throttle", -1)) - return; + return (ENXIO); /* Check for a valid duty width and parent CPU type. */ handle = acpi_get_handle(parent); if (handle == NULL) - return; + return (ENXIO); if (AcpiGbl_FADT.DutyWidth == 0 || acpi_get_type(parent) != ACPI_TYPE_PROCESSOR) - return; + return (ENXIO); /* * Add a child if there's a non-NULL P_BLK and correct length, or @@ -152,14 +175,18 @@ buf.Pointer = NULL; buf.Length = ACPI_ALLOCATE_BUFFER; if (ACPI_FAILURE(AcpiEvaluateObject(handle, NULL, NULL, &buf))) - return; + return (ENXIO); 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) + child = BUS_ADD_CHILD(parent, 0, "acpi_throttle", -1); + if (child == NULL) device_printf(parent, "add throttle child failed\n"); + else + device_probe_and_attach(child); } AcpiOsFree(obj); + return (ENXIO); } static int @@ -247,6 +274,8 @@ } if (cpu_duty_width == 0 || (thr_quirks & CPU_QUIRK_NO_THROTTLE) != 0) return (ENXIO); + if ((thr_quirks & CPU_QUIRK_PIIX4_WIDTH) != 0) + cpu_duty_width = 3; /* Validate the duty offset/width. */ duty_end = cpu_duty_offset + cpu_duty_width - 1; @@ -333,8 +362,14 @@ case PCI_REVISION_A_STEP: case PCI_REVISION_B_STEP: thr_quirks |= CPU_QUIRK_NO_THROTTLE; + device_printf(sc->cpu_dev, + "throttling is disabled for PIIX4 rev. A/B\n"); break; - default: + case PCI_REVISION_4E: + case PCI_REVISION_4M: + thr_quirks |= CPU_QUIRK_PIIX4_WIDTH; + device_printf(sc->cpu_dev, + "throttling has 12.5%% step for PIIX4E/M\n"); break; } } --------------030404020107030602050006--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47BEF0C1.5090800>