[PATCH] MIPS: cevt-r4k: Use Cause.TI for timer pending

[PATCH] MIPS: cevt-r4k: Use Cause.TI for timer pending

[James Hogan]

The cevt-r4k driver used to call into the GIC driver to find whether the
timer was pending, but only with External Interrupt Controller (EIC)
mode, where the Cause.IP bits can't be used as they encode the interrupt
priority level (Cause.RIPL) instead.

However commit e9de688dac65 ("irqchip: mips-gic: Support local
interrupts") changed the condition from cpu_has_veic to gic_present.
This fails on cores such as P5600 which have a GIC but the local
interrupts aren't routable by the GIC, causing c0_compare_int_usable()
to consider the interrupt unusable so r4k_clockevent_init() fails.

The previous behaviour wasn't really correct either though since P5600
apparently supports EIC mode too, so lets use the Cause.TI bit instead
which should be present since release 2 of the MIPS32/MIPS64
architecture. In fact multiple interrupts can be routed to that same CPU
interrupt line (e.g. performance counter and fast debug channel
interrupts), so lets use Cause.TI in preference to Cause.IP on all cores
since release 2.

Fixes: e9de688dac65 ("irqchip: mips-gic: Support local interrupts")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Andrew Bresticker <abrestic@chromium.org>
Cc: Qais Yousef <qais.yousef@imgtec.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mips@linux-mips.org
---
 arch/mips/kernel/cevt-r4k.c      | 20 +++++++++++++++-----
 drivers/irqchip/irq-mips-gic.c   |  8 --------
 include/linux/irqchip/mips-gic.h |  1 -
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
index 6acaad0480af..6747a2cbf662 100644
--- a/arch/mips/kernel/cevt-r4k.c
+++ b/arch/mips/kernel/cevt-r4k.c
@@ -11,7 +11,6 @@
 #include <linux/percpu.h>
 #include <linux/smp.h>
 #include <linux/irq.h>
-#include <linux/irqchip/mips-gic.h>
 
 #include <asm/time.h>
 #include <asm/cevt-r4k.h>
@@ -85,10 +84,21 @@ void mips_event_handler(struct clock_event_device *dev)
  */
 static int c0_compare_int_pending(void)
 {
-#ifdef CONFIG_MIPS_GIC
-	if (gic_present)
-		return gic_get_timer_pending();
-#endif
+	/*
+	 * With External Interrupt Controller (EIC) mode (which may be present
+	 * since release 2 of MIPS32/MIPS64) the interrupt pending bits
+	 * (Cause.IP) are used for the interrupt priority level (Cause.RIPL) so
+	 * we can't use it to determine whether the timer interrupt is pending.
+	 *
+	 * Instead lets use the timer pending bit (Cause.TI) which is present
+	 * since release 2, and lets use it even without EIC mode since it
+	 * unambigously specifies whether the timer interrupt is pending
+	 * regardless of what other internal or external interrupts are wired to
+	 * the same CPU IRQ line.
+	 */
+	if (cpu_has_mips_r2)
+		return read_c0_cause() & CAUSEF_TI;
+
 	return (read_c0_cause() >> cp0_compare_irq_shift) & (1ul << CAUSEB_IP);
 }
 
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index 2b0468e3df6a..e58600b1de28 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -191,14 +191,6 @@ static bool gic_local_irq_is_routable(int intr)
 	}
 }
 
