| Message ID | 1457484079-18202-1-git-send-email-okaya@codeaurora.org |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- 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
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.
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 -- 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
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 -- 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
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 -- 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
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) {} }
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 -- 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
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 >
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; - } } /*
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(-)