[PATCH 1/4] acpi,pci,irq: reduce resource requirements

[PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

Code has been redesigned to calculate penalty requirements on the fly. This
significantly simplifies the implementation and removes some of the init
calls from x86 architecture.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_link.c | 82 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 24 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index ededa90..a5a66ca 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -36,6 +36,8 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/acpi.h>
+#include <linux/irq.h>
+#include <linux/interrupt.h>
 
 #include "internal.h"
 
@@ -440,7 +442,6 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
 #define ACPI_MAX_IRQS		256
 #define ACPI_MAX_ISA_IRQ	16
 
-#define PIRQ_PENALTY_PCI_AVAILABLE	(0)
 #define PIRQ_PENALTY_PCI_POSSIBLE	(16*16)
 #define PIRQ_PENALTY_PCI_USING		(16*16*16)
 #define PIRQ_PENALTY_ISA_TYPICAL	(16*16*16*16)
@@ -457,9 +458,9 @@ static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
 	PIRQ_PENALTY_ISA_TYPICAL,	/* IRQ6 */
 	PIRQ_PENALTY_ISA_TYPICAL,	/* IRQ7 parallel, spurious */
 	PIRQ_PENALTY_ISA_TYPICAL,	/* IRQ8 rtc, sometimes */
-	PIRQ_PENALTY_PCI_AVAILABLE,	/* IRQ9  PCI, often acpi */
-	PIRQ_PENALTY_PCI_AVAILABLE,	/* IRQ10 PCI */
-	PIRQ_PENALTY_PCI_AVAILABLE,	/* IRQ11 PCI */
+	0,				/* IRQ9  PCI, often acpi */
+	0,				/* IRQ10 PCI */
+	0,				/* IRQ11 PCI */
 	PIRQ_PENALTY_ISA_USED,		/* IRQ12 mouse */
 	PIRQ_PENALTY_ISA_USED,		/* IRQ13 fpe, sometimes */
 	PIRQ_PENALTY_ISA_USED,		/* IRQ14 ide0 */
@@ -467,6 +468,49 @@ static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
 	/* >IRQ15 */
 };
 
+static int acpi_irq_pci_sharing_penalty(int irq)
+{
+	struct acpi_pci_link *link;
+	int penalty = 0;
+
+	list_for_each_entry(link, &acpi_link_list, list) {
+		/*
+		 * If a link is active, penalize its IRQ heavily
+		 * so we try to choose a different IRQ.
+		 */
+		if (link->irq.active && link->irq.active == irq)
+			penalty += PIRQ_PENALTY_PCI_USING;
+		else {
+			int i;
+
+			/*
+			 * If a link is inactive, penalize the IRQs it
+			 * might use, but not as severely.
+			 */
+			for (i = 0; i < link->irq.possible_count; i++)
+				if (link->irq.possible[i] == irq)
+					penalty += PIRQ_PENALTY_PCI_POSSIBLE /
+						link->irq.possible_count;
+		}
+	}
+
+	return penalty;
+}
+
+static int acpi_irq_get_penalty(int irq)
+{
+	int penalty = 0;
+
+	if (irq < ACPI_MAX_ISA_IRQ)
+		penalty += acpi_irq_penalty[irq];
+
+	if (irq == acpi_gbl_FADT.sci_interrupt)
+		penalty += PIRQ_PENALTY_PCI_USING;
+
+	penalty += acpi_irq_pci_sharing_penalty(irq);
+	return penalty;
+}
+
 int __init acpi_irq_penalty_init(void)
 {
 	struct acpi_pci_link *link;
@@ -568,7 +612,6 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
 			    acpi_device_bid(link->device));
 		return -ENODEV;
 	} else {
-		acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING;
 		printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
 		       acpi_device_name(link->device),
 		       acpi_device_bid(link->device), link->irq.active);
@@ -787,23 +830,24 @@ static int __init acpi_irq_penalty_update(char *str, int used)
 	for (i = 0; i < 16; i++) {
 		int retval;
 		int irq;
+		int new_penalty;
 
 		retval = get_option(&str, &irq);
 
 		if (!retval)
 			break;	/* no number found */
 
-		if (irq < 0)
-			continue;
-
-		if (irq >= ARRAY_SIZE(acpi_irq_penalty))
+		/* see if this is a ISA IRQ */
+		if ((irq < 0) || (irq >= ACPI_MAX_ISA_IRQ))
 			continue;
 
 		if (used)
-			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_USED;
+			new_penalty = acpi_irq_get_penalty(irq) +
+					PIRQ_PENALTY_ISA_USED;
 		else
-			acpi_irq_penalty[irq] = PIRQ_PENALTY_PCI_AVAILABLE;
+			new_penalty = 0;
 
+		acpi_irq_penalty[irq] = new_penalty;
 		if (retval != 2)	/* no next number */
 			break;
 	}
@@ -819,12 +863,9 @@ static int __init acpi_irq_penalty_update(char *str, int used)
  */
 void acpi_penalize_isa_irq(int irq, int active)
 {
-	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
-		if (active)
-			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_USED;
-		else
-			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
-	}
+	if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_irq_penalty)))
+		acpi_irq_penalty[irq] = active ?
+			PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
 }
 
 bool acpi_isa_irq_available(int irq)