-unsigned int gic_get_timer_pending(void)
-{
-	unsigned int vpe_pending;
-
-	vpe_pending = gic_read(GIC_REG(VPE_LOCAL, GIC_VPE_PEND));
-	return vpe_pending & GIC_VPE_PEND_TIMER_MSK;
-}
-
 static void gic_bind_eic_interrupt(int irq, int set)
 {
 	/* Convert irq vector # to hw int # */
diff --git a/include/linux/irqchip/mips-gic.h b/include/linux/irqchip/mips-gic.h
index 420f77b34d02..e6a6aac451db 100644
--- a/include/linux/irqchip/mips-gic.h
+++ b/include/linux/irqchip/mips-gic.h
@@ -243,7 +243,6 @@ extern void gic_write_cpu_compare(cycle_t cnt, int cpu);
 extern void gic_send_ipi(unsigned int intr);
 extern unsigned int plat_ipi_call_int_xlate(unsigned int);
 extern unsigned int plat_ipi_resched_int_xlate(unsigned int);
-extern unsigned int gic_get_timer_pending(void);
 extern int gic_get_c0_compare_int(void);
 extern int gic_get_c0_perfcount_int(void);
 #endif /* __LINUX_IRQCHIP_MIPS_GIC_H */
-- 
2.0.5

[PATCH] MIPS: cevt-r4k: Use Cause.TI for timer pending

[James Hogan]

The cevt-r4k driver used to call into the GIC driver to find whether the
timer was pending, but only with External Interrupt Controller (EIC)
mode, where the Cause.IP bits can't be used as they encode the interrupt
priority level (Cause.RIPL) instead.

However commit e9de688dac65 ("irqchip: mips-gic: Support local
interrupts") changed the condition from cpu_has_veic to gic_present.
This fails on cores such as P5600 which have a GIC but the local
interrupts aren't routable by the GIC, causing c0_compare_int_usable()
to consider the interrupt unusable so r4k_clockevent_init() fails.

The previous behaviour wasn't really correct either though since P5600
apparently supports EIC mode too, so lets use the Cause.TI bit instead
which should be present since release 2 of the MIPS32/MIPS64
architecture. In fact multiple interrupts can be routed to that same CPU
interrupt line (e.g. performance counter and fast debug channel
interrupts), so lets use Cause.TI in preference to Cause.IP on all cores
since release 2.

Fixes: e9de688dac65 ("irqchip: mips-gic: Support local interrupts")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Andrew Bresticker <abrestic@chromium.org>
Cc: Qais Yousef <qais.yousef@imgtec.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mips@linux-mips.org
---
 arch/mips/kernel/cevt-r4k.c      | 20 +++++++++++++++-----
 drivers/irqchip/irq-mips-gic.c   |  8 --------
 include/linux/irqchip/mips-gic.h |  1 -
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
index 6acaad0480af..6747a2cbf662 100644
--- a/arch/mips/kernel/cevt-r4k.c
+++ b/arch/mips/kernel/cevt-r4k.c
@@ -11,7 +11,6 @@
 #include <linux/percpu.h>
 #include <linux/smp.h>
 #include <linux/irq.h>
-#include <linux/irqchip/mips-gic.h>
 
 #include <asm/time.h>
 #include <asm/cevt-r4k.h>
@@ -85,10 +84,21 @@ void mips_event_handler(struct clock_event_device *dev)
  */
 static int c0_compare_int_pending(void)
 {
-#ifdef CONFIG_MIPS_GIC
-	if (gic_present)
-		return gic_get_timer_pending();
-#endif
+	/*
+	 * With External Interrupt Controller (EIC) mode (which may be present
+	 * since release 2 of MIPS32/MIPS64) the interrupt pending bits
+	 * (Cause.IP) are used for the interrupt priority level (Cause.RIPL) so
+	 * we can't use it to determine whether the timer interrupt is pending.
+	 *
+	 * Instead lets use the timer pending bit (Cause.TI) which is present
+	 * since release 2, and lets use it even without EIC mode since it
+	 * unambigously specifies whether the timer interrupt is pending
+	 * regardless of what other internal or external interrupts are wired to
+	 * the same CPU IRQ line.
+	 */
+	if (cpu_has_mips_r2)
+		return read_c0_cause() & CAUSEF_TI;
+
 	return (read_c0_cause() >> cp0_compare_irq_shift) & (1ul << CAUSEB_IP);
 }
 
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index 2b0468e3df6a..e58600b1de28 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -191,14 +191,6 @@ static bool gic_local_irq_is_routable(int intr)
 	}
 }
 
