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

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

[Lorenzo Pieralisi]

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

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

[Bjorn Helgaas]

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

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

[Marc Zyngier]

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.

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

[Duc Dang]

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.

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

[Bjorn Helgaas]

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.

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

[Marc Zyngier]

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.

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

[Duc Dang]

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.

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

[Marc Zyngier]

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.

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

[Lorenzo Pieralisi]

[ +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

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

[okaya]

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.

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

[Marc Zyngier]

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.

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

[Sinan Kaya]

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.

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

[Lorenzo Pieralisi]

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

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

[Sinan Kaya]

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

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

[Lorenzo Pieralisi]

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;

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

[Duc Dang]

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.

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

[Sinan Kaya]

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.

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

[Lorenzo Pieralisi]

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

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

[Bjorn Helgaas]

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?

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

[Lorenzo Pieralisi]

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

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

[Bjorn Helgaas]

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?

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

[Lorenzo Pieralisi]

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

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

[Rafael J. Wysocki]

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

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

[Lorenzo Pieralisi]

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

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

[Rafael J. Wysocki]

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

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

[Punit Agrawal]

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.

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

[Lorenzo Pieralisi]

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

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

[Duc Dang]

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.

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

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

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

[Marc Zyngier]

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.