[PATCH v2 0/2] irqchip: gic: Add support for GIC v2 bypass disable

[PATCH v2 0/2] irqchip: gic: Add support for GIC v2 bypass disable

[Feng Kan]

This patch series cleans up hex number in the gic driver and then adds
the code to preserve GIC v2 bypass disable bits in the GIC driver.

V2 Change:
	seem my send email was not working correctly, resending this with
        rebase pull.
	- had to pull HaoJian's change out of arm-gic.h to keep consistency.
	- replace GIC defines as noted by Marc
	- remove GIC_CPU_DISABLE since it no longer used.
	- fix gic_cpu_if_down as noted by Marc

PM path gic cpu ctrl register restore is not added here, I will have to 
test that seperately once all the PM code is fully in place and submit it
afterward.

Feng Kan (2):
  irqchip: gic: replace hex numbers with defines.
  irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

 drivers/irqchip/irq-gic.c | 82 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 62 insertions(+), 20 deletions(-)

-- 
1.9.1

[PATCH V2 1/2] irqchip: gic: replace hex numbers with defines.

[Feng Kan]

This is to cleanup some hex numbers used in the code and replace
then with defines to make the code cleaner.

Signed-off-by: Feng Kan <fkan@apm.com>
Reviewed-by: Anup Patel <apatel@apm.com>
---
 drivers/irqchip/irq-gic.c       | 62 ++++++++++++++++++++++++++++-------------
 include/linux/irqchip/arm-gic.h |  2 --
 2 files changed, 43 insertions(+), 21 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7e11c9d..9ec3f4c 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -48,6 +48,23 @@
 
 #include "irqchip.h"
 