@@ -840,13 +881,6 @@ bool acpi_isa_irq_available(int irq)
  */
 void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
 {
-	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
-		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
-		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
-			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
-		else
-			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
-	}
 }
 
 /*
-- 
1.8.2.1

[PATCH 2/4] acpi,pci,irq: remove redundant code in acpi_irq_penalty_init

[Sinan Kaya]

acpi_irq_get_penalty is now calculating the penalty on the fly now.
No need to maintain global list of penalties or calculate them
at the init time. Removing duplicate code in acpi_irq_penalty_init.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/x86/pci/acpi.c         |  1 -
 drivers/acpi/pci_link.c     | 35 -----------------------------------
 include/acpi/acpi_drivers.h |  1 -
 3 files changed, 37 deletions(-)

diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
index 3cd6983..b2a4e2a 100644
--- a/arch/x86/pci/acpi.c
+++ b/arch/x86/pci/acpi.c
@@ -396,7 +396,6 @@ int __init pci_acpi_init(void)
 		return -ENODEV;
 
 	printk(KERN_INFO "PCI: Using ACPI for IRQ routing\n");
-	acpi_irq_penalty_init();
 	pcibios_enable_irq = acpi_pci_irq_enable;
 	pcibios_disable_irq = acpi_pci_irq_disable;
 	x86_init.pci.init_irq = x86_init_noop;
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index a5a66ca..eb8e770 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -511,41 +511,6 @@ static int acpi_irq_get_penalty(int irq)
 	return penalty;
 }
 
-int __init acpi_irq_penalty_init(void)
-{
-	struct acpi_pci_link *link;
-	int i;
-
-	/*
-	 * Update penalties to facilitate IRQ balancing.
-	 */
-	list_for_each_entry(link, &acpi_link_list, list) {
-
-		/*
-		 * reflect the possible and active irqs in the penalty table --
-		 * useful for breaking ties.
-		 */
-		if (link->irq.possible_count) {
-			int penalty =
-			    PIRQ_PENALTY_PCI_POSSIBLE /
-			    link->irq.possible_count;
-
-			for (i = 0; i < link->irq.possible_count; i++) {
-				if (link->irq.possible[i] < ACPI_MAX_ISA_IRQ)
-					acpi_irq_penalty[link->irq.
-							 possible[i]] +=
-					    penalty;
-			}
-
-		} else if (link->irq.active) {
-			acpi_irq_penalty[link->irq.active] +=
-			    PIRQ_PENALTY_PCI_POSSIBLE;
-		}
-	}
-
-	return 0;
-}
-
 static int acpi_irq_balance = -1;	/* 0: static, 1: balance */
 
 static int acpi_pci_link_allocate(struct acpi_pci_link *link)
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 29c6912..797ae2e 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -78,7 +78,6 @@
 
 /* ACPI PCI Interrupt Link (pci_link.c) */
 
-int acpi_irq_penalty_init(void);
 int acpi_pci_link_allocate_irq(acpi_handle handle, int index, int *triggering,
 			       int *polarity, char **name);
 int acpi_pci_link_free_irq(acpi_handle handle);
-- 
1.8.2.1

[PATCH 3/4] acpi,pci,irq: remove SCI penalize function

[Sinan Kaya]

Removing the SCI penalize function as the penalty is now calculated on the
fly.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 arch/x86/kernel/acpi/boot.c | 1 -
 drivers/acpi/pci_link.c     | 9 ---------
 include/linux/acpi.h        | 1 -
 3 files changed, 11 deletions(-)

diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index e759076..5e99f22 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -445,7 +445,6 @@ static void __init acpi_sci_ioapic_setup(u8 bus_irq, u16 polarity, u16 trigger,
 		polarity = acpi_sci_flags & ACPI_MADT_POLARITY_MASK;
 
 	mp_override_legacy_irq(bus_irq, polarity, trigger, gsi);
-	acpi_penalize_sci_irq(bus_irq, trigger, polarity);
 
 	/*
 	 * stash over-ride to indicate we've been here
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index eb8e770..6bd77f1 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -840,15 +840,6 @@ bool acpi_isa_irq_available(int irq)
 }
 
 /*
- * Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict with
- * PCI IRQ attributes, mark ACPI SCI as ISA_ALWAYS so it won't be use for
- * PCI IRQs.
- */
-void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
-{
-}
-
-/*
  * Over-ride default table to reserve additional IRQs for use by ISA
  * e.g. acpi_irq_isa=5
  * Useful for telling ACPI how not to interfere with your ISA sound card.
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 06ed7e5..0f41317 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -311,7 +311,6 @@ struct pci_dev;
 int acpi_pci_irq_enable (struct pci_dev *dev);
 void acpi_penalize_isa_irq(int irq, int active);
 bool acpi_isa_irq_available(int irq);
-void acpi_penalize_sci_irq(int irq, int trigger, int polarity);
 void acpi_pci_irq_disable (struct pci_dev *dev);
 
 extern int ec_read(u8 addr, u8 *val);
-- 
1.8.2.1

[PATCH 4/4] Revert "Revert "ACPI, PCI, irq: remove interrupt count restriction""

[Sinan Kaya]

Revert commit e249714571db (Revert "ACPI, PCI, irq: remove interrupt
count restriction") boot regression issue is resolved now.
Removed the conflicts as code is much simpler now.

Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_link.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 6bd77f1..d1c0532 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -4,6 +4,7 @@
  *  Copyright (C) 2001, 2002 Andy Grover <andrew.grover@intel.com>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
  *  Copyright (C) 2002       Dominik Brodowski <devel@brodo.de>
+ *  Copyright (c) 2015, The Linux Foundation. All rights reserved.
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  *
@@ -439,7 +440,6 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
  * enabled system.
  */
 
-#define ACPI_MAX_IRQS		256
 #define ACPI_MAX_ISA_IRQ	16
 
 #define PIRQ_PENALTY_PCI_POSSIBLE	(16*16)
@@ -448,7 +448,7 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
 #define PIRQ_PENALTY_ISA_USED		(16*16*16*16*16)
 #define PIRQ_PENALTY_ISA_ALWAYS		(16*16*16*16*16*16)
 
-static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
+static int acpi_irq_isa_penalty[ACPI_MAX_ISA_IRQ] = {
 	PIRQ_PENALTY_ISA_ALWAYS,	/* IRQ0 timer */
 	PIRQ_PENALTY_ISA_ALWAYS,	/* IRQ1 keyboard */
 	PIRQ_PENALTY_ISA_ALWAYS,	/* IRQ2 cascade */
@@ -465,7 +465,6 @@ static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
 	PIRQ_PENALTY_ISA_USED,		/* IRQ13 fpe, sometimes */
 	PIRQ_PENALTY_ISA_USED,		/* IRQ14 ide0 */
 	PIRQ_PENALTY_ISA_USED,		/* IRQ15 ide1 */
-	/* >IRQ15 */
 };
 
 static int acpi_irq_pci_sharing_penalty(int irq)
@@ -502,7 +501,7 @@ static int acpi_irq_get_penalty(int irq)
 	int penalty = 0;
 
 	if (irq < ACPI_MAX_ISA_IRQ)
-		penalty += acpi_irq_penalty[irq];
+		penalty += acpi_irq_isa_penalty[irq];
 
 	if (irq == acpi_gbl_FADT.sci_interrupt)
 		penalty += PIRQ_PENALTY_PCI_USING;
@@ -556,12 +555,12 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
 		 * the use of IRQs 9, 10, 11, and >15.
 		 */
 		for (i = (link->irq.possible_count - 1); i >= 0; i--) {
-			if (acpi_irq_penalty[irq] >
-			    acpi_irq_penalty[link->irq.possible[i]])
+			if (acpi_irq_get_penalty(irq) >
+			    acpi_irq_get_penalty(link->irq.possible[i]))
 				irq = link->irq.possible[i];
 		}
 	}
