From owner-freebsd-acpi@FreeBSD.ORG Tue Jun 1 14:42:21 2004 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 5679916A4CF for ; Tue, 1 Jun 2004 14:42:21 -0700 (PDT) Received: from mail2.speakeasy.net (mail2.speakeasy.net [216.254.0.202]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2253143D39 for ; Tue, 1 Jun 2004 14:42:21 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Received: (qmail 26041 invoked from network); 1 Jun 2004 21:42:20 -0000 Received: from dsl027-160-063.atl1.dsl.speakeasy.net (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender ) encrypted SMTP for ; 1 Jun 2004 21:42:19 -0000 Received: from 10.50.41.233 (gw1.twc.weather.com [216.133.140.1]) by server.baldwin.cx (8.12.11/8.12.11) with ESMTP id i51LgFuB059488; Tue, 1 Jun 2004 17:42:15 -0400 (EDT) (envelope-from jhb@FreeBSD.org) From: John Baldwin To: Nate Lawson Date: Tue, 1 Jun 2004 17:42:57 -0400 User-Agent: KMail/1.6 References: <200406011531.09077.jhb@FreeBSD.org> <200406011638.04400.jhb@FreeBSD.org> <20040601141424.I29932@root.org> In-Reply-To: <20040601141424.I29932@root.org> MIME-Version: 1.0 Content-Disposition: inline Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200406011742.57991.jhb@FreeBSD.org> X-Spam-Checker-Version: SpamAssassin 2.63 (2004-01-11) on server.baldwin.cx cc: freebsd-acpi@FreeBSD.org cc: marcel@FreeBSD.org Subject: Re: Patch: Defer bus_config_intr() until bus_alloc_resource().. X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 01 Jun 2004 21:42:21 -0000 On Tuesday 01 June 2004 05:23 pm, Nate Lawson wrote: > On Tue, 1 Jun 2004, John Baldwin wrote: > > On Tuesday 01 June 2004 04:25 pm, Nate Lawson wrote: > > > On Tue, 1 Jun 2004, John Baldwin wrote: > > > > I need the patch below in order to turn on bus_config_intr() when > > > > using the I/O APICs. The original problem is that the _CRS of link > > > > devices is configured which is the PIC IRQ and thus screws up intpins > > > > when using the APIC. It basically takes bus_config_intr() out of the > > > > resource parsing code and does the config when an IRQ is allocated > > > > via > > > > bus_alloc_resource() for normal devices, and when a PCI IRQ is routed > > > > for PCI link devices. > > > > > > I appreciate what you're trying to do but I don't like this approach. > > > Deferring half the parsing to alloc time and moving it from > > > acpi_resource.c results in a lot of unnecessary duplication and > > > layering violation. The real issue you're trying to work around is > > > that you want to defer the actual config_intr until you're sure which > > > intr you're going to use. > > > > Well, arguably it exposes an improper layering violation when > > bus_config_intr() was added. bus_config_intr() was probably added in the > > place that it is now simply because it was easier to do so. That doesn't > > mean it's in the correct place. I don't really consider having an > > ACPI-specific routine understand ACPI-specific resources. One > > possibility though might be to add a wrapper function to acpi_resource.c > > that does the bus_config_intr() on a passed-in pointer to an ACPI > > resource. That might reduce at least some of the duplication. > > I still don't like this. Right now we have a single parse routine whose > goal is to parse a resource object and then store the info it finds in a > FreeBSD-compatible format, via bus_set_resource(). Just because > bus_config_intr() doesn't stick to bus_set_resource() semantics doesn't > mean we should hack acpi to bits to fix it. FreeBSD has no format for this and I don't really have time to add multiple resource types (which is what ACPI does, and probably what we will need to do eventually) to struct resource. What I have done in the current form is added some wrapper functions to acpi_resource.c (so all the magic about what rid's match up, etc. is in one file) in the patch below. > > > Some suggestions... Make polarity and trigger real resource types > > > (sys/i386/include/resource.h) and do a bus_set_resource of them in the > > > resource parsing code. Then in the alloc code do a bus_get_resource > > > for them and then call BUS_CONFIG_INTR. Additionally, instead of doing > > > the deferred BUS_CONFIG_INTR in the alloc code, it should actually be > > > done in the MD code for bus_setup_intr(). This seems cleaner since > > > allocating an irq resource shouldn't poke the hw until > > > bus_setup_intr(). > > > > *sigh* Trigger and polarity are not resources, they are a property of > > the IRQ resource perhaps. Unfortunately, our rman(9) interface doesn't > > support type-specific resource attributes and I'm really not up for > > chainsawing rman(9) enough to get that into place. Getting this bug > > fixed is holding up a lot of other work. > > Yes, I agree they are properties of the irq resource. I'm not asking you > to go the full route of implementing sub-types in rman. > > The simplest and backwards-compatible way to fix this is to add a MD > SYS_RES_IRQ_CFG resource and set that in the parse routine. Then, in the > implementation of bus_setup_intr(), look for that resource and use it for > the intr configuration. The only deficiency here is that it creates a > "top-level" resource type instead of a sub-type, but since there are only > 6 resource types in i386/include/resource.h right now, this is hardly a > huge deviation. With this change, bus_config_intr() can go away (or be a > no-op for backwards API compat). > > This seems the simplest and cleanest approach. No, I don't think this is clean. Note that ia64 needs bus_config_intr() too. In fact, that's why it was added in the first place! --- //depot/vendor/freebsd/src/sys/dev/acpica/acpi.c 2004/05/06 01:05:39 +++ //depot/user/jhb/acpipci/dev/acpica/acpi.c 2004/06/01 14:05:57 @@ -855,9 +855,21 @@ { struct acpi_device *ad = device_get_ivars(child); struct resource_list *rl = &ad->ad_rl; + struct resource *res; + ACPI_RESOURCE ares; - return (resource_list_alloc(rl, bus, child, type, rid, start, end, count, - flags)); + res = resource_list_alloc(rl, bus, child, type, rid, start, end, count, + flags); + if (res == NULL || device_get_parent(child) != bus) + return (res); + switch (type) { + case SYS_RES_IRQ: + if (ACPI_FAILURE(acpi_lookup_irq_resource(dev, *rid, res, &ares))) + break; + acpi_config_intr(dev, &ares); + break; + } + return (res); } static int --- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_pcib.c 2004/05/05 19:22:26 +++ //depot/user/jhb/acpipci/dev/acpica/acpi_pcib.c 2004/06/01 14:05:57 @@ -121,10 +121,11 @@ ACPI_FUNCTION_TRACE((char *)(uintptr_t)__func__); + crsres = NULL; buf.Pointer = NULL; crsbuf.Pointer = NULL; prsbuf.Pointer = NULL; - interrupt = 255; + interrupt = PCI_INVALID_IRQ; /* ACPI numbers pins 0-3, not 1-4 like the BIOS. */ pin--; @@ -348,6 +349,7 @@ /* XXX Data.Irq and Data.ExtendedIrq are implicitly structure-copied. */ crsbuf.Pointer = NULL; + crsres = NULL; if (prsres->Id == ACPI_RSTYPE_IRQ) { resbuf.Id = ACPI_RSTYPE_IRQ; resbuf.Length = ACPI_SIZEOF_RESOURCE(ACPI_RESOURCE_IRQ); @@ -378,6 +380,7 @@ AcpiFormatException(status)); goto out; } + crsres = &resbuf; /* Return the interrupt we just routed. */ device_printf(pcib, "slot %d INT%c routed to irq %d via %s\n", @@ -386,6 +389,8 @@ interrupt = Interrupts[0]; out: + if (PCI_INTERRUPT_VALID(interrupt) && crsres != NULL) + acpi_config_intr(dev, crsres); if (crsbuf.Pointer != NULL) AcpiOsFree(crsbuf.Pointer); if (prsbuf.Pointer != NULL) @@ -393,6 +398,5 @@ if (buf.Pointer != NULL) AcpiOsFree(buf.Pointer); - /* XXX APIC_IO interrupt mapping? */ return_VALUE (interrupt); } --- //depot/vendor/freebsd/src/sys/dev/acpica/acpi_resource.c 2004/04/09 11:15:38 +++ //depot/user/jhb/acpipci/dev/acpica/acpi_resource.c 2004/06/01 14:05:57 @@ -44,6 +44,88 @@ #define _COMPONENT ACPI_BUS ACPI_MODULE_NAME("RESOURCE") +ACPI_STATUS +acpi_lookup_irq_resource(device_t dev, int rid, struct resource *res, + ACPI_RESOURCE *acpi_res) +{ + ACPI_BUFFER buf; + ACPI_STATUS status; + ACPI_RESOURCE *ares; + char *curr, *last; + int i, found; + + buf.Length = ACPI_ALLOCATE_BUFFER; + status = AcpiGetCurrentResources(acpi_get_handle(dev), &buf); + if (ACPI_FAILURE(status)) + break; + i = 0; + found = 0; + curr = buf.Pointer; + last = (char *)buf.Pointer + buf.Length; + while (curr < last) { + ares = (ACPI_RESOURCE *)curr; + curr += ares->Length; + switch (ares->Id) { + case ACPI_RSTYPE_END_TAG: + curr = last; + break; + case ACPI_RSTYPE_IRQ: + if (ares->Data.Irq.NumberOfInterrupts != 1) + break; + if (i != rid) { + i++; + break; + } + KASSERT(ares->Data.Irq.Interrupts[0] == + rman_get_start(res), ("IRQ resources do not match")); + *acpi_res = *ares; + found++; + break; + case ACPI_RSTYPE_EXT_IRQ: + if (ares->Data.ExtendedIrq.NumberOfInterrupts != 1) + break; + if (i != rid) { + i++; + break; + } + KASSERT(ares->Data.ExtendedIrq.Interrupts[0] == + rman_get_start(res), ("IRQ resources do not match")); + *acpi_res = *ares; + found++; + break; + } + } + AcpiOsFree(buf.Pointer); + if (found == 0) + return (AE_NOT_FOUND); + return (AE_OK); +} + +void +acpi_config_intr(device_t dev, ACPI_RESOURCE *res) +{ + u_int irq; + int pol, trig; + + switch (res->Id) { + case ACPI_RSTYPE_IRQ: + irq = res->Data.Irq.Interrupts[0]; + trig = res->Data.Irq.EdgeLevel; + pol = res->Data.Irq.ActiveHighLow; + break; + case ACPI_RSTYPE_EXT_IRQ: + irq = res->Data.ExtendedIrq.Interrupts[0]; + trig = res->Data.ExtendedIrq.EdgeLevel; + pol = res->Data.ExtendedIrq.ActiveHighLow; + break; + default: + panic("%s: bad resource type %u", __func__, res->Id); + } + BUS_CONFIG_INTR(dev, irq, (trig == ACPI_EDGE_SENSITIVE) ? + INTR_TRIGGER_EDGE : INTR_TRIGGER_LEVEL, (pol == ACPI_ACTIVE_HIGH) ? + INTR_POLARITY_HIGH : INTR_POLARITY_LOW); +} + /* * Fetch a device's resources and associate them with the device. * @@ -495,9 +577,6 @@ return; bus_set_resource(dev, SYS_RES_IRQ, cp->ar_nirq++, *irq, 1); - BUS_CONFIG_INTR(dev, *irq, (trig == ACPI_EDGE_SENSITIVE) ? - INTR_TRIGGER_EDGE : INTR_TRIGGER_LEVEL, (pol == ACPI_ACTIVE_HIGH) ? - INTR_POLARITY_HIGH : INTR_POLARITY_LOW); } static void --- //depot/vendor/freebsd/src/sys/dev/acpica/acpivar.h 2004/05/06 01:05:39 +++ //depot/user/jhb/acpipci/dev/acpica/acpivar.h 2004/06/01 14:05:57 @@ -283,6 +283,11 @@ extern ACPI_STATUS acpi_parse_resources(device_t dev, ACPI_HANDLE handle, struct acpi_parse_resource_set *set, void *arg); +ACPI_STATUS acpi_lookup_irq_resource(device_t dev, int rid, struct resource *res, + ACPI_RESOURCE *acpi_res); +void acpi_config_intr(device_t dev, ACPI_RESOURCE *res); + + /* ACPI event handling */ extern UINT32 acpi_event_power_button_sleep(void *context); extern UINT32 acpi_event_power_button_wake(void *context); -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org