Fix implementation and users of gicd_set_ipriorityr()

Make gicd_set_ipriorityr() actually write to the priority register.

Also correct callers of this function which want the highest priority
to use the value zero as this is the highest priority value according
to the ARM Generic Interrupt Controller Architecture Specification.

To make this easier to get right, we introduce defines for the lowest
and highest priorities for secure and non-secure interrupts.

Fixes ARM-software/tf-issues#21

Signed-off-by: Jon Medhurst <tixy@linaro.org>

3 files changed, 33 insertions, 22 deletions

diff --git a/arch/system/gic/gic.h b/arch/system/gic/gic.h
index 9e9fb73..3266043 100644
--- a/arch/system/gic/gic.h
+++ b/arch/system/gic/gic.h
@@ -37,7 +37,11 @@
 
 #define GRP0			0
 #define GRP1			1
-#define MAX_PRI_VAL		0xff
+#define GIC_PRI_MASK		0xff
+#define GIC_HIGHEST_SEC_PRIORITY 0
+#define GIC_LOWEST_SEC_PRIORITY	127
+#define GIC_HIGHEST_NS_PRIORITY	128
+#define GIC_LOWEST_NS_PRIORITY	254 /* 255 would disable an interrupt */
 
 #define ENABLE_GRP0		(1 << 0)
 #define ENABLE_GRP1		(1 << 1)
diff --git a/arch/system/gic/gic_v2.c b/arch/system/gic/gic_v2.c
index 220ca19..6af0378 100644
--- a/arch/system/gic/gic_v2.c
+++ b/arch/system/gic/gic_v2.c
@@ -28,6 +28,7 @@
  * POSSIBILITY OF SUCH DAMAGE.
  */
 
+#include <assert.h>
 #include <gic.h>
 #include <mmio.h>
 
@@ -264,20 +265,26 @@ void gicd_set_icactiver(unsigned int base, unsigned int id)
  */
 void gicd_set_ipriorityr(unsigned int base, unsigned int id, unsigned int pri)
 {
-	unsigned byte_off = id & ((1 << ICACTIVER_SHIFT) - 1);
-	unsigned int reg_val = gicd_read_icactiver(base, id);
+	unsigned int reg = base + GICD_IPRIORITYR + (id & ~3);
+	unsigned int shift = (id & 3) << 3;
+	unsigned int reg_val = mmio_read_32(reg);
 
 	/*
 	 * Enforce ARM recommendation to manage priority values such
 	 * that group1 interrupts always have a lower priority than
-	 * group0 interrupts
+	 * group0 interrupts.
+	 * Note, lower numerical values are higher priorities so the comparison
+	 * checks below are reversed from what might be expected.
 	 */
-	if (gicd_get_igroupr(base, id) == GRP1)
-		pri |= 1 << 7;
-	else
-		pri &= ~(1 << 7);
-
-	gicd_write_icactiver(base, id, reg_val & ~(pri << (byte_off << 3)));
+	assert(gicd_get_igroupr(base, id) == GRP1 ?
+		pri >= GIC_HIGHEST_NS_PRIORITY &&
+			pri <= GIC_LOWEST_NS_PRIORITY :
+		pri >= GIC_HIGHEST_SEC_PRIORITY &&
+			pri <= GIC_LOWEST_SEC_PRIORITY);
+
+	reg_val &= ~(GIC_PRI_MASK << shift);
+	reg_val |= (pri & GIC_PRI_MASK) << shift;
+	mmio_write_32(reg, reg_val);
 }
 
 void gicd_set_itargetsr(unsigned int base, unsigned int id, unsigned int iface)
diff --git a/plat/fvp/plat_gic.c b/plat/fvp/plat_gic.c
index 6dd13ec..8929172 100644
--- a/plat/fvp/plat_gic.c
+++ b/plat/fvp/plat_gic.c
@@ -101,7 +101,7 @@ void gicv3_cpuif_setup(void)
 	val = read_icc_sre_el2();
 	write_icc_sre_el2(val | ICC_SRE_EN | ICC_SRE_SRE);
 
-	write_icc_pmr_el1(MAX_PRI_VAL);
+	write_icc_pmr_el1(GIC_PRI_MASK);
 
 	/* Restore SCR_EL3 */
 	write_scr(scr_val);
@@ -166,7 +166,7 @@ void gic_cpuif_setup(unsigned int gicc_base)
 	val = ENABLE_GRP0 | FIQ_EN | FIQ_BYP_DIS_GRP0;
 	val |= IRQ_BYP_DIS_GRP0 | FIQ_BYP_DIS_GRP1 | IRQ_BYP_DIS_GRP1;
 
-	gicc_write_pmr(gicc_base, MAX_PRI_VAL);
+	gicc_write_pmr(gicc_base, GIC_PRI_MASK);
 	gicc_write_ctlr(gicc_base, val);
 }
 
@@ -214,15 +214,15 @@ void gic_pcpu_distif_setup(unsigned int gicd_base)
 	gicd_clr_igroupr(gicd_base, IRQ_SEC_SGI_6);
 	gicd_clr_igroupr(gicd_base, IRQ_SEC_SGI_7);
 
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_PHY_TIMER, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_0, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_1, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_2, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_3, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_4, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_5, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_6, MAX_PRI_VAL);
-	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_7, MAX_PRI_VAL);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_PHY_TIMER, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_0, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_1, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_2, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_3, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_4, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_5, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_6, GIC_HIGHEST_SEC_PRIORITY);
+	gicd_set_ipriorityr(gicd_base, IRQ_SEC_SGI_7, GIC_HIGHEST_SEC_PRIORITY);
 
 	gicd_set_isenabler(gicd_base, IRQ_SEC_PHY_TIMER);
 	gicd_set_isenabler(gicd_base, IRQ_SEC_SGI_0);
@@ -261,7 +261,7 @@ void gic_distif_setup(unsigned int gicd_base)
 
 	/* Configure secure interrupts now */
 	gicd_clr_igroupr(gicd_base, IRQ_TZ_WDOG);
-	gicd_set_ipriorityr(gicd_base, IRQ_TZ_WDOG, MAX_PRI_VAL);
+	gicd_set_ipriorityr(gicd_base, IRQ_TZ_WDOG, GIC_HIGHEST_SEC_PRIORITY);
 	gicd_set_itargetsr(gicd_base, IRQ_TZ_WDOG,
 			   platform_get_core_pos(read_mpidr()));
 	gicd_set_isenabler(gicd_base, IRQ_TZ_WDOG);