-	if (acpi_irq_penalty[irq] >= PIRQ_PENALTY_ISA_ALWAYS) {
+	if (acpi_irq_get_penalty(irq) >= PIRQ_PENALTY_ISA_ALWAYS) {
 		printk(KERN_ERR PREFIX "No IRQ available for %s [%s]. "
 			    "Try pci=noacpi or acpi=off\n",
 			    acpi_device_name(link->device),
@@ -786,7 +785,7 @@ static void acpi_pci_link_remove(struct acpi_device *device)
 }
 
 /*
- * modify acpi_irq_penalty[] from cmdline
+ * modify acpi_irq_isa_penalty[] from cmdline
  */
 static int __init acpi_irq_penalty_update(char *str, int used)
 {
@@ -812,7 +811,7 @@ static int __init acpi_irq_penalty_update(char *str, int used)
 		else
 			new_penalty = 0;
 
-		acpi_irq_penalty[irq] = new_penalty;
+		acpi_irq_isa_penalty[irq] = new_penalty;
 		if (retval != 2)	/* no next number */
 			break;
 	}
@@ -828,15 +827,15 @@ static int __init acpi_irq_penalty_update(char *str, int used)
  */
 void acpi_penalize_isa_irq(int irq, int active)
 {
-	if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_irq_penalty)))
-		acpi_irq_penalty[irq] = active ?
+	if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_irq_isa_penalty)))
+		acpi_irq_isa_penalty[irq] = active ?
 			PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
 }
 
 bool acpi_isa_irq_available(int irq)
 {
-	return irq >= 0 && (irq >= ARRAY_SIZE(acpi_irq_penalty) ||
-			    acpi_irq_penalty[irq] < PIRQ_PENALTY_ISA_ALWAYS);
+	return irq >= 0 &&
+		(acpi_irq_get_penalty(irq) < PIRQ_PENALTY_ISA_ALWAYS);
 }
 
 /*
-- 
1.8.2.1

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Bjorn Helgaas]

On Tue, Mar 08, 2016 at 07:41:16PM -0500, Sinan Kaya wrote:
> Code has been redesigned to calculate penalty requirements on the fly. This
> significantly simplifies the implementation and removes some of the init
> calls from x86 architecture.
> 
> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/acpi/pci_link.c | 82 ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 58 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index ededa90..a5a66ca 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -36,6 +36,8 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
>  
>  #include "internal.h"
>  
> @@ -440,7 +442,6 @@ static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
>  #define ACPI_MAX_IRQS		256
>  #define ACPI_MAX_ISA_IRQ	16
>  
> -#define PIRQ_PENALTY_PCI_AVAILABLE	(0)
>  #define PIRQ_PENALTY_PCI_POSSIBLE	(16*16)
>  #define PIRQ_PENALTY_PCI_USING		(16*16*16)
>  #define PIRQ_PENALTY_ISA_TYPICAL	(16*16*16*16)
> @@ -457,9 +458,9 @@ static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
>  	PIRQ_PENALTY_ISA_TYPICAL,	/* IRQ6 */
>  	PIRQ_PENALTY_ISA_TYPICAL,	/* IRQ7 parallel, spurious */
>  	PIRQ_PENALTY_ISA_TYPICAL,	/* IRQ8 rtc, sometimes */
> -	PIRQ_PENALTY_PCI_AVAILABLE,	/* IRQ9  PCI, often acpi */
> -	PIRQ_PENALTY_PCI_AVAILABLE,	/* IRQ10 PCI */
> -	PIRQ_PENALTY_PCI_AVAILABLE,	/* IRQ11 PCI */
> +	0,				/* IRQ9  PCI, often acpi */
> +	0,				/* IRQ10 PCI */
> +	0,				/* IRQ11 PCI */
>  	PIRQ_PENALTY_ISA_USED,		/* IRQ12 mouse */
>  	PIRQ_PENALTY_ISA_USED,		/* IRQ13 fpe, sometimes */
>  	PIRQ_PENALTY_ISA_USED,		/* IRQ14 ide0 */
> @@ -467,6 +468,49 @@ static int acpi_irq_penalty[ACPI_MAX_IRQS] = {
>  	/* >IRQ15 */
>  };
>  
> +static int acpi_irq_pci_sharing_penalty(int irq)
> +{
> +	struct acpi_pci_link *link;
> +	int penalty = 0;
> +
> +	list_for_each_entry(link, &acpi_link_list, list) {
> +		/*
> +		 * If a link is active, penalize its IRQ heavily
> +		 * so we try to choose a different IRQ.
> +		 */
> +		if (link->irq.active && link->irq.active == irq)
> +			penalty += PIRQ_PENALTY_PCI_USING;
> +		else {
> +			int i;
> +
> +			/*
> +			 * If a link is inactive, penalize the IRQs it
> +			 * might use, but not as severely.
> +			 */
> +			for (i = 0; i < link->irq.possible_count; i++)
> +				if (link->irq.possible[i] == irq)
> +					penalty += PIRQ_PENALTY_PCI_POSSIBLE /
> +						link->irq.possible_count;
> +		}
> +	}
> +
> +	return penalty;
> +}
> +
> +static int acpi_irq_get_penalty(int irq)
> +{
> +	int penalty = 0;
> +
> +	if (irq < ACPI_MAX_ISA_IRQ)
> +		penalty += acpi_irq_penalty[irq];
> +
> +	if (irq == acpi_gbl_FADT.sci_interrupt)
> +		penalty += PIRQ_PENALTY_PCI_USING;
> +
> +	penalty += acpi_irq_pci_sharing_penalty(irq);
> +	return penalty;
> +}
> +
>  int __init acpi_irq_penalty_init(void)
>  {
>  	struct acpi_pci_link *link;
> @@ -568,7 +612,6 @@ static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>  			    acpi_device_bid(link->device));
>  		return -ENODEV;
>  	} else {
> -		acpi_irq_penalty[link->irq.active] += PIRQ_PENALTY_PCI_USING;
>  		printk(KERN_WARNING PREFIX "%s [%s] enabled at IRQ %d\n",
>  		       acpi_device_name(link->device),
>  		       acpi_device_bid(link->device), link->irq.active);
> @@ -787,23 +830,24 @@ static int __init acpi_irq_penalty_update(char *str, int used)
>  	for (i = 0; i < 16; i++) {
>  		int retval;
>  		int irq;
> +		int new_penalty;
>  
>  		retval = get_option(&str, &irq);
>  
>  		if (!retval)
>  			break;	/* no number found */
>  
> -		if (irq < 0)
> -			continue;
> -
> -		if (irq >= ARRAY_SIZE(acpi_irq_penalty))
> +		/* see if this is a ISA IRQ */
> +		if ((irq < 0) || (irq >= ACPI_MAX_ISA_IRQ))
>  			continue;
>  
>  		if (used)
> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_USED;
> +			new_penalty = acpi_irq_get_penalty(irq) +
> +					PIRQ_PENALTY_ISA_USED;
>  		else
> -			acpi_irq_penalty[irq] = PIRQ_PENALTY_PCI_AVAILABLE;
> +			new_penalty = 0;
>  
> +		acpi_irq_penalty[irq] = new_penalty;
>  		if (retval != 2)	/* no next number */
>  			break;
>  	}
> @@ -819,12 +863,9 @@ static int __init acpi_irq_penalty_update(char *str, int used)
>   */
>  void acpi_penalize_isa_irq(int irq, int active)
>  {
> -	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
> -		if (active)
> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_USED;
> -		else
> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
> -	}
> +	if ((irq >= 0) && (irq < ARRAY_SIZE(acpi_irq_penalty)))
> +		acpi_irq_penalty[irq] = active ?
> +			PIRQ_PENALTY_ISA_USED : PIRQ_PENALTY_PCI_USING;
>  }
>  
>  bool acpi_isa_irq_available(int irq)
> @@ -840,13 +881,6 @@ bool acpi_isa_irq_available(int irq)
>   */
>  void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>  {
> -	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
> -		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> -		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
> -		else
> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
> -	}

