On 11/20/2013 05:27 PM, Rafael J. Wysocki wrote:
On Wednesday, November 20, 2013 02:24:29 PM Al Stone wrote:
On 11/17/2013 03:06 PM, Rafael J. Wysocki wrote:
On Saturday, November 09, 2013 06:36:14 PM al.stone@linaro.org wrote:
From: Al Stone ahs3@redhat.com
-ENOCHANGELOG
Yup. Will be added.
Signed-off-by: Al Stone al.stone@linaro.org
drivers/acpi/bus.c | 3 ++- drivers/acpi/osl.c | 10 ++++++---- drivers/acpi/pci_link.c | 14 ++++++++------ 3 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index b587ec8..6a54dd5 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -540,7 +540,8 @@ void __init acpi_early_init(void) goto error0; }
-#ifdef CONFIG_X86 +#if (!CONFIG_ACPI_REDUCED_HARDWARE)
Why don't you use #ifndef here?
No particular reason; I'll change it.
- /* NB: in HW reduced mode, FADT sci_interrupt has no meaning */
I'm not sure what the "NB" stands for, but it looks like that's what "NOTE:" is used for elsewhere.
Ah. Whups. "NB" == "Nota Bene" -- Latin for "note well" and a personal habit when writing. Yes, it should be "NOTE:".
if (!acpi_ioapic) { /* compatible (0) means level (3) */ if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 54a20ff..017b85c 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
static acpi_osd_handler acpi_irq_handler; static void *acpi_irq_context; +static u32 acpi_irq_number; static struct workqueue_struct *kacpid_wq; static struct workqueue_struct *kacpi_notify_wq; static struct workqueue_struct *kacpi_hotplug_wq; @@ -797,9 +798,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
/* * ACPI interrupts different from the SCI in our copy of the FADT are
* not supported.
* not supported, except in HW reduced mode.
- if (gsi != acpi_gbl_FADT.sci_interrupt)
- if (!acpi_gbl_reduced_hardware && (gsi != acpi_gbl_FADT.sci_interrupt))
The inner parens are not necessary.
Ack.
Also it seems that we may need to support gsi != acpi_gbl_FADT.sci_interrupt generically, because there may be GPE device objects with interrupts different from the SCI.
In reduced HW mode, there are no GPE blocks defined; all interrupts of that nature are required to use GPIO interrupts instead, afaict.
Well, I'm not sure about that. The GPE0/1 blocks in FADT are not supposed to be present, but does that apply to GPE block devices (Section 9.10 of ACPI 5.0)?
The spec unfortunately has this info scattered through out -- the earlier parts of the spec discussing the reduced HW mode and the discussion around the FADT go into some of the details.
Any more precise pointers?
Anyway, the point was that we may need to support interrupts different from acpi_gbl_FADT.sci_interrupt even if the HW reduced mode is *not* used, so making that depend on acpi_gbl_reduced_hardware doesn't seem quite right.
Yeah, sorry, I should have included the pointers earlier. I'm basing my thinking on my understanding of these sections:
-- Section 4.1 which defines HW reduced ACPI, and specifically on 4.1.1, Hardware-Reduced Events.
-- Section 5.2.9 defining the FADT and the HW reduced restrictions
-- Section 5.6.5, GPIO-signaled ACPI events
-- Section 9.10, GPE block device
The way I read 9.10 in particular is why I'm thinking that any use of acpi_gbl_FADT.sci_interrupt needs to go away in HW reduced mode. The first sentence says:
The GPE Block device is an optional device that allows a system designer to describe GPE blocks beyond the two that are described in the FADT.
The way I am interpreting that is that a GPE block device only makes sense as an extension of the GPE0/1 blocks, not as an independent device. Since using the GPE0/1 blocks is not allowed in reduced HW mode (see 5.2.9), we cannot extend them with a GPE block device.
That being said, I agree we should be able to install interrupt handlers other than acpi_gbl_FADT.sci_interrupt regardless of whether we are in legacy or reduced HW mode. So, I'm thinking that this block:
if (gsi != acpi_gbl_FADT.sci_interrupt) return AE_BAD_PARAMETER;
should just be removed from acpi_os_install_interrupt_handler().
Does that make sense?
return AE_BAD_PARAMETER; if (acpi_irq_handler)@@ -818,13 +819,14 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler, acpi_irq_handler = NULL; return AE_NOT_ACQUIRED; }
acpi_irq_number = irq;
return AE_OK; }
acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler) {
- if (irq != acpi_gbl_FADT.sci_interrupt)
- if (!acpi_gbl_reduced_hardware && (irq != acpi_gbl_FADT.sci_interrupt))
The inner parens are not necessary.
Ack.
return AE_BAD_PARAMETER; free_irq(irq, acpi_irq);@@ -1806,7 +1808,7 @@ acpi_status __init acpi_os_initialize1(void) acpi_status acpi_os_terminate(void) { if (acpi_irq_handler) {
acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
acpi_os_remove_interrupt_handler(acpi_irq_number, acpi_irq_handler);It looks like this could be one line now?
Yup. Will fix.
}diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c index 2652a61..c0ab28a 100644 --- a/drivers/acpi/pci_link.c +++ b/drivers/acpi/pci_link.c
I'm not sure how the comment changes below belong to this patch.
Sigh. They don't. Will omit.
@@ -23,7 +23,7 @@ * * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ *
- TBD:
- TBD:
1. Support more than one IRQ resource entry per link device (index).
- Implement start/stop mechanism and use ACPI Bus Driver facilities
for IRQ management (e.g. start()->_SRS).@@ -268,8 +268,8 @@ static int acpi_pci_link_get_current(struct acpi_pci_link *link) } }
- /*
* Query and parse _CRS to get the current IRQ assignment.
/*
* Query and parse _CRS to get the current IRQ assignment.*/
status = acpi_walk_resources(link->device->handle, METHOD_NAME__CRS,
@@ -415,7 +415,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq) /* * "acpi_irq_balance" (default in APIC mode) enables ACPI to use PIC Interrupt * Link Devices to move the PIRQs around to minimize sharing.
- "acpi_irq_nobalance" (default in PIC mode) tells ACPI not to move any PIC IRQs
- that the BIOS has already set to active. This is necessary because
- ACPI has no automatic means of knowing what ISA IRQs are used. Note that
@@ -433,7 +433,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq) * * Note that PCI IRQ routers have a list of possible IRQs, * which may not include the IRQs this table says are available.
- Since this heuristic can't tell the difference between a link
- that no device will attach to, vs. a link which may be shared
- by multiple active devices -- it is not optimal.
@@ -505,7 +505,9 @@ int __init acpi_irq_penalty_init(void) } }
Why don't you simply put
if (acpi_gbl_reduced_hardware) return 0;
here?
Aha. Much more obvious. Thanks. Will fix.
/* Add a penalty for the SCI */
- acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
- if (!acpi_gbl_reduced_hardware)
acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] +=PIRQ_PENALTY_PCI_USING;