* Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-24 11:06 Duc Dang 2016-08-24 14:27 ` Lorenzo Pieralisi 2016-08-24 15:26 ` Marc Zyngier 0 siblings, 2 replies; 37+ messages in thread From: Duc Dang @ 2016-08-24 11:06 UTC (permalink / raw) To: Lorenzo Pieralisi, Rafael Wysocki Cc: linux-pci, linux-acpi, Marc Zyngier, patches [Resend in plain text mode] Hi Lorenzo, Rafael, ACPI 6.1 spec does not specify how to set interrupt polarity and trigger mode in _PRT when the interrupts are static (hardwired to specific interrupt inputs in interrupt controller). In current acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, GICv3 controllers and will cause failures in PCIe AER, PME services (on X-Gene platforms). Is there any way to specify polarity and trigger mode for static interrupts in _PRT? If not, can we introduce a _weak_ hook to specify default polarity and trigger mode for for ARM64 PCIe INTx in drivers/acpi/pci_irq.c? Regards, Duc Dang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 11:06 Defining polarity and trigger mode for static interrupts in _PRT Duc Dang @ 2016-08-24 14:27 ` Lorenzo Pieralisi 2016-08-24 19:30 ` Bjorn Helgaas 2016-08-24 15:26 ` Marc Zyngier 1 sibling, 1 reply; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-24 14:27 UTC (permalink / raw) To: Duc Dang Cc: Rafael Wysocki, linux-pci, linux-acpi, Marc Zyngier, patches, bhelgaas, punit.agrawal [ +Bjorn, Punit] On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > [Resend in plain text mode] > > Hi Lorenzo, Rafael, > > ACPI 6.1 spec does not specify how to set interrupt polarity and > trigger mode in _PRT when the interrupts are static (hardwired to > specific interrupt inputs in interrupt controller). In current > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > GICv3 controllers and will cause failures in PCIe AER, PME services > (on X-Gene platforms). > > Is there any way to specify polarity and trigger mode for static > interrupts in _PRT? If not, can we introduce a _weak_ hook to specify > default polarity and trigger mode for for ARM64 PCIe INTx in > drivers/acpi/pci_irq.c? Is there any reason why we can NOT use interrupt links (PNP0C0F) and related resources to describe those interrupts (I know that the spec says they can be used just for configurable IRQs but I suspect that was influenced by legacy x86 platforms) ? I think Rafael and Bjorn have a bit more visibility into why the interrupt links were specified that way (and I do not have enough x86 background to understand why the legacy IRQ routing was specified this way in ACPI - which was obviously biased by how x86 HW works). Lorenzo > > Regards, > Duc Dang. > ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 14:27 ` Lorenzo Pieralisi @ 2016-08-24 19:30 ` Bjorn Helgaas 2016-08-24 20:30 ` Marc Zyngier 0 siblings, 1 reply; 37+ messages in thread From: Bjorn Helgaas @ 2016-08-24 19:30 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, Marc Zyngier, patches, bhelgaas, punit.agrawal On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > [ +Bjorn, Punit] > > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > > [Resend in plain text mode] > > > > Hi Lorenzo, Rafael, > > > > ACPI 6.1 spec does not specify how to set interrupt polarity and > > trigger mode in _PRT when the interrupts are static (hardwired to > > specific interrupt inputs in interrupt controller). In current > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > > GICv3 controllers and will cause failures in PCIe AER, PME services > > (on X-Gene platforms). PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional and defined as 'level sensitive,' asserted low." I've heard before that ARM64 does this differently, but I still don't understand the difference. Obviously if you plug a legacy PCI card into an ARM64 system, it's still going to pull INTA# low to assert an interrupt. So is there something special about ARM64 that inverts that, or what? > > Is there any way to specify polarity and trigger mode for static > > interrupts in _PRT? There is no way I'm aware of in _PRT to specify polarity and trigger mode. I don't know the history, but my guess is that it would be seen as superfluous given that the PCI spec requires level, active low. Obviously I'm missing something important. Bjorn ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 19:30 ` Bjorn Helgaas @ 2016-08-24 20:30 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-24 20:30 UTC (permalink / raw) To: Bjorn Helgaas Cc: Lorenzo Pieralisi, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, bhelgaas, punit.agrawal On Wed, 24 Aug 2016 14:30:00 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > > [ +Bjorn, Punit] > > > > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > > > [Resend in plain text mode] > > > > > > Hi Lorenzo, Rafael, > > > > > > ACPI 6.1 spec does not specify how to set interrupt polarity and > > > trigger mode in _PRT when the interrupts are static (hardwired to > > > specific interrupt inputs in interrupt controller). In current > > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > > > GICv3 controllers and will cause failures in PCIe AER, PME services > > > (on X-Gene platforms). > > PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > and defined as 'level sensitive,' asserted low." > > I've heard before that ARM64 does this differently, but I still don't > understand the difference. Obviously if you plug a legacy PCI card > into an ARM64 system, it's still going to pull INTA# low to assert an > interrupt. So is there something special about ARM64 that inverts > that, or what? There is certainly an inverter somewhere on the interrupt path, because the GIC triggers on level high, not level low. But I don't think that's the issue Duc is trying to outline here, because that's not something SW can fix. I'm worried that in his system, the interrupt is edge triggered instead. > > > > Is there any way to specify polarity and trigger mode for static > > > interrupts in _PRT? > > There is no way I'm aware of in _PRT to specify polarity and trigger > mode. I don't know the history, but my guess is that it would be seen > as superfluous given that the PCI spec requires level, active low. > > Obviously I'm missing something important. Same here, unless the HW is not PCI compliant... Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-24 20:30 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-24 20:30 UTC (permalink / raw) To: Bjorn Helgaas Cc: Lorenzo Pieralisi, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, bhelgaas, punit.agrawal On Wed, 24 Aug 2016 14:30:00 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > > [ +Bjorn, Punit] > > > > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > > > [Resend in plain text mode] > > > > > > Hi Lorenzo, Rafael, > > > > > > ACPI 6.1 spec does not specify how to set interrupt polarity and > > > trigger mode in _PRT when the interrupts are static (hardwired to > > > specific interrupt inputs in interrupt controller). In current > > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > > > GICv3 controllers and will cause failures in PCIe AER, PME services > > > (on X-Gene platforms). > > PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > and defined as 'level sensitive,' asserted low." > > I've heard before that ARM64 does this differently, but I still don't > understand the difference. Obviously if you plug a legacy PCI card > into an ARM64 system, it's still going to pull INTA# low to assert an > interrupt. So is there something special about ARM64 that inverts > that, or what? There is certainly an inverter somewhere on the interrupt path, because the GIC triggers on level high, not level low. But I don't think that's the issue Duc is trying to outline here, because that's not something SW can fix. I'm worried that in his system, the interrupt is edge triggered instead. > > > > Is there any way to specify polarity and trigger mode for static > > > interrupts in _PRT? > > There is no way I'm aware of in _PRT to specify polarity and trigger > mode. I don't know the history, but my guess is that it would be seen > as superfluous given that the PCI spec requires level, active low. > > Obviously I'm missing something important. Same here, unless the HW is not PCI compliant... Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 20:30 ` Marc Zyngier (?) @ 2016-08-24 22:19 ` Duc Dang 2016-08-24 22:56 ` Bjorn Helgaas 2016-08-25 11:18 ` Marc Zyngier -1 siblings, 2 replies; 37+ messages in thread From: Duc Dang @ 2016-08-24 22:19 UTC (permalink / raw) To: Marc Zyngier Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, punit.agrawal On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On Wed, 24 Aug 2016 14:30:00 -0500 > Bjorn Helgaas <helgaas@kernel.org> wrote: > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: >> > [ +Bjorn, Punit] >> > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: >> > > [Resend in plain text mode] >> > > >> > > Hi Lorenzo, Rafael, >> > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and >> > > trigger mode in _PRT when the interrupts are static (hardwired to >> > > specific interrupt inputs in interrupt controller). In current >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, >> > > GICv3 controllers and will cause failures in PCIe AER, PME services >> > > (on X-Gene platforms). >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional >> and defined as 'level sensitive,' asserted low." >> >> I've heard before that ARM64 does this differently, but I still don't >> understand the difference. Obviously if you plug a legacy PCI card >> into an ARM64 system, it's still going to pull INTA# low to assert an >> interrupt. So is there something special about ARM64 that inverts >> that, or what? > > There is certainly an inverter somewhere on the interrupt path, because > the GIC triggers on level high, not level low. But I don't think that's > the issue Duc is trying to outline here, because that's not something > SW can fix. I'm worried that in his system, the interrupt is edge > triggered instead. Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been lucky when using trigger-rising for PCI INTx in DT boot mode. > >> >> > > Is there any way to specify polarity and trigger mode for static >> > > interrupts in _PRT? >> >> There is no way I'm aware of in _PRT to specify polarity and trigger >> mode. I don't know the history, but my guess is that it would be seen >> as superfluous given that the PCI spec requires level, active low. The device still pulls the INTx pin low to trigger interrupt, but the interrupt delivered to interrupt controller (GIC in this case) is not necessarily to be level-low. Current code assume level-low mode to program to the interrupt controller for INTx, and fails for GIC, GICv2m and GICv3. >> >> Obviously I'm missing something important. > > Same here, unless the HW is not PCI compliant... > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny. Regards, Duc Dang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 22:19 ` Duc Dang @ 2016-08-24 22:56 ` Bjorn Helgaas 2016-08-25 11:18 ` Marc Zyngier 1 sibling, 0 replies; 37+ messages in thread From: Bjorn Helgaas @ 2016-08-24 22:56 UTC (permalink / raw) To: Duc Dang Cc: Marc Zyngier, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, punit.agrawal On Wed, Aug 24, 2016 at 03:19:21PM -0700, Duc Dang wrote: > On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On Wed, 24 Aug 2016 14:30:00 -0500 > > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > >> > [ +Bjorn, Punit] > >> > > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > >> > > [Resend in plain text mode] > >> > > > >> > > Hi Lorenzo, Rafael, > >> > > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and > >> > > trigger mode in _PRT when the interrupts are static (hardwired to > >> > > specific interrupt inputs in interrupt controller). In current > >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > >> > > GICv3 controllers and will cause failures in PCIe AER, PME services > >> > > (on X-Gene platforms). > >> > >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > >> and defined as 'level sensitive,' asserted low." > >> > >> I've heard before that ARM64 does this differently, but I still don't > >> understand the difference. Obviously if you plug a legacy PCI card > >> into an ARM64 system, it's still going to pull INTA# low to assert an > >> interrupt. So is there something special about ARM64 that inverts > >> that, or what? > > > > There is certainly an inverter somewhere on the interrupt path, because > > the GIC triggers on level high, not level low. But I don't think that's > > the issue Duc is trying to outline here, because that's not something > > SW can fix. I'm worried that in his system, the interrupt is edge > > triggered instead. > > Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC > as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been > lucky when using trigger-rising for PCI INTx in DT boot mode. I'd say the code in drivers/acpi/pci_irq.c should be generic and assume what's in the PCI spec, i.e., level-triggered, active low. If a platform needs to do something else, that exception should be handled in platform-specific code somehow, not in pci_irq.c. > >> > > Is there any way to specify polarity and trigger mode for static > >> > > interrupts in _PRT? > >> > >> There is no way I'm aware of in _PRT to specify polarity and trigger > >> mode. I don't know the history, but my guess is that it would be seen > >> as superfluous given that the PCI spec requires level, active low. > > The device still pulls the INTx pin low to trigger interrupt, but the > interrupt delivered > to interrupt controller (GIC in this case) is not necessarily to be > level-low. Current code > assume level-low mode to program to the interrupt controller for INTx, > and fails for > GIC, GICv2m and GICv3. > > >> > >> Obviously I'm missing something important. > > > > Same here, unless the HW is not PCI compliant... > > > > Thanks, > > > > M. > > -- > > Jazz is not dead. It just smells funny. > > Regards, > Duc Dang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 22:19 ` Duc Dang @ 2016-08-25 11:18 ` Marc Zyngier 2016-08-25 11:18 ` Marc Zyngier 1 sibling, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-25 11:18 UTC (permalink / raw) To: Duc Dang Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, punit.agrawal On Wed, 24 Aug 2016 15:19:21 -0700 Duc Dang <dhdang@apm.com> wrote: > On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On Wed, 24 Aug 2016 14:30:00 -0500 > > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > >> > [ +Bjorn, Punit] > >> > > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > >> > > [Resend in plain text mode] > >> > > > >> > > Hi Lorenzo, Rafael, > >> > > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and > >> > > trigger mode in _PRT when the interrupts are static (hardwired to > >> > > specific interrupt inputs in interrupt controller). In current > >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > >> > > GICv3 controllers and will cause failures in PCIe AER, PME services > >> > > (on X-Gene platforms). > >> > >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > >> and defined as 'level sensitive,' asserted low." > >> > >> I've heard before that ARM64 does this differently, but I still don't > >> understand the difference. Obviously if you plug a legacy PCI card > >> into an ARM64 system, it's still going to pull INTA# low to assert an > >> interrupt. So is there something special about ARM64 that inverts > >> that, or what? > > > > There is certainly an inverter somewhere on the interrupt path, because > > the GIC triggers on level high, not level low. But I don't think that's > > the issue Duc is trying to outline here, because that's not something > > SW can fix. I'm worried that in his system, the interrupt is edge > > triggered instead. > > Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC > as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been > lucky when using trigger-rising for PCI INTx in DT boot mode. > > > > >> > >> > > Is there any way to specify polarity and trigger mode for static > >> > > interrupts in _PRT? > >> > >> There is no way I'm aware of in _PRT to specify polarity and trigger > >> mode. I don't know the history, but my guess is that it would be seen > >> as superfluous given that the PCI spec requires level, active low. > > The device still pulls the INTx pin low to trigger interrupt, but the > interrupt delivered > to interrupt controller (GIC in this case) is not necessarily to be > level-low. Current code > assume level-low mode to program to the interrupt controller for INTx, > and fails for > GIC, GICv2m and GICv3. Well, there's nothing that can't be fixed. The GIC doesn't have a programmatic notion of what is high or low. It only knows about level interrupts. But the HW only knows about level_high. Obviously, for things to work, the integrator has to put an inverter on the line to cope with level_low. If the driver code insist on using level_low, we can address this pretty easily, and just warn about the oddity: diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 6fc56c3..b3755a3 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -306,9 +306,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) return -EINVAL; /* SPIs have restrictions on the supported types */ - if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && - type != IRQ_TYPE_EDGE_RISING) - return -EINVAL; + if (irq >= 32) { + unsigned int tmp = type; + if (type == IRQ_TYPE_LEVEL_LOW) + type = IRQ_TYPE_LEVEL_HIGH; + if (type == IRQ_TYPE_EDGE_FALLING) + type = IRQ_TYPE_EDGE_RISING; + if (tmp != type) + pr_warn("Overriding IRQ%d type from %d to %d\n", + d->irq, tmp, type); + } if (gic_irq_in_rdist(d)) { base = gic_data_rdist_sgi_base(); diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index c2cab57..0d187dc 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -280,9 +280,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) return -EINVAL; /* SPIs have restrictions on the supported types */ - if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && - type != IRQ_TYPE_EDGE_RISING) - return -EINVAL; + if (gicirq >= 32) { + unsigned int tmp = type; + if (type == IRQ_TYPE_LEVEL_LOW) + type = IRQ_TYPE_LEVEL_HIGH; + if (type == IRQ_TYPE_EDGE_FALLING) + type = IRQ_TYPE_EDGE_RISING; + if (tmp != type) + pr_warn("Overriding IRQ%d type from %d to %d\n", + d->irq, tmp, type); + } return gic_configure_irq(gicirq, type, base, NULL); } Does this work for you? Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-25 11:18 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-25 11:18 UTC (permalink / raw) To: Duc Dang Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, punit.agrawal On Wed, 24 Aug 2016 15:19:21 -0700 Duc Dang <dhdang@apm.com> wrote: > On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On Wed, 24 Aug 2016 14:30:00 -0500 > > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > >> > [ +Bjorn, Punit] > >> > > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > >> > > [Resend in plain text mode] > >> > > > >> > > Hi Lorenzo, Rafael, > >> > > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and > >> > > trigger mode in _PRT when the interrupts are static (hardwired to > >> > > specific interrupt inputs in interrupt controller). In current > >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > >> > > GICv3 controllers and will cause failures in PCIe AER, PME services > >> > > (on X-Gene platforms). > >> > >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > >> and defined as 'level sensitive,' asserted low." > >> > >> I've heard before that ARM64 does this differently, but I still don't > >> understand the difference. Obviously if you plug a legacy PCI card > >> into an ARM64 system, it's still going to pull INTA# low to assert an > >> interrupt. So is there something special about ARM64 that inverts > >> that, or what? > > > > There is certainly an inverter somewhere on the interrupt path, because > > the GIC triggers on level high, not level low. But I don't think that's > > the issue Duc is trying to outline here, because that's not something > > SW can fix. I'm worried that in his system, the interrupt is edge > > triggered instead. > > Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC > as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been > lucky when using trigger-rising for PCI INTx in DT boot mode. > > > > >> > >> > > Is there any way to specify polarity and trigger mode for static > >> > > interrupts in _PRT? > >> > >> There is no way I'm aware of in _PRT to specify polarity and trigger > >> mode. I don't know the history, but my guess is that it would be seen > >> as superfluous given that the PCI spec requires level, active low. > > The device still pulls the INTx pin low to trigger interrupt, but the > interrupt delivered > to interrupt controller (GIC in this case) is not necessarily to be > level-low. Current code > assume level-low mode to program to the interrupt controller for INTx, > and fails for > GIC, GICv2m and GICv3. Well, there's nothing that can't be fixed. The GIC doesn't have a programmatic notion of what is high or low. It only knows about level interrupts. But the HW only knows about level_high. Obviously, for things to work, the integrator has to put an inverter on the line to cope with level_low. If the driver code insist on using level_low, we can address this pretty easily, and just warn about the oddity: diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index 6fc56c3..b3755a3 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -306,9 +306,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) return -EINVAL; /* SPIs have restrictions on the supported types */ - if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && - type != IRQ_TYPE_EDGE_RISING) - return -EINVAL; + if (irq >= 32) { + unsigned int tmp = type; + if (type == IRQ_TYPE_LEVEL_LOW) + type = IRQ_TYPE_LEVEL_HIGH; + if (type == IRQ_TYPE_EDGE_FALLING) + type = IRQ_TYPE_EDGE_RISING; + if (tmp != type) + pr_warn("Overriding IRQ%d type from %d to %d\n", + d->irq, tmp, type); + } if (gic_irq_in_rdist(d)) { base = gic_data_rdist_sgi_base(); diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index c2cab57..0d187dc 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -280,9 +280,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) return -EINVAL; /* SPIs have restrictions on the supported types */ - if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && - type != IRQ_TYPE_EDGE_RISING) - return -EINVAL; + if (gicirq >= 32) { + unsigned int tmp = type; + if (type == IRQ_TYPE_LEVEL_LOW) + type = IRQ_TYPE_LEVEL_HIGH; + if (type == IRQ_TYPE_EDGE_FALLING) + type = IRQ_TYPE_EDGE_RISING; + if (tmp != type) + pr_warn("Overriding IRQ%d type from %d to %d\n", + d->irq, tmp, type); + } return gic_configure_irq(gicirq, type, base, NULL); } Does this work for you? Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-25 11:18 ` Marc Zyngier (?) @ 2016-08-25 16:52 ` Duc Dang 2016-08-25 18:59 ` Marc Zyngier -1 siblings, 1 reply; 37+ messages in thread From: Duc Dang @ 2016-08-25 16:52 UTC (permalink / raw) To: Marc Zyngier Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Thu, Aug 25, 2016 at 4:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On Wed, 24 Aug 2016 15:19:21 -0700 > Duc Dang <dhdang@apm.com> wrote: > >> On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: >> > On Wed, 24 Aug 2016 14:30:00 -0500 >> > Bjorn Helgaas <helgaas@kernel.org> wrote: >> > >> >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: >> >> > [ +Bjorn, Punit] >> >> > >> >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: >> >> > > [Resend in plain text mode] >> >> > > >> >> > > Hi Lorenzo, Rafael, >> >> > > >> >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and >> >> > > trigger mode in _PRT when the interrupts are static (hardwired to >> >> > > specific interrupt inputs in interrupt controller). In current >> >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by >> >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to >> >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, >> >> > > GICv3 controllers and will cause failures in PCIe AER, PME services >> >> > > (on X-Gene platforms). >> >> >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional >> >> and defined as 'level sensitive,' asserted low." >> >> >> >> I've heard before that ARM64 does this differently, but I still don't >> >> understand the difference. Obviously if you plug a legacy PCI card >> >> into an ARM64 system, it's still going to pull INTA# low to assert an >> >> interrupt. So is there something special about ARM64 that inverts >> >> that, or what? >> > >> > There is certainly an inverter somewhere on the interrupt path, because >> > the GIC triggers on level high, not level low. But I don't think that's >> > the issue Duc is trying to outline here, because that's not something >> > SW can fix. I'm worried that in his system, the interrupt is edge >> > triggered instead. >> >> Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC >> as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been >> lucky when using trigger-rising for PCI INTx in DT boot mode. >> >> > >> >> >> >> > > Is there any way to specify polarity and trigger mode for static >> >> > > interrupts in _PRT? >> >> >> >> There is no way I'm aware of in _PRT to specify polarity and trigger >> >> mode. I don't know the history, but my guess is that it would be seen >> >> as superfluous given that the PCI spec requires level, active low. >> >> The device still pulls the INTx pin low to trigger interrupt, but the >> interrupt delivered >> to interrupt controller (GIC in this case) is not necessarily to be >> level-low. Current code >> assume level-low mode to program to the interrupt controller for INTx, >> and fails for >> GIC, GICv2m and GICv3. > > Well, there's nothing that can't be fixed. The GIC doesn't have a > programmatic notion of what is high or low. It only knows about level > interrupts. But the HW only knows about level_high. Obviously, for > things to work, the integrator has to put an inverter on the line to > cope with level_low. > > If the driver code insist on using level_low, we can address this > pretty easily, and just warn about the oddity: > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index 6fc56c3..b3755a3 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -306,9 +306,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > return -EINVAL; > > /* SPIs have restrictions on the supported types */ > - if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > - type != IRQ_TYPE_EDGE_RISING) > - return -EINVAL; > + if (irq >= 32) { > + unsigned int tmp = type; > + if (type == IRQ_TYPE_LEVEL_LOW) > + type = IRQ_TYPE_LEVEL_HIGH; > + if (type == IRQ_TYPE_EDGE_FALLING) > + type = IRQ_TYPE_EDGE_RISING; > + if (tmp != type) > + pr_warn("Overriding IRQ%d type from %d to %d\n", > + d->irq, tmp, type); > + } > > if (gic_irq_in_rdist(d)) { > base = gic_data_rdist_sgi_base(); > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > index c2cab57..0d187dc 100644 > --- a/drivers/irqchip/irq-gic.c > +++ b/drivers/irqchip/irq-gic.c > @@ -280,9 +280,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > return -EINVAL; > > /* SPIs have restrictions on the supported types */ > - if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > - type != IRQ_TYPE_EDGE_RISING) > - return -EINVAL; > + if (gicirq >= 32) { > + unsigned int tmp = type; > + if (type == IRQ_TYPE_LEVEL_LOW) > + type = IRQ_TYPE_LEVEL_HIGH; > + if (type == IRQ_TYPE_EDGE_FALLING) > + type = IRQ_TYPE_EDGE_RISING; > + if (tmp != type) > + pr_warn("Overriding IRQ%d type from %d to %d\n", > + d->irq, tmp, type); > + } > > return gic_configure_irq(gicirq, type, base, NULL); > } > > > > Does this work for you? Thanks, Marc! It works, I tested on current X-Gene platforms that uses GICv2 and GICv2m. Will you commit this change? It will be a huge help as going with interrupt link will require firmware change. > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny. Regards, Duc Dang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-25 16:52 ` Duc Dang @ 2016-08-25 18:59 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-25 18:59 UTC (permalink / raw) To: Duc Dang Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Thu, 25 Aug 2016 09:52:56 -0700 Duc Dang <dhdang@apm.com> wrote: > On Thu, Aug 25, 2016 at 4:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On Wed, 24 Aug 2016 15:19:21 -0700 > > Duc Dang <dhdang@apm.com> wrote: > > > >> On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > >> > On Wed, 24 Aug 2016 14:30:00 -0500 > >> > Bjorn Helgaas <helgaas@kernel.org> wrote: > >> > > >> >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > >> >> > [ +Bjorn, Punit] > >> >> > > >> >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > >> >> > > [Resend in plain text mode] > >> >> > > > >> >> > > Hi Lorenzo, Rafael, > >> >> > > > >> >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and > >> >> > > trigger mode in _PRT when the interrupts are static (hardwired to > >> >> > > specific interrupt inputs in interrupt controller). In current > >> >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > >> >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > >> >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > >> >> > > GICv3 controllers and will cause failures in PCIe AER, PME services > >> >> > > (on X-Gene platforms). > >> >> > >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > >> >> and defined as 'level sensitive,' asserted low." > >> >> > >> >> I've heard before that ARM64 does this differently, but I still don't > >> >> understand the difference. Obviously if you plug a legacy PCI card > >> >> into an ARM64 system, it's still going to pull INTA# low to assert an > >> >> interrupt. So is there something special about ARM64 that inverts > >> >> that, or what? > >> > > >> > There is certainly an inverter somewhere on the interrupt path, because > >> > the GIC triggers on level high, not level low. But I don't think that's > >> > the issue Duc is trying to outline here, because that's not something > >> > SW can fix. I'm worried that in his system, the interrupt is edge > >> > triggered instead. > >> > >> Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC > >> as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been > >> lucky when using trigger-rising for PCI INTx in DT boot mode. > >> > >> > > >> >> > >> >> > > Is there any way to specify polarity and trigger mode for static > >> >> > > interrupts in _PRT? > >> >> > >> >> There is no way I'm aware of in _PRT to specify polarity and trigger > >> >> mode. I don't know the history, but my guess is that it would be seen > >> >> as superfluous given that the PCI spec requires level, active low. > >> > >> The device still pulls the INTx pin low to trigger interrupt, but the > >> interrupt delivered > >> to interrupt controller (GIC in this case) is not necessarily to be > >> level-low. Current code > >> assume level-low mode to program to the interrupt controller for INTx, > >> and fails for > >> GIC, GICv2m and GICv3. > > > > Well, there's nothing that can't be fixed. The GIC doesn't have a > > programmatic notion of what is high or low. It only knows about level > > interrupts. But the HW only knows about level_high. Obviously, for > > things to work, the integrator has to put an inverter on the line to > > cope with level_low. > > > > If the driver code insist on using level_low, we can address this > > pretty easily, and just warn about the oddity: > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index 6fc56c3..b3755a3 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -306,9 +306,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > return -EINVAL; > > > > /* SPIs have restrictions on the supported types */ > > - if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > - type != IRQ_TYPE_EDGE_RISING) > > - return -EINVAL; > > + if (irq >= 32) { > > + unsigned int tmp = type; > > + if (type == IRQ_TYPE_LEVEL_LOW) > > + type = IRQ_TYPE_LEVEL_HIGH; > > + if (type == IRQ_TYPE_EDGE_FALLING) > > + type = IRQ_TYPE_EDGE_RISING; > > + if (tmp != type) > > + pr_warn("Overriding IRQ%d type from %d to %d\n", > > + d->irq, tmp, type); > > + } > > > > if (gic_irq_in_rdist(d)) { > > base = gic_data_rdist_sgi_base(); > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > > index c2cab57..0d187dc 100644 > > --- a/drivers/irqchip/irq-gic.c > > +++ b/drivers/irqchip/irq-gic.c > > @@ -280,9 +280,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > return -EINVAL; > > > > /* SPIs have restrictions on the supported types */ > > - if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > - type != IRQ_TYPE_EDGE_RISING) > > - return -EINVAL; > > + if (gicirq >= 32) { > > + unsigned int tmp = type; > > + if (type == IRQ_TYPE_LEVEL_LOW) > > + type = IRQ_TYPE_LEVEL_HIGH; > > + if (type == IRQ_TYPE_EDGE_FALLING) > > + type = IRQ_TYPE_EDGE_RISING; > > + if (tmp != type) > > + pr_warn("Overriding IRQ%d type from %d to %d\n", > > + d->irq, tmp, type); > > + } > > > > return gic_configure_irq(gicirq, type, base, NULL); > > } > > > > > > > > Does this work for you? > > Thanks, Marc! It works, I tested on current X-Gene platforms that uses > GICv2 and GICv2m. > > Will you commit this change? It will be a huge help as going with > interrupt link will require firmware change. Not for the time being. We now have an understanding of *why* things do not work, but Lorenzo seems to have a good grasp on what we can do to address it correctly, and we should explore this first. If (and only if) there is a consensus that firmware already does all it should, then I'll turn this hack into a proper series. Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-25 18:59 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-25 18:59 UTC (permalink / raw) To: Duc Dang Cc: Bjorn Helgaas, Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Thu, 25 Aug 2016 09:52:56 -0700 Duc Dang <dhdang@apm.com> wrote: > On Thu, Aug 25, 2016 at 4:18 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > > On Wed, 24 Aug 2016 15:19:21 -0700 > > Duc Dang <dhdang@apm.com> wrote: > > > >> On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@arm.com> wrote: > >> > On Wed, 24 Aug 2016 14:30:00 -0500 > >> > Bjorn Helgaas <helgaas@kernel.org> wrote: > >> > > >> >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > >> >> > [ +Bjorn, Punit] > >> >> > > >> >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > >> >> > > [Resend in plain text mode] > >> >> > > > >> >> > > Hi Lorenzo, Rafael, > >> >> > > > >> >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and > >> >> > > trigger mode in _PRT when the interrupts are static (hardwired to > >> >> > > specific interrupt inputs in interrupt controller). In current > >> >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > >> >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > >> >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > >> >> > > GICv3 controllers and will cause failures in PCIe AER, PME services > >> >> > > (on X-Gene platforms). > >> >> > >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > >> >> and defined as 'level sensitive,' asserted low." > >> >> > >> >> I've heard before that ARM64 does this differently, but I still don't > >> >> understand the difference. Obviously if you plug a legacy PCI card > >> >> into an ARM64 system, it's still going to pull INTA# low to assert an > >> >> interrupt. So is there something special about ARM64 that inverts > >> >> that, or what? > >> > > >> > There is certainly an inverter somewhere on the interrupt path, because > >> > the GIC triggers on level high, not level low. But I don't think that's > >> > the issue Duc is trying to outline here, because that's not something > >> > SW can fix. I'm worried that in his system, the interrupt is edge > >> > triggered instead. > >> > >> Yes, there is an inverter in the interrupt path to deliver interrupt to the GIC > >> as level-high. X-Gene GIC uses level high for PCI INTx. I myself has been > >> lucky when using trigger-rising for PCI INTx in DT boot mode. > >> > >> > > >> >> > >> >> > > Is there any way to specify polarity and trigger mode for static > >> >> > > interrupts in _PRT? > >> >> > >> >> There is no way I'm aware of in _PRT to specify polarity and trigger > >> >> mode. I don't know the history, but my guess is that it would be seen > >> >> as superfluous given that the PCI spec requires level, active low. > >> > >> The device still pulls the INTx pin low to trigger interrupt, but the > >> interrupt delivered > >> to interrupt controller (GIC in this case) is not necessarily to be > >> level-low. Current code > >> assume level-low mode to program to the interrupt controller for INTx, > >> and fails for > >> GIC, GICv2m and GICv3. > > > > Well, there's nothing that can't be fixed. The GIC doesn't have a > > programmatic notion of what is high or low. It only knows about level > > interrupts. But the HW only knows about level_high. Obviously, for > > things to work, the integrator has to put an inverter on the line to > > cope with level_low. > > > > If the driver code insist on using level_low, we can address this > > pretty easily, and just warn about the oddity: > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index 6fc56c3..b3755a3 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -306,9 +306,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > return -EINVAL; > > > > /* SPIs have restrictions on the supported types */ > > - if (irq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > - type != IRQ_TYPE_EDGE_RISING) > > - return -EINVAL; > > + if (irq >= 32) { > > + unsigned int tmp = type; > > + if (type == IRQ_TYPE_LEVEL_LOW) > > + type = IRQ_TYPE_LEVEL_HIGH; > > + if (type == IRQ_TYPE_EDGE_FALLING) > > + type = IRQ_TYPE_EDGE_RISING; > > + if (tmp != type) > > + pr_warn("Overriding IRQ%d type from %d to %d\n", > > + d->irq, tmp, type); > > + } > > > > if (gic_irq_in_rdist(d)) { > > base = gic_data_rdist_sgi_base(); > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c > > index c2cab57..0d187dc 100644 > > --- a/drivers/irqchip/irq-gic.c > > +++ b/drivers/irqchip/irq-gic.c > > @@ -280,9 +280,16 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > return -EINVAL; > > > > /* SPIs have restrictions on the supported types */ > > - if (gicirq >= 32 && type != IRQ_TYPE_LEVEL_HIGH && > > - type != IRQ_TYPE_EDGE_RISING) > > - return -EINVAL; > > + if (gicirq >= 32) { > > + unsigned int tmp = type; > > + if (type == IRQ_TYPE_LEVEL_LOW) > > + type = IRQ_TYPE_LEVEL_HIGH; > > + if (type == IRQ_TYPE_EDGE_FALLING) > > + type = IRQ_TYPE_EDGE_RISING; > > + if (tmp != type) > > + pr_warn("Overriding IRQ%d type from %d to %d\n", > > + d->irq, tmp, type); > > + } > > > > return gic_configure_irq(gicirq, type, base, NULL); > > } > > > > > > > > Does this work for you? > > Thanks, Marc! It works, I tested on current X-Gene platforms that uses > GICv2 and GICv2m. > > Will you commit this change? It will be a huge help as going with > interrupt link will require firmware change. Not for the time being. We now have an understanding of *why* things do not work, but Lorenzo seems to have a good grasp on what we can do to address it correctly, and we should explore this first. If (and only if) there is a consensus that firmware already does all it should, then I'll turn this hack into a proper series. Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-25 18:59 ` Marc Zyngier (?) @ 2016-08-26 9:08 ` Lorenzo Pieralisi 2016-08-26 11:04 ` okaya 2016-08-26 12:08 ` Marc Zyngier -1 siblings, 2 replies; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-26 9:08 UTC (permalink / raw) To: Marc Zyngier Cc: Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal, okaya [ +Sinan ] On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: [...] > > Thanks, Marc! It works, I tested on current X-Gene platforms that uses > > GICv2 and GICv2m. > > > > Will you commit this change? It will be a huge help as going with > > interrupt link will require firmware change. > > Not for the time being. We now have an understanding of *why* things do > not work, but Lorenzo seems to have a good grasp on what we can do to > address it correctly, and we should explore this first. If (and only if) > there is a consensus that firmware already does all it should, then > I'll turn this hack into a proper series. For the records, it is a discussion that already took place: https://lkml.org/lkml/2016/3/14/923 As I have said there are already ARM64 systems with ACPI tables out there using PCI interrupt links; I doubt that Qualcomm systems allow to reconfigure the GIC interrupt pin allocated to legacy PCI IRQs through interrupt links _SRS (hey it is *empty* :)), therefore: a) Some (the above is just an example from the mailing lists I am not picking on anyone it is just for the sake of this discussion, I have not dumped all ARM partners _PRT from their ACPI tables to check) ACPI tables must be rewritten because they are not FW compliant OR b) We allow PCI interrupt links to be used for static interrupt configurations OR c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder how GIC code can detect from which part of the kernel the interrupt request is coming, unless we implement an ACPI-PCI-legacy-IRQ-special gem) Comments ? Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-26 9:08 ` Lorenzo Pieralisi @ 2016-08-26 11:04 ` okaya 2016-08-26 12:08 ` Marc Zyngier 1 sibling, 0 replies; 37+ messages in thread From: okaya @ 2016-08-26 11:04 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marc Zyngier, Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On 2016-08-26 05:08, Lorenzo Pieralisi wrote: > [ +Sinan ] > > On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: > > [...] > >> > Thanks, Marc! It works, I tested on current X-Gene platforms that uses >> > GICv2 and GICv2m. >> > >> > Will you commit this change? It will be a huge help as going with >> > interrupt link will require firmware change. >> >> Not for the time being. We now have an understanding of *why* things >> do >> not work, but Lorenzo seems to have a good grasp on what we can do to >> address it correctly, and we should explore this first. If (and only >> if) >> there is a consensus that firmware already does all it should, then >> I'll turn this hack into a proper series. > > For the records, it is a discussion that already took place: > > https://lkml.org/lkml/2016/3/14/923 > > As I have said there are already ARM64 systems with ACPI tables > out there using PCI interrupt links; I doubt that Qualcomm systems > allow to reconfigure the GIC interrupt pin allocated to legacy PCI > IRQs through interrupt links _SRS (hey it is *empty* :)), > therefore: > > a) Some (the above is just an example from the mailing lists I am not > picking on anyone it is just for the sake of this discussion, I have > not dumped all ARM partners _PRT from their ACPI tables to check) > ACPI tables must be rewritten because they are not FW compliant > > OR > > b) We allow PCI interrupt links to be used for static interrupt > configurations > > OR > > c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder > how GIC code can detect from which part of the kernel the interrupt > request is coming, unless we implement an > ACPI-PCI-legacy-IRQ-special > gem) > > Comments ? > > Lorenzo I complained about active low assumption before when reviewing the series from Tomasz too. The above discussion took place when refactoring the pci link code. From Acpi spec perspective, both types of declarations are valid. It is just one syntax doesn't work on arm64. The other syntax is more expressive and it works. That is the reason why link objects are used in QCOM platforms. I think we need to fix the code by either changing the active low assumption or the gic code. Changing it at the gic level opens door for bugs that wouldn't be otherwise caught. I would make this change in pci code. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-26 9:08 ` Lorenzo Pieralisi @ 2016-08-26 12:08 ` Marc Zyngier 2016-08-26 12:08 ` Marc Zyngier 1 sibling, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-26 12:08 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal, okaya On Fri, 26 Aug 2016 10:08:13 +0100 Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > [ +Sinan ] > > On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: > > [...] > > > > Thanks, Marc! It works, I tested on current X-Gene platforms that uses > > > GICv2 and GICv2m. > > > > > > Will you commit this change? It will be a huge help as going with > > > interrupt link will require firmware change. > > > > Not for the time being. We now have an understanding of *why* things do > > not work, but Lorenzo seems to have a good grasp on what we can do to > > address it correctly, and we should explore this first. If (and only if) > > there is a consensus that firmware already does all it should, then > > I'll turn this hack into a proper series. > > For the records, it is a discussion that already took place: > > https://lkml.org/lkml/2016/3/14/923 > > As I have said there are already ARM64 systems with ACPI tables > out there using PCI interrupt links; I doubt that Qualcomm systems > allow to reconfigure the GIC interrupt pin allocated to legacy PCI > IRQs through interrupt links _SRS (hey it is *empty* :)), > therefore: > > a) Some (the above is just an example from the mailing lists I am not > picking on anyone it is just for the sake of this discussion, I have > not dumped all ARM partners _PRT from their ACPI tables to check) > ACPI tables must be rewritten because they are not FW compliant > > OR > > b) We allow PCI interrupt links to be used for static interrupt > configurations > > OR > > c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder > how GIC code can detect from which part of the kernel the interrupt > request is coming, unless we implement an ACPI-PCI-legacy-IRQ-special > gem) > > Comments ? I'm not overly keen on (c), as it is pretty hard to find out where the request is coming from (and the hack I previously posted opens the door to all kind of undetected misconfiguration). We *could* use a stacked irqchip to represent the inverter, but it feels like using a car sized hammer to squash a tiny fly. (b) seems like the right thing to do, but I cannot comment on whether or not this is compliant with the specification. Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-26 12:08 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-26 12:08 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal, okaya On Fri, 26 Aug 2016 10:08:13 +0100 Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > [ +Sinan ] > > On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: > > [...] > > > > Thanks, Marc! It works, I tested on current X-Gene platforms that uses > > > GICv2 and GICv2m. > > > > > > Will you commit this change? It will be a huge help as going with > > > interrupt link will require firmware change. > > > > Not for the time being. We now have an understanding of *why* things do > > not work, but Lorenzo seems to have a good grasp on what we can do to > > address it correctly, and we should explore this first. If (and only if) > > there is a consensus that firmware already does all it should, then > > I'll turn this hack into a proper series. > > For the records, it is a discussion that already took place: > > https://lkml.org/lkml/2016/3/14/923 > > As I have said there are already ARM64 systems with ACPI tables > out there using PCI interrupt links; I doubt that Qualcomm systems > allow to reconfigure the GIC interrupt pin allocated to legacy PCI > IRQs through interrupt links _SRS (hey it is *empty* :)), > therefore: > > a) Some (the above is just an example from the mailing lists I am not > picking on anyone it is just for the sake of this discussion, I have > not dumped all ARM partners _PRT from their ACPI tables to check) > ACPI tables must be rewritten because they are not FW compliant > > OR > > b) We allow PCI interrupt links to be used for static interrupt > configurations > > OR > > c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder > how GIC code can detect from which part of the kernel the interrupt > request is coming, unless we implement an ACPI-PCI-legacy-IRQ-special > gem) > > Comments ? I'm not overly keen on (c), as it is pretty hard to find out where the request is coming from (and the hack I previously posted opens the door to all kind of undetected misconfiguration). We *could* use a stacked irqchip to represent the inverter, but it feels like using a car sized hammer to squash a tiny fly. (b) seems like the right thing to do, but I cannot comment on whether or not this is compliant with the specification. Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-26 12:08 ` Marc Zyngier (?) @ 2016-08-26 14:07 ` Sinan Kaya 2016-08-26 17:06 ` Lorenzo Pieralisi -1 siblings, 1 reply; 37+ messages in thread From: Sinan Kaya @ 2016-08-26 14:07 UTC (permalink / raw) To: Marc Zyngier, Lorenzo Pieralisi Cc: Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On 8/26/2016 8:08 AM, Marc Zyngier wrote: > On Fri, 26 Aug 2016 10:08:13 +0100 > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > >> [ +Sinan ] >> >> On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: >> >> [...] >> >>>> Thanks, Marc! It works, I tested on current X-Gene platforms that uses >>>> GICv2 and GICv2m. >>>> >>>> Will you commit this change? It will be a huge help as going with >>>> interrupt link will require firmware change. >>> >>> Not for the time being. We now have an understanding of *why* things do >>> not work, but Lorenzo seems to have a good grasp on what we can do to >>> address it correctly, and we should explore this first. If (and only if) >>> there is a consensus that firmware already does all it should, then >>> I'll turn this hack into a proper series. >> >> For the records, it is a discussion that already took place: >> >> https://lkml.org/lkml/2016/3/14/923 >> >> As I have said there are already ARM64 systems with ACPI tables >> out there using PCI interrupt links; I doubt that Qualcomm systems >> allow to reconfigure the GIC interrupt pin allocated to legacy PCI >> IRQs through interrupt links _SRS (hey it is *empty* :)), >> therefore: >> >> a) Some (the above is just an example from the mailing lists I am not >> picking on anyone it is just for the sake of this discussion, I have >> not dumped all ARM partners _PRT from their ACPI tables to check) >> ACPI tables must be rewritten because they are not FW compliant >> >> OR >> >> b) We allow PCI interrupt links to be used for static interrupt >> configurations >> >> OR >> >> c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder >> how GIC code can detect from which part of the kernel the interrupt >> request is coming, unless we implement an ACPI-PCI-legacy-IRQ-special >> gem) >> >> Comments ? > > I'm not overly keen on (c), as it is pretty hard to find out where the > request is coming from (and the hack I previously posted opens the door > to all kind of undetected misconfiguration). We *could* use a stacked > irqchip to represent the inverter, but it feels like using a car sized > hammer to squash a tiny fly. > > (b) seems like the right thing to do, but I cannot comment on whether > or not this is compliant with the specification. > > Thanks, > > M. > Let me throw option d here. I know Bjorn wants to keep ACTIVE_LOW in the code for common code but can't we override this in an arch specific way (arm64's pci.c) while creating the root bridge? If the ARCH override doesn't exist, ACTIVE LOW still remains the default. There could be another arch that could have the same problem in the future. This way, we don't need to touch irqchip (GIC) driver or introduce a new API and/or introduce bugs for the rest of the non-PCI code. >From what I see in the ACPI spec, both _PRT approaches are correct and they need to be supported by Linux. -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-26 14:07 ` Sinan Kaya @ 2016-08-26 17:06 ` Lorenzo Pieralisi 2016-08-26 22:53 ` Sinan Kaya 0 siblings, 1 reply; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-26 17:06 UTC (permalink / raw) To: Sinan Kaya Cc: Marc Zyngier, Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Fri, Aug 26, 2016 at 10:07:31AM -0400, Sinan Kaya wrote: > On 8/26/2016 8:08 AM, Marc Zyngier wrote: > > On Fri, 26 Aug 2016 10:08:13 +0100 > > Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > > >> [ +Sinan ] > >> > >> On Thu, Aug 25, 2016 at 07:59:17PM +0100, Marc Zyngier wrote: > >> > >> [...] > >> > >>>> Thanks, Marc! It works, I tested on current X-Gene platforms that uses > >>>> GICv2 and GICv2m. > >>>> > >>>> Will you commit this change? It will be a huge help as going with > >>>> interrupt link will require firmware change. > >>> > >>> Not for the time being. We now have an understanding of *why* things do > >>> not work, but Lorenzo seems to have a good grasp on what we can do to > >>> address it correctly, and we should explore this first. If (and only if) > >>> there is a consensus that firmware already does all it should, then > >>> I'll turn this hack into a proper series. > >> > >> For the records, it is a discussion that already took place: > >> > >> https://lkml.org/lkml/2016/3/14/923 > >> > >> As I have said there are already ARM64 systems with ACPI tables > >> out there using PCI interrupt links; I doubt that Qualcomm systems > >> allow to reconfigure the GIC interrupt pin allocated to legacy PCI > >> IRQs through interrupt links _SRS (hey it is *empty* :)), > >> therefore: > >> > >> a) Some (the above is just an example from the mailing lists I am not > >> picking on anyone it is just for the sake of this discussion, I have > >> not dumped all ARM partners _PRT from their ACPI tables to check) > >> ACPI tables must be rewritten because they are not FW compliant > >> > >> OR > >> > >> b) We allow PCI interrupt links to be used for static interrupt > >> configurations > >> > >> OR > >> > >> c) We ignore the polarity flag (only for PCI legacy IRQs ? I wonder > >> how GIC code can detect from which part of the kernel the interrupt > >> request is coming, unless we implement an ACPI-PCI-legacy-IRQ-special > >> gem) > >> > >> Comments ? > > > > I'm not overly keen on (c), as it is pretty hard to find out where the > > request is coming from (and the hack I previously posted opens the door > > to all kind of undetected misconfiguration). We *could* use a stacked > > irqchip to represent the inverter, but it feels like using a car sized > > hammer to squash a tiny fly. > > > > (b) seems like the right thing to do, but I cannot comment on whether > > or not this is compliant with the specification. > > > > Thanks, > > > > M. > > > > Let me throw option d here. > > I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > can't we override this in an arch specific way (arm64's pci.c) while > creating the root bridge? On what basis ? You were not copied in from the beginning, but that is not different from Duc's initial proposal, which Marc discarded because it should not be done at arch level, it depends on the interrupt controller. Possibly a hook to be called from GIC code to override the default ACPI PCI IRQ polarity, I think that's _horrible_ but if we want to successfully boot APM's platforms that are $SUBJECT of this thread something has to be done (and it is not patching FW because we can't). > If the ARCH override doesn't exist, ACTIVE LOW still remains the > default. There could be another arch that could have the same problem > in the future. See above. > This way, we don't need to touch irqchip (GIC) driver or introduce a > new API and/or introduce bugs for the rest of the non-PCI code. > > From what I see in the ACPI spec, both _PRT approaches are correct and > they need to be supported by Linux. They are; ..but the spec says that your ACPI tables are buggy, because you are using a PCI interrupt link for an interrupt that is not configurable (frankly I still do not understand why as I explained). And then there is FW that has already shipped and we can't patch and it is not using PCI interrupt links so we have to quirk it in the kernel somehow (ie I am not sure APM platforms are the only ones containing _PRT with static PCI legacy IRQ entries, unfortunately). Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-26 17:06 ` Lorenzo Pieralisi @ 2016-08-26 22:53 ` Sinan Kaya 2016-08-30 10:08 ` Lorenzo Pieralisi 0 siblings, 1 reply; 37+ messages in thread From: Sinan Kaya @ 2016-08-26 22:53 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Marc Zyngier, Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal >> Let me throw option d here. >> >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but >> can't we override this in an arch specific way (arm64's pci.c) while >> creating the root bridge? > > On what basis ? You were not copied in from the beginning, but that > is not different from Duc's initial proposal, which Marc discarded > because it should not be done at arch level, it depends on the interrupt > controller. I happen to watch the linux-pci and linux-acpi mail-lists. I also saw Duc's initial proposal. I can't imagine someone building an ACPI compliant ARM64 platform without a GIC interrupt controller. The SBSA spec already mentions what kind of compatibility should be maintained with respect to GIC. You can't have an ACPI system that's SBSA compliant and not using GIC. Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI is defined. Why do we have to reach out to the interrupt controller? I just don't want GIC code to do auto-correction on interrupt levels on behalf of the firmware and hide firmware problems for non-PCI devices. We are talking about an ACPI only problem for PCI devices only. Coming back to the problem, I complained about this last year November here. It didn't get enough attention probably because we were trying to get the base PCI support into the kernel. https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html I was watching this thread to see where it is going. > > Possibly a hook to be called from GIC code to override the default > ACPI PCI IRQ polarity, I think that's _horrible_ but if we want to > successfully boot APM's platforms that are $SUBJECT of this thread > something has to be done (and it is not patching FW because we > can't). > >> If the ARCH override doesn't exist, ACTIVE LOW still remains the >> default. There could be another arch that could have the same problem >> in the future. > > See above. > >> This way, we don't need to touch irqchip (GIC) driver or introduce a >> new API and/or introduce bugs for the rest of the non-PCI code. >> >> From what I see in the ACPI spec, both _PRT approaches are correct and >> they need to be supported by Linux. > > They are; ..but the spec says that your ACPI tables are buggy, because > you are using a PCI interrupt link for an interrupt that is not > configurable (frankly I still do not understand why as I explained). > If you look at my email above, I tried getting rid of PCI Link object and I couldn't. I sticked to only thing that works. > And then there is FW that has already shipped and we can't patch > and it is not using PCI interrupt links so we have to quirk it in > the kernel somehow (ie I am not sure APM platforms are the only ones > containing _PRT with static PCI legacy IRQ entries, unfortunately). > > Lorenzo > -- > 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 Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-26 22:53 ` Sinan Kaya @ 2016-08-30 10:08 ` Lorenzo Pieralisi 2016-08-30 15:51 ` Duc Dang 2016-08-31 13:05 ` Bjorn Helgaas 0 siblings, 2 replies; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-30 10:08 UTC (permalink / raw) To: Sinan Kaya Cc: Marc Zyngier, Duc Dang, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > >> Let me throw option d here. > >> > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > >> can't we override this in an arch specific way (arm64's pci.c) while > >> creating the root bridge? > > > > On what basis ? You were not copied in from the beginning, but that > > is not different from Duc's initial proposal, which Marc discarded > > because it should not be done at arch level, it depends on the interrupt > > controller. > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > Duc's initial proposal. > > I can't imagine someone building an ACPI compliant ARM64 platform > without a GIC interrupt controller. > > The SBSA spec already mentions what kind of compatibility should be > maintained with respect to GIC. You can't have an ACPI system that's > SBSA compliant and not using GIC. > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > is defined. Why do we have to reach out to the interrupt controller? Patch below (horrible but no solution will be shiny either). > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html [...] > If you look at my email above, I tried getting rid of PCI Link object > and I couldn't. I sticked to only thing that works. That's what I object to. If the ACPI bindings do not work for ARM we do not work around issues, we upgrade the specs because what may work for you has to work on all ARM platforms (and all FW developers have to be aware of that). Granted, this is a tiny snag, but the point is that no one knows what's the correct way of describing PCI legacy IRQs on ARM and we need that rectified. This does the trick for me (I can turn it into a function/look-up that returns the polarity), I am sure it will ruffle feathers but we have to find a solution so here it is (that acpi_irq_model gem is already used in generic code drivers/acpi/pci_link.c ;-)) -- >8 -- diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c index 2c45dd3..c9b8c85 100644 --- a/drivers/acpi/pci_irq.c +++ b/drivers/acpi/pci_irq.c @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) int gsi; u8 pin; int triggering = ACPI_LEVEL_SENSITIVE; - int polarity = ACPI_ACTIVE_LOW; + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; char *link = NULL; char link_desc[16]; int rc; ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-30 10:08 ` Lorenzo Pieralisi @ 2016-08-30 15:51 ` Duc Dang 2016-08-30 17:54 ` Sinan Kaya 2016-08-31 13:05 ` Bjorn Helgaas 1 sibling, 1 reply; 37+ messages in thread From: Duc Dang @ 2016-08-30 15:51 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Sinan Kaya, Marc Zyngier, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Tue, Aug 30, 2016 at 3:08 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: >> >> >> Let me throw option d here. >> >> >> >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but >> >> can't we override this in an arch specific way (arm64's pci.c) while >> >> creating the root bridge? >> > >> > On what basis ? You were not copied in from the beginning, but that >> > is not different from Duc's initial proposal, which Marc discarded >> > because it should not be done at arch level, it depends on the interrupt >> > controller. >> >> I happen to watch the linux-pci and linux-acpi mail-lists. I also saw >> Duc's initial proposal. >> >> I can't imagine someone building an ACPI compliant ARM64 platform >> without a GIC interrupt controller. >> >> The SBSA spec already mentions what kind of compatibility should be >> maintained with respect to GIC. You can't have an ACPI system that's >> SBSA compliant and not using GIC. >> >> Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI >> is defined. Why do we have to reach out to the interrupt controller? > > Patch below (horrible but no solution will be shiny either). > >> https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > [...] > >> If you look at my email above, I tried getting rid of PCI Link object >> and I couldn't. I sticked to only thing that works. > > That's what I object to. If the ACPI bindings do not work for ARM > we do not work around issues, we upgrade the specs because what may work > for you has to work on all ARM platforms (and all FW developers have > to be aware of that). Granted, this is a tiny snag, but the point is > that no one knows what's the correct way of describing PCI legacy IRQs > on ARM and we need that rectified. > > This does the trick for me (I can turn it into a function/look-up > that returns the polarity), I am sure it will ruffle feathers but > we have to find a solution so here it is (that acpi_irq_model gem > is already used in generic code drivers/acpi/pci_link.c ;-)) > Good catch! This acpi_irq_model gem does help X-Gene :) > -- >8 -- > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > index 2c45dd3..c9b8c85 100644 > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > int gsi; > u8 pin; > int triggering = ACPI_LEVEL_SENSITIVE; > - int polarity = ACPI_ACTIVE_LOW; > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > char *link = NULL; > char link_desc[16]; > int rc; Regards, Duc Dang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-30 15:51 ` Duc Dang @ 2016-08-30 17:54 ` Sinan Kaya 2016-08-31 10:07 ` Lorenzo Pieralisi 0 siblings, 1 reply; 37+ messages in thread From: Sinan Kaya @ 2016-08-30 17:54 UTC (permalink / raw) To: Duc Dang, Lorenzo Pieralisi Cc: Marc Zyngier, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On 8/30/2016 11:51 AM, Duc Dang wrote: > On Tue, Aug 30, 2016 at 3:08 AM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: >> On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: >>> >>>>> Let me throw option d here. >>>>> >>>>> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but >>>>> can't we override this in an arch specific way (arm64's pci.c) while >>>>> creating the root bridge? >>>> >>>> On what basis ? You were not copied in from the beginning, but that >>>> is not different from Duc's initial proposal, which Marc discarded >>>> because it should not be done at arch level, it depends on the interrupt >>>> controller. >>> >>> I happen to watch the linux-pci and linux-acpi mail-lists. I also saw >>> Duc's initial proposal. >>> >>> I can't imagine someone building an ACPI compliant ARM64 platform >>> without a GIC interrupt controller. >>> >>> The SBSA spec already mentions what kind of compatibility should be >>> maintained with respect to GIC. You can't have an ACPI system that's >>> SBSA compliant and not using GIC. >>> >>> Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI >>> is defined. Why do we have to reach out to the interrupt controller? >> >> Patch below (horrible but no solution will be shiny either). >> >>> https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html >> >> [...] >> >>> If you look at my email above, I tried getting rid of PCI Link object >>> and I couldn't. I sticked to only thing that works. >> >> That's what I object to. If the ACPI bindings do not work for ARM >> we do not work around issues, we upgrade the specs because what may work >> for you has to work on all ARM platforms (and all FW developers have >> to be aware of that). Granted, this is a tiny snag, but the point is >> that no one knows what's the correct way of describing PCI legacy IRQs >> on ARM and we need that rectified. >> >> This does the trick for me (I can turn it into a function/look-up >> that returns the polarity), I am sure it will ruffle feathers but >> we have to find a solution so here it is (that acpi_irq_model gem >> is already used in generic code drivers/acpi/pci_link.c ;-)) >> > > Good catch! This acpi_irq_model gem does help X-Gene :) > +1 to this too. We don't need to invent some fake API or push stuff to the arch directory. >> -- >8 -- >> diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c >> index 2c45dd3..c9b8c85 100644 >> --- a/drivers/acpi/pci_irq.c >> +++ b/drivers/acpi/pci_irq.c >> @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) >> int gsi; >> u8 pin; >> int triggering = ACPI_LEVEL_SENSITIVE; >> - int polarity = ACPI_ACTIVE_LOW; >> + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? >> + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; >> char *link = NULL; >> char link_desc[16]; >> int rc; > Regards, > Duc Dang. > -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-30 17:54 ` Sinan Kaya @ 2016-08-31 10:07 ` Lorenzo Pieralisi 0 siblings, 0 replies; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-31 10:07 UTC (permalink / raw) To: Sinan Kaya Cc: Duc Dang, Marc Zyngier, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Tue, Aug 30, 2016 at 01:54:24PM -0400, Sinan Kaya wrote: > On 8/30/2016 11:51 AM, Duc Dang wrote: > > On Tue, Aug 30, 2016 at 3:08 AM, Lorenzo Pieralisi > > <lorenzo.pieralisi@arm.com> wrote: > >> On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > >>> > >>>>> Let me throw option d here. > >>>>> > >>>>> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > >>>>> can't we override this in an arch specific way (arm64's pci.c) while > >>>>> creating the root bridge? > >>>> > >>>> On what basis ? You were not copied in from the beginning, but that > >>>> is not different from Duc's initial proposal, which Marc discarded > >>>> because it should not be done at arch level, it depends on the interrupt > >>>> controller. > >>> > >>> I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > >>> Duc's initial proposal. > >>> > >>> I can't imagine someone building an ACPI compliant ARM64 platform > >>> without a GIC interrupt controller. > >>> > >>> The SBSA spec already mentions what kind of compatibility should be > >>> maintained with respect to GIC. You can't have an ACPI system that's > >>> SBSA compliant and not using GIC. > >>> > >>> Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > >>> is defined. Why do we have to reach out to the interrupt controller? > >> > >> Patch below (horrible but no solution will be shiny either). > >> > >>> https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > >> > >> [...] > >> > >>> If you look at my email above, I tried getting rid of PCI Link object > >>> and I couldn't. I sticked to only thing that works. > >> > >> That's what I object to. If the ACPI bindings do not work for ARM > >> we do not work around issues, we upgrade the specs because what may work > >> for you has to work on all ARM platforms (and all FW developers have > >> to be aware of that). Granted, this is a tiny snag, but the point is > >> that no one knows what's the correct way of describing PCI legacy IRQs > >> on ARM and we need that rectified. > >> > >> This does the trick for me (I can turn it into a function/look-up > >> that returns the polarity), I am sure it will ruffle feathers but > >> we have to find a solution so here it is (that acpi_irq_model gem > >> is already used in generic code drivers/acpi/pci_link.c ;-)) > >> > > > > Good catch! This acpi_irq_model gem does help X-Gene :) > > > > +1 to this too. > > We don't need to invent some fake API or push stuff to the arch directory. Ok, I will make it a proper patch and post it then. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-30 10:08 ` Lorenzo Pieralisi 2016-08-30 15:51 ` Duc Dang @ 2016-08-31 13:05 ` Bjorn Helgaas 2016-08-31 13:34 ` Lorenzo Pieralisi 1 sibling, 1 reply; 37+ messages in thread From: Bjorn Helgaas @ 2016-08-31 13:05 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > >> Let me throw option d here. > > >> > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > >> can't we override this in an arch specific way (arm64's pci.c) while > > >> creating the root bridge? > > > > > > On what basis ? You were not copied in from the beginning, but that > > > is not different from Duc's initial proposal, which Marc discarded > > > because it should not be done at arch level, it depends on the interrupt > > > controller. > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > Duc's initial proposal. > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > without a GIC interrupt controller. > > > > The SBSA spec already mentions what kind of compatibility should be > > maintained with respect to GIC. You can't have an ACPI system that's > > SBSA compliant and not using GIC. > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > is defined. Why do we have to reach out to the interrupt controller? > > Patch below (horrible but no solution will be shiny either). > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > [...] > > > If you look at my email above, I tried getting rid of PCI Link object > > and I couldn't. I sticked to only thing that works. > > That's what I object to. If the ACPI bindings do not work for ARM > we do not work around issues, we upgrade the specs because what may work > for you has to work on all ARM platforms (and all FW developers have > to be aware of that). Granted, this is a tiny snag, but the point is > that no one knows what's the correct way of describing PCI legacy IRQs > on ARM and we need that rectified. > > This does the trick for me (I can turn it into a function/look-up > that returns the polarity), I am sure it will ruffle feathers but > we have to find a solution so here it is (that acpi_irq_model gem > is already used in generic code drivers/acpi/pci_link.c ;-)) > > -- >8 -- > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > index 2c45dd3..c9b8c85 100644 > --- a/drivers/acpi/pci_irq.c > +++ b/drivers/acpi/pci_irq.c > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > int gsi; > u8 pin; > int triggering = ACPI_LEVEL_SENSITIVE; > - int polarity = ACPI_ACTIVE_LOW; > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > char *link = NULL; > char link_desc[16]; > int rc; This still seems weird to me. If I understand correctly, this GIC has several inputs, all active high. Some of those inputs are connected to inverters and then to PCI INTx wires. A generic device driver knows about the hardware it drives, including the properties of its interrupt wires. PCI drivers and the ACPI/PCI core know that conventional PCI device INTx wires are active low. These drivers, being generic, do not know about the GIC inverters. The patch above basically says "if ACPI tells us about a PCI interrupt connected to a GIC, *assume* there is an inverter on the input." But there's no actual description of that inverter anywhere in ACPI or a device tree. Shouldn't that be made explicit somewhere? If we connect a non-PCI device to a GIC, we need to know whether there's an inverter. How could we figure that out? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-31 13:05 ` Bjorn Helgaas @ 2016-08-31 13:34 ` Lorenzo Pieralisi 2016-08-31 16:05 ` Bjorn Helgaas 0 siblings, 1 reply; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-31 13:34 UTC (permalink / raw) To: Bjorn Helgaas Cc: Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Wed, Aug 31, 2016 at 08:05:06AM -0500, Bjorn Helgaas wrote: > On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > > > >> Let me throw option d here. > > > >> > > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > > >> can't we override this in an arch specific way (arm64's pci.c) while > > > >> creating the root bridge? > > > > > > > > On what basis ? You were not copied in from the beginning, but that > > > > is not different from Duc's initial proposal, which Marc discarded > > > > because it should not be done at arch level, it depends on the interrupt > > > > controller. > > > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > > Duc's initial proposal. > > > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > > without a GIC interrupt controller. > > > > > > The SBSA spec already mentions what kind of compatibility should be > > > maintained with respect to GIC. You can't have an ACPI system that's > > > SBSA compliant and not using GIC. > > > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > > is defined. Why do we have to reach out to the interrupt controller? > > > > Patch below (horrible but no solution will be shiny either). > > > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > > > [...] > > > > > If you look at my email above, I tried getting rid of PCI Link object > > > and I couldn't. I sticked to only thing that works. > > > > That's what I object to. If the ACPI bindings do not work for ARM > > we do not work around issues, we upgrade the specs because what may work > > for you has to work on all ARM platforms (and all FW developers have > > to be aware of that). Granted, this is a tiny snag, but the point is > > that no one knows what's the correct way of describing PCI legacy IRQs > > on ARM and we need that rectified. > > > > This does the trick for me (I can turn it into a function/look-up > > that returns the polarity), I am sure it will ruffle feathers but > > we have to find a solution so here it is (that acpi_irq_model gem > > is already used in generic code drivers/acpi/pci_link.c ;-)) > > > > -- >8 -- > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > > index 2c45dd3..c9b8c85 100644 > > --- a/drivers/acpi/pci_irq.c > > +++ b/drivers/acpi/pci_irq.c > > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > > int gsi; > > u8 pin; > > int triggering = ACPI_LEVEL_SENSITIVE; > > - int polarity = ACPI_ACTIVE_LOW; > > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > > char *link = NULL; > > char link_desc[16]; > > int rc; > > This still seems weird to me. > > If I understand correctly, this GIC has several inputs, all active > high. Some of those inputs are connected to inverters and then to PCI > INTx wires. > > A generic device driver knows about the hardware it drives, including > the properties of its interrupt wires. PCI drivers and the ACPI/PCI > core know that conventional PCI device INTx wires are active low. > These drivers, being generic, do not know about the GIC inverters. > > The patch above basically says "if ACPI tells us about a PCI interrupt > connected to a GIC, *assume* there is an inverter on the input." But > there's no actual description of that inverter anywhere in ACPI or a > device tree. Shouldn't that be made explicit somewhere? It is explicit for all IRQs other than PCI legacy IRQs ;-), that's the message I wanted to get across and I failed so far. For "normal" IRQs we can use Extended Interrupt Descriptors, that allow us to describe polarity. For PCI legacy IRQs we could use extended interrupt descriptors if we were allowed to use PCI interrupt links, except that, according to current ACPI specs, PCI interrupt links can only be used for *configurable* interrupt pins. So, some ARM vendors stuck to the static/hardwired configuration in the _PRT, that does not give us a chance to describe polarity. I think that we should be allowed to use interrupt links, but that would not comply with the specs (and on top of that there is FW that is already shipped in ACPI tables that we can't change anymore). > If we connect a non-PCI device to a GIC, we need to know whether > there's an inverter. How could we figure that out? Through an Extended Interrupt Descriptor. How are we solving this ? Thanks, Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-31 13:34 ` Lorenzo Pieralisi @ 2016-08-31 16:05 ` Bjorn Helgaas 2016-08-31 16:37 ` Lorenzo Pieralisi 0 siblings, 1 reply; 37+ messages in thread From: Bjorn Helgaas @ 2016-08-31 16:05 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Wed, Aug 31, 2016 at 02:34:54PM +0100, Lorenzo Pieralisi wrote: > On Wed, Aug 31, 2016 at 08:05:06AM -0500, Bjorn Helgaas wrote: > > On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > > > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > > > > > >> Let me throw option d here. > > > > >> > > > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > > > >> can't we override this in an arch specific way (arm64's pci.c) while > > > > >> creating the root bridge? > > > > > > > > > > On what basis ? You were not copied in from the beginning, but that > > > > > is not different from Duc's initial proposal, which Marc discarded > > > > > because it should not be done at arch level, it depends on the interrupt > > > > > controller. > > > > > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > > > Duc's initial proposal. > > > > > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > > > without a GIC interrupt controller. > > > > > > > > The SBSA spec already mentions what kind of compatibility should be > > > > maintained with respect to GIC. You can't have an ACPI system that's > > > > SBSA compliant and not using GIC. > > > > > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > > > is defined. Why do we have to reach out to the interrupt controller? > > > > > > Patch below (horrible but no solution will be shiny either). > > > > > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > > > > > [...] > > > > > > > If you look at my email above, I tried getting rid of PCI Link object > > > > and I couldn't. I sticked to only thing that works. > > > > > > That's what I object to. If the ACPI bindings do not work for ARM > > > we do not work around issues, we upgrade the specs because what may work > > > for you has to work on all ARM platforms (and all FW developers have > > > to be aware of that). Granted, this is a tiny snag, but the point is > > > that no one knows what's the correct way of describing PCI legacy IRQs > > > on ARM and we need that rectified. > > > > > > This does the trick for me (I can turn it into a function/look-up > > > that returns the polarity), I am sure it will ruffle feathers but > > > we have to find a solution so here it is (that acpi_irq_model gem > > > is already used in generic code drivers/acpi/pci_link.c ;-)) > > > > > > -- >8 -- > > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > > > index 2c45dd3..c9b8c85 100644 > > > --- a/drivers/acpi/pci_irq.c > > > +++ b/drivers/acpi/pci_irq.c > > > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > > > int gsi; > > > u8 pin; > > > int triggering = ACPI_LEVEL_SENSITIVE; > > > - int polarity = ACPI_ACTIVE_LOW; > > > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > > > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > > > char *link = NULL; > > > char link_desc[16]; > > > int rc; > > > > This still seems weird to me. > > > > If I understand correctly, this GIC has several inputs, all active > > high. Some of those inputs are connected to inverters and then to PCI > > INTx wires. > > > > A generic device driver knows about the hardware it drives, including > > the properties of its interrupt wires. PCI drivers and the ACPI/PCI > > core know that conventional PCI device INTx wires are active low. > > These drivers, being generic, do not know about the GIC inverters. > > > > The patch above basically says "if ACPI tells us about a PCI interrupt > > connected to a GIC, *assume* there is an inverter on the input." But > > there's no actual description of that inverter anywhere in ACPI or a > > device tree. Shouldn't that be made explicit somewhere? > > It is explicit for all IRQs other than PCI legacy IRQs ;-), that's > the message I wanted to get across and I failed so far. > > For "normal" IRQs we can use Extended Interrupt Descriptors, that allow > us to describe polarity. For PCI legacy IRQs we could use extended > interrupt descriptors if we were allowed to use PCI interrupt links, > except that, according to current ACPI specs, PCI interrupt links can > only be used for *configurable* interrupt pins. > > So, some ARM vendors stuck to the static/hardwired configuration > in the _PRT, that does not give us a chance to describe polarity. > > I think that we should be allowed to use interrupt links, but that would > not comply with the specs (and on top of that there is FW that is > already shipped in ACPI tables that we can't change anymore). > > > If we connect a non-PCI device to a GIC, we need to know whether > > there's an inverter. How could we figure that out? > > Through an Extended Interrupt Descriptor. How are we solving this ? OK, I think I'm convinced. What in the spec says you can't use PCI Interrupt Links for this case? I see the example in ACPI 5.0 sec 6.2.12 that only shows them changing interrupt numbers with Interrupt descriptors. Is there something that prohibits Extended Interrupt descriptors for PNP0C0F devices? Is there something in the code that doesn't handle that? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-31 16:05 ` Bjorn Helgaas @ 2016-08-31 16:37 ` Lorenzo Pieralisi 2016-08-31 23:08 ` Rafael J. Wysocki 0 siblings, 1 reply; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-31 16:37 UTC (permalink / raw) To: Bjorn Helgaas Cc: Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Wed, Aug 31, 2016 at 11:05:10AM -0500, Bjorn Helgaas wrote: > On Wed, Aug 31, 2016 at 02:34:54PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Aug 31, 2016 at 08:05:06AM -0500, Bjorn Helgaas wrote: > > > On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > > > > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > > > > > > > >> Let me throw option d here. > > > > > >> > > > > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > > > > >> can't we override this in an arch specific way (arm64's pci.c) while > > > > > >> creating the root bridge? > > > > > > > > > > > > On what basis ? You were not copied in from the beginning, but that > > > > > > is not different from Duc's initial proposal, which Marc discarded > > > > > > because it should not be done at arch level, it depends on the interrupt > > > > > > controller. > > > > > > > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > > > > Duc's initial proposal. > > > > > > > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > > > > without a GIC interrupt controller. > > > > > > > > > > The SBSA spec already mentions what kind of compatibility should be > > > > > maintained with respect to GIC. You can't have an ACPI system that's > > > > > SBSA compliant and not using GIC. > > > > > > > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > > > > is defined. Why do we have to reach out to the interrupt controller? > > > > > > > > Patch below (horrible but no solution will be shiny either). > > > > > > > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > > > > > > > [...] > > > > > > > > > If you look at my email above, I tried getting rid of PCI Link object > > > > > and I couldn't. I sticked to only thing that works. > > > > > > > > That's what I object to. If the ACPI bindings do not work for ARM > > > > we do not work around issues, we upgrade the specs because what may work > > > > for you has to work on all ARM platforms (and all FW developers have > > > > to be aware of that). Granted, this is a tiny snag, but the point is > > > > that no one knows what's the correct way of describing PCI legacy IRQs > > > > on ARM and we need that rectified. > > > > > > > > This does the trick for me (I can turn it into a function/look-up > > > > that returns the polarity), I am sure it will ruffle feathers but > > > > we have to find a solution so here it is (that acpi_irq_model gem > > > > is already used in generic code drivers/acpi/pci_link.c ;-)) > > > > > > > > -- >8 -- > > > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > > > > index 2c45dd3..c9b8c85 100644 > > > > --- a/drivers/acpi/pci_irq.c > > > > +++ b/drivers/acpi/pci_irq.c > > > > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > > > > int gsi; > > > > u8 pin; > > > > int triggering = ACPI_LEVEL_SENSITIVE; > > > > - int polarity = ACPI_ACTIVE_LOW; > > > > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > > > > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > > > > char *link = NULL; > > > > char link_desc[16]; > > > > int rc; > > > > > > This still seems weird to me. > > > > > > If I understand correctly, this GIC has several inputs, all active > > > high. Some of those inputs are connected to inverters and then to PCI > > > INTx wires. > > > > > > A generic device driver knows about the hardware it drives, including > > > the properties of its interrupt wires. PCI drivers and the ACPI/PCI > > > core know that conventional PCI device INTx wires are active low. > > > These drivers, being generic, do not know about the GIC inverters. > > > > > > The patch above basically says "if ACPI tells us about a PCI interrupt > > > connected to a GIC, *assume* there is an inverter on the input." But > > > there's no actual description of that inverter anywhere in ACPI or a > > > device tree. Shouldn't that be made explicit somewhere? > > > > It is explicit for all IRQs other than PCI legacy IRQs ;-), that's > > the message I wanted to get across and I failed so far. > > > > For "normal" IRQs we can use Extended Interrupt Descriptors, that allow > > us to describe polarity. For PCI legacy IRQs we could use extended > > interrupt descriptors if we were allowed to use PCI interrupt links, > > except that, according to current ACPI specs, PCI interrupt links can > > only be used for *configurable* interrupt pins. > > > > So, some ARM vendors stuck to the static/hardwired configuration > > in the _PRT, that does not give us a chance to describe polarity. > > > > I think that we should be allowed to use interrupt links, but that would > > not comply with the specs (and on top of that there is FW that is > > already shipped in ACPI tables that we can't change anymore). > > > > > If we connect a non-PCI device to a GIC, we need to know whether > > > there's an inverter. How could we figure that out? > > > > Through an Extended Interrupt Descriptor. How are we solving this ? > > OK, I think I'm convinced. What in the spec says you can't use PCI > Interrupt Links for this case? I see the example in ACPI 5.0 sec > 6.2.12 that only shows them changing interrupt numbers with Interrupt > descriptors. Is there something that prohibits Extended Interrupt > descriptors for PNP0C0F devices? Is there something in the code that > doesn't handle that? ACPI 6.1 (6.2.13, page 335), that describes the two ways _PRT can be used. The problem is not about whether we can use Extended Interrupt Descriptors for PNP0C0F, we *can* use them in their (CRS,SRS,PRS), the problem is that by our specs reading, PNP0C0F PCI interrupt link devices can *only* describe interrupt pins that are configurable; the GIC pins are not (ie every PCI INTx is connected to a specific GIC pin and can't be routed dynamically - it has an inverter in the path though ;-)). I suspect that's because a) PNP0C0F devices can have a _PRS (that can be empty) b) that's how IRQ chips work on x86. What (some) ARM vendors did, given the above, is hardcode the _PRT source field to 0 and add the GSI in the source index (as per specs); that does not work because it implies level low, and this thread is the outcome. So, there are two niggles to sort out: 1) Kernel code handles PCI interrupt links and we do not need anything on top of that; we do need to update the specs to allow ARM platforms to use them though 2) We still need a point hack (as the one I inlined) to cater for platforms that in current FW do NOT describe legacy IRQs with PCI interrupt link devices, is the patch I inlined ok ? Thanks ! Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-31 16:37 ` Lorenzo Pieralisi @ 2016-08-31 23:08 ` Rafael J. Wysocki 2016-09-02 11:09 ` Lorenzo Pieralisi 0 siblings, 1 reply; 37+ messages in thread From: Rafael J. Wysocki @ 2016-08-31 23:08 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Wednesday, August 31, 2016 05:37:47 PM Lorenzo Pieralisi wrote: > On Wed, Aug 31, 2016 at 11:05:10AM -0500, Bjorn Helgaas wrote: > > On Wed, Aug 31, 2016 at 02:34:54PM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Aug 31, 2016 at 08:05:06AM -0500, Bjorn Helgaas wrote: > > > > On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > > > > > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > > > > > > > > > >> Let me throw option d here. > > > > > > >> > > > > > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > > > > > >> can't we override this in an arch specific way (arm64's pci.c) while > > > > > > >> creating the root bridge? > > > > > > > > > > > > > > On what basis ? You were not copied in from the beginning, but that > > > > > > > is not different from Duc's initial proposal, which Marc discarded > > > > > > > because it should not be done at arch level, it depends on the interrupt > > > > > > > controller. > > > > > > > > > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > > > > > Duc's initial proposal. > > > > > > > > > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > > > > > without a GIC interrupt controller. > > > > > > > > > > > > The SBSA spec already mentions what kind of compatibility should be > > > > > > maintained with respect to GIC. You can't have an ACPI system that's > > > > > > SBSA compliant and not using GIC. > > > > > > > > > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > > > > > is defined. Why do we have to reach out to the interrupt controller? > > > > > > > > > > Patch below (horrible but no solution will be shiny either). > > > > > > > > > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > > > > > > > > > [...] > > > > > > > > > > > If you look at my email above, I tried getting rid of PCI Link object > > > > > > and I couldn't. I sticked to only thing that works. > > > > > > > > > > That's what I object to. If the ACPI bindings do not work for ARM > > > > > we do not work around issues, we upgrade the specs because what may work > > > > > for you has to work on all ARM platforms (and all FW developers have > > > > > to be aware of that). Granted, this is a tiny snag, but the point is > > > > > that no one knows what's the correct way of describing PCI legacy IRQs > > > > > on ARM and we need that rectified. > > > > > > > > > > This does the trick for me (I can turn it into a function/look-up > > > > > that returns the polarity), I am sure it will ruffle feathers but > > > > > we have to find a solution so here it is (that acpi_irq_model gem > > > > > is already used in generic code drivers/acpi/pci_link.c ;-)) > > > > > > > > > > -- >8 -- > > > > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > > > > > index 2c45dd3..c9b8c85 100644 > > > > > --- a/drivers/acpi/pci_irq.c > > > > > +++ b/drivers/acpi/pci_irq.c > > > > > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > > > > > int gsi; > > > > > u8 pin; > > > > > int triggering = ACPI_LEVEL_SENSITIVE; > > > > > - int polarity = ACPI_ACTIVE_LOW; > > > > > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > > > > > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > > > > > char *link = NULL; > > > > > char link_desc[16]; > > > > > int rc; > > > > > > > > This still seems weird to me. > > > > > > > > If I understand correctly, this GIC has several inputs, all active > > > > high. Some of those inputs are connected to inverters and then to PCI > > > > INTx wires. > > > > > > > > A generic device driver knows about the hardware it drives, including > > > > the properties of its interrupt wires. PCI drivers and the ACPI/PCI > > > > core know that conventional PCI device INTx wires are active low. > > > > These drivers, being generic, do not know about the GIC inverters. > > > > > > > > The patch above basically says "if ACPI tells us about a PCI interrupt > > > > connected to a GIC, *assume* there is an inverter on the input." But > > > > there's no actual description of that inverter anywhere in ACPI or a > > > > device tree. Shouldn't that be made explicit somewhere? > > > > > > It is explicit for all IRQs other than PCI legacy IRQs ;-), that's > > > the message I wanted to get across and I failed so far. > > > > > > For "normal" IRQs we can use Extended Interrupt Descriptors, that allow > > > us to describe polarity. For PCI legacy IRQs we could use extended > > > interrupt descriptors if we were allowed to use PCI interrupt links, > > > except that, according to current ACPI specs, PCI interrupt links can > > > only be used for *configurable* interrupt pins. > > > > > > So, some ARM vendors stuck to the static/hardwired configuration > > > in the _PRT, that does not give us a chance to describe polarity. > > > > > > I think that we should be allowed to use interrupt links, but that would > > > not comply with the specs (and on top of that there is FW that is > > > already shipped in ACPI tables that we can't change anymore). > > > > > > > If we connect a non-PCI device to a GIC, we need to know whether > > > > there's an inverter. How could we figure that out? > > > > > > Through an Extended Interrupt Descriptor. How are we solving this ? > > > > OK, I think I'm convinced. What in the spec says you can't use PCI > > Interrupt Links for this case? I see the example in ACPI 5.0 sec > > 6.2.12 that only shows them changing interrupt numbers with Interrupt > > descriptors. Is there something that prohibits Extended Interrupt > > descriptors for PNP0C0F devices? Is there something in the code that > > doesn't handle that? > > ACPI 6.1 (6.2.13, page 335), that describes the two ways _PRT can be used. > > The problem is not about whether we can use Extended Interrupt > Descriptors for PNP0C0F, we *can* use them in their (CRS,SRS,PRS), the > problem is that by our specs reading, PNP0C0F PCI interrupt link devices > can *only* describe interrupt pins that are configurable; That would be the case if _PRS/_SRS/_DIS were required for those objects. It doesn't seem to be the case, though. Thanks, Rafael ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-31 23:08 ` Rafael J. Wysocki @ 2016-09-02 11:09 ` Lorenzo Pieralisi 2016-09-02 21:28 ` Rafael J. Wysocki 0 siblings, 1 reply; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-09-02 11:09 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Thu, Sep 01, 2016 at 01:08:19AM +0200, Rafael J. Wysocki wrote: > On Wednesday, August 31, 2016 05:37:47 PM Lorenzo Pieralisi wrote: > > On Wed, Aug 31, 2016 at 11:05:10AM -0500, Bjorn Helgaas wrote: > > > On Wed, Aug 31, 2016 at 02:34:54PM +0100, Lorenzo Pieralisi wrote: > > > > On Wed, Aug 31, 2016 at 08:05:06AM -0500, Bjorn Helgaas wrote: > > > > > On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > > > > > > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > > > > > > > > > > > >> Let me throw option d here. > > > > > > > >> > > > > > > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > > > > > > >> can't we override this in an arch specific way (arm64's pci.c) while > > > > > > > >> creating the root bridge? > > > > > > > > > > > > > > > > On what basis ? You were not copied in from the beginning, but that > > > > > > > > is not different from Duc's initial proposal, which Marc discarded > > > > > > > > because it should not be done at arch level, it depends on the interrupt > > > > > > > > controller. > > > > > > > > > > > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > > > > > > Duc's initial proposal. > > > > > > > > > > > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > > > > > > without a GIC interrupt controller. > > > > > > > > > > > > > > The SBSA spec already mentions what kind of compatibility should be > > > > > > > maintained with respect to GIC. You can't have an ACPI system that's > > > > > > > SBSA compliant and not using GIC. > > > > > > > > > > > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > > > > > > is defined. Why do we have to reach out to the interrupt controller? > > > > > > > > > > > > Patch below (horrible but no solution will be shiny either). > > > > > > > > > > > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > > > > > > > > > > > [...] > > > > > > > > > > > > > If you look at my email above, I tried getting rid of PCI Link object > > > > > > > and I couldn't. I sticked to only thing that works. > > > > > > > > > > > > That's what I object to. If the ACPI bindings do not work for ARM > > > > > > we do not work around issues, we upgrade the specs because what may work > > > > > > for you has to work on all ARM platforms (and all FW developers have > > > > > > to be aware of that). Granted, this is a tiny snag, but the point is > > > > > > that no one knows what's the correct way of describing PCI legacy IRQs > > > > > > on ARM and we need that rectified. > > > > > > > > > > > > This does the trick for me (I can turn it into a function/look-up > > > > > > that returns the polarity), I am sure it will ruffle feathers but > > > > > > we have to find a solution so here it is (that acpi_irq_model gem > > > > > > is already used in generic code drivers/acpi/pci_link.c ;-)) > > > > > > > > > > > > -- >8 -- > > > > > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > > > > > > index 2c45dd3..c9b8c85 100644 > > > > > > --- a/drivers/acpi/pci_irq.c > > > > > > +++ b/drivers/acpi/pci_irq.c > > > > > > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > > > > > > int gsi; > > > > > > u8 pin; > > > > > > int triggering = ACPI_LEVEL_SENSITIVE; > > > > > > - int polarity = ACPI_ACTIVE_LOW; > > > > > > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > > > > > > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > > > > > > char *link = NULL; > > > > > > char link_desc[16]; > > > > > > int rc; > > > > > > > > > > This still seems weird to me. > > > > > > > > > > If I understand correctly, this GIC has several inputs, all active > > > > > high. Some of those inputs are connected to inverters and then to PCI > > > > > INTx wires. > > > > > > > > > > A generic device driver knows about the hardware it drives, including > > > > > the properties of its interrupt wires. PCI drivers and the ACPI/PCI > > > > > core know that conventional PCI device INTx wires are active low. > > > > > These drivers, being generic, do not know about the GIC inverters. > > > > > > > > > > The patch above basically says "if ACPI tells us about a PCI interrupt > > > > > connected to a GIC, *assume* there is an inverter on the input." But > > > > > there's no actual description of that inverter anywhere in ACPI or a > > > > > device tree. Shouldn't that be made explicit somewhere? > > > > > > > > It is explicit for all IRQs other than PCI legacy IRQs ;-), that's > > > > the message I wanted to get across and I failed so far. > > > > > > > > For "normal" IRQs we can use Extended Interrupt Descriptors, that allow > > > > us to describe polarity. For PCI legacy IRQs we could use extended > > > > interrupt descriptors if we were allowed to use PCI interrupt links, > > > > except that, according to current ACPI specs, PCI interrupt links can > > > > only be used for *configurable* interrupt pins. > > > > > > > > So, some ARM vendors stuck to the static/hardwired configuration > > > > in the _PRT, that does not give us a chance to describe polarity. > > > > > > > > I think that we should be allowed to use interrupt links, but that would > > > > not comply with the specs (and on top of that there is FW that is > > > > already shipped in ACPI tables that we can't change anymore). > > > > > > > > > If we connect a non-PCI device to a GIC, we need to know whether > > > > > there's an inverter. How could we figure that out? > > > > > > > > Through an Extended Interrupt Descriptor. How are we solving this ? > > > > > > OK, I think I'm convinced. What in the spec says you can't use PCI > > > Interrupt Links for this case? I see the example in ACPI 5.0 sec > > > 6.2.12 that only shows them changing interrupt numbers with Interrupt > > > descriptors. Is there something that prohibits Extended Interrupt > > > descriptors for PNP0C0F devices? Is there something in the code that > > > doesn't handle that? > > > > ACPI 6.1 (6.2.13, page 335), that describes the two ways _PRT can be used. > > > > The problem is not about whether we can use Extended Interrupt > > Descriptors for PNP0C0F, we *can* use them in their (CRS,SRS,PRS), the > > problem is that by our specs reading, PNP0C0F PCI interrupt link devices > > can *only* describe interrupt pins that are configurable; > > That would be the case if _PRS/_SRS/_DIS were required for those objects. > It doesn't seem to be the case, though. The kernel currently does not cope with eg missing _SRS on PCI interrupt link devices (and I suspect that's the case for other OSs), therefore it would only work on ARM64 with an empty _SRS and hardcoded _PRS. We should reword the specs, since this has created/is creating confusion (which resulted in buggy ACPI tables - ie this thread) and unfortunately we still need my hack to make sure static legacy IRQ configurations work on ARM64 systems, I would avoid it but there is not much we can do. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-09-02 11:09 ` Lorenzo Pieralisi @ 2016-09-02 21:28 ` Rafael J. Wysocki 0 siblings, 0 replies; 37+ messages in thread From: Rafael J. Wysocki @ 2016-09-02 21:28 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Bjorn Helgaas, Sinan Kaya, Marc Zyngier, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas, Punit Agrawal On Friday, September 02, 2016 12:09:13 PM Lorenzo Pieralisi wrote: > On Thu, Sep 01, 2016 at 01:08:19AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, August 31, 2016 05:37:47 PM Lorenzo Pieralisi wrote: > > > On Wed, Aug 31, 2016 at 11:05:10AM -0500, Bjorn Helgaas wrote: > > > > On Wed, Aug 31, 2016 at 02:34:54PM +0100, Lorenzo Pieralisi wrote: > > > > > On Wed, Aug 31, 2016 at 08:05:06AM -0500, Bjorn Helgaas wrote: > > > > > > On Tue, Aug 30, 2016 at 11:08:34AM +0100, Lorenzo Pieralisi wrote: > > > > > > > On Fri, Aug 26, 2016 at 06:53:29PM -0400, Sinan Kaya wrote: > > > > > > > > > > > > > > > > >> Let me throw option d here. > > > > > > > > >> > > > > > > > > >> I know Bjorn wants to keep ACTIVE_LOW in the code for common code but > > > > > > > > >> can't we override this in an arch specific way (arm64's pci.c) while > > > > > > > > >> creating the root bridge? > > > > > > > > > > > > > > > > > > On what basis ? You were not copied in from the beginning, but that > > > > > > > > > is not different from Duc's initial proposal, which Marc discarded > > > > > > > > > because it should not be done at arch level, it depends on the interrupt > > > > > > > > > controller. > > > > > > > > > > > > > > > > I happen to watch the linux-pci and linux-acpi mail-lists. I also saw > > > > > > > > Duc's initial proposal. > > > > > > > > > > > > > > > > I can't imagine someone building an ACPI compliant ARM64 platform > > > > > > > > without a GIC interrupt controller. > > > > > > > > > > > > > > > > The SBSA spec already mentions what kind of compatibility should be > > > > > > > > maintained with respect to GIC. You can't have an ACPI system that's > > > > > > > > SBSA compliant and not using GIC. > > > > > > > > > > > > > > > > Can't we just hard code this to ACTIVE_HIGH in arch directory if ACPI > > > > > > > > is defined. Why do we have to reach out to the interrupt controller? > > > > > > > > > > > > > > Patch below (horrible but no solution will be shiny either). > > > > > > > > > > > > > > > https://lists.linaro.org/pipermail/linaro-acpi/2015-November/005973.html > > > > > > > > > > > > > > [...] > > > > > > > > > > > > > > > If you look at my email above, I tried getting rid of PCI Link object > > > > > > > > and I couldn't. I sticked to only thing that works. > > > > > > > > > > > > > > That's what I object to. If the ACPI bindings do not work for ARM > > > > > > > we do not work around issues, we upgrade the specs because what may work > > > > > > > for you has to work on all ARM platforms (and all FW developers have > > > > > > > to be aware of that). Granted, this is a tiny snag, but the point is > > > > > > > that no one knows what's the correct way of describing PCI legacy IRQs > > > > > > > on ARM and we need that rectified. > > > > > > > > > > > > > > This does the trick for me (I can turn it into a function/look-up > > > > > > > that returns the polarity), I am sure it will ruffle feathers but > > > > > > > we have to find a solution so here it is (that acpi_irq_model gem > > > > > > > is already used in generic code drivers/acpi/pci_link.c ;-)) > > > > > > > > > > > > > > -- >8 -- > > > > > > > diff --git a/drivers/acpi/pci_irq.c b/drivers/acpi/pci_irq.c > > > > > > > index 2c45dd3..c9b8c85 100644 > > > > > > > --- a/drivers/acpi/pci_irq.c > > > > > > > +++ b/drivers/acpi/pci_irq.c > > > > > > > @@ -411,7 +411,8 @@ int acpi_pci_irq_enable(struct pci_dev *dev) > > > > > > > int gsi; > > > > > > > u8 pin; > > > > > > > int triggering = ACPI_LEVEL_SENSITIVE; > > > > > > > - int polarity = ACPI_ACTIVE_LOW; > > > > > > > + int polarity = acpi_irq_model == ACPI_IRQ_MODEL_GIC ? > > > > > > > + ACPI_ACTIVE_HIGH : ACPI_ACTIVE_LOW; > > > > > > > char *link = NULL; > > > > > > > char link_desc[16]; > > > > > > > int rc; > > > > > > > > > > > > This still seems weird to me. > > > > > > > > > > > > If I understand correctly, this GIC has several inputs, all active > > > > > > high. Some of those inputs are connected to inverters and then to PCI > > > > > > INTx wires. > > > > > > > > > > > > A generic device driver knows about the hardware it drives, including > > > > > > the properties of its interrupt wires. PCI drivers and the ACPI/PCI > > > > > > core know that conventional PCI device INTx wires are active low. > > > > > > These drivers, being generic, do not know about the GIC inverters. > > > > > > > > > > > > The patch above basically says "if ACPI tells us about a PCI interrupt > > > > > > connected to a GIC, *assume* there is an inverter on the input." But > > > > > > there's no actual description of that inverter anywhere in ACPI or a > > > > > > device tree. Shouldn't that be made explicit somewhere? > > > > > > > > > > It is explicit for all IRQs other than PCI legacy IRQs ;-), that's > > > > > the message I wanted to get across and I failed so far. > > > > > > > > > > For "normal" IRQs we can use Extended Interrupt Descriptors, that allow > > > > > us to describe polarity. For PCI legacy IRQs we could use extended > > > > > interrupt descriptors if we were allowed to use PCI interrupt links, > > > > > except that, according to current ACPI specs, PCI interrupt links can > > > > > only be used for *configurable* interrupt pins. > > > > > > > > > > So, some ARM vendors stuck to the static/hardwired configuration > > > > > in the _PRT, that does not give us a chance to describe polarity. > > > > > > > > > > I think that we should be allowed to use interrupt links, but that would > > > > > not comply with the specs (and on top of that there is FW that is > > > > > already shipped in ACPI tables that we can't change anymore). > > > > > > > > > > > If we connect a non-PCI device to a GIC, we need to know whether > > > > > > there's an inverter. How could we figure that out? > > > > > > > > > > Through an Extended Interrupt Descriptor. How are we solving this ? > > > > > > > > OK, I think I'm convinced. What in the spec says you can't use PCI > > > > Interrupt Links for this case? I see the example in ACPI 5.0 sec > > > > 6.2.12 that only shows them changing interrupt numbers with Interrupt > > > > descriptors. Is there something that prohibits Extended Interrupt > > > > descriptors for PNP0C0F devices? Is there something in the code that > > > > doesn't handle that? > > > > > > ACPI 6.1 (6.2.13, page 335), that describes the two ways _PRT can be used. > > > > > > The problem is not about whether we can use Extended Interrupt > > > Descriptors for PNP0C0F, we *can* use them in their (CRS,SRS,PRS), the > > > problem is that by our specs reading, PNP0C0F PCI interrupt link devices > > > can *only* describe interrupt pins that are configurable; > > > > That would be the case if _PRS/_SRS/_DIS were required for those objects. > > It doesn't seem to be the case, though. > > The kernel currently does not cope with eg missing _SRS on PCI interrupt > link devices (and I suspect that's the case for other OSs), therefore it > would only work on ARM64 with an empty _SRS and hardcoded _PRS. We should > reword the specs, since this has created/is creating confusion (which > resulted in buggy ACPI tables - ie this thread) and unfortunately we still > need my hack to make sure static legacy IRQ configurations work on > ARM64 systems, I would avoid it but there is not much we can do. Fair enough. Thanks, Rafael ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 20:30 ` Marc Zyngier @ 2016-08-25 10:04 ` Punit Agrawal -1 siblings, 0 replies; 37+ messages in thread From: Punit Agrawal @ 2016-08-25 10:04 UTC (permalink / raw) To: Marc Zyngier Cc: Bjorn Helgaas, Lorenzo Pieralisi, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, bhelgaas Marc Zyngier <marc.zyngier@arm.com> writes: > On Wed, 24 Aug 2016 14:30:00 -0500 > Bjorn Helgaas <helgaas@kernel.org> wrote: > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: >> > [ +Bjorn, Punit] >> > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: >> > > [Resend in plain text mode] >> > > >> > > Hi Lorenzo, Rafael, >> > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and >> > > trigger mode in _PRT when the interrupts are static (hardwired to >> > > specific interrupt inputs in interrupt controller). In current >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, >> > > GICv3 controllers and will cause failures in PCIe AER, PME services >> > > (on X-Gene platforms). >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional >> and defined as 'level sensitive,' asserted low." >> >> I've heard before that ARM64 does this differently, but I still don't >> understand the difference. Obviously if you plug a legacy PCI card >> into an ARM64 system, it's still going to pull INTA# low to assert an >> interrupt. So is there something special about ARM64 that inverts >> that, or what? > > There is certainly an inverter somewhere on the interrupt path, because > the GIC triggers on level high, not level low. But I don't think that's > the issue Duc is trying to outline here, because that's not something > SW can fix. I'm worried that in his system, the interrupt is edge > triggered instead. > >> >> > > Is there any way to specify polarity and trigger mode for static >> > > interrupts in _PRT? >> >> There is no way I'm aware of in _PRT to specify polarity and trigger >> mode. I don't know the history, but my guess is that it would be seen >> as superfluous given that the PCI spec requires level, active low. >> >> Obviously I'm missing something important. > > Same here, unless the HW is not PCI compliant... I had faced this issue on Juno r2[0] a few months back - though AFAICS, it wasn't preventing anything to work but printed an annoying message on boot. [ 1.353696] genirq: Setting trigger mode 8 for irq 9 failed (gic_set_type+0x0/0x5c) [ 1.478286] genirq: Setting trigger mode 8 for irq 17 failed (gic_set_type+0x0/0x5c) The ACPI code in the kernel (drivers/acpi/pci_irq.c) is behaving as per spec, so nothing to be done there IMHO. The problem arises due to the integration of two mismatched components - PCI is level low and the GIC supports only level high - making it necessary to introduce glue elements like the inverter. This would all be OK if ACPI had a mechanism to specify the interrupt type (trigger and polarity). As an alternative, for Juno I created a link device (as Lorenzo suggests) to provide this information to the kernel. With this fix the warnings went away and I suspect this will address Duc's issue as well. But that is playing naughty with the spec (ACPI 6.1 Section. 6.2.13). If there are no good reason to restrict using link devices to configurable interrupts, perhaps the spec needs an update. Perhaps Rafael knows why is there a restriction on using link devices for fixed interrupts in the ACPI spec... [0] https://lists.linaro.org/pipermail/linaro-acpi/2016-May/006880.html > > Thanks, > > M. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-25 10:04 ` Punit Agrawal 0 siblings, 0 replies; 37+ messages in thread From: Punit Agrawal @ 2016-08-25 10:04 UTC (permalink / raw) To: Marc Zyngier Cc: Bjorn Helgaas, Lorenzo Pieralisi, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, bhelgaas Marc Zyngier <marc.zyngier@arm.com> writes: > On Wed, 24 Aug 2016 14:30:00 -0500 > Bjorn Helgaas <helgaas@kernel.org> wrote: > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: >> > [ +Bjorn, Punit] >> > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: >> > > [Resend in plain text mode] >> > > >> > > Hi Lorenzo, Rafael, >> > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and >> > > trigger mode in _PRT when the interrupts are static (hardwired to >> > > specific interrupt inputs in interrupt controller). In current >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, >> > > GICv3 controllers and will cause failures in PCIe AER, PME services >> > > (on X-Gene platforms). >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional >> and defined as 'level sensitive,' asserted low." >> >> I've heard before that ARM64 does this differently, but I still don't >> understand the difference. Obviously if you plug a legacy PCI card >> into an ARM64 system, it's still going to pull INTA# low to assert an >> interrupt. So is there something special about ARM64 that inverts >> that, or what? > > There is certainly an inverter somewhere on the interrupt path, because > the GIC triggers on level high, not level low. But I don't think that's > the issue Duc is trying to outline here, because that's not something > SW can fix. I'm worried that in his system, the interrupt is edge > triggered instead. > >> >> > > Is there any way to specify polarity and trigger mode for static >> > > interrupts in _PRT? >> >> There is no way I'm aware of in _PRT to specify polarity and trigger >> mode. I don't know the history, but my guess is that it would be seen >> as superfluous given that the PCI spec requires level, active low. >> >> Obviously I'm missing something important. > > Same here, unless the HW is not PCI compliant... I had faced this issue on Juno r2[0] a few months back - though AFAICS, it wasn't preventing anything to work but printed an annoying message on boot. [ 1.353696] genirq: Setting trigger mode 8 for irq 9 failed (gic_set_type+0x0/0x5c) [ 1.478286] genirq: Setting trigger mode 8 for irq 17 failed (gic_set_type+0x0/0x5c) The ACPI code in the kernel (drivers/acpi/pci_irq.c) is behaving as per spec, so nothing to be done there IMHO. The problem arises due to the integration of two mismatched components - PCI is level low and the GIC supports only level high - making it necessary to introduce glue elements like the inverter. This would all be OK if ACPI had a mechanism to specify the interrupt type (trigger and polarity). As an alternative, for Juno I created a link device (as Lorenzo suggests) to provide this information to the kernel. With this fix the warnings went away and I suspect this will address Duc's issue as well. But that is playing naughty with the spec (ACPI 6.1 Section. 6.2.13). If there are no good reason to restrict using link devices to configurable interrupts, perhaps the spec needs an update. Perhaps Rafael knows why is there a restriction on using link devices for fixed interrupts in the ACPI spec... [0] https://lists.linaro.org/pipermail/linaro-acpi/2016-May/006880.html > > Thanks, > > M. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-25 10:04 ` Punit Agrawal (?) @ 2016-08-25 11:14 ` Lorenzo Pieralisi 2016-08-25 16:46 ` Duc Dang -1 siblings, 1 reply; 37+ messages in thread From: Lorenzo Pieralisi @ 2016-08-25 11:14 UTC (permalink / raw) To: Punit Agrawal Cc: Marc Zyngier, Bjorn Helgaas, Duc Dang, Rafael Wysocki, linux-pci, linux-acpi, patches, bhelgaas On Thu, Aug 25, 2016 at 11:04:02AM +0100, Punit Agrawal wrote: > Marc Zyngier <marc.zyngier@arm.com> writes: > > > On Wed, 24 Aug 2016 14:30:00 -0500 > > Bjorn Helgaas <helgaas@kernel.org> wrote: > > > >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: > >> > [ +Bjorn, Punit] > >> > > >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: > >> > > [Resend in plain text mode] > >> > > > >> > > Hi Lorenzo, Rafael, > >> > > > >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and > >> > > trigger mode in _PRT when the interrupts are static (hardwired to > >> > > specific interrupt inputs in interrupt controller). In current > >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > >> > > GICv3 controllers and will cause failures in PCIe AER, PME services > >> > > (on X-Gene platforms). > >> > >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional > >> and defined as 'level sensitive,' asserted low." > >> > >> I've heard before that ARM64 does this differently, but I still don't > >> understand the difference. Obviously if you plug a legacy PCI card > >> into an ARM64 system, it's still going to pull INTA# low to assert an > >> interrupt. So is there something special about ARM64 that inverts > >> that, or what? > > > > There is certainly an inverter somewhere on the interrupt path, because > > the GIC triggers on level high, not level low. But I don't think that's > > the issue Duc is trying to outline here, because that's not something > > SW can fix. I'm worried that in his system, the interrupt is edge > > triggered instead. It would be nice if Duc reported the "failures" he is noticing, instead of us having to guess them. > >> > > Is there any way to specify polarity and trigger mode for static > >> > > interrupts in _PRT? > >> > >> There is no way I'm aware of in _PRT to specify polarity and trigger > >> mode. I don't know the history, but my guess is that it would be seen > >> as superfluous given that the PCI spec requires level, active low. > >> > >> Obviously I'm missing something important. > > > > Same here, unless the HW is not PCI compliant... > > I had faced this issue on Juno r2[0] a few months back - though AFAICS, > it wasn't preventing anything to work but printed an annoying message on > boot. > > [ 1.353696] genirq: Setting trigger mode 8 for irq 9 failed (gic_set_type+0x0/0x5c) > [ 1.478286] genirq: Setting trigger mode 8 for irq 17 failed (gic_set_type+0x0/0x5c) This is not just annoying, that's the kernel failing to set-up legacy IRQs IIUC and unless we have a way to specify interrupt polarity that's going to happen on all ARM64 platforms booting with ACPI having a GIC interrupt controller. I suspect most of ARM partners are already using interrupt links to work around this and Duc was the first one reporting the issue with the ACPI specs (ie interrupt links should just be used for configurable interrupt pins - which, I will say it again - it is likely to be a consequence of how x86 and their APIC works). > The ACPI code in the kernel (drivers/acpi/pci_irq.c) is behaving as per > spec, so nothing to be done there IMHO. The problem arises due to the > integration of two mismatched components - PCI is level low and the GIC > supports only level high - making it necessary to introduce glue > elements like the inverter. That glue can well be the interrupt link. I reiterate my point: I will start dumping x86 ACPI tables and peruse the _PRT on x86 systems, but in principle even on x86 the legacy PCI interrupt can be re-routed and their polarity specified through interrupt links. It is not a given that the legacy IRQs are active low at interrupt controller level even on x86, at least that's not ruled out by ACPI specs. Am I wrong ? The inverter you and Marc mentioned can be described through an interrupt link defining the interrupt polarity. On x86 there is probably an IRQ router sitting downstream the ACPI that allows to a) configure the IOAPIC pin for the legacy IRQ and b) control the polarity, but as I mentioned I do not have enough x86 knowledge, so I am just guessing. > This would all be OK if ACPI had a mechanism to specify the interrupt > type (trigger and polarity). As an alternative, for Juno I created a > link device (as Lorenzo suggests) to provide this information to the > kernel. > > With this fix the warnings went away and I suspect this will address > Duc's issue as well. But that is playing naughty with the spec (ACPI 6.1 > Section. 6.2.13). ACPI specs were written for x86 boxes, there is nothing naughty in asking for them to be updated and work on other architectures. The only issue I see with interrupt links is that they may allow reprogramming through the _SRS method, which is optional BTW. > If there are no good reason to restrict using link devices to > configurable interrupts, perhaps the spec needs an update. Yes and that's what I am going to ask if nobody complains. > Perhaps Rafael knows why is there a restriction on using link devices > for fixed interrupts in the ACPI spec... See above, I do not know why that restriction is there given that to me an interrupt link is a superset of static values, I do not see why they should be prevented for non-configurable interrupts, I am happy to be corrected if there is something we are missing. Lorenzo ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-25 11:14 ` Lorenzo Pieralisi @ 2016-08-25 16:46 ` Duc Dang 2016-08-25 17:20 ` Bjorn Helgaas 0 siblings, 1 reply; 37+ messages in thread From: Duc Dang @ 2016-08-25 16:46 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Punit Agrawal, Marc Zyngier, Bjorn Helgaas, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas On Thu, Aug 25, 2016 at 4:14 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > On Thu, Aug 25, 2016 at 11:04:02AM +0100, Punit Agrawal wrote: >> Marc Zyngier <marc.zyngier@arm.com> writes: >> >> > On Wed, 24 Aug 2016 14:30:00 -0500 >> > Bjorn Helgaas <helgaas@kernel.org> wrote: >> > >> >> On Wed, Aug 24, 2016 at 03:27:23PM +0100, Lorenzo Pieralisi wrote: >> >> > [ +Bjorn, Punit] >> >> > >> >> > On Wed, Aug 24, 2016 at 04:06:13AM -0700, Duc Dang wrote: >> >> > > [Resend in plain text mode] >> >> > > >> >> > > Hi Lorenzo, Rafael, >> >> > > >> >> > > ACPI 6.1 spec does not specify how to set interrupt polarity and >> >> > > trigger mode in _PRT when the interrupts are static (hardwired to >> >> > > specific interrupt inputs in interrupt controller). In current >> >> > > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by >> >> > > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to >> >> > > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, >> >> > > GICv3 controllers and will cause failures in PCIe AER, PME services >> >> > > (on X-Gene platforms). >> >> >> >> PCI (not PCIe) r3.0, sec 2.2.6, says "Interrupts on PCI are optional >> >> and defined as 'level sensitive,' asserted low." >> >> >> >> I've heard before that ARM64 does this differently, but I still don't >> >> understand the difference. Obviously if you plug a legacy PCI card >> >> into an ARM64 system, it's still going to pull INTA# low to assert an >> >> interrupt. So is there something special about ARM64 that inverts >> >> that, or what? >> > >> > There is certainly an inverter somewhere on the interrupt path, because >> > the GIC triggers on level high, not level low. But I don't think that's >> > the issue Duc is trying to outline here, because that's not something >> > SW can fix. I'm worried that in his system, the interrupt is edge >> > triggered instead. > > It would be nice if Duc reported the "failures" he is noticing, > instead of us having to guess them. I am sorry that I should be more specific. Please see failure information below. > >> >> > > Is there any way to specify polarity and trigger mode for static >> >> > > interrupts in _PRT? >> >> >> >> There is no way I'm aware of in _PRT to specify polarity and trigger >> >> mode. I don't know the history, but my guess is that it would be seen >> >> as superfluous given that the PCI spec requires level, active low. >> >> >> >> Obviously I'm missing something important. >> > >> > Same here, unless the HW is not PCI compliant... >> >> I had faced this issue on Juno r2[0] a few months back - though AFAICS, >> it wasn't preventing anything to work but printed an annoying message on >> boot. >> >> [ 1.353696] genirq: Setting trigger mode 8 for irq 9 failed (gic_set_type+0x0/0x5c) >> [ 1.478286] genirq: Setting trigger mode 8 for irq 17 failed (gic_set_type+0x0/0x5c) I got the same warning: [ 6.576842] genirq: Setting trigger mode 8 for irq 50 failed (gic_set_type+0x0/0x64) > > This is not just annoying, that's the kernel failing to set-up legacy > IRQs IIUC and unless we have a way to specify interrupt polarity that's > going to happen on all ARM64 platforms booting with ACPI having a GIC > interrupt controller. The warning about gic_set_type above also happens with previous kernel, but INTx interrupt handler is still registered successfully, so things that use INTx still work. I dump a trace of PCIe AER service initialization in 4.6 and 4.8-rc1 and the code path leading to gic_set_type is different: in 4.8-rc1, request_irq fails when gic_set_type fails and the handler cannot be registered: 4.6-rc2 code path: [ 6.455289] [<ffff0000083624d0>] gic_set_type+0x50/0x64 [ 6.460943] [<ffff0000080fb984>] __irq_set_trigger+0x58/0x178 [ 6.467176] [<ffff0000080fd0e8>] irq_set_irq_type+0x30/0x5c [ 6.473169] [<ffff000008100dcc>] irq_create_fwspec_mapping+0x194/0x1e4 [ 6.480121] [<ffff0000083d5368>] acpi_register_gsi+0x54/0x5c [ 6.486303] [<ffff0000083d2660>] acpi_pci_irq_enable+0xc8/0x14c [ 6.492771] [<ffff00000809382c>] pcibios_alloc_irq+0x20/0x58 [ 6.498825] [<ffff000008398928>] pci_device_probe+0x28/0x108 [ 6.504844] [<ffff000008481a20>] driver_probe_device+0x200/0x2a0 [ 6.511241] [<ffff000008481b6c>] __driver_attach+0xac/0xb0 [ 6.517138] [<ffff00000847fa90>] bus_for_each_dev+0x58/0x98 [ 6.523138] [<ffff000008481330>] driver_attach+0x20/0x28 [ 6.528792] [<ffff000008480f48>] bus_add_driver+0x1c8/0x22c [ 6.534741] [<ffff000008482424>] driver_register+0x60/0xf4 [ 6.540594] [<ffff0000083977b8>] __pci_register_driver+0x40/0x48 [ 6.546993] [<ffff000008b4fb20>] pcie_portdrv_init+0x84/0xa4 [ 6.553175] [<ffff0000080829d8>] do_one_initcall+0x8c/0x19c [ 6.559176] [<ffff000008b2eb00>] kernel_init_freeable+0x150/0x1f0 [ 6.565748] [<ffff0000087f05c0>] kernel_init+0x10/0xfc [ 6.571213] [<ffff000008085e10>] ret_from_fork+0x10/0x40 [ 6.576842] genirq: Setting trigger mode 8 for irq 50 failed (gic_set_type+0x0/0x64) [ 6.585689] aer 0000:00:00.0:pcie02: service driver aer loaded 4.8-rc1 code path: [ 5.829216] [<ffff000008384fb4>] gic_set_type+0x60/0x74 [ 5.834778] [<ffff0000080fe344>] __irq_set_trigger+0x58/0x178 [ 5.840890] [<ffff0000080fea3c>] __setup_irq+0x5d8/0x65c [ 5.846536] [<ffff0000080fec9c>] request_threaded_irq+0x114/0x1c4 [ 5.853038] [<ffff0000083db4bc>] aer_probe+0xd4/0x274 [ 5.858418] [<ffff0000083d9064>] pcie_port_probe_service+0x38/0x80 [ 5.865007] [<ffff0000084c36b0>] driver_probe_device+0x200/0x2a0 [ 5.871378] [<ffff0000084c37fc>] __driver_attach+0xac/0xb0 [ 5.877215] [<ffff0000084c1720>] bus_for_each_dev+0x58/0x98 [ 5.883138] [<ffff0000084c2fc0>] driver_attach+0x20/0x28 [ 5.888800] [<ffff0000084c2bd8>] bus_add_driver+0x1c8/0x22c [ 5.894749] [<ffff0000084c40b4>] driver_register+0x60/0xf4 [ 5.900611] [<ffff0000083d8fb0>] pcie_port_service_register+0x58/0x68 [ 5.907477] [<ffff000008ce3dd8>] aer_service_init+0x30/0x38 [ 5.913400] [<ffff000008083334>] do_one_initcall+0x38/0x128 [ 5.919323] [<ffff000008cc0cc8>] kernel_init_freeable+0x150/0x1f0 [ 5.925809] [<ffff0000088f4278>] kernel_init+0x10/0xfc [ 5.931239] [<ffff000008082e90>] ret_from_fork+0x10/0x40 [ 5.936913] genirq: Setting trigger mode 8 for irq 50 failed (gic_set_type+0x0/0x74) [ 5.945163] aer 0000:00:00.0:pcie002: request IRQ failed [ 5.950844] aer: probe of 0000:00:00.0:pcie002 failed with error -22 > > I suspect most of ARM partners are already using interrupt links to > work around this and Duc was the first one reporting the issue > with the ACPI specs (ie interrupt links should just be used for > configurable interrupt pins - which, I will say it again - it is > likely to be a consequence of how x86 and their APIC works). I tried your suggestion about using interrupt links and it works. But unfortunately, it requires firmware change. > >> The ACPI code in the kernel (drivers/acpi/pci_irq.c) is behaving as per >> spec, so nothing to be done there IMHO. The problem arises due to the >> integration of two mismatched components - PCI is level low and the GIC >> supports only level high - making it necessary to introduce glue >> elements like the inverter. > > That glue can well be the interrupt link. I reiterate my point: > I will start dumping x86 ACPI tables and peruse the _PRT on x86 systems, > but in principle even on x86 the legacy PCI interrupt can be re-routed > and their polarity specified through interrupt links. It is not a > given that the legacy IRQs are active low at interrupt controller > level even on x86, at least that's not ruled out by ACPI specs. > > Am I wrong ? > > The inverter you and Marc mentioned can be described through an > interrupt link defining the interrupt polarity. On x86 there is > probably an IRQ router sitting downstream the ACPI that allows > to a) configure the IOAPIC pin for the legacy IRQ and b) control > the polarity, but as I mentioned I do not have enough x86 knowledge, > so I am just guessing. > >> This would all be OK if ACPI had a mechanism to specify the interrupt >> type (trigger and polarity). As an alternative, for Juno I created a >> link device (as Lorenzo suggests) to provide this information to the >> kernel. >> >> With this fix the warnings went away and I suspect this will address >> Duc's issue as well. But that is playing naughty with the spec (ACPI 6.1 >> Section. 6.2.13). > > ACPI specs were written for x86 boxes, there is nothing naughty > in asking for them to be updated and work on other architectures. > > The only issue I see with interrupt links is that they may allow > reprogramming through the _SRS method, which is optional BTW. > >> If there are no good reason to restrict using link devices to >> configurable interrupts, perhaps the spec needs an update. > > Yes and that's what I am going to ask if nobody complains. In my opinion, if the interrupt is not configurable, I should be able to choose not to use interrupt link, which requires a spec. update as well to specify the polarity of the fixed interrupts? > >> Perhaps Rafael knows why is there a restriction on using link devices >> for fixed interrupts in the ACPI spec... > > See above, I do not know why that restriction is there given that > to me an interrupt link is a superset of static values, I do not > see why they should be prevented for non-configurable interrupts, > I am happy to be corrected if there is something we are missing. > > Lorenzo Regards, Duc Dang. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-25 16:46 ` Duc Dang @ 2016-08-25 17:20 ` Bjorn Helgaas 0 siblings, 0 replies; 37+ messages in thread From: Bjorn Helgaas @ 2016-08-25 17:20 UTC (permalink / raw) To: Duc Dang Cc: Lorenzo Pieralisi, Punit Agrawal, Marc Zyngier, Rafael Wysocki, linux-pci, linux-acpi, patches, Bjorn Helgaas On Thu, Aug 25, 2016 at 09:46:25AM -0700, Duc Dang wrote: > On Thu, Aug 25, 2016 at 4:14 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > > On Thu, Aug 25, 2016 at 11:04:02AM +0100, Punit Agrawal wrote: > >> If there are no good reason to restrict using link devices to > >> configurable interrupts, perhaps the spec needs an update. > > > > Yes and that's what I am going to ask if nobody complains. > > In my opinion, if the interrupt is not configurable, I should be able > to choose not to use interrupt link, which requires a spec. update as > well to specify the polarity of the fixed interrupts? This is a little out of my area, but I really don't see the point of an ACPI spec update. PCI has specified level/low and ACPI systems have worked that way for decades. Can't you just make your GIC driver aware of the fact that some of its inputs have inverters on them and are effectively active low? Then all the generic code should work just like it does on other systems. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT 2016-08-24 11:06 Defining polarity and trigger mode for static interrupts in _PRT Duc Dang @ 2016-08-24 15:26 ` Marc Zyngier 2016-08-24 15:26 ` Marc Zyngier 1 sibling, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-24 15:26 UTC (permalink / raw) To: Duc Dang Cc: Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches On Wed, 24 Aug 2016 04:06:13 -0700 Duc Dang <dhdang@apm.com> wrote: > [Resend in plain text mode] > > Hi Lorenzo, Rafael, > > ACPI 6.1 spec does not specify how to set interrupt polarity and > trigger mode in _PRT when the interrupts are static (hardwired to > specific interrupt inputs in interrupt controller). In current > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > GICv3 controllers and will cause failures in PCIe AER, PME services > (on X-Gene platforms). > > Is there any way to specify polarity and trigger mode for static > interrupts in _PRT? If not, can we introduce a _weak_ hook to specify > default polarity and trigger mode for for ARM64 PCIe INTx in > drivers/acpi/pci_irq.c? A weak symbol is going to affect all arm64 platforms, which then would need to be quirked accordingly depending on the PCIe controller being used. This feels very cumbersome, and probably unmaintainable in the long run. I'd rather explore Lorenzo's option of using interrupt links to describe this. Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Defining polarity and trigger mode for static interrupts in _PRT @ 2016-08-24 15:26 ` Marc Zyngier 0 siblings, 0 replies; 37+ messages in thread From: Marc Zyngier @ 2016-08-24 15:26 UTC (permalink / raw) To: Duc Dang Cc: Lorenzo Pieralisi, Rafael Wysocki, linux-pci, linux-acpi, patches On Wed, 24 Aug 2016 04:06:13 -0700 Duc Dang <dhdang@apm.com> wrote: > [Resend in plain text mode] > > Hi Lorenzo, Rafael, > > ACPI 6.1 spec does not specify how to set interrupt polarity and > trigger mode in _PRT when the interrupts are static (hardwired to > specific interrupt inputs in interrupt controller). In current > acpi_pci_irq_enable (drivers/acpi/pci_irq.c) implementation, by > default the trigger mode is set to LEVEL_SENSITIVE, polarity is set to > ACTIVE_LOW. This default setting won't work for ARM64 GICv2, GICv2m, > GICv3 controllers and will cause failures in PCIe AER, PME services > (on X-Gene platforms). > > Is there any way to specify polarity and trigger mode for static > interrupts in _PRT? If not, can we introduce a _weak_ hook to specify > default polarity and trigger mode for for ARM64 PCIe INTx in > drivers/acpi/pci_irq.c? A weak symbol is going to affect all arm64 platforms, which then would need to be quirked accordingly depending on the PCIe controller being used. This feels very cumbersome, and probably unmaintainable in the long run. I'd rather explore Lorenzo's option of using interrupt links to describe this. Thanks, M. -- Jazz is not dead. It just smells funny. ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2016-09-02 21:23 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-24 11:06 Defining polarity and trigger mode for static interrupts in _PRT Duc Dang 2016-08-24 14:27 ` Lorenzo Pieralisi 2016-08-24 19:30 ` Bjorn Helgaas 2016-08-24 20:30 ` Marc Zyngier 2016-08-24 20:30 ` Marc Zyngier 2016-08-24 22:19 ` Duc Dang 2016-08-24 22:56 ` Bjorn Helgaas 2016-08-25 11:18 ` Marc Zyngier 2016-08-25 11:18 ` Marc Zyngier 2016-08-25 16:52 ` Duc Dang 2016-08-25 18:59 ` Marc Zyngier 2016-08-25 18:59 ` Marc Zyngier 2016-08-26 9:08 ` Lorenzo Pieralisi 2016-08-26 11:04 ` okaya 2016-08-26 12:08 ` Marc Zyngier 2016-08-26 12:08 ` Marc Zyngier 2016-08-26 14:07 ` Sinan Kaya 2016-08-26 17:06 ` Lorenzo Pieralisi 2016-08-26 22:53 ` Sinan Kaya 2016-08-30 10:08 ` Lorenzo Pieralisi 2016-08-30 15:51 ` Duc Dang 2016-08-30 17:54 ` Sinan Kaya 2016-08-31 10:07 ` Lorenzo Pieralisi 2016-08-31 13:05 ` Bjorn Helgaas 2016-08-31 13:34 ` Lorenzo Pieralisi 2016-08-31 16:05 ` Bjorn Helgaas 2016-08-31 16:37 ` Lorenzo Pieralisi 2016-08-31 23:08 ` Rafael J. Wysocki 2016-09-02 11:09 ` Lorenzo Pieralisi 2016-09-02 21:28 ` Rafael J. Wysocki 2016-08-25 10:04 ` Punit Agrawal 2016-08-25 10:04 ` Punit Agrawal 2016-08-25 11:14 ` Lorenzo Pieralisi 2016-08-25 16:46 ` Duc Dang 2016-08-25 17:20 ` Bjorn Helgaas 2016-08-24 15:26 ` Marc Zyngier 2016-08-24 15:26 ` Marc Zyngier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.