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@xxxxxxxxxx wrote:
> >> From: Al Stone <ahs3@xxxxxxxxxx>
> >
> > -ENOCHANGELOG
>
> Yup. Will be added.
>
> >> Signed-off-by: Al Stone <al.stone@xxxxxxxxxx>
> >> ---
> >> 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.
> >> 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).
> >> * 2. 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;
> >> return 0;
> >> }
> >>
> >>
>
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
