Re: Defining polarity and trigger mode for static interrupts in _PRT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 24 Aug 2016 15:19:21 -0700
Duc Dang <dhdang@xxxxxxx> wrote:

> On Wed, Aug 24, 2016 at 1:30 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> > On Wed, 24 Aug 2016 14:30:00 -0500
> > Bjorn Helgaas <helgaas@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux