irqchip: vt8500: Switch to a simple write clear for Interrupt Status Register

PATCH Superseded Axel Lin 2 participants 3 messages Apr 27, 2014 – Apr 28, 2014 irqchip drivers

Share & tools

Axel Lin

author

2014-04-27 15:14 UTC #0 lore raw

PATCH 1/1 irqchip: vt8500: Switch to a simple write clear for Interrupt Status Register

According to the datasheet, the attribute of Interrupt Status Register is RW0S, which means:
Software can read the register.
Software can also "write 1 to clear". "write 0" has no effect.
Thus switch the read/modify/write to a simple write clear.

A read/modify/write does not make sense for an irq status register like this, since otherwise a read/modify/write can race with a device raising an interrupt and then clear the pending bit unintentionally.

Signed-off-by Axel Lin <[email protected]>

---
Hi Tony,
I don't have this h/w to test, I'd appreciate if you can review and test this patch.

Regards,
Axel

 drivers/irqchip/irq-vt8500.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/irqchip/irq-vt8500.c +1 -5

index eb6e91e..a0085bc 100644
@@ -87,14 +87,10 @@ static void vt8500_irq_mask(struct irq_data *d)
 	void __iomem *base = priv->base;
 	void __iomem *stat_reg = base + VT8500_ICIS + (d->hwirq < 32 ? 0 : 4);
 	u8 edge, dctr;
-	u32 status;
 
 	edge = readb(base + VT8500_ICDC + d->hwirq) & VT8500_EDGE;
 	if (edge) {
-		status = readl(stat_reg);
-
-		status |= (1 << (d->hwirq & 0x1f));
-		writel(status, stat_reg);
+		writel(BIT(d->hwirq & 0x1f), stat_reg);
 	} else {
 		dctr = readb(base + VT8500_ICDC + d->hwirq);
 		dctr &= ~VT8500_INT_ENABLE;

Signature · 1 line

1.8.3.2

Thomas Gleixner

2014-04-27 18:34 UTC #1 lore raw

Quoted 9 lines · Axel Lin

According to the datasheet, the attribute of Interrupt Status Register is RW0S, which means:
Software can read the register.
Software can also "write 1 to clear". "write 0" has no effect.
Thus switch the read/modify/write to a simple write clear.
A read/modify/write does not make sense for an irq status register like this, since otherwise a read/modify/write can race with a device raising an interrupt and then clear the pending bit unintentionally.

That's right, but what you are not seeing is that this is the mask callback which is supposed to mask the interrupt at the interrupt controller level.

I have a hard time to see (even without reading the data sheet) how clearing a potentially pending interrupt in the status register will mask the interrupt line.

Thanks,

tglx

Axel Lin

author

2014-04-28 03:34 UTC #2 lore raw

2014-04-28 2:34 GMT+08:00 Thomas Gleixner <[email protected]>:

Quoted 12 lines · Axel Lin

Quoted 10 lines
According to the datasheet, the attribute of Interrupt Status Register is RW0S, which means:
Software can read the register.
Software can also "write 1 to clear". "write 0" has no effect.
Thus switch the read/modify/write to a simple write clear.
A read/modify/write does not make sense for an irq status register like this, since otherwise a read/modify/write can race with a device raising an interrupt and then clear the pending bit unintentionally.

Quoted 7 lines

That's right, but what you are not seeing is that this is the mask callback which is supposed to mask the interrupt at the interrupt controller level.
I have a hard time to see (even without reading the data sheet) how clearing a potentially pending interrupt in the status register will mask the interrupt line.

ok, I'll send a v2 to update the commit log.
Thanks for the review.

Regards,
Axel