I think we lost the validation of trigger mode and polarity, didn't
we?

>  }
>  
>  /*
> -- 
> 1.8.2.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

Hi Bjorn,

On 3/14/2016 2:52 PM, Bjorn Helgaas wrote:
>> bool acpi_isa_irq_available(int irq)
>> > @@ -840,13 +881,6 @@ bool acpi_isa_irq_available(int irq)
>> >   */
>> >  void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>> >  {
>> > -	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
>> > -		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>> > -		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>> > -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
>> > -		else
>> > -			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
>> > -	}
> I think we lost the validation of trigger mode and polarity, didn't
> we?
> 

This function gets called to inform ACPI that this is the SCI interrupt
and, trigger and polarity are their attributes.

The return value is void and the caller is not interested in what ACPI thinks
about. 

This function adjusts the SCI penalty based on correct attributes passed
(ISA_ALWAYS vs. PCI_USING).

I agree that we lost this validation. 

I can keep sci_trigger/sci_polarity somewhere and keep that into the calculation
in get function.

Like this for instance,

	if (irq == acpi_gbl_FADT.sci_interrupt) {
+		if (sci_trigger != ACPI_MADT_TRIGGER_LEVEL ||
+		    sci_polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
+			penalty += PIRQ_PENALTY_ISA_ALWAYS;
+		else
			penalty += PIRQ_PENALTY_PCI_USING; 
	}

Then, we can't get rid of the function just we can reduce the contents.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Bjorn Helgaas]

On Mon, Mar 14, 2016 at 04:37:51PM -0400, Sinan Kaya wrote:
> Hi Bjorn,
> 
> On 3/14/2016 2:52 PM, Bjorn Helgaas wrote:
> >> bool acpi_isa_irq_available(int irq)
> >> > @@ -840,13 +881,6 @@ bool acpi_isa_irq_available(int irq)
> >> >   */
> >> >  void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
> >> >  {
> >> > -	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
> >> > -		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> >> > -		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> >> > -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
> >> > -		else
> >> > -			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
> >> > -	}
> > I think we lost the validation of trigger mode and polarity, didn't
> > we?
> > 
> 
> This function gets called to inform ACPI that this is the SCI interrupt
> and, trigger and polarity are their attributes.
> 
> The return value is void and the caller is not interested in what ACPI thinks
> about. 
> 
> This function adjusts the SCI penalty based on correct attributes passed
> (ISA_ALWAYS vs. PCI_USING).
> 
> I agree that we lost this validation. 
> 
> I can keep sci_trigger/sci_polarity somewhere and keep that into the calculation
> in get function.
> 
> Like this for instance,
> 
> 	if (irq == acpi_gbl_FADT.sci_interrupt) {
> +		if (sci_trigger != ACPI_MADT_TRIGGER_LEVEL ||
> +		    sci_polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> +			penalty += PIRQ_PENALTY_ISA_ALWAYS;
> +		else
> 			penalty += PIRQ_PENALTY_PCI_USING; 
> 	}
> 
> Then, we can't get rid of the function just we can reduce the contents.

I think it's important to keep that check.

I raised the possibility of using irq_get_trigger_type() for all IRQs
(not just the SCI).  Did you have a chance to look into that at all?

Bjorn

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

On 3/14/2016 5:01 PM, Bjorn Helgaas wrote:
> On Mon, Mar 14, 2016 at 04:37:51PM -0400, Sinan Kaya wrote:
>> Hi Bjorn,
>>
>> On 3/14/2016 2:52 PM, Bjorn Helgaas wrote:
>>>> bool acpi_isa_irq_available(int irq)
>>>>> @@ -840,13 +881,6 @@ bool acpi_isa_irq_available(int irq)
>>>>>   */
>>>>>  void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
>>>>>  {
>>>>> -	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
>>>>> -		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
>>>>> -		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>>>>> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
>>>>> -		else
>>>>> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
>>>>> -	}
>>> I think we lost the validation of trigger mode and polarity, didn't
>>> we?
>>>
>>
>> This function gets called to inform ACPI that this is the SCI interrupt
>> and, trigger and polarity are their attributes.
>>
>> The return value is void and the caller is not interested in what ACPI thinks
>> about. 
>>
>> This function adjusts the SCI penalty based on correct attributes passed
>> (ISA_ALWAYS vs. PCI_USING).
>>
>> I agree that we lost this validation. 
>>
>> I can keep sci_trigger/sci_polarity somewhere and keep that into the calculation
>> in get function.
>>
>> Like this for instance,
>>
>> 	if (irq == acpi_gbl_FADT.sci_interrupt) {
>> +		if (sci_trigger != ACPI_MADT_TRIGGER_LEVEL ||
>> +		    sci_polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
>> +			penalty += PIRQ_PENALTY_ISA_ALWAYS;
>> +		else
>> 			penalty += PIRQ_PENALTY_PCI_USING; 
>> 	}
>>
>> Then, we can't get rid of the function just we can reduce the contents.
> 
> I think it's important to keep that check.
> 
> I raised the possibility of using irq_get_trigger_type() for all IRQs
> (not just the SCI).  Did you have a chance to look into that at all?
> 
> Bjorn
> 

Let's take care of SCI first. Is this OK? Would you include IRQ_TYPE_NONE below?

get_penalty
{
...
	if (irq == acpi_gbl_FADT.sci_interrupt) {
		if (irq_get_trigger_type(irq) == IRQ_TYPE_LEVEL_LOW)
			penalty += PIRQ_PENALTY_PCI_USING;
		else
			penalty += PIRQ_PENALTY_ISA_ALWAYS;
	}
}


Yes, I read your email and asked how you would deal with ARM64. There was
silence after that :) ARM64 doesn't have SCI and the PCI interrupts are
active high and level.

I pasted the code here again. It looks like you want to validate that
PCI interrupts are always level low. 

There could be also some other existing Intel platform or architecture 
that quite doesn't encode the interrupt properly.

 
   static int pci_compatible_trigger(int irq)
   {
     int type = irq_get_trigger_type(irq);

     return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
   }


   static unsigned int acpi_irq_get_penalty(int irq)
   {
     unsigned int penalty = 0;

     if (irq == acpi_gbl_FADT.sci_interrupt)
       	penalty += PIRQ_PENALTY_PCI_USING;

     penalty += acpi_irq_pci_sharing_penalty(irq);
     return penalty;
   }

   static int acpi_pci_link_allocate(struct acpi_pci_link *link)
   {
     unsigned int best = ~0;
     ...

     for (i = (link->irq.possible_count - 1); i >= 0; i--) {
       candidate = link->irq.possible[i];
       if (!pci_compatible_trigger(candidate))
         continue;

       penalty = acpi_irq_get_penalty(candidate);
       if (penalty < best) {
       	    irq = candidate;
            best = penalty;
       }
     }
   }


Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Bjorn Helgaas]

On Mon, Mar 14, 2016 at 05:50:42PM -0400, Sinan Kaya wrote:
> On 3/14/2016 5:01 PM, Bjorn Helgaas wrote:
> > On Mon, Mar 14, 2016 at 04:37:51PM -0400, Sinan Kaya wrote:
> >> Hi Bjorn,
> >>
> >> On 3/14/2016 2:52 PM, Bjorn Helgaas wrote:
> >>>> bool acpi_isa_irq_available(int irq)
> >>>>> @@ -840,13 +881,6 @@ bool acpi_isa_irq_available(int irq)
> >>>>>   */
> >>>>>  void acpi_penalize_sci_irq(int irq, int trigger, int polarity)
> >>>>>  {
> >>>>> -	if (irq >= 0 && irq < ARRAY_SIZE(acpi_irq_penalty)) {
> >>>>> -		if (trigger != ACPI_MADT_TRIGGER_LEVEL ||
> >>>>> -		    polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> >>>>> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_ISA_ALWAYS;
> >>>>> -		else
> >>>>> -			acpi_irq_penalty[irq] += PIRQ_PENALTY_PCI_USING;
> >>>>> -	}
> >>> I think we lost the validation of trigger mode and polarity, didn't
> >>> we?
> >>>
> >>
> >> This function gets called to inform ACPI that this is the SCI interrupt
> >> and, trigger and polarity are their attributes.
> >>
> >> The return value is void and the caller is not interested in what ACPI thinks
> >> about. 
> >>
> >> This function adjusts the SCI penalty based on correct attributes passed
> >> (ISA_ALWAYS vs. PCI_USING).
> >>
> >> I agree that we lost this validation. 
> >>
> >> I can keep sci_trigger/sci_polarity somewhere and keep that into the calculation
> >> in get function.
> >>
> >> Like this for instance,
> >>
> >> 	if (irq == acpi_gbl_FADT.sci_interrupt) {
> >> +		if (sci_trigger != ACPI_MADT_TRIGGER_LEVEL ||
> >> +		    sci_polarity != ACPI_MADT_POLARITY_ACTIVE_LOW)
> >> +			penalty += PIRQ_PENALTY_ISA_ALWAYS;
> >> +		else
> >> 			penalty += PIRQ_PENALTY_PCI_USING; 
> >> 	}
> >>
> >> Then, we can't get rid of the function just we can reduce the contents.
> > 
> > I think it's important to keep that check.
> > 
> > I raised the possibility of using irq_get_trigger_type() for all IRQs
> > (not just the SCI).  Did you have a chance to look into that at all?
> > 
> > Bjorn
> > 
> 
> Let's take care of SCI first. Is this OK? Would you include IRQ_TYPE_NONE below?
> 
> get_penalty
> {
> ...
> 	if (irq == acpi_gbl_FADT.sci_interrupt) {
> 		if (irq_get_trigger_type(irq) == IRQ_TYPE_LEVEL_LOW)
> 			penalty += PIRQ_PENALTY_PCI_USING;
> 		else
> 			penalty += PIRQ_PENALTY_ISA_ALWAYS;
> 	}
> }
> 
> 
> Yes, I read your email and asked how you would deal with ARM64. There was
> silence after that :) 

Sorry, I missed that.  Trying to juggle too many conversations at
once, I guess.

> ARM64 doesn't have SCI

That's not a problem; we just never have to penalize an IRQ because
SCI is also using it.

> and the PCI interrupts are
> active high and level.

This I don't understand.  I already cited PCI Spec r3.0, sec 2.2.6,
which says PCI interrupts are level-sensitive, asserted low.

If you go to Fry's and buy a conventional PCI card, it is going to
pull INTA# low to assert an interrupt.  It doesn't matter whether you
put it in an x86 system or an arm64 system.

> I pasted the code here again. It looks like you want to validate that
> PCI interrupts are always level low. 

I don't really care whether PCI interrupts are always level low.  What
matters is that the PCI interrupt line matches the configuration of
the interrupt controller input.

If the PCI interrupt can be a different type, e.g., level high, and
there's a way to discover that, we can check that against the
interrupt controller configuration.

This is all in the context of conventional PCI, and we're probably
talking about arm64 PCIe systems, not conventional PCI.  I'm not sure
what an Interrupt Link device means in PCIe.  I suppose it would have
to connect an INTx virtual wire to a system interrupt?  The PCIe spec
says this sort of mapping is system implementation specific (r3.0, sec
2.2.8.1).

> There could be also some other existing Intel platform or architecture 
> that quite doesn't encode the interrupt properly.
> 
>  
>    static int pci_compatible_trigger(int irq)
>    {
>      int type = irq_get_trigger_type(irq);
> 
>      return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
>    }
> 
> 
>    static unsigned int acpi_irq_get_penalty(int irq)
>    {
>      unsigned int penalty = 0;
> 
>      if (irq == acpi_gbl_FADT.sci_interrupt)
>        	penalty += PIRQ_PENALTY_PCI_USING;
> 
>      penalty += acpi_irq_pci_sharing_penalty(irq);
>      return penalty;
>    }
> 
>    static int acpi_pci_link_allocate(struct acpi_pci_link *link)
>    {
>      unsigned int best = ~0;
>      ...
> 
>      for (i = (link->irq.possible_count - 1); i >= 0; i--) {
>        candidate = link->irq.possible[i];
>        if (!pci_compatible_trigger(candidate))
>          continue;
> 
>        penalty = acpi_irq_get_penalty(candidate);
>        if (penalty < best) {
>        	    irq = candidate;
>             best = penalty;
>        }
>      }
>    }
> 
> 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

On 3/14/2016 9:48 PM, Bjorn Helgaas wrote:

Yes. I was talking about PCIe on ARM64. 

> If you go to Fry's and buy a conventional PCI card, it is going to
> pull INTA# low to assert an interrupt.  It doesn't matter whether you
> put it in an x86 system or an arm64 system.
> 

I don't see INTA# of the PCIe card at the system level. The PCIe wire 
interrupt gets converted to the system level interrupt by the PCIe controller.

>> > I pasted the code here again. It looks like you want to validate that
>> > PCI interrupts are always level low. 
> I don't really care whether PCI interrupts are always level low.  What
> matters is that the PCI interrupt line matches the configuration of
> the interrupt controller input.
> 

Agreed. But the interrupt controller configuration is system specific. How would
you check interrupt line against what the interrupt controller requires
on each architecture as this is common code?


> If the PCI interrupt can be a different type, e.g., level high, and
> there's a way to discover that, we can check that against the
> interrupt controller configuration.
> 
> This is all in the context of conventional PCI, and we're probably
> talking about arm64 PCIe systems, not conventional PCI. 

INTx interrupts are TLP messages on PCIe as you already know. There is no INTA 
interrupt wire.

"6.1.2. PCI Compatible INTx Emulation" section of the PCIe spec describes
INTx emulation on PCIe.

I pasted that section here for your reference.

"Two types of Messages are defined, Assert_INTx and Deassert_INTx, for emulation of PCI INTx
signaling, where x is A, B, C, and D for respective PCI interrupt signals. These Messages are used to
provide “virtual wires” for signaling interrupts across a Link. Switches collect these virtual wires and
present a combined set at the Switch’s Upstream Port. Ultimately, the virtual wires are routed to the
Root Complex which maps the virtual wires to system interrupt resources. Devices must use
10 assert/de-assert Messages in pairs to emulate PCI interrupt level-triggered signaling. Actual
mapping of PCI Express INTx emulation to system interrupts is implementation specific as is
mapping of physical interrupt signals in conventional PCI."


> I'm not sure what an Interrupt Link device means in PCIe.  I suppose it would have
> to connect an INTx virtual wire to a system interrupt?  The PCIe spec
> says this sort of mapping is system implementation specific (r3.0, sec
> 2.2.8.1).
> 

The INTx messages are converted to the system interrupt by the PCIe controller.
The interrupt type between the PCIe controller and the ARM GIC interrupt 
controller is dictated by the ARM GIC Interrupt Controller Specification for
ARM64.

Here is what ACPI table looks like for reference

	Name(_PRT, Package(){
		Package(){0x0FFFF, 0, \_SB.LN0A, 0},  // Slot 0, INTA
		Package(){0x0FFFF, 1, \_SB.LN0B, 0},  // Slot 0, INTB
		Package(){0x0FFFF, 2, \_SB.LN0C, 0},  // Slot 0, INTC
		Package(){0x0FFFF, 3, \_SB.LN0D, 0}   // Slot 0, INTD
	}) 

Device(LN0A){
	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
	Name(_UID, 1)
	Name(_PRS, ResourceTemplate(){
	Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0x123}
	})
	Method(_DIS) {}
	Method(_CRS) { Return (_PRS) }
	Method(_SRS, 1) {}
} 




-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Bjorn Helgaas]

