Dove: Attempt to fix PMU/RTC interrupts

PATCH New Russell King - ARM Linux 4 participants 13 messages Nov 18, 2012 – Nov 21, 2012

Subsystems (5)

Arm (32-bit) Sound Arm sub-architectures ASoC +1 more subsystems

lore

Share & tools

lore

Russell King - ARM Linux

author

2012-11-18 16:29 UTC #0 lore raw

PATCH 1/1 Dove: Attempt to fix PMU/RTC interrupts

Fix the acknowledgement of PMU interrupts on Dove: some Dove hardware has not been sensibly designed so that interrupts can be handled in a race free manner. The PMU is one such instance.

The pending (aka 'cause') register is a bunch of RW bits, meaning that these bits can be both cleared and set by software (confirmed on the Armada-510 on the cubox.)

Hardware sets the appropriate bit when an interrupt is asserted, and software is required to clear the bits which are to be processed. If we write ~(1 << bit), then we end up asserting every other interrupt except the one we're processing. So, we need to do a read-modify-write cycle to clear the asserted bit.

However, any interrupts which occur in the middle of this cycle will also be written back as zero, which will also clear the new interrupts.

The upshot of this is: there is _no_ way to safely clear down interrupts in this register (and other similarly behaving interrupt pending registers on this device.) The patch below at least stops us creating new interrupts.

Signed-off-by Russell King <[email protected]>

Signature · 6 lines

 arch/arm/mach-dove/irq.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

This should be -rc material - it's a bug; the existing code will cause
an interrupt storm should more than one interrupt in this register be
enabled at the same time.

arch/arm/mach-dove/irq.c +13 -1

index 9bc97a5..8c861ae 100644
@@ -45,8 +45,20 @@ static void pmu_irq_ack(struct irq_data *d)
 	int pin = irq_to_pmu(d->irq);
 	u32 u;
 
+	/*
+	 * The PMU mask register is not RW0C: it is RW.  This means that
+	 * the bits take whatever value is written to them; if you write
+	 * a '1', you will set the interrupt.
+	 *
+	 * Unfortunately this means there is NO race free way to clear
+	 * these interrupts.
+	 *
+	 * So, let's structure the code so that the window is as small as
+	 * possible.
+	 */
 	u = ~(1 << (pin & 31));
