[PATCH v2 0/4] irq: sun4i IRQ 0 / ENMI fixes

[PATCH v2 0/4] irq: sun4i IRQ 0 / ENMI fixes

[Hans de Goede]

Hi All,

Here is v2 of my sun4i IRQ 0 / ENMI fixes fixes, changelog below:

v2:

PATCH 1/4 irq: Add handle_fasteoi_late_irq irq handler
New patch by Thomas Gleixner, adding a new irq handler with the "call eoi
after the irq-handler has run its course" behavior we need to avoid double
irqs on the ENMI.

PATCH 2/4 irqchip: sun4i: Fix irq 0 not working
Modified to avoid spurious calls to the irq 0 handler, as suggested by Maxime.

PATCH 3/4 irqchip: sun4i: Fix a comment about mask register
Added Maxime's Acked-by

PATCH 4/4 irqchip: sun4i: Use handle_fasteoi_late_irq for the
New patch switching the handler for irq 0 over to the new
handle_fasteoi_late_irq handler, thereby fixing the double interrupts issue

Thanks for all the help to all involved.

Regards,

Hans

[PATCH v2 1/4] irq: Add handle_fasteoi_late_irq irq handler

[Hans de Goede]

From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>

Add an irq handler for transparent controllers

Only a single callback will be issued to the chip: an ->eoi() call when
the interrupt has been serviced. Same as handle_fasteoi_irq, but
we avoid the eoi when the interrupt is masked due to a
threaded handler. The eoi will be issued right before the unmask.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> In some cases we want to do an ack right before the unmask of an irq.
>
> A typical example of such a case is having an i2c device which uses a level
> interrupt. Such devices usually have an interrupt status register with
> write to clear status bits. This means that the interrupt handler needs to
> sleep to do i2c transfers to read / write the interrupt status register.
>
> This means using a threaded interrupt handler and IRQF_ONESHOT so that the
> interrupt does not re-trigger while the interrupt handler is waiting for the
> i2c transfer.
>
> Without ack-before-unmask the sequence of a single interrupt in this setup
> looks like this:
>
> i2c-device raises interrupt line
> interrupt gets triggered
> interrupt gets masked
> interrupt gets acked
> interrupt handling thread starts running
> interrupt handling thread does its stuff, write-clears irq-status register
>  in i2c-device
> i2c-device lowers interrupt line (note this happens after the ack!)
> interrupt handling thread is done
> interrupt gets unmasked
> interrupt immediately retriggers again, because the line has been high
>  after the ack
> interrupt gets masked
> interrupt gets acked
> interrupt handling thread starts running
> interrupt handling thread reads irq-status register, has nothing todo
> interrupt handling thread is done
> interrupt gets unmasked
>
> So in essence we get and handle 2 interrupts for what is 1 interrupt from
> the i2c-device pov. By doing an ack before the unmask, the second interrupt
> is avoided since at the point of the unmask the irq-thread has done its
> job and cleared the interrupt source.

And how is that different from the non threaded case?

    mask()
    ack()	<-- irq line is still active
    handle()	<-- irq line goes inactive. So this happens
    		     after the ack as above
    unmask()

> Note that things work fine without this patch, the purpose of this patch is
> soley to avoid the second unneeded interrupt.
>
> This patch uses an interrupt flag for this and not an irqchip flag because
> the need for ack before unmask is based on the device and not on the irqchip.

No, it's not a device property. Its an irq chip property. There are
interrupt chips which handle that case fine and making this a device
property might even break such interrupt chips. e.g. ioapic handles
that nicely but it would use ack_edge() on a level interrupt when the
device driver requests that. And the edge and level mode of the ioapic
are substantially different. Go figure.

The general policy is that device drivers should be oblivious of the
underlying interrupt controller implementation as much as possible.

If the interrupt chip has this behaviour then handle_level_irq as the
flow handler is the wrong thing to start with because it always acks
before calling the handler.

Due to the fact that we run the primary handler with interrupts
disabled, we should avoid the mask/unmask dance for the non threaded
case completely. handle_fasteoi_irq does that, except that it does not
provide the eoi() before unmask in the threaded case and it has an
extra eoi() even in the masked case which is pointless for interrupt
chips like the one you are dealing with.