On Mon, Mar 14, 2016 at 10:28:11PM -0400, Sinan Kaya wrote:
> On 3/14/2016 9:48 PM, Bjorn Helgaas wrote:
> 
> Yes. I was talking about PCIe on ARM64. 
> 
> > If you go to Fry's and buy a conventional PCI card, it is going to
> > pull INTA# low to assert an interrupt.  It doesn't matter whether you
> > put it in an x86 system or an arm64 system.
> 
> I don't see INTA# of the PCIe card at the system level. The PCIe wire 
> interrupt gets converted to the system level interrupt by the PCIe controller.

That's why I said *conventional PCI*.  If you have a conventional PCI
device below either a conventional PCI host controller or a PCIe-to-PCI
bridge, there are real INTx wires, not virtual wires, and they are
level/low.  But I think you pointed out the key below (that the
Interrupt resource in a PNP0C0F device encodes the trigger type).

> >> > I pasted the code here again. It looks like you want to validate that
> >> > PCI interrupts are always level low. 
> > I don't really care whether PCI interrupts are always level low.  What
> > matters is that the PCI interrupt line matches the configuration of
> > the interrupt controller input.
> > 
> 
> Agreed. But the interrupt controller configuration is system specific. How would
> you check interrupt line against what the interrupt controller requires
> on each architecture as this is common code?
> 
> 
> > If the PCI interrupt can be a different type, e.g., level high, and
> > there's a way to discover that, we can check that against the
> > interrupt controller configuration.
> > 
> > This is all in the context of conventional PCI, and we're probably
> > talking about arm64 PCIe systems, not conventional PCI. 
> 
> INTx interrupts are TLP messages on PCIe as you already know. There is no INTA 
> interrupt wire.