+#define GIC_DIST_ENABLE			0x1
+#define GIC_DIST_DISABLE		0x0
+#define GIC_DIST_INT_CLR_EN_32		0xffffffff
+#define GIC_DIST_INT_SET_EN_SGI		0x0000ffff
+#define GIC_DIST_INT_CLR_EN_PPI		0xffff0000
+#define GIC_DIST_INT_LVL_TRIGGER	0x0
+#define GIC_DIST_INT_DEF_PRI		0xa0
+#define GIC_DIST_INT_DEF_PRI_4		((GIC_DIST_INT_DEF_PRI << 24) |\
+					(GIC_DIST_INT_DEF_PRI << 16) |\
+					(GIC_DIST_INT_DEF_PRI << 8) |\
+					GIC_DIST_INT_DEF_PRI)
+
+#define GIC_CPU_ENABLE			0x1
+#define GIC_CPU_INT_PRI_THRESHOLD	0xf0
+#define GIC_CPU_INT_SPURIOUS		1023
+#define GIC_CPU_INT_ID_MASK		0x3ff
+
 union gic_base {
 	void __iomem *common_base;
 	void __percpu * __iomem *percpu_base;
@@ -291,7 +308,7 @@ static void __exception_irq_entry gic_handle_irq(struct pt_regs *regs)
 
 	do {
 		irqstat = readl_relaxed(cpu_base + GIC_CPU_INTACK);
-		irqnr = irqstat & GICC_IAR_INT_ID_MASK;
+		irqnr = irqstat & GIC_CPU_INT_ID_MASK;
 
 		if (likely(irqnr > 15 && irqnr < 1021)) {
 			irqnr = irq_find_mapping(gic->domain, irqnr);
@@ -322,8 +339,8 @@ static void gic_handle_cascade_irq(unsigned int irq, struct irq_desc *desc)
 	status = readl_relaxed(gic_data_cpu_base(chip_data) + GIC_CPU_INTACK);
 	raw_spin_unlock(&irq_controller_lock);
 
-	gic_irq = (status & 0x3ff);
-	if (gic_irq == 1023)
+	gic_irq = (status & GIC_CPU_INT_ID_MASK);
+	if (gic_irq == GIC_CPU_INT_SPURIOUS)
 		goto out;
 
 	cascade_irq = irq_find_mapping(chip_data->domain, gic_irq);
@@ -384,13 +401,14 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	unsigned int gic_irqs = gic->gic_irqs;
 	void __iomem *base = gic_data_dist_base(gic);
 
-	writel_relaxed(0, base + GIC_DIST_CTRL);
+	writel_relaxed(GIC_DIST_DISABLE, base + GIC_DIST_CTRL);
 
 	/*
 	 * Set all global interrupts to be level triggered, active low.
 	 */
 	for (i = 32; i < gic_irqs; i += 16)
-		writel_relaxed(0, base + GIC_DIST_CONFIG + i * 4 / 16);
+		writel_relaxed(GIC_DIST_INT_LVL_TRIGGER,
+				base + GIC_DIST_CONFIG + i * 4 / 16);
 
 	/*
 	 * Set all global interrupts to this CPU only.
@@ -405,16 +423,18 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	 * Set priority on all global interrupts.
 	 */
 	for (i = 32; i < gic_irqs; i += 4)
-		writel_relaxed(0xa0a0a0a0, base + GIC_DIST_PRI + i * 4 / 4);
+		writel_relaxed(GIC_DIST_INT_DEF_PRI_4,
+					base + GIC_DIST_PRI + i * 4 / 4);
 
 	/*
 	 * Disable all interrupts.  Leave the PPI and SGIs alone
 	 * as these enables are banked registers.
 	 */
 	for (i = 32; i < gic_irqs; i += 32)
-		writel_relaxed(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
+		writel_relaxed(GIC_DIST_INT_CLR_EN_32,
+				base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
 
-	writel_relaxed(1, base + GIC_DIST_CTRL);
+	writel_relaxed(GIC_DIST_ENABLE, base + GIC_DIST_CTRL);
 }
 
 static void gic_cpu_init(struct gic_chip_data *gic)
@@ -443,17 +463,20 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 	 * Deal with the banked PPI and SGI interrupts - disable all
 	 * PPI interrupts, ensure all SGI interrupts are enabled.
 	 */
-	writel_relaxed(0xffff0000, dist_base + GIC_DIST_ENABLE_CLEAR);
-	writel_relaxed(0x0000ffff, dist_base + GIC_DIST_ENABLE_SET);
+	writel_relaxed(GIC_DIST_INT_CLR_EN_PPI,
+			dist_base + GIC_DIST_ENABLE_CLEAR);
+	writel_relaxed(GIC_DIST_INT_SET_EN_SGI,
+			dist_base + GIC_DIST_ENABLE_SET);
 
 	/*
 	 * Set priority on PPI and SGI interrupts
 	 */
 	for (i = 0; i < 32; i += 4)
-		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4);
+		writel_relaxed(GIC_DIST_INT_DEF_PRI_4,
+				dist_base + GIC_DIST_PRI + i * 4 / 4);
 
-	writel_relaxed(0xf0, base + GIC_CPU_PRIMASK);
-	writel_relaxed(1, base + GIC_CPU_CTRL);
+	writel_relaxed(GIC_CPU_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
+	writel_relaxed(GIC_CPU_ENABLE, base + GIC_CPU_CTRL);
 }
 
 void gic_cpu_if_down(void)
@@ -519,14 +542,14 @@ static void gic_dist_restore(unsigned int gic_nr)
 	if (!dist_base)
 		return;
 
-	writel_relaxed(0, dist_base + GIC_DIST_CTRL);
+	writel_relaxed(GIC_DIST_DISABLE, dist_base + GIC_DIST_CTRL);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
 		writel_relaxed(gic_data[gic_nr].saved_spi_conf[i],
 			dist_base + GIC_DIST_CONFIG + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
-		writel_relaxed(0xa0a0a0a0,
+		writel_relaxed(GIC_DIST_INT_DEF_PRI_4,
 			dist_base + GIC_DIST_PRI + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 4); i++)
@@ -537,7 +560,7 @@ static void gic_dist_restore(unsigned int gic_nr)
 		writel_relaxed(gic_data[gic_nr].saved_spi_enable[i],
 			dist_base + GIC_DIST_ENABLE_SET + i * 4);
 
-	writel_relaxed(1, dist_base + GIC_DIST_CTRL);
+	writel_relaxed(GIC_DIST_ENABLE, dist_base + GIC_DIST_CTRL);
 }
 
 static void gic_cpu_save(unsigned int gic_nr)
@@ -591,10 +614,11 @@ static void gic_cpu_restore(unsigned int gic_nr)
 		writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);
 
 	for (i = 0; i < DIV_ROUND_UP(32, 4); i++)
-		writel_relaxed(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4);
+		writel_relaxed(GIC_DIST_INT_DEF_PRI_4,
+					dist_base + GIC_DIST_PRI + i * 4);
 
-	writel_relaxed(0xf0, cpu_base + GIC_CPU_PRIMASK);
-	writel_relaxed(1, cpu_base + GIC_CPU_CTRL);
+	writel_relaxed(GIC_CPU_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
+	writel_relaxed(GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
 }
 
 static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h
index 45e2d8c..7ed92d0 100644
--- a/include/linux/irqchip/arm-gic.h
+++ b/include/linux/irqchip/arm-gic.h
@@ -21,8 +21,6 @@
 #define GIC_CPU_ACTIVEPRIO		0xd0
 #define GIC_CPU_IDENT			0xfc
 
-#define GICC_IAR_INT_ID_MASK		0x3ff
-
 #define GIC_DIST_CTRL			0x000
 #define GIC_DIST_CTR			0x004
 #define GIC_DIST_IGROUP			0x080
-- 
1.9.1

[PATCH V2 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Feng Kan]

This change is made to preserve the GIC v2 bypass bits in the
GIC_CPU_CTRL register (also known as the GICC_CTLR register in spec).
This code will preserve all bits configured by the bootloader regarding
v2 bypass group bits. In the X-Gene platform, the bypass functionality
is not used and bypass bits should not be changed by the kernel gic
code as it could lead to incorrect behavior.

Signed-off-by: Vinayak Kale <vkale@apm.com>
Signed-off-by: Feng Kan <fkan@apm.com>
Reviewed-by: Anup Patel <apatel@apm.com>
---
 drivers/irqchip/irq-gic.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 9ec3f4c..e70ef38 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -64,6 +64,7 @@
 #define GIC_CPU_INT_PRI_THRESHOLD	0xf0
 #define GIC_CPU_INT_SPURIOUS		1023
 #define GIC_CPU_INT_ID_MASK		0x3ff
+#define GIC_CPU_DIS_BYPASS_MASK		0x1e0
 
 union gic_base {
 	void __iomem *common_base;
@@ -394,6 +395,20 @@ static u8 gic_get_cpumask(struct gic_chip_data *gic)
 	return mask;
 }
 
+static void gic_cpu_if_up(void)
+{
+	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
+	u32 bypass;
+
+	/*
+	 * Preserve bypass disable bits to be written back later
+	 */
+	bypass = readl(cpu_base + GIC_CPU_CTRL);
+	bypass &= GIC_CPU_DIS_BYPASS_MASK;
+
+	writel_relaxed(bypass | GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
+}
+
 static void __init gic_dist_init(struct gic_chip_data *gic)
 {
 	unsigned int i;
@@ -476,13 +491,17 @@ static void gic_cpu_init(struct gic_chip_data *gic)
 				dist_base + GIC_DIST_PRI + i * 4 / 4);
 
 	writel_relaxed(GIC_CPU_INT_PRI_THRESHOLD, base + GIC_CPU_PRIMASK);
-	writel_relaxed(GIC_CPU_ENABLE, base + GIC_CPU_CTRL);
+	gic_cpu_if_up();
 }
 
 void gic_cpu_if_down(void)
 {
 	void __iomem *cpu_base = gic_data_cpu_base(&gic_data[0]);
-	writel_relaxed(0, cpu_base + GIC_CPU_CTRL);
+	u32 val;
+
+	val = readl(cpu_base + GIC_CPU_CTRL);
+	val &= ~GIC_CPU_ENABLE;
+	writel_relaxed(val, cpu_base + GIC_CPU_CTRL);
 }
 
 #ifdef CONFIG_CPU_PM
@@ -618,7 +637,7 @@ static void gic_cpu_restore(unsigned int gic_nr)
 					dist_base + GIC_DIST_PRI + i * 4);
 
 	writel_relaxed(GIC_CPU_INT_PRI_THRESHOLD, cpu_base + GIC_CPU_PRIMASK);
-	writel_relaxed(GIC_CPU_ENABLE, cpu_base + GIC_CPU_CTRL);
+	gic_cpu_if_up();
 }
 
 static int gic_notifier(struct notifier_block *self, unsigned long cmd,	void *v)
-- 
1.9.1

Re: [PATCH V2 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Thomas Gleixner]

On Wed, 25 Jun 2014, Feng Kan wrote:

> This change is made to preserve the GIC v2 bypass bits in the
> GIC_CPU_CTRL register (also known as the GICC_CTLR register in spec).
> This code will preserve all bits configured by the bootloader regarding
> v2 bypass group bits. In the X-Gene platform, the bypass functionality
> is not used and bypass bits should not be changed by the kernel gic
> code as it could lead to incorrect behavior.
> 
> Signed-off-by: Vinayak Kale <vkale@apm.com>
> Signed-off-by: Feng Kan <fkan@apm.com>

Who wrote the patch? According to the SOB chain it's Vinayak, but your
patch is missing the:

From: Vinayak Kale <vkale@apm.com>

before the actual changelog starts. 

Re: [PATCH V2 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Feng Kan]

On Wed, Jun 25, 2014 at 6:05 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 25 Jun 2014, Feng Kan wrote:
>
>> This change is made to preserve the GIC v2 bypass bits in the
>> GIC_CPU_CTRL register (also known as the GICC_CTLR register in spec).
>> This code will preserve all bits configured by the bootloader regarding
>> v2 bypass group bits. In the X-Gene platform, the bypass functionality
>> is not used and bypass bits should not be changed by the kernel gic
>> code as it could lead to incorrect behavior.
>>
>> Signed-off-by: Vinayak Kale <vkale@apm.com>
>> Signed-off-by: Feng Kan <fkan@apm.com>
>
> Who wrote the patch? According to the SOB chain it's Vinayak, but your
> patch is missing the:
I wrote the patch which was based on Vinayak's original change. I can
change it to
Reviewed-by. I was trying to give him credit.

>
> From: Vinayak Kale <vkale@apm.com>
>
> before the actual changelog starts.

Re: [PATCH V2 2/2] irqchip: gic: preserve gic V2 bypass bits in cpu ctrl register

[Jason Cooper]

On Thu, Jun 26, 2014 at 09:48:25AM -0700, Feng Kan wrote:
> On Wed, Jun 25, 2014 at 6:05 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Wed, 25 Jun 2014, Feng Kan wrote:
> >
> >> This change is made to preserve the GIC v2 bypass bits in the
> >> GIC_CPU_CTRL register (also known as the GICC_CTLR register in spec).
> >> This code will preserve all bits configured by the bootloader regarding
> >> v2 bypass group bits. In the X-Gene platform, the bypass functionality
> >> is not used and bypass bits should not be changed by the kernel gic
> >> code as it could lead to incorrect behavior.
> >>
> >> Signed-off-by: Vinayak Kale <vkale@apm.com>
> >> Signed-off-by: Feng Kan <fkan@apm.com>
> >
> > Who wrote the patch? According to the SOB chain it's Vinayak, but your
> > patch is missing the:
> 
> I wrote the patch which was based on Vinayak's original change. I can
> change it to
> Reviewed-by. I was trying to give him credit.
> 

Then please leave the S-o-b's alone and append a note describing the
changes you made to his original patch.

eg:

----->8-------------
From: Vinayak Kale <vkale@apm.com>

This change is made to preserve the GIC v2 bypass bits in the
GIC_CPU_CTRL register (also known as the GICC_CTLR register in spec).
This code will preserve all bits configured by the bootloader regarding
v2 bypass group bits. In the X-Gene platform, the bypass functionality
is not used and bypass bits should not be changed by the kernel gic
code as it could lead to incorrect behavior.

[Feng Kan: Rebased on magic number removal patch, added feature X, fixed
bug Y.]

Signed-off-by: Vinayak Kale <vkale@apm.com>
Signed-off-by: Feng Kan <fkan@apm.com>
----->8-------------

'git am' will use the info from the last 'From:' as the author of the
commit.

It goes without saying (even though I'm mentioning it ;-) ) the S-o-b
means a very specific thing.  It is *not* there to enhance credit.  It
is there to indicate that the person referenced (yourself and Vinayak)
have read, understood, and consent to the Developer's Certificate of
Origin [1].  Nothing else.

The commit message tags have one purpose:  To accurately render
information about a commit.  Who wrote it, who applied it, who reviewed
it, who let it go by a different tree than the default for a subsystem,
etc.

Attempts to (mis)use them as a corporate evaluation metric only leads to
developers trying to game the system.  More importantly, it wastes
maintainers time because now we have to question whether the appended
tags are indeed factual. :(

thx,

Jason.

[1] Documentation/SubmittingPatches, Section 12.