Untested patch below.

Thanks,

	tglx

---------------->
---
 include/linux/irq.h    |  4 +++
 kernel/irq/chip.c      | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/irq/internals.h |  1 +
 kernel/irq/manage.c    |  2 +-
 4 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 7dc1003..51c199e 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -349,6 +349,8 @@ struct irq_chip {
  * IRQCHIP_ONOFFLINE_ENABLED:	Only call irq_on/off_line callbacks
  *				when irq enabled
  * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
+ * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
+ * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
  */
 enum {
 	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
@@ -357,6 +359,7 @@ enum {
 	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
 	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
 	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
+	IRQCHIP_EOI_THREADED		= (1 <<  6),
 };
 
 /* This include will go away once we isolated irq_desc usage to core code */
@@ -412,6 +415,7 @@ static inline int irq_set_parent(int irq, int parent_irq)
  */
 extern void handle_level_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
+extern void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
 extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index dc04c16..bc3c099 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -281,6 +281,19 @@ void unmask_irq(struct irq_desc *desc)
 	}
 }
 
+void unmask_threaded_irq(struct irq_desc *desc)
+{
+	struct irq_chip *chip = desc->irq_data.chip;
+
+	if (chip->flags & IRQCHIP_EOI_THREADED)
+		chip->irq_eoi(&desc->irq_data);
+
+	if (chip->irq_unmask) {
+		chip->irq_unmask(&desc->irq_data);
+		irq_state_clr_masked(desc);
+	}
+}
+
 /*
  *	handle_nested_irq - Handle a nested irq from a irq thread
  *	@irq:	the interrupt number
@@ -487,6 +500,66 @@ out:
 	goto out_unlock;
 }
 
+static void cond_unmask_and_eoi_irq(struct irq_desc *desc)
+{
+	if (!(desc->istate & IRQS_ONESHOT)) {
+		desc->irq_data.chip->irq_eoi(&desc->irq_data);
+		return;
+	}
+	/*
+	 * We need to unmask in the following cases:
+	 * - Oneshot irq which did not wake the thread (caused by a
+	 *   spurious interrupt or a primary handler handling it
+	 *   completely).
+	 */
+	if (!irqd_irq_disabled(&desc->irq_data) &&
+	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
+		desc->irq_data.chip->irq_eoi(&desc->irq_data);
+		unmask_irq(desc);
+	}
+}
+
+/**
+ *	handle_fasteoi_late_irq - irq handler for transparent controllers
+ *	@irq:	the interrupt number
+ *	@desc:	the interrupt description structure for this irq
+ *
+ *	Only a single callback will be issued to the chip: an ->eoi()
+ *	call when the interrupt has been serviced. Same as above, but
+ *	we avoid the eoi when the interrupt is masked due to a
+ *	threaded handler. The eoi will be issued right before the unmask.
+ */
+void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc)
+{
+	raw_spin_lock(&desc->lock);
+
+	if (unlikely(irqd_irq_inprogress(&desc->irq_data)))
+		if (!irq_check_poll(desc))
+			goto out;
+
+	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
+	kstat_incr_irqs_this_cpu(irq, desc);
+
+	/*
+	 * If its disabled or no action available
+	 * then mask it and get out of here:
+	 */
+	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
+		desc->istate |= IRQS_PENDING;
+		mask_irq(desc);
+		goto out;
+	}
+
+	if (desc->istate & IRQS_ONESHOT)
+		mask_irq(desc);
+
+	handle_irq_event(desc);
+
+	cond_unmask_and_eoi_irq(desc);
+out:
+	raw_spin_unlock(&desc->lock);
+}
+
 /**
  *	handle_edge_irq - edge type IRQ handler
  *	@irq:	the interrupt number
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 001fa5b..e98bb56 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -73,6 +73,7 @@ extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
 extern void mask_irq(struct irq_desc *desc);
 extern void unmask_irq(struct irq_desc *desc);
+extern void unmask_threaded_irq(struct irq_desc *desc);
 
 extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 481a13c..9147fef 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -718,7 +718,7 @@ again:
 
 	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
 	    irqd_irq_masked(&desc->irq_data))
-		unmask_irq(desc);
+		unmask_threaded_irq(desc);
 
 out_unlock:
 	raw_spin_unlock_irq(&desc->lock);
-- 
1.9.0

Re: [PATCH v2 1/4] irq: Add handle_fasteoi_late_irq irq handler

[Hans de Goede]

Hi,

On 03/12/2014 06:17 PM, Hans de Goede wrote:
> From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> 
> Add an irq handler for transparent controllers
> 
> Only a single callback will be issued to the chip: an ->eoi() call when
> the interrupt has been serviced. Same as handle_fasteoi_irq, but
> we avoid the eoi when the interrupt is masked due to a
> threaded handler. The eoi will be issued right before the unmask.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Oops everything below this was not intended to be part of the commit msg.

Regards,

Hans

>> In some cases we want to do an ack right before the unmask of an irq.
>>
>> A typical example of such a case is having an i2c device which uses a level
>> interrupt. Such devices usually have an interrupt status register with
>> write to clear status bits. This means that the interrupt handler needs to
>> sleep to do i2c transfers to read / write the interrupt status register.
>>
>> This means using a threaded interrupt handler and IRQF_ONESHOT so that the
>> interrupt does not re-trigger while the interrupt handler is waiting for the
>> i2c transfer.
>>
>> Without ack-before-unmask the sequence of a single interrupt in this setup
>> looks like this:
>>
>> i2c-device raises interrupt line
>> interrupt gets triggered
>> interrupt gets masked
>> interrupt gets acked
>> interrupt handling thread starts running
>> interrupt handling thread does its stuff, write-clears irq-status register
>>  in i2c-device
>> i2c-device lowers interrupt line (note this happens after the ack!)
>> interrupt handling thread is done
>> interrupt gets unmasked
>> interrupt immediately retriggers again, because the line has been high
>>  after the ack
>> interrupt gets masked
>> interrupt gets acked
>> interrupt handling thread starts running
>> interrupt handling thread reads irq-status register, has nothing todo
>> interrupt handling thread is done
>> interrupt gets unmasked
>>
>> So in essence we get and handle 2 interrupts for what is 1 interrupt from
>> the i2c-device pov. By doing an ack before the unmask, the second interrupt
>> is avoided since at the point of the unmask the irq-thread has done its
>> job and cleared the interrupt source.
> 
> And how is that different from the non threaded case?
> 
>     mask()
>     ack()	<-- irq line is still active
>     handle()	<-- irq line goes inactive. So this happens
>     		     after the ack as above
>     unmask()
> 
>> Note that things work fine without this patch, the purpose of this patch is
>> soley to avoid the second unneeded interrupt.
>>
>> This patch uses an interrupt flag for this and not an irqchip flag because
>> the need for ack before unmask is based on the device and not on the irqchip.
> 
> No, it's not a device property. Its an irq chip property. There are
> interrupt chips which handle that case fine and making this a device
> property might even break such interrupt chips. e.g. ioapic handles
> that nicely but it would use ack_edge() on a level interrupt when the
> device driver requests that. And the edge and level mode of the ioapic
> are substantially different. Go figure.
> 
> The general policy is that device drivers should be oblivious of the
> underlying interrupt controller implementation as much as possible.
> 
> If the interrupt chip has this behaviour then handle_level_irq as the
> flow handler is the wrong thing to start with because it always acks
> before calling the handler.
> 
> Due to the fact that we run the primary handler with interrupts
> disabled, we should avoid the mask/unmask dance for the non threaded
> case completely. handle_fasteoi_irq does that, except that it does not
> provide the eoi() before unmask in the threaded case and it has an
> extra eoi() even in the masked case which is pointless for interrupt
> chips like the one you are dealing with.
> 
> Untested patch below.
> 
> Thanks,
> 
> 	tglx
> 
> ---------------->
> ---
>  include/linux/irq.h    |  4 +++
>  kernel/irq/chip.c      | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  kernel/irq/internals.h |  1 +
>  kernel/irq/manage.c    |  2 +-
>  4 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 7dc1003..51c199e 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -349,6 +349,8 @@ struct irq_chip {
>   * IRQCHIP_ONOFFLINE_ENABLED:	Only call irq_on/off_line callbacks
>   *				when irq enabled
>   * IRQCHIP_SKIP_SET_WAKE:	Skip chip.irq_set_wake(), for this irq chip
> + * IRQCHIP_ONESHOT_SAFE:	One shot does not require mask/unmask
> + * IRQCHIP_EOI_THREADED:	Chip requires eoi() on unmask in threaded mode
>   */
>  enum {
>  	IRQCHIP_SET_TYPE_MASKED		= (1 <<  0),
> @@ -357,6 +359,7 @@ enum {
>  	IRQCHIP_ONOFFLINE_ENABLED	= (1 <<  3),
>  	IRQCHIP_SKIP_SET_WAKE		= (1 <<  4),
>  	IRQCHIP_ONESHOT_SAFE		= (1 <<  5),
> +	IRQCHIP_EOI_THREADED		= (1 <<  6),
>  };
>  
>  /* This include will go away once we isolated irq_desc usage to core code */
> @@ -412,6 +415,7 @@ static inline int irq_set_parent(int irq, int parent_irq)
>   */
>  extern void handle_level_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc);
> +extern void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_edge_eoi_irq(unsigned int irq, struct irq_desc *desc);
>  extern void handle_simple_irq(unsigned int irq, struct irq_desc *desc);
> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
> index dc04c16..bc3c099 100644
> --- a/kernel/irq/chip.c
> +++ b/kernel/irq/chip.c
> @@ -281,6 +281,19 @@ void unmask_irq(struct irq_desc *desc)
>  	}
>  }
>  
> +void unmask_threaded_irq(struct irq_desc *desc)
> +{
> +	struct irq_chip *chip = desc->irq_data.chip;
> +
> +	if (chip->flags & IRQCHIP_EOI_THREADED)
> +		chip->irq_eoi(&desc->irq_data);
> +
> +	if (chip->irq_unmask) {
> +		chip->irq_unmask(&desc->irq_data);
> +		irq_state_clr_masked(desc);
> +	}
> +}
> +
>  /*
>   *	handle_nested_irq - Handle a nested irq from a irq thread
>   *	@irq:	the interrupt number
> @@ -487,6 +500,66 @@ out:
>  	goto out_unlock;
>  }
>  
> +static void cond_unmask_and_eoi_irq(struct irq_desc *desc)
> +{
> +	if (!(desc->istate & IRQS_ONESHOT)) {
> +		desc->irq_data.chip->irq_eoi(&desc->irq_data);
> +		return;
> +	}
> +	/*
> +	 * We need to unmask in the following cases:
> +	 * - Oneshot irq which did not wake the thread (caused by a
> +	 *   spurious interrupt or a primary handler handling it
> +	 *   completely).
> +	 */
> +	if (!irqd_irq_disabled(&desc->irq_data) &&
> +	    irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
> +		desc->irq_data.chip->irq_eoi(&desc->irq_data);
> +		unmask_irq(desc);
> +	}
> +}
> +
> +/**
> + *	handle_fasteoi_late_irq - irq handler for transparent controllers
> + *	@irq:	the interrupt number
> + *	@desc:	the interrupt description structure for this irq
> + *
> + *	Only a single callback will be issued to the chip: an ->eoi()
> + *	call when the interrupt has been serviced. Same as above, but
> + *	we avoid the eoi when the interrupt is masked due to a
> + *	threaded handler. The eoi will be issued right before the unmask.
> + */
> +void handle_fasteoi_late_irq(unsigned int irq, struct irq_desc *desc)
> +{
> +	raw_spin_lock(&desc->lock);
> +
> +	if (unlikely(irqd_irq_inprogress(&desc->irq_data)))
> +		if (!irq_check_poll(desc))
> +			goto out;
> +
> +	desc->istate &= ~(IRQS_REPLAY | IRQS_WAITING);
> +	kstat_incr_irqs_this_cpu(irq, desc);
> +
> +	/*
> +	 * If its disabled or no action available
> +	 * then mask it and get out of here:
> +	 */
> +	if (unlikely(!desc->action || irqd_irq_disabled(&desc->irq_data))) {
> +		desc->istate |= IRQS_PENDING;
> +		mask_irq(desc);
> +		goto out;
> +	}
> +
> +	if (desc->istate & IRQS_ONESHOT)
> +		mask_irq(desc);
> +
> +	handle_irq_event(desc);
> +
> +	cond_unmask_and_eoi_irq(desc);
> +out:
> +	raw_spin_unlock(&desc->lock);
> +}
> +
>  /**
>   *	handle_edge_irq - edge type IRQ handler
>   *	@irq:	the interrupt number
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index 001fa5b..e98bb56 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -73,6 +73,7 @@ extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
>  extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
>  extern void mask_irq(struct irq_desc *desc);
>  extern void unmask_irq(struct irq_desc *desc);
> +extern void unmask_threaded_irq(struct irq_desc *desc);
>  
>  extern void init_kstat_irqs(struct irq_desc *desc, int node, int nr);
>  
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 481a13c..9147fef 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -718,7 +718,7 @@ again:
>  
>  	if (!desc->threads_oneshot && !irqd_irq_disabled(&desc->irq_data) &&
>  	    irqd_irq_masked(&desc->irq_data))
> -		unmask_irq(desc);
> +		unmask_threaded_irq(desc);
>  
>  out_unlock:
>  	raw_spin_unlock_irq(&desc->lock);
> 

[PATCH v2 2/4] irqchip: sun4i: Fix irq 0 not working

[Hans de Goede]

SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 3 things:
1) no more irqs pending
2) irq 0 pending
3) spurious irq

So if we immediately get a reading of 0, check the irq-pending reg
to differentiate between 2 and 3. We only do this once to avoid
the extra check in the common case of 1) hapening after having
read the vector-reg once.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/irqchip/irq-sun4i.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index a5438d8..5c25048 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -140,10 +140,24 @@ static asmlinkage void __exception_irq_entry sun4i_handle_irq(struct pt_regs *re
 {
 	u32 irq, hwirq;
 
+	/*
+	 * hwirq == 0 can mean one of 3 things:
+	 * 1) no more irqs pending
+	 * 2) irq 0 pending
+	 * 3) spurious irq
+	 * So if we immediately get a reading of 0, check the irq-pending reg
+	 * to differentiate between 2 and 3. We only do this once to avoid
+	 * the extra check in the common case of 1 hapening after having
+	 * read the vector-reg once.
+	 */
 	hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