Yes, that's why I mentioned PCIe sec 2.2.8.1 below.

> "6.1.2. PCI Compatible INTx Emulation" section of the PCIe spec describes
> INTx emulation on PCIe.
>  ...
> 
> > I'm not sure what an Interrupt Link device means in PCIe.  I suppose it would have
> > to connect an INTx virtual wire to a system interrupt?  The PCIe spec
> > says this sort of mapping is system implementation specific (r3.0, sec
> > 2.2.8.1).
> 
> The INTx messages are converted to the system interrupt by the PCIe controller.
> The interrupt type between the PCIe controller and the ARM GIC interrupt 
> controller is dictated by the ARM GIC Interrupt Controller Specification for
> ARM64.
> 
> Here is what ACPI table looks like for reference
> 
> 	Name(_PRT, Package(){
> 		Package(){0x0FFFF, 0, \_SB.LN0A, 0},  // Slot 0, INTA
> 		Package(){0x0FFFF, 1, \_SB.LN0B, 0},  // Slot 0, INTB
> 		Package(){0x0FFFF, 2, \_SB.LN0C, 0},  // Slot 0, INTC
> 		Package(){0x0FFFF, 3, \_SB.LN0D, 0}   // Slot 0, INTD
> 	}) 
> 
> Device(LN0A){
> 	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
> 	Name(_UID, 1)
> 	Name(_PRS, ResourceTemplate(){
> 	Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0x123}
> 	})