-	writel(u, PMU_INTERRUPT_CAUSE);
+	u &= readl_relaxed(PMU_INTERRUPT_CAUSE);
+	writel_relaxed(u, PMU_INTERRUPT_CAUSE);
 }
 
 static struct irq_chip pmu_irq_chip = {

Russell King - ARM Linux

author

2012-11-18 16:39 UTC #1 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

PMU interrupts start at IRQ_DOVE_PMU_START, not IRQ_DOVE_PMU_START + 1. Fix the condition. (It may have been less likely to occur had the code been written "if (irq >= IRQ_DOVE_PMU_START" which imho is the easier to understand notation, and matches the normal way of thinking about these things.)

Signed-off-by Russell King <[email protected]>

Signature · 2 lines

 arch/arm/mach-dove/include/mach/pm.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/arm/mach-dove/include/mach/pm.h +1 -1

index 1c942c0..02a0073 100644
@@ -184,7 +184,7 @@ static inline int pmu_to_irq(int pin)
 
 static inline int irq_to_pmu(int irq)
 {
-	if (IRQ_DOVE_PMU_START < irq && irq < NR_IRQS)
+	if (IRQ_DOVE_PMU_START <= irq && irq < NR_IRQS)
 		return irq - IRQ_DOVE_PMU_START;
 
 	return -EINVAL;

Sergei Shtylyov

2012-11-18 19:00 UTC #2 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

Hello.

Quoted 8 lines · Russell King - ARM Linux

PMU interrupts start at IRQ_DOVE_PMU_START, not IRQ_DOVE_PMU_START + 1. Fix the condition. (It may have been less likely to occur had the code been written "if (irq >= IRQ_DOVE_PMU_START" which imho is the easier to understand notation, and matches the normal way of thinking about these things.)
Signed-off-by Russell King <[email protected]>
--

Should be "---", or somebody (you?) will have to hand edit the patch when
applying...

WBR, Sergei

Russell King - ARM Linux

author

2012-11-18 20:38 UTC #3 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

Quoted 10 lines · Sergei Shtylyov

Hello.
Quoted 5 lines · Russell King - ARM Linux
PMU interrupts start at IRQ_DOVE_PMU_START, not IRQ_DOVE_PMU_START + 1. Fix the condition. (It may have been less likely to occur had the code been written "if (irq >= IRQ_DOVE_PMU_START" which imho is the easier to understand notation, and matches the normal way of thinking about these things.)

Quoted 5 lines

Quoted 2 lines
Signed-off-by Russell King <[email protected]>
--
Should be "---", or somebody (you?) will have to hand edit the patch
when applying...

Sorry, can never remember that...

Jason Cooper

2012-11-18 21:02 UTC #4 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

Quoted 11 lines · Russell King - ARM Linux

Quoted 10 lines · Sergei Shtylyov
Hello.
Quoted 5 lines · Russell King - ARM Linux
PMU interrupts start at IRQ_DOVE_PMU_START, not IRQ_DOVE_PMU_START + 1. Fix the condition. (It may have been less likely to occur had the code been written "if (irq >= IRQ_DOVE_PMU_START" which imho is the easier to understand notation, and matches the normal way of thinking about these things.)

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Signed-off-by Russell King <[email protected]>
--
Should be "---", or somebody (you?) will have to hand edit the patch
when applying...
Sorry, can never remember that...

No problem, I'll handle it when I pull it in.

thx,

Jason.

Russell King - ARM Linux

author

2012-11-18 22:31 UTC #5 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

Quoted 15 lines · Jason Cooper

Quoted 14 lines · Russell King - ARM Linux
Quoted 13 lines · Sergei Shtylyov
Hello.
Quoted 8 lines · Russell King - ARM Linux
PMU interrupts start at IRQ_DOVE_PMU_START, not IRQ_DOVE_PMU_START + 1. Fix the condition. (It may have been less likely to occur had the code been written "if (irq >= IRQ_DOVE_PMU_START" which imho is the easier to understand notation, and matches the normal way of thinking about these things.)
Signed-off-by Russell King <[email protected]>
--

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
Should be "---", or somebody (you?) will have to hand edit the patch
when applying...
Sorry, can never remember that...
No problem, I'll handle it when I pull it in.

And there's yet another bug... this time in the kirkwood audio driver:

static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)

{
        struct kirkwood_dma_priv *prdata = dev_id;
        struct kirkwood_dma_data *priv = prdata->data;
        unsigned long mask, status, cause;   
        
        mask = readl(priv->io + KIRKWOOD_INT_MASK);
        status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;

        cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
        if (unlikely(cause)) {
                printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
                                __func__, cause);
                writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
                return IRQ_HANDLED;
        }

What happens here if an underrun (0x20) interrupt happens? Well, the kernel log looks something like this:

kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
...

and the system spins doing nothing but that message. It's not because the interrupt isn't being cleared (it's a sensible write-1-to-clear register).

If you have (frigged) orion_wdt to work, then you'll get a reset after 25s, otherwise you'll have what appears to be a totally dead system with (due to the log buffer rewrite) no kernel message output, no interrupts, no nothing.

It's taken a full day to track this bug down...

As for what causes the underrun, that's a different (and as yet unanswered) question... but without the patch below which turns off the error reporting after the first (until it's re-opened) DVD quality MPEG2 decoding fails in less than one minute with ubuntu precise 12.10 userspace with the machine locking solid... doesn't matter if it's software or hardware decode.

sound/soc/kirkwood/kirkwood-dma.c +2 -0

index 690260b..26a1d36 100644
@@ -73,10 +73,12 @@ static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
 	status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
 
 	cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
+	cause &= readl(priv->io + KIRKWOOD_ERR_MASK);
 	if (unlikely(cause)) {
 		printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
 				__func__, cause);
 		writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
+		writel(0, priv->io + KIRKWOOD_ERR_MASK);
 		return IRQ_HANDLED;
 	}

Now I'm getting to the point of wondering how well tested any of this dove/kirkwood/orion code actually is... it all looks rather insanely fragile to me tonight.

Russell King - ARM Linux

author

2012-11-18 23:08 UTC #6 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

Quoted 19 lines · Russell King - ARM Linux

And there's yet another bug... this time in the kirkwood audio driver:

static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)

{
        struct kirkwood_dma_priv *prdata = dev_id;
        struct kirkwood_dma_data *priv = prdata->data;
        unsigned long mask, status, cause;   
        
        mask = readl(priv->io + KIRKWOOD_INT_MASK);
        status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;

        cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
        if (unlikely(cause)) {
                printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
                                __func__, cause);
                writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
                return IRQ_HANDLED;
        }

Quoted 2 lines

What happens here if an underrun (0x20) interrupt happens? Well, the kernel log looks something like this:

Actually, it's _far_ worse. This interrupt handler is hooked into the _normal_ I2S IRQ (21) - errors are reported via a separate IRQ (22).