-	while (hwirq != 0) {
+	if (hwirq == 0 &&
+		  !(readl(sun4i_irq_base + SUN4I_IRQ_PENDING_REG(0)) & BIT(0)))
+		return;
+
+	do {
 		irq = irq_find_mapping(sun4i_irq_domain, hwirq);
 		handle_IRQ(irq, regs);
 		hwirq = readl(sun4i_irq_base + SUN4I_IRQ_VECTOR_REG) >> 2;
-	}
+	} while (hwirq != 0);
 }
-- 
1.9.0

Re: [PATCH v2 2/4] irqchip: sun4i: Fix irq 0 not working

[Maxime Ripard]

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

On Wed, Mar 12, 2014 at 06:17:05PM +0100, Hans de Goede wrote:
> SUN4I_IRQ_VECTOR_REG containing 0 can mean one of 3 things:
> 1) no more irqs pending
> 2) irq 0 pending
> 3) spurious irq
> 
> So if we immediately get a reading of 0, check the irq-pending reg
> to differentiate between 2 and 3. We only do this once to avoid
> the extra check in the common case of 1) hapening after having
> read the vector-reg once.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

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 --]

[PATCH v2 3/4] irqchip: sun4i: Fix a comment about mask register initialization

[Hans de Goede]

The comment was claiming that we were masking all irqs, while the code actually
*un*masks all of them.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Acked-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 drivers/irqchip/irq-sun4i.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index 5c25048..8a2fbee 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -109,7 +109,7 @@ static int __init sun4i_of_init(struct device_node *node,
 	writel(0, sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(1));
 	writel(0, sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(2));
 
-	/* Mask all the interrupts */
+	/* Unmask all the interrupts, ENABLE_REG(x) is used for masking */
 	writel(0, sun4i_irq_base + SUN4I_IRQ_MASK_REG(0));
 	writel(0, sun4i_irq_base + SUN4I_IRQ_MASK_REG(1));
 	writel(0, sun4i_irq_base + SUN4I_IRQ_MASK_REG(2));
-- 
1.9.0

[PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Hans de Goede]

The ENMI needs to have the ack done *after* clearing the interrupt source,
otherwise we will get a spurious interrupt for each real interrupt. Switch
to the new handle_fasteoi_late_irq handler which gives us the desired behavior.

Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/irqchip/irq-sun4i.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index 8a2fbee..4b1c874 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */
 	.irq_mask	= sun4i_irq_mask,
 	.irq_unmask	= sun4i_irq_unmask,
+	.flags		= IRQCHIP_EOI_THREADED, /* Only affects the ENMI */
 };
 
 static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
 			 irq_hw_number_t hw)
 {
-	irq_set_chip_and_handler(virq, &sun4i_irq_chip,
-				 handle_level_irq);
+	if (hw == 0) /* IRQ 0, the ENMI needs special handling */
+		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
+					 handle_fasteoi_late_irq);
+	else
+		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
+					 handle_level_irq);
+
 	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
 
 	return 0;