I forgot that the link already include the trigger mode in it.  Maybe we
can check for that instead of assuming level/low.

Bjorn

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

On 3/14/2016 10:36 PM, Bjorn Helgaas wrote:
> On Mon, Mar 14, 2016 at 10:28:11PM -0400, Sinan Kaya wrote:
>> On 3/14/2016 9:48 PM, Bjorn Helgaas wrote:
>>
>> Yes. I was talking about PCIe on ARM64. 
>>
>>> If you go to Fry's and buy a conventional PCI card, it is going to
>>> pull INTA# low to assert an interrupt.  It doesn't matter whether you
>>> put it in an x86 system or an arm64 system.
>>
>> I don't see INTA# of the PCIe card at the system level. The PCIe wire 
>> interrupt gets converted to the system level interrupt by the PCIe controller.
> 
> That's why I said *conventional PCI*.  If you have a conventional PCI
> device below either a conventional PCI host controller or a PCIe-to-PCI
> bridge, there are real INTx wires, not virtual wires, and they are
> level/low.  But I think you pointed out the key below (that the
> Interrupt resource in a PNP0C0F device encodes the trigger type).
> 
>>>>> I pasted the code here again. It looks like you want to validate that
>>>>> PCI interrupts are always level low. 
>>> I don't really care whether PCI interrupts are always level low.  What
>>> matters is that the PCI interrupt line matches the configuration of
>>> the interrupt controller input.
>>>
>>
>> Agreed. But the interrupt controller configuration is system specific. How would
>> you check interrupt line against what the interrupt controller requires
>> on each architecture as this is common code?
>>
>>
>>> If the PCI interrupt can be a different type, e.g., level high, and
>>> there's a way to discover that, we can check that against the
>>> interrupt controller configuration.
>>>
>>> This is all in the context of conventional PCI, and we're probably
>>> talking about arm64 PCIe systems, not conventional PCI. 
>>
>> INTx interrupts are TLP messages on PCIe as you already know. There is no INTA 
>> interrupt wire.
> 
> Yes, that's why I mentioned PCIe sec 2.2.8.1 below.
> 
>> "6.1.2. PCI Compatible INTx Emulation" section of the PCIe spec describes
>> INTx emulation on PCIe.
>>  ...
>>
>>> I'm not sure what an Interrupt Link device means in PCIe.  I suppose it would have
>>> to connect an INTx virtual wire to a system interrupt?  The PCIe spec
>>> says this sort of mapping is system implementation specific (r3.0, sec
>>> 2.2.8.1).
>>
>> The INTx messages are converted to the system interrupt by the PCIe controller.
>> The interrupt type between the PCIe controller and the ARM GIC interrupt 
>> controller is dictated by the ARM GIC Interrupt Controller Specification for
>> ARM64.
>>
>> Here is what ACPI table looks like for reference
>>
>> 	Name(_PRT, Package(){
>> 		Package(){0x0FFFF, 0, \_SB.LN0A, 0},  // Slot 0, INTA
>> 		Package(){0x0FFFF, 1, \_SB.LN0B, 0},  // Slot 0, INTB
>> 		Package(){0x0FFFF, 2, \_SB.LN0C, 0},  // Slot 0, INTC
>> 		Package(){0x0FFFF, 3, \_SB.LN0D, 0}   // Slot 0, INTD
>> 	}) 
>>
>> Device(LN0A){
>> 	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
>> 	Name(_UID, 1)
>> 	Name(_PRS, ResourceTemplate(){
>> 	Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0x123}
>> 	})
> 
> I forgot that the link already include the trigger mode in it.  Maybe we
> can check for that instead of assuming level/low.
> 

Let me explore this area.

> Bjorn
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

Hi Bjorn,

>>>
>>> Device(LN0A){
>>> 	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
>>> 	Name(_UID, 1)
>>> 	Name(_PRS, ResourceTemplate(){
>>> 	Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0x123}
>>> 	})
>>
>> I forgot that the link already include the trigger mode in it.  Maybe we
>> can check for that instead of assuming level/low.
>>
> 
> Let me explore this area.
> 
>> Bjorn

I have been implementing this. 

I see that placing the check in allocate function is not right. The interrupts get
registered with ACPI subsystem after locating the interrupt in register_gsi function.
Trying to find the IRQ type with get_trigger_type() for an unregistered interrupt
didn't look right to me.

I think it is best if we move these checks into get_penalty function.

I also see that the code is currently allowing EDGE type PCI interrupts assuming they are
exclusive only. Let me if this looks alright. I'm also curious about the PCI checks.

static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)

I know you want to validate that all PCI interrupts are level, it looks like the code
is allowing other combinations.

diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 85be579e..5056a9c 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -495,6 +495,45 @@ static int acpi_irq_pci_sharing_penalty(int irq)
 	return penalty;
 }
 
