[PATCH] powerpc/mpic: Add DT option to skip readback after EOI

[PATCH] powerpc/mpic: Add DT option to skip readback after EOI

[Bogdan Purcareata]

The readback is necessary in order to handle PCI posted
writes, or when the MPIC is handling interrupts in a loop
(ppc_md.get_irq). Newer MPIC versions don't require this
readback. Leave the option configurable using a device
tree entry.

This saves a MMIO trap per interrupt.

Signed-off-by: Scott Wood <scottwood@freescale.com>
Signed-off-by: Bogdan Purcareata <bogdan.purcareata@freescale.com>
---
 Documentation/devicetree/bindings/powerpc/fsl/mpic.txt | 13 +++++++++++++
 arch/powerpc/include/asm/mpic.h                        |  2 ++
 arch/powerpc/sysdev/mpic.c                             |  8 +++++++-
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt b/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
index dc57446..9789094 100644
--- a/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
+++ b/Documentation/devicetree/bindings/powerpc/fsl/mpic.txt
@@ -77,6 +77,19 @@ PROPERTIES
           in the global feature registers.  If specified, this field will
           override the value read from MPIC_GREG_FEATURE_LAST_SRC.
 
+  - mpic-eoi-no-readback
+      Usage: optional
+      Value type: <empty>
+      Definition: The presence of this property specifies that the
+          MPIC will not issue a readback when delivering the EOI for an
+          external interrupt. The readback operation is done by reading
+          the CPU WHOAMI register after writing to the CPU EOI register.
+          Originally, this was required due to the fact that the MPIC
+          operates at lower frequencies, or in scenarios where the MPIC
+          is connected through PCI with write posting. This is not the
+          case in an emulated environment (e.g. KVM guest), or in scenarios
+          where interrupts are not handled in a loop of get_irq() calls.
+
 INTERRUPT SPECIFIER DEFINITION
 
   Interrupt specifiers consists of 4 cells encoded as
diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
index 754f93d..e2a4146 100644
--- a/arch/powerpc/include/asm/mpic.h
+++ b/arch/powerpc/include/asm/mpic.h
@@ -386,6 +386,8 @@ extern struct bus_type mpic_subsys;
  * from the BRR1 register).
 */
 #define MPIC_FSL_HAS_EIMR		0x00010000