-- 
1.9.0

Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Maxime Ripard]

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

On Wed, Mar 12, 2014 at 06:17:07PM +0100, Hans de Goede wrote:
> The ENMI needs to have the ack done *after* clearing the interrupt source,
> otherwise we will get a spurious interrupt for each real interrupt. Switch
> to the new handle_fasteoi_late_irq handler which gives us the desired behavior.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/irqchip/irq-sun4i.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> index 8a2fbee..4b1c874 100644
> --- a/drivers/irqchip/irq-sun4i.c
> +++ b/drivers/irqchip/irq-sun4i.c
> @@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */

Hmmm, I wonder if that actually does something.

There's been a patch floating around that I was sure was merged, but
apparently wasn't that remove sun4i_irq_ack, because the register we
were writing to are in read only, and it wasn't doing anything.

>  	.irq_mask	= sun4i_irq_mask,
>  	.irq_unmask	= sun4i_irq_unmask,
> +	.flags		= IRQCHIP_EOI_THREADED, /* Only affects the ENMI */
>  };
>  
>  static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
>  			 irq_hw_number_t hw)
>  {
> -	irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> -				 handle_level_irq);
> +	if (hw == 0) /* IRQ 0, the ENMI needs special handling */
> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> +					 handle_fasteoi_late_irq);
> +	else
> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> +					 handle_level_irq);
> +
>  	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);