+static int acpi_link_trigger(int irq, u8 *polarity, u8 *triggering)
+{
+	struct acpi_pci_link *link;
+	int link_trigger = 0;
+
+	list_for_each_entry(link, &acpi_link_list, list) {
+		int i;
+
+		if (link->irq.active && link->irq.active == irq) {
+			*polarity = link->polarity;
+			*triggering = link->triggering;
+			return 0;
+		}
+
+		for (i = 0; i < link->irq.possible_count; i++)
+			if (link->irq.possible[i] == irq) {
+				*polarity = link->polarity;
+				*triggering = link->triggering;
+				return 0;
+			}
+	}
+
+	return -EINVAL;
+}
+
+static bool acpi_pci_compatible_trigger(int irq)
+{
+	u8 polarity;
+	u8 triggering;
+	int type = irq_get_trigger_type(irq);
+	int rc;
+
+	rc = acpi_link_trigger(irq, &polarity, &triggering, &sharing)
+	if (rc)
+		return false;
+
+	return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
+}
+
 static int acpi_irq_get_penalty(int irq)
 {
 	int penalty = 0;
@@ -502,8 +541,21 @@ static int acpi_irq_get_penalty(int irq)
 	if (irq < ACPI_MAX_ISA_IRQ)
 		penalty += acpi_irq_isa_penalty[irq];
 
-	if (irq == acpi_gbl_FADT.sci_interrupt)
-		penalty += PIRQ_PENALTY_PCI_USING;
+	if (irq == acpi_gbl_FADT.sci_interrupt) {
+		u8 sci_polarity;
+		u8 sci_trigger;
+
+		rc = acpi_link_trigger(irq, &polarity, &sci_trigger);
+		if (!rc) {
+			if (sci_trigger != ACPI_LEVEL_SENSITIVE ||
+			    sci_polarity != ACPI_ACTIVE_LOW)
+				penalty += PIRQ_PENALTY_ISA_ALWAYS;
+			else
+				penalty += PIRQ_PENALTY_PCI_USING;
+		} else {
+			return ~0;
+		}
+	}
 
 	penalty += acpi_irq_pci_sharing_penalty(irq);
 	return penalty; 


-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Bjorn Helgaas]

On Sun, Mar 20, 2016 at 02:17:09PM -0400, Sinan Kaya wrote:
> Hi Bjorn,
> 
> >>>
> >>> Device(LN0A){
> >>> 	Name(_HID, EISAID("PNP0C0F")) // PCI interrupt link
> >>> 	Name(_UID, 1)
> >>> 	Name(_PRS, ResourceTemplate(){
> >>> 	Interrupt(ResourceProducer, Level, ActiveHigh, Exclusive, , ,) {0x123}
> >>> 	})
> >>
> >> I forgot that the link already include the trigger mode in it.  Maybe we
> >> can check for that instead of assuming level/low.
> >>
> > 
> > Let me explore this area.
> > 
> >> Bjorn
> 
> I have been implementing this. 
> 
> I see that placing the check in allocate function is not right. The interrupts get
> registered with ACPI subsystem after locating the interrupt in register_gsi function.
> Trying to find the IRQ type with get_trigger_type() for an unregistered interrupt
> didn't look right to me.
> 
> I think it is best if we move these checks into get_penalty function.
> 
> I also see that the code is currently allowing EDGE type PCI interrupts assuming they are
> exclusive only. Let me if this looks alright. I'm also curious about the PCI checks.
> 
> static int acpi_pci_link_set(struct acpi_pci_link *link, int irq)
> 
> I know you want to validate that all PCI interrupts are level, it looks like the code
> is allowing other combinations.

It's not that we need to validate that all PCI interrupts are level.  What
we need is to make sure all users sharing an IRQ agree on the mode.

> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 85be579e..5056a9c 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -495,6 +495,45 @@ static int acpi_irq_pci_sharing_penalty(int irq)
>  	return penalty;
>  }
>  
> +static int acpi_link_trigger(int irq, u8 *polarity, u8 *triggering)
> +{
> +	struct acpi_pci_link *link;
> +	int link_trigger = 0;
> +
> +	list_for_each_entry(link, &acpi_link_list, list) {
> +		int i;
> +
> +		if (link->irq.active && link->irq.active == irq) {
> +			*polarity = link->polarity;
> +			*triggering = link->triggering;
> +			return 0;
> +		}
> +
> +		for (i = 0; i < link->irq.possible_count; i++)
> +			if (link->irq.possible[i] == irq) {
> +				*polarity = link->polarity;
> +				*triggering = link->triggering;
> +				return 0;
> +			}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static bool acpi_pci_compatible_trigger(int irq)
> +{
> +	u8 polarity;
> +	u8 triggering;
> +	int type = irq_get_trigger_type(irq);
> +	int rc;
> +
> +	rc = acpi_link_trigger(irq, &polarity, &triggering, &sharing)
> +	if (rc)
> +		return false;
> +
> +	return (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_NONE);
> +}
> +
>  static int acpi_irq_get_penalty(int irq)
>  {
>  	int penalty = 0;
> @@ -502,8 +541,21 @@ static int acpi_irq_get_penalty(int irq)
>  	if (irq < ACPI_MAX_ISA_IRQ)
>  		penalty += acpi_irq_isa_penalty[irq];
>  
> -	if (irq == acpi_gbl_FADT.sci_interrupt)
> -		penalty += PIRQ_PENALTY_PCI_USING;
> +	if (irq == acpi_gbl_FADT.sci_interrupt) {
> +		u8 sci_polarity;
> +		u8 sci_trigger;
> +
> +		rc = acpi_link_trigger(irq, &polarity, &sci_trigger);
> +		if (!rc) {
> +			if (sci_trigger != ACPI_LEVEL_SENSITIVE ||
> +			    sci_polarity != ACPI_ACTIVE_LOW)
> +				penalty += PIRQ_PENALTY_ISA_ALWAYS;
> +			else
> +				penalty += PIRQ_PENALTY_PCI_USING;
> +		} else {
> +			return ~0;
> +		}
> +	}
>  
>  	penalty += acpi_irq_pci_sharing_penalty(irq);
>  	return penalty; 
> 
> 
> -- 
> Sinan Kaya
> Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] acpi,pci,irq: reduce resource requirements

[Sinan Kaya]

Hi Bjorn,

On 4/5/2016 7:21 PM, Bjorn Helgaas wrote:
>> I know you want to validate that all PCI interrupts are level, it looks like the code
>> > is allowing other combinations.
> It's not that we need to validate that all PCI interrupts are level.  What
> we need is to make sure all users sharing an IRQ agree on the mode.
> 

I posted v2. Let me know if the implementation feels like what you are looking for.

Sinan

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project