Re: [PATCH 04/12] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Al Stone <al.stone@linaro.org>]

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.  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.

>>   		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;
>>   }
>>
>>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------