[PATCH RFT] irqchip: sun4i: Remove wrong irq_ack callback implementation

[PATCH RFT] irqchip: sun4i: Remove wrong irq_ack callback implementation

[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

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
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(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index b66d4ae..29b75c0a 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -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);
 
-- 
1.8.1.2

Re: [PATCH RFT] irqchip: sun4i: Remove wrong irq_ack callback implementation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

Hi Axel,

On Sat, Jul 06, 2013 at 10:35:29PM +0800, Axel Lin wrote:
> 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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFT] irqchip: sun4i: Remove wrong irq_ack callback implementation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]

Hi Axel,

Sorry for the late testing,

On Sat, Jul 06, 2013 at 10:35:29PM +0800, Axel Lin wrote:
> 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
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> 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

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 
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFT] irqchip: sun4i: Remove wrong irq_ack callback implementation

[Geert Uytterhoeven]

On Thu, Jul 11, 2013 at 6:13 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Sat, Jul 06, 2013 at 10:35:29PM +0800, Axel Lin wrote:
>> 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 -- geert@linux-m68k.org

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

Re: [PATCH RFT] irqchip: sun4i: Remove wrong irq_ack callback implementation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

Hi Geert,

On Fri, Jul 19, 2013 at 11:29:30AM +0200, Geert Uytterhoeven wrote:
> On Thu, Jul 11, 2013 at 6:13 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Sat, Jul 06, 2013 at 10:35:29PM +0800, Axel Lin wrote:
> >> 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.

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]