Could that solve the other NMI-related issue we have with Carlo's
patches?

-- 
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: Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Carlo Caione]

On Thu, Mar 13, 2014 at 10:34:57AM +0100, Maxime Ripard wrote:
> On Wed, Mar 12, 2014 at 06:17:07PM +0100, Hans de Goede wrote:
> > The ENMI needs to have the ack done *after* clearing the interrupt source,
> > otherwise we will get a spurious interrupt for each real interrupt. Switch
> > to the new handle_fasteoi_late_irq handler which gives us the desired behavior.
> > 
> > Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/irqchip/irq-sun4i.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> > index 8a2fbee..4b1c874 100644
> > --- a/drivers/irqchip/irq-sun4i.c
> > +++ b/drivers/irqchip/irq-sun4i.c
> > @@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */
> 
> Hmmm, I wonder if that actually does something.
> 
> There's been a patch floating around that I was sure was merged, but
> apparently wasn't that remove sun4i_irq_ack, because the register we
> were writing to are in read only, and it wasn't doing anything.
> 
> >  	.irq_mask	= sun4i_irq_mask,
> >  	.irq_unmask	= sun4i_irq_unmask,
> > +	.flags		= IRQCHIP_EOI_THREADED, /* Only affects the ENMI */
> >  };
> >  
> >  static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
> >  			 irq_hw_number_t hw)
> >  {
> > -	irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> > -				 handle_level_irq);
> > +	if (hw == 0) /* IRQ 0, the ENMI needs special handling */
> > +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> > +					 handle_fasteoi_late_irq);
> > +	else
> > +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> > +					 handle_level_irq);
> > +
> >  	set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
> 
> Could that solve the other NMI-related issue we have with Carlo's
> patches?