-unsigned int gic_get_timer_pending(void)
-{
-	unsigned int vpe_pending;
-
-	vpe_pending = gic_read(GIC_REG(VPE_LOCAL, GIC_VPE_PEND));
-	return vpe_pending & GIC_VPE_PEND_TIMER_MSK;
-}
-
 static void gic_bind_eic_interrupt(int irq, int set)
 {
 	/* Convert irq vector # to hw int # */
diff --git a/include/linux/irqchip/mips-gic.h b/include/linux/irqchip/mips-gic.h
index 420f77b34d02..e6a6aac451db 100644
--- a/include/linux/irqchip/mips-gic.h
+++ b/include/linux/irqchip/mips-gic.h
@@ -243,7 +243,6 @@ extern void gic_write_cpu_compare(cycle_t cnt, int cpu);
 extern void gic_send_ipi(unsigned int intr);
 extern unsigned int plat_ipi_call_int_xlate(unsigned int);
 extern unsigned int plat_ipi_resched_int_xlate(unsigned int);
-extern unsigned int gic_get_timer_pending(void);
 extern int gic_get_c0_compare_int(void);
 extern int gic_get_c0_perfcount_int(void);
 #endif /* __LINUX_IRQCHIP_MIPS_GIC_H */
-- 
2.0.5

Re: [PATCH] MIPS: cevt-r4k: Use Cause.TI for timer pending

[Andrew Bresticker]

On Thu, Jan 8, 2015 at 7:06 AM, James Hogan <james.hogan@imgtec.com> wrote:
> The cevt-r4k driver used to call into the GIC driver to find whether the
> timer was pending, but only with External Interrupt Controller (EIC)
> mode, where the Cause.IP bits can't be used as they encode the interrupt
> priority level (Cause.RIPL) instead.
>
> However commit e9de688dac65 ("irqchip: mips-gic: Support local
> interrupts") changed the condition from cpu_has_veic to gic_present.
> This fails on cores such as P5600 which have a GIC but the local
> interrupts aren't routable by the GIC, causing c0_compare_int_usable()
> to consider the interrupt unusable so r4k_clockevent_init() fails.
>
> The previous behaviour wasn't really correct either though since P5600
> apparently supports EIC mode too, so lets use the Cause.TI bit instead
> which should be present since release 2 of the MIPS32/MIPS64
> architecture. In fact multiple interrupts can be routed to that same CPU
> interrupt line (e.g. performance counter and fast debug channel
> interrupts), so lets use Cause.TI in preference to Cause.IP on all cores
> since release 2.
>
> Fixes: e9de688dac65 ("irqchip: mips-gic: Support local interrupts")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Andrew Bresticker <abrestic@chromium.org>
> Cc: Qais Yousef <qais.yousef@imgtec.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-mips@linux-mips.org

Thanks for catching this!

Reviewed-by: Andrew Bresticker <abrestic@chromium.org>

Re: [PATCH] MIPS: cevt-r4k: Use Cause.TI for timer pending

[James Hogan]

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

On 08/01/15 15:06, James Hogan wrote:
> The cevt-r4k driver used to call into the GIC driver to find whether the
> timer was pending, but only with External Interrupt Controller (EIC)
> mode, where the Cause.IP bits can't be used as they encode the interrupt
> priority level (Cause.RIPL) instead.
> 
> However commit e9de688dac65 ("irqchip: mips-gic: Support local
> interrupts") changed the condition from cpu_has_veic to gic_present.
> This fails on cores such as P5600 which have a GIC but the local
> interrupts aren't routable by the GIC, causing c0_compare_int_usable()
> to consider the interrupt unusable so r4k_clockevent_init() fails.
> 
> The previous behaviour wasn't really correct either though since P5600
> apparently supports EIC mode too, so lets use the Cause.TI bit instead
> which should be present since release 2 of the MIPS32/MIPS64
> architecture. In fact multiple interrupts can be routed to that same CPU
> interrupt line (e.g. performance counter and fast debug channel
> interrupts), so lets use Cause.TI in preference to Cause.IP on all cores
> since release 2.
> 
> Fixes: e9de688dac65 ("irqchip: mips-gic: Support local interrupts")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Andrew Bresticker <abrestic@chromium.org>
> Cc: Qais Yousef <qais.yousef@imgtec.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-mips@linux-mips.org
> ---
>  arch/mips/kernel/cevt-r4k.c      | 20 +++++++++++++++-----
>  drivers/irqchip/irq-mips-gic.c   |  8 --------
>  include/linux/irqchip/mips-gic.h |  1 -
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
> index 6acaad0480af..6747a2cbf662 100644
> --- a/arch/mips/kernel/cevt-r4k.c
> +++ b/arch/mips/kernel/cevt-r4k.c
> @@ -11,7 +11,6 @@
>  #include <linux/percpu.h>
>  #include <linux/smp.h>
>  #include <linux/irq.h>
> -#include <linux/irqchip/mips-gic.h>
>  
>  #include <asm/time.h>
>  #include <asm/cevt-r4k.h>
> @@ -85,10 +84,21 @@ void mips_event_handler(struct clock_event_device *dev)
>   */
>  static int c0_compare_int_pending(void)
>  {
> -#ifdef CONFIG_MIPS_GIC
> -	if (gic_present)
> -		return gic_get_timer_pending();
> -#endif
> +	/*
> +	 * With External Interrupt Controller (EIC) mode (which may be present
> +	 * since release 2 of MIPS32/MIPS64) the interrupt pending bits
> +	 * (Cause.IP) are used for the interrupt priority level (Cause.RIPL) so
> +	 * we can't use it to determine whether the timer interrupt is pending.
> +	 *
> +	 * Instead lets use the timer pending bit (Cause.TI) which is present
> +	 * since release 2, and lets use it even without EIC mode since it
> +	 * unambigously specifies whether the timer interrupt is pending
> +	 * regardless of what other internal or external interrupts are wired to
> +	 * the same CPU IRQ line.
> +	 */
> +	if (cpu_has_mips_r2)
> +		return read_c0_cause() & CAUSEF_TI;

Hmm, just realised that the code below already handles this case
equivalently thanks to per_cpu_trap_init doing the slightly sneaky:
if (cpu_has_mips_r2)
	cp0_compare_irq_shift = CAUSEB_TI - CAUSEB_IP;

I'll drop the cpu_has_mips_r2 conditional here, add a comment, and repost.

Cheers
James

> +
>  	return (read_c0_cause() >> cp0_compare_irq_shift) & (1ul << CAUSEB_IP);
>  }
>  
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 2b0468e3df6a..e58600b1de28 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -191,14 +191,6 @@ static bool gic_local_irq_is_routable(int intr)
>  	}
>  }
>  
> -unsigned int gic_get_timer_pending(void)
> -{
> -	unsigned int vpe_pending;
> -
> -	vpe_pending = gic_read(GIC_REG(VPE_LOCAL, GIC_VPE_PEND));
> -	return vpe_pending & GIC_VPE_PEND_TIMER_MSK;
> -}
> -
>  static void gic_bind_eic_interrupt(int irq, int set)
>  {
>  	/* Convert irq vector # to hw int # */
> diff --git a/include/linux/irqchip/mips-gic.h b/include/linux/irqchip/mips-gic.h
> index 420f77b34d02..e6a6aac451db 100644
> --- a/include/linux/irqchip/mips-gic.h
> +++ b/include/linux/irqchip/mips-gic.h
> @@ -243,7 +243,6 @@ extern void gic_write_cpu_compare(cycle_t cnt, int cpu);
>  extern void gic_send_ipi(unsigned int intr);
>  extern unsigned int plat_ipi_call_int_xlate(unsigned int);
>  extern unsigned int plat_ipi_resched_int_xlate(unsigned int);
> -extern unsigned int gic_get_timer_pending(void);
>  extern int gic_get_c0_compare_int(void);
>  extern int gic_get_c0_perfcount_int(void);
>  #endif /* __LINUX_IRQCHIP_MIPS_GIC_H */
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] MIPS: cevt-r4k: Use Cause.TI for timer pending

[James Hogan]

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

On 08/01/15 15:06, James Hogan wrote:
> The cevt-r4k driver used to call into the GIC driver to find whether the
> timer was pending, but only with External Interrupt Controller (EIC)
> mode, where the Cause.IP bits can't be used as they encode the interrupt
> priority level (Cause.RIPL) instead.
> 
> However commit e9de688dac65 ("irqchip: mips-gic: Support local
> interrupts") changed the condition from cpu_has_veic to gic_present.
> This fails on cores such as P5600 which have a GIC but the local
> interrupts aren't routable by the GIC, causing c0_compare_int_usable()
> to consider the interrupt unusable so r4k_clockevent_init() fails.
> 
> The previous behaviour wasn't really correct either though since P5600
> apparently supports EIC mode too, so lets use the Cause.TI bit instead
> which should be present since release 2 of the MIPS32/MIPS64
> architecture. In fact multiple interrupts can be routed to that same CPU
> interrupt line (e.g. performance counter and fast debug channel
> interrupts), so lets use Cause.TI in preference to Cause.IP on all cores
> since release 2.
> 
> Fixes: e9de688dac65 ("irqchip: mips-gic: Support local interrupts")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Andrew Bresticker <abrestic@chromium.org>
> Cc: Qais Yousef <qais.yousef@imgtec.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-mips@linux-mips.org
> ---
>  arch/mips/kernel/cevt-r4k.c      | 20 +++++++++++++++-----
>  drivers/irqchip/irq-mips-gic.c   |  8 --------
>  include/linux/irqchip/mips-gic.h |  1 -
>  3 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/mips/kernel/cevt-r4k.c b/arch/mips/kernel/cevt-r4k.c
> index 6acaad0480af..6747a2cbf662 100644
> --- a/arch/mips/kernel/cevt-r4k.c
> +++ b/arch/mips/kernel/cevt-r4k.c
> @@ -11,7 +11,6 @@
>  #include <linux/percpu.h>
>  #include <linux/smp.h>
>  #include <linux/irq.h>
> -#include <linux/irqchip/mips-gic.h>
>  
>  #include <asm/time.h>
>  #include <asm/cevt-r4k.h>
> @@ -85,10 +84,21 @@ void mips_event_handler(struct clock_event_device *dev)
>   */
>  static int c0_compare_int_pending(void)
>  {
> -#ifdef CONFIG_MIPS_GIC
> -	if (gic_present)
> -		return gic_get_timer_pending();
> -#endif
> +	/*
> +	 * With External Interrupt Controller (EIC) mode (which may be present
> +	 * since release 2 of MIPS32/MIPS64) the interrupt pending bits
> +	 * (Cause.IP) are used for the interrupt priority level (Cause.RIPL) so
> +	 * we can't use it to determine whether the timer interrupt is pending.
> +	 *
> +	 * Instead lets use the timer pending bit (Cause.TI) which is present
> +	 * since release 2, and lets use it even without EIC mode since it
> +	 * unambigously specifies whether the timer interrupt is pending
> +	 * regardless of what other internal or external interrupts are wired to
> +	 * the same CPU IRQ line.
> +	 */
> +	if (cpu_has_mips_r2)
> +		return read_c0_cause() & CAUSEF_TI;

Hmm, just realised that the code below already handles this case
equivalently thanks to per_cpu_trap_init doing the slightly sneaky:
if (cpu_has_mips_r2)
	cp0_compare_irq_shift = CAUSEB_TI - CAUSEB_IP;

I'll drop the cpu_has_mips_r2 conditional here, add a comment, and repost.

Cheers
James

> +
>  	return (read_c0_cause() >> cp0_compare_irq_shift) & (1ul << CAUSEB_IP);
>  }
>  
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 2b0468e3df6a..e58600b1de28 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -191,14 +191,6 @@ static bool gic_local_irq_is_routable(int intr)
>  	}
>  }
>  
> -unsigned int gic_get_timer_pending(void)
> -{
> -	unsigned int vpe_pending;
> -
> -	vpe_pending = gic_read(GIC_REG(VPE_LOCAL, GIC_VPE_PEND));
> -	return vpe_pending & GIC_VPE_PEND_TIMER_MSK;
> -}
> -
>  static void gic_bind_eic_interrupt(int irq, int set)
>  {
>  	/* Convert irq vector # to hw int # */
> diff --git a/include/linux/irqchip/mips-gic.h b/include/linux/irqchip/mips-gic.h
> index 420f77b34d02..e6a6aac451db 100644
> --- a/include/linux/irqchip/mips-gic.h
> +++ b/include/linux/irqchip/mips-gic.h
> @@ -243,7 +243,6 @@ extern void gic_write_cpu_compare(cycle_t cnt, int cpu);
>  extern void gic_send_ipi(unsigned int intr);
>  extern unsigned int plat_ipi_call_int_xlate(unsigned int);
>  extern unsigned int plat_ipi_resched_int_xlate(unsigned int);
> -extern unsigned int gic_get_timer_pending(void);
>  extern int gic_get_c0_compare_int(void);
>  extern int gic_get_c0_perfcount_int(void);
>  #endif /* __LINUX_IRQCHIP_MIPS_GIC_H */
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]