So what happens is a normal IRQ (in INT_CAUSE) gets signalled. It's at that point we notice that an error occured... and clear the error, leaving the normal interrupt set in INT_CAUSE, and return lying that we'd serviced the interrupt.

The result is... IRQ21 is still pending, and ends up being the higher priority interrupt, so we service IRQ21 again... and find that the error cause bit has been set again... and return yet again IRQ_HANDLED without actually servicing the interrupt...

I think we're far better off ripping out this "error handling". If we want to have this, then we should hook the _proper_ error interrupt, and have some kind of back-off on it (which I think is the right solution). Or if we merely want to report that an error occurred, then just get rid of that "return IRQ_HANDLED" there.

Jason Cooper

2012-11-18 23:12 UTC #7 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

Russell,

adding relevant folks to the CC:

Quoted 36 lines · Russell King - ARM Linux

Quoted 19 lines · Russell King - ARM Linux
And there's yet another bug... this time in the kirkwood audio driver:
static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
{
        struct kirkwood_dma_priv *prdata = dev_id;
        struct kirkwood_dma_data *priv = prdata->data;
        unsigned long mask, status, cause;   
        
        mask = readl(priv->io + KIRKWOOD_INT_MASK);
        status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;

        cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
        if (unlikely(cause)) {
                printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
                                __func__, cause);
                writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
                return IRQ_HANDLED;
        }
Quoted 2 lines
What happens here if an underrun (0x20) interrupt happens? Well, the kernel log looks something like this:
Actually, it's _far_ worse. This interrupt handler is hooked into the _normal_ I2S IRQ (21) - errors are reported via a separate IRQ (22).
So what happens is a normal IRQ (in INT_CAUSE) gets signalled. It's at that point we notice that an error occured... and clear the error, leaving the normal interrupt set in INT_CAUSE, and return lying that we'd serviced the interrupt.
The result is... IRQ21 is still pending, and ends up being the higher priority interrupt, so we service IRQ21 again... and find that the error cause bit has been set again... and return yet again IRQ_HANDLED without actually servicing the interrupt...

Quoted 5 lines

I think we're far better off ripping out this "error handling". If we want to have this, then we should hook the _proper_ error interrupt, and have some kind of back-off on it (which I think is the right solution). Or if we merely want to report that an error occurred, then just get rid of that "return IRQ_HANDLED" there.

thx,

Jason.

Jason Cooper

Acked-by Liam Girdwood on v1 1/1

2012-11-18 23:09 UTC #8 lore raw

kirkwood i2s lockup, was: Re: [PATCH] Dove: Fix irq_to_pmu()

In reply to Russell King - ARM Linux

Quoted 86 lines · Russell King - ARM Linux

Quoted 15 lines · Jason Cooper
Quoted 14 lines · Russell King - ARM Linux
Quoted 13 lines · Sergei Shtylyov
Hello.
Quoted 8 lines · Russell King - ARM Linux
PMU interrupts start at IRQ_DOVE_PMU_START, not IRQ_DOVE_PMU_START + 1. Fix the condition. (It may have been less likely to occur had the code been written "if (irq >= IRQ_DOVE_PMU_START" which imho is the easier to understand notation, and matches the normal way of thinking about these things.)
Signed-off-by Russell King <[email protected]>
--
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Should be "---", or somebody (you?) will have to hand edit the patch
when applying...
Sorry, can never remember that...
No problem, I'll handle it when I pull it in.
And there's yet another bug... this time in the kirkwood audio driver:
static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
{
        struct kirkwood_dma_priv *prdata = dev_id;
        struct kirkwood_dma_data *priv = prdata->data;
        unsigned long mask, status, cause;   
        
        mask = readl(priv->io + KIRKWOOD_INT_MASK);
        status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;

        cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
        if (unlikely(cause)) {
                printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
                                __func__, cause);
                writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
                return IRQ_HANDLED;
        }
What happens here if an underrun (0x20) interrupt happens? Well, the kernel log looks something like this:
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
kirkwood_dma_irq: got err interrupt 0x20
...
and the system spins doing nothing but that message. It's not because the interrupt isn't being cleared (it's a sensible write-1-to-clear register).
If you have (frigged) orion_wdt to work, then you'll get a reset after 25s, otherwise you'll have what appears to be a totally dead system with (due to the log buffer rewrite) no kernel message output, no interrupts, no nothing.
It's taken a full day to track this bug down...
As for what causes the underrun, that's a different (and as yet unanswered) question... but without the patch below which turns off the error reporting after the first (until it's re-opened) DVD quality MPEG2 decoding fails in less than one minute with ubuntu precise 12.10 userspace with the machine locking solid... doesn't matter if it's software or hardware decode.
sound/soc/kirkwood/kirkwood-dma.c +2 -0
index 690260b..26a1d36 100644
@@ -73,10 +73,12 @@ static irqreturn_t kirkwood_dma_irq(int irq, void *dev_id)
 	status = readl(priv->io + KIRKWOOD_INT_CAUSE) & mask;
 
 	cause = readl(priv->io + KIRKWOOD_ERR_CAUSE);