Yes, I think that the issue is the same. I'll submit a new revision for
the irqchip patchset soon.
The only thing that it is not clear to me is whether the problem is
still there for the NMI controller also for edge-triggered interrupts or
not (hard to verify it experimentally).

Regards,

-- 
Carlo Caione

Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Thomas Gleixner]

On Thu, 13 Mar 2014, Maxime Ripard wrote:

> On Wed, Mar 12, 2014 at 06:17:07PM +0100, Hans de Goede wrote:
> > The ENMI needs to have the ack done *after* clearing the interrupt source,
> > otherwise we will get a spurious interrupt for each real interrupt. Switch
> > to the new handle_fasteoi_late_irq handler which gives us the desired behavior.
> > 
> > Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/irqchip/irq-sun4i.c | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> > index 8a2fbee..4b1c874 100644
> > --- a/drivers/irqchip/irq-sun4i.c
> > +++ b/drivers/irqchip/irq-sun4i.c
> > @@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */
> 
> Hmmm, I wonder if that actually does something.
> 
> There's been a patch floating around that I was sure was merged, but
> apparently wasn't that remove sun4i_irq_ack, because the register we
> were writing to are in read only, and it wasn't doing anything.

Well, it looks like it does something otherwise Hans would not see any
improvement of the situation.

Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Hans de Goede]