+/* Dont bother with readback after MPIC EOI */
+#define MPIC_EOI_NO_READBACK	0x00020000
 
 /* MPIC HW modification ID */
 #define MPIC_REGSET_MASK		0xf0000000
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index f3e8624..431f68e 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -656,7 +656,9 @@ static inline struct mpic * mpic_from_irq_data(struct irq_data *d)
 static inline void mpic_eoi(struct mpic *mpic)
 {
 	mpic_cpu_write(MPIC_INFO(CPU_EOI), 0);
-	(void)mpic_cpu_read(MPIC_INFO(CPU_WHOAMI));
+
+	if (!(mpic->flags & MPIC_EOI_NO_READBACK))
+		(void)mpic_cpu_read(MPIC_INFO(CPU_WHOAMI));
 }
 
 /*
@@ -1290,6 +1292,10 @@ struct mpic * __init mpic_alloc(struct device_node *node,
 		flags |= MPIC_SINGLE_DEST_CPU;
 	if (of_device_is_compatible(node, "fsl,mpic"))
 		flags |= MPIC_FSL | MPIC_LARGE_VECTORS;
+	if (of_get_property(node, "mpic-eoi-no-readback", NULL)) {
+		pr_debug("mpic: no readback activated");
+		flags |= MPIC_EOI_NO_READBACK;
+	}
 
 	mpic = kzalloc(sizeof(struct mpic), GFP_KERNEL);
 	if (mpic == NULL)
-- 
2.1.3

Re: [PATCH] powerpc/mpic: Add DT option to skip readback after EOI

[Scott Wood]

On Wed, 2015-01-14 at 11:57 +0000, Bogdan Purcareata wrote:
> The readback is necessary in order to handle PCI posted
> writes,

That is unclear.

>  or when the MPIC is handling interrupts in a loop
> (ppc_md.get_irq).

I'm questioning this one as well -- if reading WHOAMI is sufficient to
sync with the EOI, why wouldn't reading INTACK work as well?

>  Newer MPIC versions don't require this readback. Leave the option
> configurable using a device tree entry.
> 
> This saves a MMIO trap per interrupt.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> Signed-off-by: Bogdan Purcareata <bogdan.purcareata@freescale.com>

You should note changes (e.g. with a [] note between signoffs) if it
differs from the original...

-Scott

Re: [PATCH] powerpc/mpic: Add DT option to skip readback after EOI

[Purcareata Bogdan]

On 14.01.2015 19:58, Scott Wood wrote:
> On Wed, 2015-01-14 at 11:57 +0000, Bogdan Purcareata wrote:
>> The readback is necessary in order to handle PCI posted
>> writes,
>
> That is unclear.

I'm going to try an venture a different explanation here.

I found a good explanation in Writing PCI Drivers [1] as to why it's 
necessary that a read is performed before returning from an EOI write:

"A good example of such a case is when a driver’s Interrupt Service 
Routine (ISR) is dealing with the Interrupt Request Register (IRR) on a 
card. Clearing a bit in the register indicates that the interrupt has 
been serviced. This is done by posting a write to the register. If the 
driver posts this write and exits its ISR, it could get interrupted 
again immediately because the write hadn’t yet reached the bit in the 
IRR to tell it to stop trying to interrupt. One solution to this 
potential problem is to make sure to read back the value in the IRR 
before exiting from the ISR. Most drivers do this anyway so they can 
handle multiple interrupts in the same ISR visit."

This is designed to work for legacy line interrupts. I understand MSI 
has its way to order writes on the bus, and also it won't send another 
MSI, signaling a new interrupt, unless there's a state change in the IRR 
on the card. When using line interrupts, the line stays asserted as long 
as IRR is set, so the MPIC could trigger an additional interrupt if the 
EOI write is posted and the ISR bit is cleared.

>
>>   or when the MPIC is handling interrupts in a loop
>> (ppc_md.get_irq).
>
> I'm questioning this one as well -- if reading WHOAMI is sufficient to
> sync with the EOI, why wouldn't reading INTACK work as well?

I think I'm going to drop this part of the description. However, I don't 
think reading INTACK is good, since it has some additional side effects 
- interrupt pending register is cleared for edge-sensitive interrupts, 
the in-service register is updated, and the int output signal from the 
MPIC is negated. This is available for Freescale SoCs. This also 
signifies entering the processing cycle of a new interrupt.

Normally, the readback should be done on the EOI register - this is 
stated in the OpenPIC specification and actually was the initial 
implementation of the driver. However, this can't be done since, at 
least for Freescale SoCs, the EOI register is write only.

However, in the light of the above explanation related to PCI posted 
writes, I don't see how performing a readback on the WHOAMI register, 
which is specific to the MPIC, would have any effect in synchronizing 
with the PCI device registers. I guess it's related to the bus 
interconnect on the SoC.

>>   Newer MPIC versions don't require this readback. Leave the option
>> configurable using a device tree entry.
>>
>> This saves a MMIO trap per interrupt.
>>
>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>> Signed-off-by: Bogdan Purcareata <bogdan.purcareata@freescale.com>
>
> You should note changes (e.g. with a [] note between signoffs) if it
> differs from the original...

Will do, sorry for omitting this.

>
> -Scott

[1] http://h21007.www2.hp.com/portal/download/files/unprot/ddk/ddg/chap8.pdf

Best regards,
Bogdan P.

Re: [PATCH] powerpc/mpic: Add DT option to skip readback after EOI

[Scott Wood]

On Wed, 2015-01-21 at 11:35 +0200, Purcareata Bogdan wrote:
> On 14.01.2015 19:58, Scott Wood wrote:
> > On Wed, 2015-01-14 at 11:57 +0000, Bogdan Purcareata wrote:
> >> The readback is necessary in order to handle PCI posted
> >> writes,
> >
> > That is unclear.
> 
> I'm going to try an venture a different explanation here.
> 
> I found a good explanation in Writing PCI Drivers [1] as to why it's 
> necessary that a read is performed before returning from an EOI write:
> 
> "A good example of such a case is when a driver’s Interrupt Service 
> Routine (ISR) is dealing with the Interrupt Request Register (IRR) on a 
> card. Clearing a bit in the register indicates that the interrupt has 
> been serviced. This is done by posting a write to the register. If the 
> driver posts this write and exits its ISR, it could get interrupted 
> again immediately because the write hadn’t yet reached the bit in the 
> IRR to tell it to stop trying to interrupt. One solution to this 
> potential problem is to make sure to read back the value in the IRR 
> before exiting from the ISR. Most drivers do this anyway so they can 
> handle multiple interrupts in the same ISR visit."

That's an issue of ordering the device write versus the EOI -- it
doesn't explain why we'd need to order the EOI versus rfi, except maybe
that the extra delay just happens to be enough to give the device the
time it needs to deassert the second interrupt before it gets to the
point of interrupting the CPU.

FWIW, I'm skeptical of the "most drivers do this anyway" claim.

> >>   or when the MPIC is handling interrupts in a loop
> >> (ppc_md.get_irq).
> >
> > I'm questioning this one as well -- if reading WHOAMI is sufficient to
> > sync with the EOI, why wouldn't reading INTACK work as well?
> 
> I think I'm going to drop this part of the description. However, I don't 
> think reading INTACK is good, since it has some additional side effects 
> - interrupt pending register is cleared for edge-sensitive interrupts, 
> the in-service register is updated, and the int output signal from the 
> MPIC is negated. This is available for Freescale SoCs. This also 
> signifies entering the processing cycle of a new interrupt.

I wasn't suggesting that we start reading INTACK there.  I'm asking why,
back when we did read INTACK in a loop, that wasn't ordered relative to
the EOI write (but reading WHOAMI was).

> Normally, the readback should be done on the EOI register - this is 
> stated in the OpenPIC specification and actually was the initial 
> implementation of the driver. However, this can't be done since, at 
> least for Freescale SoCs, the EOI register is write only.
> 
> However, in the light of the above explanation related to PCI posted 
> writes, I don't see how performing a readback on the WHOAMI register, 
> which is specific to the MPIC, would have any effect in synchronizing 
> with the PCI device registers. I guess it's related to the bus 
> interconnect on the SoC.

I wonder if anyone has checked whether the spurious interrupts that
reading EOI got rid of have come back on those systems, now that we read
WHOAMI instead.

-Scott