+	cause &= readl(priv->io + KIRKWOOD_ERR_MASK);
 	if (unlikely(cause)) {
 		printk(KERN_WARNING "%s: got err interrupt 0x%lx\n",
 				__func__, cause);
 		writel(cause, priv->io + KIRKWOOD_ERR_CAUSE);
+		writel(0, priv->io + KIRKWOOD_ERR_MASK);
 		return IRQ_HANDLED;
 	}
 

Quoted 3 lines

Now I'm getting to the point of wondering how well tested any of this dove/kirkwood/orion code actually is... it all looks rather insanely fragile to me tonight.

I won't deny that at all, I think the CuBox is the first platform to actually use the Marvell audio/multimedia code. Anywho, git blame points at:

commit f9b95980f87f021f8c69646738929189838ad035

Author: apatard at mandriva.com <[email protected]>
Date:   Mon May 31 13:49:14 2010 +0200

ASoC: kirkwood: Add i2s support

This patch enables support for the i2s controller available on kirkwood platforms

Signed-off-by Arnaud Patard <[email protected]>

Acked-by Liam Girdwood <[email protected]>

Signed-off-by Mark Brown <[email protected]>

which is the original commit adding the driver. I've added them to the reply to see if they have any insight.

thx,

Jason.

Andrew Lunn

maintainer

2012-11-19 06:11 UTC #9 lore raw

PATCH 1/1 Dove: Fix irq_to_pmu()

In reply to Russell King - ARM Linux

Quoted 3 lines · Russell King - ARM Linux

Now I'm getting to the point of wondering how well tested any of this dove/kirkwood/orion code actually is... it all looks rather insanely fragile to me tonight.

Hi Russell

I would say, anything a NAS box typically uses, is well tested. When you get away from that use case, expect problems.

Andrew

Andrew Lunn

maintainer

2012-11-21 15:59 UTC #10 lore raw

PATCH 1/1 Dove: Attempt to fix PMU/RTC interrupts

In reply to Russell King - ARM Linux

Quoted 3 lines · Russell King - ARM Linux

Fix the acknowledgement of PMU interrupts on Dove: some Dove hardware has not been sensibly designed so that interrupts can be handled in a race free manner. The PMU is one such instance.

Hi Russel

I checked with Marvell. They confirmed there is no race free way to clear these interrupts :-(

Andrew

Russell King - ARM Linux

author

2012-11-21 16:53 UTC #11 lore raw

PATCH 1/1 Dove: Attempt to fix PMU/RTC interrupts

Quoted 9 lines · Andrew Lunn

Quoted 3 lines · Russell King - ARM Linux
Fix the acknowledgement of PMU interrupts on Dove: some Dove hardware has not been sensibly designed so that interrupts can be handled in a race free manner. The PMU is one such instance.
Hi Russel
I checked with Marvell. They confirmed there is no race free way to clear these interrupts :-(

That's what I thought given how the hardware behaves and what the documentation says. So the best we can do is to ensure that the window between reading and writing is as small as possible. Not perfect but it's the best we can do with the buggy hardware we have. :(

Jason Cooper

2012-11-21 16:57 UTC #12 lore raw

PATCH 1/1 Dove: Attempt to fix PMU/RTC interrupts

Quoted 8 lines · Russell King - ARM Linux

Quoted 7 lines · Andrew Lunn
Quoted 3 lines · Russell King - ARM Linux
Fix the acknowledgement of PMU interrupts on Dove: some Dove hardware has not been sensibly designed so that interrupts can be handled in a race free manner. The PMU is one such instance.
Hi Russel

Quoted 7 lines

Quoted 2 lines
I checked with Marvell. They confirmed there is no race free way to clear these interrupts :-(
That's what I thought given how the hardware behaves and what the documentation says. So the best we can do is to ensure that the window between reading and writing is as small as possible. Not perfect but it's the best we can do with the buggy hardware we have. :(

Russell,

I'll go ahead and pull this in to mvebu/fixes and flag it for stable.

thx,

Jason.