Hi,

On 03/13/2014 12:12 PM, Thomas Gleixner wrote:
> On Thu, 13 Mar 2014, Maxime Ripard wrote:
> 
>> On Wed, Mar 12, 2014 at 06:17:07PM +0100, Hans de Goede wrote:
>>> The ENMI needs to have the ack done *after* clearing the interrupt source,
>>> otherwise we will get a spurious interrupt for each real interrupt. Switch
>>> to the new handle_fasteoi_late_irq handler which gives us the desired behavior.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/irqchip/irq-sun4i.c | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
>>> index 8a2fbee..4b1c874 100644
>>> --- a/drivers/irqchip/irq-sun4i.c
>>> +++ b/drivers/irqchip/irq-sun4i.c
>>> @@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */
>>
>> Hmmm, I wonder if that actually does something.
>>
>> There's been a patch floating around that I was sure was merged, but
>> apparently wasn't that remove sun4i_irq_ack, because the register we
>> were writing to are in read only, and it wasn't doing anything.
> 
> Well, it looks like it does something otherwise Hans would not see any
> improvement of the situation.

Right, I think that what Maxime meant is dropping the ack for all but IRQ 0,
and I've just run some tests and the ack indeed seems to be unnecessary for
all the other IRQs on this irqchip.

Regards,

Hans

Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Thomas Gleixner]

On Wed, 12 Mar 2014, Hans de Goede wrote:

> The ENMI needs to have the ack done *after* clearing the interrupt source,
> otherwise we will get a spurious interrupt for each real interrupt. Switch
> to the new handle_fasteoi_late_irq handler which gives us the desired behavior.
> 
> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/irqchip/irq-sun4i.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
> index 8a2fbee..4b1c874 100644
> --- a/drivers/irqchip/irq-sun4i.c
> +++ b/drivers/irqchip/irq-sun4i.c
> @@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */
>  	.irq_mask	= sun4i_irq_mask,
>  	.irq_unmask	= sun4i_irq_unmask,
> +	.flags		= IRQCHIP_EOI_THREADED, /* Only affects the ENMI */

That's not really true. The flags affect all interrupts which share
that chip.

