From owner-freebsd-acpi@FreeBSD.ORG Sat Mar 1 08:50:15 2008 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C7CA31065673; Sat, 1 Mar 2008 08:50:15 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from falcon.cybervisiontech.com (falcon.cybervisiontech.com [217.20.163.9]) by mx1.freebsd.org (Postfix) with ESMTP id 4CCCD8FC13; Sat, 1 Mar 2008 08:50:14 +0000 (UTC) (envelope-from avg@icyb.net.ua) Received: from localhost (localhost [127.0.0.1]) by falcon.cybervisiontech.com (Postfix) with ESMTP id 7475A744013; Sat, 1 Mar 2008 10:50:12 +0200 (EET) X-Virus-Scanned: Debian amavisd-new at falcon.cybervisiontech.com Received: from falcon.cybervisiontech.com ([127.0.0.1]) by localhost (falcon.cybervisiontech.com [127.0.0.1]) (amavisd-new, port 10027) with ESMTP id IGj4xrRe+dry; Sat, 1 Mar 2008 10:50:12 +0200 (EET) Received: from [91.193.172.111] (unknown [91.193.172.111]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by falcon.cybervisiontech.com (Postfix) with ESMTP id 58E2C74400D; Sat, 1 Mar 2008 10:50:11 +0200 (EET) Message-ID: <47C918B7.9040504@icyb.net.ua> Date: Sat, 01 Mar 2008 10:49:59 +0200 From: Andriy Gapon User-Agent: Thunderbird 2.0.0.9 (X11/20071208) MIME-Version: 1.0 To: John Baldwin References: <47B96989.6070008@icyb.net.ua> <200802271126.30132.jhb@freebsd.org> <47C5E0A6.5030102@icyb.net.ua> <200802280856.31548.jhb@freebsd.org> In-Reply-To: <200802280856.31548.jhb@freebsd.org> Content-Type: multipart/mixed; boundary="------------000002010403000307090000" Cc: freebsd-acpi@freebsd.org Subject: Re: acpi_throttle: quirk based on pci info X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 01 Mar 2008 08:50:16 -0000 This is a multi-part message in MIME format. --------------000002010403000307090000 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit on 28/02/2008 15:56 John Baldwin said the following: > On Wednesday 27 February 2008 05:13:58 pm Andriy Gapon wrote: >> on 27/02/2008 18:26 John Baldwin said the following: >>> On Wednesday 27 February 2008 02:54:38 am Andriy Gapon wrote: >> [snip] >> John, >> >> thank you for this detailed reply! >> I looked through the code and I think that ichss is the only device that >> explicitly requires cpu and pci buses to be "configured". acpi_throttle >> is another one that implicitly requires that. >> My personal preference is to probe/attach pci first and then go with >> cpu. This is mostly because pci can provide a lot of useful information >> and resources to various devices. On the other hand, cpu mostly exists >> so that others could attach to it (it does provide a little bit, but >> it's a very little bit). So, in my opinion, it is more likely that a >> child of cpu would need something from pci than vice versa. >> >> If we agree on this order and implement it, then I agree with you that >> it would be quite easy to modify ichss to be a "normal" child of cpu and >> use pci_find_device to find a proper pci device. And the rest of the >> code that uses pci_read_config, bus_set_resource and >> bus_alloc_resource_any would remain practically the same. >> I'd even say that this would be a trivial change. >> And I'd even say that this would be a change in right direction, because >> ichss would lose most of its 'specialness'. > > Actually, we can make ichss rather simple w/o changing it much by having it > just set a global variable in its PCI probe routine and checking that global > when attaching to the CPU. > > One other thing that concerns me is that cpufreq drivers want to know about > each other (e.g. smist checks for ichss0, etc.). I'd rather that if we have > multiple drivers controlling the same back-end hardware (via difference > interfaces) they all use the same driver name (e.g. speedstep0) and use probe > priority to decide which driver wins if both ichss0 and smist0 can handle a > CPU for example. > > Here is a patch to make CPUs come after PCI and an attempt to fix ICH. Note > that if we made ichss_identify() manually look for the PCI device by bsf > instead of using a probe routine to find it we could remove the pci "driver" > completely and make it work on kldload. It also fixes a bug that the attempt > to enable SS via a PCI config register write could never work as it passed a > cpu0 device_t to pci_read_config/pci_write_config in ichss_probe() > previously. I moved this to attach() (where it belongs) and used the right > device_t so this should work now. I have no hardware to test it on though. > Here is a patch that implements find by bsf idea in ichss that we also discussed. I don't own the actual hardware but I "tested" the patch by temporarily changing some ids and adding printfs instead of some actions. Another little difference that I've added (just in case) a protection against double-adding. I also tested acpi part of your patch - works great! Here's one strange thing - in your patch you accidentally have parameters of device_identify switched, I initially inherited that bug too. I got no error/warning from compiler whatsoever. It wasn't until I saw that device_get_unit(parent) returned garbage that I my untrained eye noticed the mistake. -- Andriy Gapon --------------000002010403000307090000 Content-Type: text/x-patch; name="ichss.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="ichss.patch" --- ichss.c.orig 2006-05-16 17:36:24.000000000 +0300 +++ ichss.c 2008-03-01 01:37:53.000000000 +0200 @@ -91,7 +91,7 @@ (bus_space_write_1(rman_get_bustag((reg)), \ rman_get_bushandle((reg)), 0, (val))) -static int ichss_pci_probe(device_t dev); +static void ichss_identify(driver_t *driver, device_t parent); static int ichss_probe(device_t dev); static int ichss_attach(device_t dev); static int ichss_detach(device_t dev); @@ -103,6 +103,7 @@ static device_method_t ichss_methods[] = { /* Device interface */ + DEVMETHOD(device_identify, ichss_identify), DEVMETHOD(device_probe, ichss_probe), DEVMETHOD(device_attach, ichss_attach), DEVMETHOD(device_detach, ichss_detach), @@ -120,15 +121,7 @@ static devclass_t ichss_devclass; DRIVER_MODULE(ichss, cpu, ichss_driver, ichss_devclass, 0, 0); -static device_method_t ichss_pci_methods[] = { - DEVMETHOD(device_probe, ichss_pci_probe), - {0, 0} -}; -static driver_t ichss_pci_driver = { - "ichss_pci", ichss_pci_methods, 0 -}; -static devclass_t ichss_pci_devclass; -DRIVER_MODULE(ichss_pci, pci, ichss_pci_driver, ichss_pci_devclass, 0, 0); +static device_t ich_device; #if 0 #define DPRINT(x...) printf(x) @@ -136,70 +129,66 @@ #define DPRINT(x...) #endif -/* - * We detect the chipset by looking for its LPC bus ID during the PCI - * scan and reading its config registers during the probe. However, - * we add the ichss child under the cpu device since even though the - * chipset provides the control, it really affects the cpu only. - * - * XXX This approach does not work if the module is loaded after boot. - */ -static int -ichss_pci_probe(device_t dev) +static void +ichss_identify(driver_t *driver, device_t parent) { - device_t child, parent; + device_t child; uint32_t pmbase; + if (resource_disabled("ichss", 0)) + return; + /* - * TODO: add a quirk to disable if we see the 82815_MC along - * with the 82801BA and revision < 5. + * It appears that ICH SpeedStep only requires a single CPU to + * set the value (since the chipset is shared by all CPUs.) + * Thus, we only add a child to cpu 0. */ - if (pci_get_vendor(dev) != PCI_VENDOR_INTEL || - (pci_get_device(dev) != PCI_DEV_82801BA && - pci_get_device(dev) != PCI_DEV_82801CA && - pci_get_device(dev) != PCI_DEV_82801DB)) - return (ENXIO); + if (device_get_unit(parent) != 0) + return; - /* Only one CPU is supported for this hardware. */ - if (devclass_get_device(ichss_devclass, 0)) - return (ENXIO); + /* Avoid duplicates. */ + if (device_find_child(parent, "ichss", -1)) + return; /* - * Add a child under the CPU parent. It appears that ICH SpeedStep - * only requires a single CPU to set the value (since the chipset - * is shared by all CPUs.) Thus, we only add a child to cpu 0. + * ICH2/3/4-M I/O Controller Hub is at bus 0, slot 1F, function 0. + * E.g. see Section 6.1 "PCI Devices and Functions" and table 6.1 of + * Intel(r) 82801BA I/O Controller Hub 2 (ICH2) and Intel(r) 82801BAM + * I/O Controller Hub 2 Mobile (ICH2-M). */ - parent = devclass_get_device(devclass_find("cpu"), 0); - KASSERT(parent != NULL, ("cpu parent is NULL")); - child = BUS_ADD_CHILD(parent, 0, "ichss", 0); - if (child == NULL) { - device_printf(parent, "add SpeedStep child failed\n"); - return (ENXIO); - } + ich_device = pci_find_bsf(0, 0x1f, 0); + if (ich_device == NULL || + pci_get_vendor(ich_device) != PCI_VENDOR_INTEL || + (pci_get_device(ich_device) != PCI_DEV_82801BA && + pci_get_device(ich_device) != PCI_DEV_82801CA && + pci_get_device(ich_device) != PCI_DEV_82801DB)) + return; /* Find the PMBASE register from our PCI config header. */ - pmbase = pci_read_config(dev, ICHSS_PMBASE_OFFSET, sizeof(pmbase)); + pmbase = pci_read_config(ich_device, ICHSS_PMBASE_OFFSET, + sizeof(pmbase)); if ((pmbase & ICHSS_IO_REG) == 0) { printf("ichss: invalid PMBASE memory type\n"); - return (ENXIO); + return; } pmbase &= ICHSS_PMBASE_MASK; if (pmbase == 0) { printf("ichss: invalid zero PMBASE address\n"); - return (ENXIO); + return; } DPRINT("ichss: PMBASE is %#x\n", pmbase); + child = BUS_ADD_CHILD(parent, 0, "ichss", 0); + if (child == NULL) { + device_printf(parent, "add SpeedStep child failed\n"); + return; + } + /* Add the bus master arbitration and control registers. */ bus_set_resource(child, SYS_RES_IOPORT, 0, pmbase + ICHSS_BM_OFFSET, 1); bus_set_resource(child, SYS_RES_IOPORT, 1, pmbase + ICHSS_CTRL_OFFSET, 1); - - /* Attach the new CPU child now. */ - device_probe_and_attach(child); - - return (ENXIO); } static int @@ -207,10 +196,6 @@ { device_t est_dev, perf_dev; int error, type; - uint16_t ss_en; - - if (resource_disabled("ichss", 0)) - return (ENXIO); /* * If the ACPI perf driver has attached and is not just offering @@ -227,14 +212,6 @@ if (est_dev && device_is_attached(est_dev)) return (ENXIO); - /* Activate SpeedStep control if not already enabled. */ - ss_en = pci_read_config(dev, ICHSS_PMCFG_OFFSET, sizeof(ss_en)); - if ((ss_en & ICHSS_ENABLE) == 0) { - printf("ichss: enabling SpeedStep support\n"); - pci_write_config(dev, ICHSS_PMCFG_OFFSET, - ss_en | ICHSS_ENABLE, sizeof(ss_en)); - } - device_set_desc(dev, "SpeedStep ICH"); return (-1000); } @@ -243,6 +220,7 @@ ichss_attach(device_t dev) { struct ichss_softc *sc; + uint16_t ss_en; sc = device_get_softc(dev); sc->dev = dev; @@ -264,6 +242,14 @@ return (ENXIO); } + /* Activate SpeedStep control if not already enabled. */ + ss_en = pci_read_config(ich_device, ICHSS_PMCFG_OFFSET, sizeof(ss_en)); + if ((ss_en & ICHSS_ENABLE) == 0) { + device_printf(dev, "enabling SpeedStep support\n"); + pci_write_config(ich_device, ICHSS_PMCFG_OFFSET, + ss_en | ICHSS_ENABLE, sizeof(ss_en)); + } + /* Setup some defaults for our exported settings. */ sc->sets[0].freq = CPUFREQ_VAL_UNKNOWN; sc->sets[0].volts = CPUFREQ_VAL_UNKNOWN; --------------000002010403000307090000--