Date: Sat, 01 Mar 2008 10:49:59 +0200 From: Andriy Gapon <avg@icyb.net.ua> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-acpi@freebsd.org Subject: Re: acpi_throttle: quirk based on pci info Message-ID: <47C918B7.9040504@icyb.net.ua> In-Reply-To: <200802280856.31548.jhb@freebsd.org> References: <47B96989.6070008@icyb.net.ua> <200802271126.30132.jhb@freebsd.org> <47C5E0A6.5030102@icyb.net.ua> <200802280856.31548.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?47C918B7.9040504>