Skip to contentSkip to conversation

irqchip: sun4i: Remove wrong irq_ack callback implementation

PATCH Superseded Axel Lin 3 participants 5 messages Jul 6, 2013 – Jul 24, 2013

Subsystems (2)
lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 irqchip: sun4i: Remove wrong irq_ack callback implementation

According to the datasheet[1], the Interrupt IRQ Pending Registers are read-only. The implementation of sun4i_irq_ack() looks wrong because it writes to these read-only registers.

This patch removes the wrong irq_ack callback implementation and all the code writing to these read-only registers in sun4i_of_init().

[1] http://dl.linux-sunxi.org/A10/A10%20User%20Manual%20-%20v1.20%20%282012-04-09%2c%20DECRYPTED%29.pdf

---
Hi Maxime,
I don't have this hardware, I'd appreciate if you can test this patch. If this patch works, I think we can just drop the patch I sent yesterday. ( It's "irqchip: sun4i: Staticize sun4i_irq_ack()"

    https://lkml.org/lkml/2013/7/5/43 )

Thanks,
Axel

 drivers/irqchip/irq-sun4i.c | 18 ------------------
 1 file changed, 18 deletions(-)
drivers/irqchip/irq-sun4i.c +0 -18
index b66d4ae..29b75c0a 100644
@@ -38,18 +38,6 @@ static struct irq_domain *sun4i_irq_domain;
 
 static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *regs);
 
-void sun4i_irq_ack(struct irq_data *irqd)
-{
-	unsigned int irq = irqd_to_hwirq(irqd);
-	unsigned int irq_off = irq % 32;
-	int reg = irq / 32;
-	u32 val;
-
-	val = readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
-	writel(val | (1 << irq_off),
-	       sun4i_irq_base + SUN4I_IRQ_PENDING_REG(reg));
-}
-
 static void sun4i_irq_mask(struct irq_data *irqd)
 {
 	unsigned int irq = irqd_to_hwirq(irqd);
@@ -76,7 +64,6 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
 
 static struct irq_chip sun4i_irq_chip = {
 	.name		= "sun4i_irq",
-	.irq_ack	= sun4i_irq_ack,
 	.irq_mask	= sun4i_irq_mask,
 	.irq_unmask	= sun4i_irq_unmask,
 };
@@ -114,11 +101,6 @@ static int __init sun4i_of_init(struct device_node *node,
 	writel(0, sun4i_irq_base + SUN4I_IRQ_MASK_REG(1));
 	writel(0, sun4i_irq_base + SUN4I_IRQ_MASK_REG(2));
 
-	/* Clear all the pending interrupts */
-	writel(0xffffffff, sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0));
-	writel(0xffffffff, sun4i_irq_base + SUN4I_IRQ_PENDING_REG(1));
-	writel(0xffffffff, sun4i_irq_base + SUN4I_IRQ_PENDING_REG(2));
-
 	/* Enable protection mode */
 	writel(0x01, sun4i_irq_base + SUN4I_IRQ_PROTECTION_REG);
 
Signature · 1 line
1.8.1.2
#1 lore raw

Hi Axel,

Quoted 3 lines · Axel Lin

According to the datasheet[1], the Interrupt IRQ Pending Registers are read-only. The implementation of sun4i_irq_ack() looks wrong because it writes to these read-only registers.

Right, but the datasheet also says that these registers are "clear" register, which might indicate that it's actually R/W.

I actually used one of the code dump from Allwinner for this driver, so I'm not exactly sure about wether irq_ack is needed or not.

I'll test this and let you know.

Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Acked-by on v1 1/1
#4 lore raw
In reply to Axel Lin

Hi Axel,

Sorry for the late testing,

Quoted 15 lines · Axel Lin

According to the datasheet[1], the Interrupt IRQ Pending Registers are read-only. The implementation of sun4i_irq_ack() looks wrong because it writes to these read-only registers.

This patch removes the wrong irq_ack callback implementation and all the code writing to these read-only registers in sun4i_of_init().

[1] http://dl.linux-sunxi.org/A10/A10%20User%20Manual%20-%20v1.20%20%282012-04-09%2c%20DECRYPTED%29.pdf

---
Hi Maxime,
I don't have this hardware, I'd appreciate if you can test this patch. If this patch works, I think we can just drop the patch I sent yesterday. ( It's "irqchip: sun4i: Staticize sun4i_irq_ack()"

Quoted 4 lines
    https://lkml.org/lkml/2013/7/5/43 )

Thanks,
Axel

Since your previous patch was merged, it generates a merge conflict when applied on top of 3.11.

I guess you can rebase it on top of 3.11, and then you can add my

Thanks!
Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#2 lore raw
In reply to Maxime Ripard
Quoted 7 lines · Maxime Ripard
Quoted 3 lines · Axel Lin

According to the datasheet[1], the Interrupt IRQ Pending Registers are read-only. The implementation of sun4i_irq_ack() looks wrong because it writes to these read-only registers.

Right, but the datasheet also says that these registers are "clear" register, which might indicate that it's actually R/W.

I guess it means that the flags are cleared automatically on read? That's not so uncommon for interrupt pending registers.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

#3 lore raw

Hi Geert,

Quoted 7 lines · Geert Uytterhoeven
Quoted 5 lines · Maxime Ripard
Quoted 3 lines · Axel Lin

According to the datasheet[1], the Interrupt IRQ Pending Registers are read-only. The implementation of sun4i_irq_ack() looks wrong because it writes to these read-only registers.

Quoted 5 lines
Quoted 2 lines

Right, but the datasheet also says that these registers are "clear" register, which might indicate that it's actually R/W.

I guess it means that the flags are cleared automatically on read? That's not so uncommon for interrupt pending registers.

My understanding in this case is that the pending register is just a "copy" of the pending bits in the related hardware IP, so that clearing it in the interrupt controller won't do much, since it will be re-set just after the clear, while if you do clear the interrupt bit in the IP, it will clear it automatically in the interrupt controller.

Maxime

Signature · 3 lines
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com