Date: Thu, 28 Feb 2008 08:56:31 -0500 From: John Baldwin <jhb@freebsd.org> To: Andriy Gapon <avg@icyb.net.ua> Cc: freebsd-acpi@freebsd.org Subject: Re: acpi_throttle: quirk based on pci info Message-ID: <200802280856.31548.jhb@freebsd.org> In-Reply-To: <47C5E0A6.5030102@icyb.net.ua> References: <47B96989.6070008@icyb.net.ua> <200802271126.30132.jhb@freebsd.org> <47C5E0A6.5030102@icyb.net.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
--- //depot/vendor/freebsd/src/sys/dev/acpica/acpi.c 2008/01/28 02:00:16
+++ //depot/user/jhb/acpipci/dev/acpica/acpi.c 2008/02/28 13:34:35
@@ -1533,18 +1523,31 @@
static int
acpi_probe_order(ACPI_HANDLE handle, int *order)
{
+ ACPI_OBJECT_TYPE type;
+ u_int addr;
/*
* 1. I/O port and memory system resource holders
* 2. Embedded controllers (to handle early accesses)
* 3. PCI Link Devices
+ * 11 - 266. Host-PCI bridges sorted by _ADR
+ * 280. CPUs
*/
+ AcpiGetType(handle, &type);
if (acpi_MatchHid(handle, "PNP0C01") || acpi_MatchHid(handle, "PNP0C02"))
*order = 1;
else if (acpi_MatchHid(handle, "PNP0C09"))
*order = 2;
else if (acpi_MatchHid(handle, "PNP0C0F"))
*order = 3;
+ else if (acpi_MatchHid(handle, "PNP0A03")) {
+ if (ACPI_SUCCESS(acpi_GetInteger(handle, "_ADR", &addr)))
+ *order = 11 + ACPI_ADR_PCI_SLOT(addr) * (PCI_FUNCMAX + 1) +
+ ACPI_ADR_PCI_FUNC(addr);
+ else
+ *order = 11;
+ } else if (type == ACPI_TYPE_PROCESSOR)
+ *order = 280;
return (0);
}
@@ -1591,14 +1594,17 @@
break;
/*
- * Create a placeholder device for this node. Sort the placeholder
- * so that the probe/attach passes will run breadth-first. Orders
- * less than ACPI_DEV_BASE_ORDER are reserved for special objects
- * (i.e., system resources). Larger values are used for all other
- * devices.
+ * Create a placeholder device for this node. Sort the
+ * placeholder so that the probe/attach passes will run
+ * breadth-first. Orders less than ACPI_DEV_BASE_ORDER
+ * are reserved for special objects (i.e., system
+ * resources). Orders between ACPI_DEV_BASE_ORDER and 300
+ * are used for Host-PCI bridges (and effectively all
+ * their children) and CPUs. Larger values are used for
+ * all other devices.
*/
ACPI_DEBUG_PRINT((ACPI_DB_OBJECTS, "scanning '%s'\n", handle_str));
- order = (level + 1) * ACPI_DEV_BASE_ORDER;
+ order = level * 10 + 300;
acpi_probe_order(handle, &order);
child = BUS_ADD_CHILD(bus, order, NULL, -1);
if (child == NULL)
--- //depot/vendor/freebsd/src/sys/dev/cpufreq/ichss.c 2006/05/16 14:41:44
+++ //depot/user/jhb/acpipci/dev/cpufreq/ichss.c 2008/02/28 13:47:21
@@ -92,6 +92,7 @@
rman_get_bushandle((reg)), 0, (val)))
static int ichss_pci_probe(device_t dev);
+static void ichss_identify(device_t parent, driver_t *driver);
static int ichss_probe(device_t dev);
static int ichss_attach(device_t dev);
static int ichss_detach(device_t dev);
@@ -103,6 +104,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),
@@ -130,6 +132,8 @@
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)
#else
@@ -147,8 +151,6 @@
static int
ichss_pci_probe(device_t dev)
{
- device_t child, parent;
- uint32_t pmbase;
/*
* TODO: add a quirk to disable if we see the 82815_MC along
@@ -160,46 +162,55 @@
pci_get_device(dev) != PCI_DEV_82801DB))
return (ENXIO);
- /* Only one CPU is supported for this hardware. */
- if (devclass_get_device(ichss_devclass, 0))
- return (ENXIO);
+ ich_device = dev;
+ return (ENXIO);
+}
+
+static void
+ichss_identify(device_t parent, driver_t *driver)
+{
+ device_t child;
+ uint32_t pmbase;
+
+ if (ich_device == NULL)
+ return;
+
+ if (resource_disabled("ichss", 0))
+ 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.
+ * 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.
*/
- 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);
- }
+ if (device_get_unit(parent) != 0)
+ 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,11 +218,7 @@
{
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
* info, let it manage things. Also, if Enhanced SpeedStep is
@@ -227,14 +234,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 +242,7 @@
ichss_attach(device_t dev)
{
struct ichss_softc *sc;
+ uint16_t ss_en;
sc = device_get_softc(dev);
sc->dev = dev;
@@ -264,6 +264,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;
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200802280856.31548.jhb>