>  };
>  
>  static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
>  			 irq_hw_number_t hw)
>  {
> -	irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> -				 handle_level_irq);
> +	if (hw == 0) /* IRQ 0, the ENMI needs special handling */
> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> +					 handle_fasteoi_late_irq);
> +	else
> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> +					 handle_level_irq);

I wonder what happens when you use the fasteoi handler for all of
them.

Thanks,

	tglx

Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Hans de Goede]

Hi,

On 03/13/2014 03:46 PM, Thomas Gleixner wrote:
> On Wed, 12 Mar 2014, Hans de Goede wrote:
> 
>> The ENMI needs to have the ack done *after* clearing the interrupt source,
>> otherwise we will get a spurious interrupt for each real interrupt. Switch
>> to the new handle_fasteoi_late_irq handler which gives us the desired behavior.
>>
>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/irqchip/irq-sun4i.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
>> index 8a2fbee..4b1c874 100644
>> --- a/drivers/irqchip/irq-sun4i.c
>> +++ b/drivers/irqchip/irq-sun4i.c
>> @@ -77,15 +77,22 @@ 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_eoi	= sun4i_irq_ack, /* For the ENMI */
>>  	.irq_mask	= sun4i_irq_mask,
>>  	.irq_unmask	= sun4i_irq_unmask,
>> +	.flags		= IRQCHIP_EOI_THREADED, /* Only affects the ENMI */
> 
> That's not really true. The flags affect all interrupts which share
> that chip.

Yep, I figured out as much myself too while thinking a bit more about this
this morning.

So what I'm going to do in my next version of this patch is use 2
irqchip structures for the sun4i irqchip, one to describe the special
IRQ 0 and for all the others.

> 
>>  };
>>  
>>  static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
>>  			 irq_hw_number_t hw)
>>  {
>> -	irq_set_chip_and_handler(virq, &sun4i_irq_chip,
>> -				 handle_level_irq);
>> +	if (hw == 0) /* IRQ 0, the ENMI needs special handling */
>> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
>> +					 handle_fasteoi_late_irq);
>> +	else
>> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
>> +					 handle_level_irq);
> 
> I wonder what happens when you use the fasteoi handler for all of
> them.

As mentioned in my previous mail doing an ack (or an eio) seems to
be unnecessary for all but IRQ 0.

I do wonder if handle_level_irq is the right handle*irq function
to use in this case, since this is strictly used in the non smp
case I think that the mask / unmask done by handle_level_irq is
not necessary for non threaded handlers. So what would be the
correct handle*irq function to use in this case ?

Note the irqs are level irqs. IOW they may stay asserted while
the handler runs because of the handler and a new irq raising.

Regards,

Hans

Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)

[Thomas Gleixner]

On Thu, 13 Mar 2014, Hans de Goede wrote:
> On 03/13/2014 03:46 PM, Thomas Gleixner wrote:
> >>  static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
> >>  			 irq_hw_number_t hw)
> >>  {
> >> -	irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> >> -				 handle_level_irq);
> >> +	if (hw == 0) /* IRQ 0, the ENMI needs special handling */
> >> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> >> +					 handle_fasteoi_late_irq);
> >> +	else
> >> +		irq_set_chip_and_handler(virq, &sun4i_irq_chip,
> >> +					 handle_level_irq);
> > 
> > I wonder what happens when you use the fasteoi handler for all of
> > them.
> 
> As mentioned in my previous mail doing an ack (or an eio) seems to
> be unnecessary for all but IRQ 0.
> 
> I do wonder if handle_level_irq is the right handle*irq function
> to use in this case, since this is strictly used in the non smp
> case I think that the mask / unmask done by handle_level_irq is
> not necessary for non threaded handlers. So what would be the
> correct handle*irq function to use in this case ?
> 
> Note the irqs are level irqs. IOW they may stay asserted while
> the handler runs because of the handler and a new irq raising.

Right. You could be creative and use fasteoi plus an empty eoi
callback in the chip for irq 1-N. That way you only mask and unmask in
the threaded case.

Thanks,

	tglx