Date: Tue, 1 Jun 2004 17:42:57 -0400 From: John Baldwin <jhb@FreeBSD.org> To: Nate Lawson <nate@root.org> Cc: marcel@FreeBSD.org Subject: Re: Patch: Defer bus_config_intr() until bus_alloc_resource().. Message-ID: <200406011742.57991.jhb@FreeBSD.org> In-Reply-To: <20040601141424.I29932@root.org> References: <200406011531.09077.jhb@FreeBSD.org> <200406011638.04400.jhb@FreeBSD.org> <20040601141424.I29932@root.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve" = http://www.FreeBSD.org
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200406011742.57991.jhb>