[patch 00/13] irqchip: spear_shirq: Cleanup the bitrot

[patch 00/13] irqchip: spear_shirq: Cleanup the bitrot

[Thomas Gleixner]

The driver is broken for spear320 since commit 80515a5a(ARM: SPEAr3xx:
shirq: simplify and move the shared irq multiplexor to DT). Clearly
never tested on spear320.

Aside of that it's an unreadable overengineered trainwreck with lots
of obscure "functionality". 

Make it work and clean it up.

Thanks,

	tglx
---
 include/linux/irqchip/spear-shirq.h |   64 -------
 linux/drivers/irqchip/spear-shirq.c |  306 ++++++++++++++++--------------------
 2 files changed, 138 insertions(+), 232 deletions(-)

[patch 01/13] irqchip: spear_shirq: Fix interrupt offset

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-fix-offsets.patch --]
[-- Type: text/plain, Size: 926 bytes --]

The ras3 block on spear320 claims to have 3 interrupts. In fact it has
one and 6 reserved interrupts. Account the 6 reserved to this block so
it has 7 interrupts total. That matches the datasheet and the device
tree entries.

Broken since commit 80515a5a(ARM: SPEAr3xx: shirq: simplify and move
the shared irq multiplexor to DT). Testing is overrated....

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 drivers/irqchip/spear-shirq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -125,7 +125,7 @@ static struct spear_shirq spear320_shirq
 };
 
 static struct spear_shirq spear320_shirq_ras3 = {
-	.irq_nr = 3,
+	.irq_nr = 7,
 	.irq_bit_off = 0,
 	.invalid_irq = 1,
 	.regs = {

Re: [patch 01/13] irqchip: spear_shirq: Fix interrupt offset

[Jason Cooper]

On Thu, Jun 19, 2014 at 09:34:37PM -0000, Thomas Gleixner wrote:
> The ras3 block on spear320 claims to have 3 interrupts. In fact it has
> one and 6 reserved interrupts. Account the 6 reserved to this block so
> it has 7 interrupts total. That matches the datasheet and the device
> tree entries.
> 
> Broken since commit 80515a5a(ARM: SPEAr3xx: shirq: simplify and move
> the shared irq multiplexor to DT). Testing is overrated....
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/irqchip/spear-shirq.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to irqchip/urgent noting that it's for stable v3.8+ with
Viresh's Ack.

thx,

Jason.

[patch 02/13] irqchip: spear_shirq: Kill pointless static

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-kill-pointless-static.patch --]
[-- Type: text/plain, Size: 603 bytes --]

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -252,7 +252,7 @@ static int __init shirq_init(struct spea
 		struct device_node *np)
 {
 	int i, irq_base, hwirq = 0, irq_nr = 0;
-	static struct irq_domain *shirq_domain;
+	struct irq_domain *shirq_domain;
 	void __iomem *base;
 
 	base = of_iomap(np, 0);

[patch 03/13] irqchip: spear_shirq: Move private structs to source

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-move-private-structs-to-source.patch --]
[-- Type: text/plain, Size: 5032 bytes --]

No point in having them in a separate header file. Make the init
functions static.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c       |   52 +++++++++++++++++++++++++----
 include/linux/irqchip/spear-shirq.h |   64 ------------------------------------
 2 files changed, 45 insertions(+), 71 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -19,7 +19,6 @@
 #include <linux/io.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
-#include <linux/irqchip/spear-shirq.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
@@ -27,6 +26,45 @@
 
 #include "irqchip.h"
 
+/*
+ * struct shirq_regs: shared irq register configuration
+ *
+ * enb_reg: enable register offset
+ * reset_to_enb: val 1 indicates, we need to clear bit for enabling interrupt
+ * status_reg: status register offset
+ * status_reg_mask: status register valid mask
+ * clear_reg: clear register offset
+ * reset_to_clear: val 1 indicates, we need to clear bit for clearing interrupt
+ */
+struct shirq_regs {
+	u32 enb_reg;
+	u32 reset_to_enb;
+	u32 status_reg;
+	u32 clear_reg;
+	u32 reset_to_clear;
+};
+
+/*
+ * struct spear_shirq: shared irq structure
+ *
+ * irq: hardware irq number
+ * irq_base: base irq in linux domain
+ * irq_nr: no. of shared interrupts in a particular block
+ * irq_bit_off: starting bit offset in the status register
+ * invalid_irq: irq group is currently disabled
+ * base: base address of shared irq register
+ * regs: register configuration for shared irq block
+ */
+struct spear_shirq {
+	u32 irq;
+	u32 irq_base;
+	u32 irq_nr;
+	u32 irq_bit_off;
+	int invalid_irq;
+	void __iomem *base;
+	struct shirq_regs regs;
+};
+
 static DEFINE_SPINLOCK(lock);
 
 /* spear300 shared irq registers offsets and masks */
@@ -296,24 +334,24 @@ err_unmap:
 	return -ENXIO;
 }
 
-int __init spear300_shirq_of_init(struct device_node *np,
-		struct device_node *parent)
+static int __init spear300_shirq_of_init(struct device_node *np,
+					 struct device_node *parent)
 {
 	return shirq_init(spear300_shirq_blocks,
 			ARRAY_SIZE(spear300_shirq_blocks), np);
 }
 IRQCHIP_DECLARE(spear300_shirq, "st,spear300-shirq", spear300_shirq_of_init);
 
-int __init spear310_shirq_of_init(struct device_node *np,
-		struct device_node *parent)
+static int __init spear310_shirq_of_init(struct device_node *np,
+					 struct device_node *parent)
 {
 	return shirq_init(spear310_shirq_blocks,
 			ARRAY_SIZE(spear310_shirq_blocks), np);
 }
 IRQCHIP_DECLARE(spear310_shirq, "st,spear310-shirq", spear310_shirq_of_init);
 
-int __init spear320_shirq_of_init(struct device_node *np,
-		struct device_node *parent)
+static int __init spear320_shirq_of_init(struct device_node *np,
+					 struct device_node *parent)
 {
 	return shirq_init(spear320_shirq_blocks,
 			ARRAY_SIZE(spear320_shirq_blocks), np);
Index: linux/include/linux/irqchip/spear-shirq.h
===================================================================
--- linux.orig/include/linux/irqchip/spear-shirq.h
+++ /dev/null
@@ -1,64 +0,0 @@
-/*
- * SPEAr platform shared irq layer header file
- *
- * Copyright (C) 2009-2012 ST Microelectronics
- * Viresh Kumar <viresh.linux@gmail.com>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#ifndef __SPEAR_SHIRQ_H
-#define __SPEAR_SHIRQ_H
-
-#include <linux/irq.h>
-#include <linux/types.h>
-
-/*
- * struct shirq_regs: shared irq register configuration
- *
- * enb_reg: enable register offset
- * reset_to_enb: val 1 indicates, we need to clear bit for enabling interrupt
- * status_reg: status register offset
- * status_reg_mask: status register valid mask
- * clear_reg: clear register offset
- * reset_to_clear: val 1 indicates, we need to clear bit for clearing interrupt
- */
-struct shirq_regs {
-	u32 enb_reg;
-	u32 reset_to_enb;
-	u32 status_reg;
-	u32 clear_reg;
-	u32 reset_to_clear;
-};
-
-/*
- * struct spear_shirq: shared irq structure
- *
- * irq: hardware irq number
- * irq_base: base irq in linux domain
- * irq_nr: no. of shared interrupts in a particular block
- * irq_bit_off: starting bit offset in the status register
- * invalid_irq: irq group is currently disabled
- * base: base address of shared irq register
- * regs: register configuration for shared irq block
- */
-struct spear_shirq {
-	u32 irq;
-	u32 irq_base;
-	u32 irq_nr;
-	u32 irq_bit_off;
-	int invalid_irq;
-	void __iomem *base;
-	struct shirq_regs regs;
-};
-
-int __init spear300_shirq_of_init(struct device_node *np,
-		struct device_node *parent);
-int __init spear310_shirq_of_init(struct device_node *np,
-		struct device_node *parent);
-int __init spear320_shirq_of_init(struct device_node *np,
-		struct device_node *parent);
-
-#endif /* __SPEAR_SHIRQ_H */

[patch 05/13] irqchip: spear_shirq: Namespace cleanup

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-namespace-cleanup.patch --]
[-- Type: text/plain, Size: 8055 bytes --]

The struct members of the shirq block struct are named to confuse the
hell out of the casual reader. Clean it up.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |  106 +++++++++++++++++++++---------------------
 1 file changed, 53 insertions(+), 53 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -47,20 +47,20 @@ struct shirq_regs {
 /*
  * struct spear_shirq: shared irq structure
  *
- * irq_base: base irq in linux domain
- * irq_nr: no. of shared interrupts in a particular block
- * irq_bit_off: starting bit offset in the status register
- * invalid_irq: irq group is currently disabled
- * base: base address of shared irq register
- * regs: register configuration for shared irq block
+ * base:	Base register address
+ * regs:	Register configuration for shared irq block
+ * virq_base:	Base virtual interrupt number
+ * nr_irqs:	Number of interrupts handled by this block
+ * offset:	Bit offset of the first interrupt
+ * disabled:	Group is disabled, but accounted
  */
 struct spear_shirq {
-	u32 irq_base;
-	u32 irq_nr;
-	u32 irq_bit_off;
-	int invalid_irq;
-	void __iomem *base;
-	struct shirq_regs regs;
+	void __iomem		*base;
+	struct shirq_regs	regs;
+	u32			virq_base;
+	u32			nr_irqs;
+	u32			offset;
+	bool			disabled;
 };
 
 static DEFINE_SPINLOCK(lock);
@@ -70,8 +70,8 @@ static DEFINE_SPINLOCK(lock);
 #define SPEAR300_INT_STS_MASK_REG	0x58
 
 static struct spear_shirq spear300_shirq_ras1 = {
-	.irq_nr = 9,
-	.irq_bit_off = 0,
+	.offset		= 0,
+	.nr_irqs	= 9,
 	.regs = {
 		.enb_reg = SPEAR300_INT_ENB_MASK_REG,
 		.status_reg = SPEAR300_INT_STS_MASK_REG,
@@ -87,8 +87,8 @@ static struct spear_shirq *spear300_shir
 #define SPEAR310_INT_STS_MASK_REG	0x04
 
 static struct spear_shirq spear310_shirq_ras1 = {
-	.irq_nr = 8,
-	.irq_bit_off = 0,
+	.offset		= 0,
+	.nr_irqs	= 8,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -97,8 +97,8 @@ static struct spear_shirq spear310_shirq
 };
 
 static struct spear_shirq spear310_shirq_ras2 = {
-	.irq_nr = 5,
-	.irq_bit_off = 8,
+	.offset		= 8,
+	.nr_irqs	= 5,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -107,8 +107,8 @@ static struct spear_shirq spear310_shirq
 };
 
 static struct spear_shirq spear310_shirq_ras3 = {
-	.irq_nr = 1,
-	.irq_bit_off = 13,
+	.offset		= 13,
+	.nr_irqs	= 1,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -117,8 +117,8 @@ static struct spear_shirq spear310_shirq
 };
 
 static struct spear_shirq spear310_shirq_intrcomm_ras = {
-	.irq_nr = 3,
-	.irq_bit_off = 14,
+	.offset		= 14,
+	.nr_irqs	= 3,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -139,8 +139,8 @@ static struct spear_shirq *spear310_shir
 #define SPEAR320_INT_ENB_MASK_REG		0x08
 
 static struct spear_shirq spear320_shirq_ras1 = {
-	.irq_nr = 3,
-	.irq_bit_off = 7,
+	.offset		= 7,
+	.nr_irqs	= 3,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -150,8 +150,8 @@ static struct spear_shirq spear320_shirq
 };
 
 static struct spear_shirq spear320_shirq_ras2 = {
-	.irq_nr = 1,
-	.irq_bit_off = 10,
+	.offset		= 10,
+	.nr_irqs	= 1,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -161,9 +161,9 @@ static struct spear_shirq spear320_shirq
 };
 
 static struct spear_shirq spear320_shirq_ras3 = {
-	.irq_nr = 7,
-	.irq_bit_off = 0,
-	.invalid_irq = 1,
+	.offset		= 0,
+	.nr_irqs	= 7,
+	.disabled	= 1,
 	.regs = {
 		.enb_reg = SPEAR320_INT_ENB_MASK_REG,
 		.reset_to_enb = 1,
@@ -174,8 +174,8 @@ static struct spear_shirq spear320_shirq
 };
 
 static struct spear_shirq spear320_shirq_intrcomm_ras = {
-	.irq_nr = 11,
-	.irq_bit_off = 11,
+	.offset		= 11,
+	.nr_irqs	= 11,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -194,7 +194,7 @@ static struct spear_shirq *spear320_shir
 static void shirq_irq_mask_unmask(struct irq_data *d, bool mask)
 {
 	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
-	u32 val, offset = d->irq - shirq->irq_base;
+	u32 val, offset = d->irq - shirq->virq_base;
 	unsigned long flags;
 
 	if (shirq->regs.enb_reg == -1)
@@ -204,9 +204,9 @@ static void shirq_irq_mask_unmask(struct
 	val = readl(shirq->base + shirq->regs.enb_reg);
 
 	if (mask ^ shirq->regs.reset_to_enb)
-		val &= ~(0x1 << shirq->irq_bit_off << offset);
+		val &= ~(0x1 << shirq->offset << offset);
 	else
-		val |= 0x1 << shirq->irq_bit_off << offset;
+		val |= 0x1 << shirq->offset << offset;
 
 	writel(val, shirq->base + shirq->regs.enb_reg);
 	spin_unlock_irqrestore(&lock, flags);
@@ -239,17 +239,17 @@ static void shirq_handler(unsigned irq,
 	chip = irq_get_chip(irq);
 	chip->irq_ack(&desc->irq_data);
 
-	mask = ((0x1 << shirq->irq_nr) - 1) << shirq->irq_bit_off;
+	mask = ((0x1 << shirq->nr_irqs) - 1) << shirq->offset;
 	while ((val = readl(shirq->base + shirq->regs.status_reg) &
 				mask)) {
 
-		val >>= shirq->irq_bit_off;
-		for (i = 0, j = 1; i < shirq->irq_nr; i++, j <<= 1) {
+		val >>= shirq->offset;
+		for (i = 0, j = 1; i < shirq->nr_irqs; i++, j <<= 1) {
 
 			if (!(j & val))
 				continue;
 
-			generic_handle_irq(shirq->irq_base + i);
+			generic_handle_irq(shirq->virq_base + i);
 
 			/* clear interrupt */
 			if (shirq->regs.clear_reg == -1)
@@ -257,9 +257,9 @@ static void shirq_handler(unsigned irq,
 
 			tmp = readl(shirq->base + shirq->regs.clear_reg);
 			if (shirq->regs.reset_to_clear)
-				tmp &= ~(j << shirq->irq_bit_off);
+				tmp &= ~(j << shirq->offset);
 			else
-				tmp |= (j << shirq->irq_bit_off);
+				tmp |= (j << shirq->offset);
 			writel(tmp, shirq->base + shirq->regs.clear_reg);
 		}
 	}
@@ -271,24 +271,24 @@ static void __init spear_shirq_register(
 {
 	int i;
 
-	if (shirq->invalid_irq)
+	if (shirq->disabled)
 		return;
 
 	irq_set_chained_handler(parent_irq, shirq_handler);
 	irq_set_handler_data(parent_irq, shirq);
 
-	for (i = 0; i < shirq->irq_nr; i++) {
-		irq_set_chip_and_handler(shirq->irq_base + i,
+	for (i = 0; i < shirq->nr_irqs; i++) {
+		irq_set_chip_and_handler(shirq->virq_base + i,
 					 &shirq_chip, handle_simple_irq);
-		set_irq_flags(shirq->irq_base + i, IRQF_VALID);
-		irq_set_chip_data(shirq->irq_base + i, shirq);
+		set_irq_flags(shirq->virq_base + i, IRQF_VALID);
+		irq_set_chip_data(shirq->virq_base + i, shirq);
 	}
 }
 
 static int __init shirq_init(struct spear_shirq **shirq_blocks, int block_nr,
 		struct device_node *np)
 {
-	int i, parent_irq, irq_base, hwirq = 0, irq_nr = 0;
+	int i, parent_irq, virq_base, hwirq = 0, nr_irqs = 0;
 	struct irq_domain *shirq_domain;
 	void __iomem *base;
 
@@ -299,15 +299,15 @@ static int __init shirq_init(struct spea
 	}
 
 	for (i = 0; i < block_nr; i++)
-		irq_nr += shirq_blocks[i]->irq_nr;
+		nr_irqs += shirq_blocks[i]->nr_irqs;
 
-	irq_base = irq_alloc_descs(-1, 0, irq_nr, 0);
-	if (IS_ERR_VALUE(irq_base)) {
+	virq_base = irq_alloc_descs(-1, 0, nr_irqs, 0);
+	if (IS_ERR_VALUE(virq_base)) {
 		pr_err("%s: irq desc alloc failed\n", __func__);
 		goto err_unmap;
 	}
 
-	shirq_domain = irq_domain_add_legacy(np, irq_nr, irq_base, 0,
+	shirq_domain = irq_domain_add_legacy(np, nr_irqs, virq_base, 0,
 			&irq_domain_simple_ops, NULL);
 	if (WARN_ON(!shirq_domain)) {
 		pr_warn("%s: irq domain init failed\n", __func__);
@@ -316,18 +316,18 @@ static int __init shirq_init(struct spea
 
 	for (i = 0; i < block_nr; i++) {
 		shirq_blocks[i]->base = base;
-		shirq_blocks[i]->irq_base = irq_find_mapping(shirq_domain,
+		shirq_blocks[i]->virq_base = irq_find_mapping(shirq_domain,
 				hwirq);
 
 		parent_irq = irq_of_parse_and_map(np, i);
 		spear_shirq_register(shirq_blocks[i], parent_irq);
-		hwirq += shirq_blocks[i]->irq_nr;
+		hwirq += shirq_blocks[i]->nr_irqs;
 	}
 
 	return 0;
 
 err_free_desc:
-	irq_free_descs(irq_base, irq_nr);
+	irq_free_descs(virq_base, nr_irqs);
 err_unmap:
 	iounmap(base);
 	return -ENXIO;

[patch 04/13] irqchip: spear_shirq: No point in storing the parent irq

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-get-irq-of-misnomed-irq-member.patch --]
[-- Type: text/plain, Size: 2299 bytes --]

The struct member is pointless and a nismomer as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -47,7 +47,6 @@ struct shirq_regs {
 /*
  * struct spear_shirq: shared irq structure
  *
- * irq: hardware irq number
  * irq_base: base irq in linux domain
  * irq_nr: no. of shared interrupts in a particular block
  * irq_bit_off: starting bit offset in the status register
@@ -56,7 +55,6 @@ struct shirq_regs {
  * regs: register configuration for shared irq block
  */
 struct spear_shirq {
-	u32 irq;
 	u32 irq_base;
 	u32 irq_nr;
 	u32 irq_bit_off;
@@ -268,28 +266,29 @@ static void shirq_handler(unsigned irq,
 	chip->irq_unmask(&desc->irq_data);
 }
 
-static void __init spear_shirq_register(struct spear_shirq *shirq)
+static void __init spear_shirq_register(struct spear_shirq *shirq,
+					int parent_irq)
 {
 	int i;
 
 	if (shirq->invalid_irq)
 		return;
 
-	irq_set_chained_handler(shirq->irq, shirq_handler);
+	irq_set_chained_handler(parent_irq, shirq_handler);
+	irq_set_handler_data(parent_irq, shirq);
+
 	for (i = 0; i < shirq->irq_nr; i++) {
 		irq_set_chip_and_handler(shirq->irq_base + i,
 					 &shirq_chip, handle_simple_irq);
 		set_irq_flags(shirq->irq_base + i, IRQF_VALID);
 		irq_set_chip_data(shirq->irq_base + i, shirq);
 	}
-
-	irq_set_handler_data(shirq->irq, shirq);
 }
 
 static int __init shirq_init(struct spear_shirq **shirq_blocks, int block_nr,
 		struct device_node *np)
 {
-	int i, irq_base, hwirq = 0, irq_nr = 0;
+	int i, parent_irq, irq_base, hwirq = 0, irq_nr = 0;
 	struct irq_domain *shirq_domain;
 	void __iomem *base;
 
@@ -319,9 +318,9 @@ static int __init shirq_init(struct spea
 		shirq_blocks[i]->base = base;
 		shirq_blocks[i]->irq_base = irq_find_mapping(shirq_domain,
 				hwirq);
-		shirq_blocks[i]->irq = irq_of_parse_and_map(np, i);
 
-		spear_shirq_register(shirq_blocks[i]);
+		parent_irq = irq_of_parse_and_map(np, i);
+		spear_shirq_register(shirq_blocks[i], parent_irq);
 		hwirq += shirq_blocks[i]->irq_nr;
 	}
 

[patch 07/13] irqchip: spear_shirq: Use the proper interfaces

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-use-proper-interfaces.patch --]
[-- Type: text/plain, Size: 1275 bytes --]

No point in doing a full irq lookup, when the desc pointer is
available.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -232,12 +232,12 @@ static struct irq_chip shirq_chip = {
 
 static void shirq_handler(unsigned irq, struct irq_desc *desc)
 {
-	u32 i, j, val, mask, tmp;
-	struct irq_chip *chip;
 	struct spear_shirq *shirq = irq_get_handler_data(irq);
+	struct irq_data *idata = irq_desc_get_irq_data(desc);
+	struct irq_chip *chip = irq_data_get_irq_chip(idata);
+	u32 i, j, val, mask, tmp;
 
-	chip = irq_get_chip(irq);
-	chip->irq_ack(&desc->irq_data);
+	chip->irq_ack(idata);
 
 	mask = ((0x1 << shirq->nr_irqs) - 1) << shirq->offset;
 	while ((val = readl(shirq->base + shirq->regs.status_reg) &
@@ -263,7 +263,7 @@ static void shirq_handler(unsigned irq,
 			writel(tmp, shirq->base + shirq->regs.clear_reg);
 		}
 	}
-	chip->irq_unmask(&desc->irq_data);
+	chip->irq_unmask(idata);
 }
 
 static void __init spear_shirq_register(struct spear_shirq *shirq,

[patch 06/13] irqchip: spear_shirq: Reorder the spear320 ras blocks

[Thomas Gleixner]

[-- Attachment #1: spear-reorder-320.patch --]
[-- Type: text/plain, Size: 1688 bytes --]

Order the ras blocks in the order of interrupts not alphabetically.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -138,20 +138,22 @@ static struct spear_shirq *spear310_shir
 #define SPEAR320_INT_CLR_MASK_REG		0x04
 #define SPEAR320_INT_ENB_MASK_REG		0x08
 
-static struct spear_shirq spear320_shirq_ras1 = {
-	.offset		= 7,
-	.nr_irqs	= 3,
+static struct spear_shirq spear320_shirq_ras3 = {
+	.offset		= 0,
+	.nr_irqs	= 7,
+	.disabled	= 1,
 	.regs = {
-		.enb_reg = -1,
+		.enb_reg = SPEAR320_INT_ENB_MASK_REG,
+		.reset_to_enb = 1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
 		.clear_reg = SPEAR320_INT_CLR_MASK_REG,
 		.reset_to_clear = 1,
 	},
 };
 
-static struct spear_shirq spear320_shirq_ras2 = {
-	.offset		= 10,
-	.nr_irqs	= 1,
+static struct spear_shirq spear320_shirq_ras1 = {
+	.offset		= 7,
+	.nr_irqs	= 3,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -160,13 +162,11 @@ static struct spear_shirq spear320_shirq
 	},
 };
 
-static struct spear_shirq spear320_shirq_ras3 = {
-	.offset		= 0,
-	.nr_irqs	= 7,
-	.disabled	= 1,
+static struct spear_shirq spear320_shirq_ras2 = {
+	.offset		= 10,
+	.nr_irqs	= 1,
 	.regs = {
-		.enb_reg = SPEAR320_INT_ENB_MASK_REG,
-		.reset_to_enb = 1,
+		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
 		.clear_reg = SPEAR320_INT_CLR_MASK_REG,
 		.reset_to_clear = 1,

[patch 08/13] irqchip: spear_shirq: Precalculate status mask

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-precalc-mask.patch --]
[-- Type: text/plain, Size: 3455 bytes --]

Calculate the status mask at compile time, not at runtime.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -49,6 +49,7 @@ struct shirq_regs {
  *
  * base:	Base register address
  * regs:	Register configuration for shared irq block
+ * mask:	Mask to apply to the status register
  * virq_base:	Base virtual interrupt number
  * nr_irqs:	Number of interrupts handled by this block
  * offset:	Bit offset of the first interrupt
@@ -57,6 +58,7 @@ struct shirq_regs {
 struct spear_shirq {
 	void __iomem		*base;
 	struct shirq_regs	regs;
+	u32			mask;
 	u32			virq_base;
 	u32			nr_irqs;
 	u32			offset;
@@ -72,6 +74,7 @@ static DEFINE_SPINLOCK(lock);
 static struct spear_shirq spear300_shirq_ras1 = {
 	.offset		= 0,
 	.nr_irqs	= 9,
+	.mask		= ((0x1 << 9) - 1) << 0,
 	.regs = {
 		.enb_reg = SPEAR300_INT_ENB_MASK_REG,
 		.status_reg = SPEAR300_INT_STS_MASK_REG,
@@ -89,6 +92,7 @@ static struct spear_shirq *spear300_shir
 static struct spear_shirq spear310_shirq_ras1 = {
 	.offset		= 0,
 	.nr_irqs	= 8,
+	.mask		= ((0x1 << 8) - 1) << 0,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -99,6 +103,7 @@ static struct spear_shirq spear310_shirq
 static struct spear_shirq spear310_shirq_ras2 = {
 	.offset		= 8,
 	.nr_irqs	= 5,
+	.mask		= ((0x1 << 5) - 1) << 8,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -109,6 +114,7 @@ static struct spear_shirq spear310_shirq
 static struct spear_shirq spear310_shirq_ras3 = {
 	.offset		= 13,
 	.nr_irqs	= 1,
+	.mask		= ((0x1 << 1) - 1) << 13,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -119,6 +125,7 @@ static struct spear_shirq spear310_shirq
 static struct spear_shirq spear310_shirq_intrcomm_ras = {
 	.offset		= 14,
 	.nr_irqs	= 3,
+	.mask		= ((0x1 << 3) - 1) << 14,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -141,6 +148,7 @@ static struct spear_shirq *spear310_shir
 static struct spear_shirq spear320_shirq_ras3 = {
 	.offset		= 0,
 	.nr_irqs	= 7,
+	.mask		= ((0x1 << 7) - 1) << 0,
 	.disabled	= 1,
 	.regs = {
 		.enb_reg = SPEAR320_INT_ENB_MASK_REG,
@@ -154,6 +162,7 @@ static struct spear_shirq spear320_shirq
 static struct spear_shirq spear320_shirq_ras1 = {
 	.offset		= 7,
 	.nr_irqs	= 3,
+	.mask		= ((0x1 << 3) - 1) << 7,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -165,6 +174,7 @@ static struct spear_shirq spear320_shirq
 static struct spear_shirq spear320_shirq_ras2 = {
 	.offset		= 10,
 	.nr_irqs	= 1,
+	.mask		= ((0x1 << 1) - 1) << 10,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -176,6 +186,7 @@ static struct spear_shirq spear320_shirq
 static struct spear_shirq spear320_shirq_intrcomm_ras = {
 	.offset		= 11,
 	.nr_irqs	= 11,
+	.mask		= ((0x1 << 11) - 1) << 11,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
@@ -239,7 +250,7 @@ static void shirq_handler(unsigned irq,
 
 	chip->irq_ack(idata);
 
-	mask = ((0x1 << shirq->nr_irqs) - 1) << shirq->offset;
+	mask = shirq->mask;
 	while ((val = readl(shirq->base + shirq->regs.status_reg) &
 				mask)) {
 

Re: [patch 08/13] irqchip: spear_shirq: Precalculate status mask

[Viresh Kumar]

On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> Calculate the status mask at compile time, not at runtime.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  drivers/irqchip/spear-shirq.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux/drivers/irqchip/spear-shirq.c
> ===================================================================
> --- linux.orig/drivers/irqchip/spear-shirq.c
> +++ linux/drivers/irqchip/spear-shirq.c
> @@ -49,6 +49,7 @@ struct shirq_regs {
>   *
>   * base:       Base register address
>   * regs:       Register configuration for shared irq block
> + * mask:       Mask to apply to the status register
>   * virq_base:  Base virtual interrupt number
>   * nr_irqs:    Number of interrupts handled by this block
>   * offset:     Bit offset of the first interrupt
> @@ -57,6 +58,7 @@ struct shirq_regs {
>  struct spear_shirq {
>         void __iomem            *base;
>         struct shirq_regs       regs;
> +       u32                     mask;
>         u32                     virq_base;
>         u32                     nr_irqs;
>         u32                     offset;
> @@ -72,6 +74,7 @@ static DEFINE_SPINLOCK(lock);
>  static struct spear_shirq spear300_shirq_ras1 = {
>         .offset         = 0,
>         .nr_irqs        = 9,
> +       .mask           = ((0x1 << 9) - 1) << 0,
>         .regs = {
>                 .enb_reg = SPEAR300_INT_ENB_MASK_REG,
>                 .status_reg = SPEAR300_INT_STS_MASK_REG,
> @@ -89,6 +92,7 @@ static struct spear_shirq *spear300_shir
>  static struct spear_shirq spear310_shirq_ras1 = {
>         .offset         = 0,
>         .nr_irqs        = 8,
> +       .mask           = ((0x1 << 8) - 1) << 0,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR310_INT_STS_MASK_REG,
> @@ -99,6 +103,7 @@ static struct spear_shirq spear310_shirq
>  static struct spear_shirq spear310_shirq_ras2 = {
>         .offset         = 8,
>         .nr_irqs        = 5,
> +       .mask           = ((0x1 << 5) - 1) << 8,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR310_INT_STS_MASK_REG,
> @@ -109,6 +114,7 @@ static struct spear_shirq spear310_shirq
>  static struct spear_shirq spear310_shirq_ras3 = {
>         .offset         = 13,
>         .nr_irqs        = 1,
> +       .mask           = ((0x1 << 1) - 1) << 13,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR310_INT_STS_MASK_REG,
> @@ -119,6 +125,7 @@ static struct spear_shirq spear310_shirq
>  static struct spear_shirq spear310_shirq_intrcomm_ras = {
>         .offset         = 14,
>         .nr_irqs        = 3,
> +       .mask           = ((0x1 << 3) - 1) << 14,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR310_INT_STS_MASK_REG,
> @@ -141,6 +148,7 @@ static struct spear_shirq *spear310_shir
>  static struct spear_shirq spear320_shirq_ras3 = {
>         .offset         = 0,
>         .nr_irqs        = 7,
> +       .mask           = ((0x1 << 7) - 1) << 0,
>         .disabled       = 1,
>         .regs = {
>                 .enb_reg = SPEAR320_INT_ENB_MASK_REG,
> @@ -154,6 +162,7 @@ static struct spear_shirq spear320_shirq
>  static struct spear_shirq spear320_shirq_ras1 = {
>         .offset         = 7,
>         .nr_irqs        = 3,
> +       .mask           = ((0x1 << 3) - 1) << 7,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR320_INT_STS_MASK_REG,
> @@ -165,6 +174,7 @@ static struct spear_shirq spear320_shirq
>  static struct spear_shirq spear320_shirq_ras2 = {
>         .offset         = 10,
>         .nr_irqs        = 1,
> +       .mask           = ((0x1 << 1) - 1) << 10,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR320_INT_STS_MASK_REG,
> @@ -176,6 +186,7 @@ static struct spear_shirq spear320_shirq
>  static struct spear_shirq spear320_shirq_intrcomm_ras = {
>         .offset         = 11,
>         .nr_irqs        = 11,
> +       .mask           = ((0x1 << 11) - 1) << 11,
>         .regs = {
>                 .enb_reg = -1,
>                 .status_reg = SPEAR320_INT_STS_MASK_REG,

If you like, maybe this instead of above diff:

diff --git a/drivers/irqchip/spear-shirq.c b/drivers/irqchip/spear-shirq.c
index 3fdda3a..40c5c1b 100644
--- a/drivers/irqchip/spear-shirq.c
+++ b/drivers/irqchip/spear-shirq.c
@@ -281,6 +281,8 @@ static int __init shirq_init(struct spear_shirq
**shirq_blocks, int block_nr,
                shirq_blocks[i]->base = base;
                shirq_blocks[i]->irq_base = irq_find_mapping(shirq_domain,
                                hwirq);
+               shirq_blocks[i]->mask = ((0x1 << shirq_blocks[i]->nr_irqs) - 1)
+                                       << shirq_blocks[i]->offset;
                shirq_blocks[i]->irq = irq_of_parse_and_map(np, i);

                spear_shirq_register(shirq_blocks[i]);


> @@ -239,7 +250,7 @@ static void shirq_handler(unsigned irq,
>
>         chip->irq_ack(idata);
>
> -       mask = ((0x1 << shirq->nr_irqs) - 1) << shirq->offset;
> +       mask = shirq->mask;
>         while ((val = readl(shirq->base + shirq->regs.status_reg) &
>                                 mask)) {
>
>
>

Re: [patch 08/13] irqchip: spear_shirq: Precalculate status mask

[Thomas Gleixner]

On Fri, 20 Jun 2014, Viresh Kumar wrote:
> On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > @@ -176,6 +186,7 @@ static struct spear_shirq spear320_shirq
> >  static struct spear_shirq spear320_shirq_intrcomm_ras = {
> >         .offset         = 11,
> >         .nr_irqs        = 11,
> > +       .mask           = ((0x1 << 11) - 1) << 11,
> >         .regs = {
> >                 .enb_reg = -1,
> >                 .status_reg = SPEAR320_INT_STS_MASK_REG,
> 
> If you like, maybe this instead of above diff:

What's wrong with letting the compiler fill it in?
 
> diff --git a/drivers/irqchip/spear-shirq.c b/drivers/irqchip/spear-shirq.c
> index 3fdda3a..40c5c1b 100644
> --- a/drivers/irqchip/spear-shirq.c
> +++ b/drivers/irqchip/spear-shirq.c
> @@ -281,6 +281,8 @@ static int __init shirq_init(struct spear_shirq
> **shirq_blocks, int block_nr,
>                 shirq_blocks[i]->base = base;
>                 shirq_blocks[i]->irq_base = irq_find_mapping(shirq_domain,
>                                 hwirq);
> +               shirq_blocks[i]->mask = ((0x1 << shirq_blocks[i]->nr_irqs) - 1)
> +                                       << shirq_blocks[i]->offset;
>                 shirq_blocks[i]->irq = irq_of_parse_and_map(np, i);
> 
>                 spear_shirq_register(shirq_blocks[i]);
> 
> 
> > @@ -239,7 +250,7 @@ static void shirq_handler(unsigned irq,
> >
> >         chip->irq_ack(idata);
> >
> > -       mask = ((0x1 << shirq->nr_irqs) - 1) << shirq->offset;
> > +       mask = shirq->mask;
> >         while ((val = readl(shirq->base + shirq->regs.status_reg) &
> >                                 mask)) {
> >
> >
> >
> 

Re: [patch 08/13] irqchip: spear_shirq: Precalculate status mask

[Viresh Kumar]

On 20 June 2014 13:36, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 20 Jun 2014, Viresh Kumar wrote:
>> On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > @@ -176,6 +186,7 @@ static struct spear_shirq spear320_shirq
>> >  static struct spear_shirq spear320_shirq_intrcomm_ras = {
>> >         .offset         = 11,
>> >         .nr_irqs        = 11,
>> > +       .mask           = ((0x1 << 11) - 1) << 11,
>> >         .regs = {
>> >                 .enb_reg = -1,
>> >                 .status_reg = SPEAR320_INT_STS_MASK_REG,
>>
>> If you like, maybe this instead of above diff:
>
> What's wrong with letting the compiler fill it in?

>> +               shirq_blocks[i]->mask = ((0x1 << shirq_blocks[i]->nr_irqs) - 1)
>> +                                       << shirq_blocks[i]->offset;

Just for better readability as it doesn't have magic numbers in it
and the performance impact wouldn't be much as its done just
once on boot.

[patch 09/13] irqchip: spear_shirq: Kill the clear_reg nonsense

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-kill-clear-reg-nonsense.patch --]
[-- Type: text/plain, Size: 3610 bytes --]

None of the chips has a ACK register. The code brainlessly fiddles
with the enable register, so it might even reenable a disabled
interrupt at least on spear300.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -33,15 +33,11 @@
  * reset_to_enb: val 1 indicates, we need to clear bit for enabling interrupt
  * status_reg: status register offset
  * status_reg_mask: status register valid mask
- * clear_reg: clear register offset
- * reset_to_clear: val 1 indicates, we need to clear bit for clearing interrupt
  */
 struct shirq_regs {
 	u32 enb_reg;
 	u32 reset_to_enb;
 	u32 status_reg;
-	u32 clear_reg;
-	u32 reset_to_clear;
 };
 
 /*
@@ -78,7 +74,6 @@ static struct spear_shirq spear300_shirq
 	.regs = {
 		.enb_reg = SPEAR300_INT_ENB_MASK_REG,
 		.status_reg = SPEAR300_INT_STS_MASK_REG,
-		.clear_reg = -1,
 	},
 };
 
@@ -96,7 +91,6 @@ static struct spear_shirq spear310_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
-		.clear_reg = -1,
 	},
 };
 
@@ -107,7 +101,6 @@ static struct spear_shirq spear310_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
-		.clear_reg = -1,
 	},
 };
 
@@ -118,7 +111,6 @@ static struct spear_shirq spear310_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
-		.clear_reg = -1,
 	},
 };
 
@@ -129,7 +121,6 @@ static struct spear_shirq spear310_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
-		.clear_reg = -1,
 	},
 };
 
@@ -150,13 +141,6 @@ static struct spear_shirq spear320_shirq
 	.nr_irqs	= 7,
 	.mask		= ((0x1 << 7) - 1) << 0,
 	.disabled	= 1,
-	.regs = {
-		.enb_reg = SPEAR320_INT_ENB_MASK_REG,
-		.reset_to_enb = 1,
-		.status_reg = SPEAR320_INT_STS_MASK_REG,
-		.clear_reg = SPEAR320_INT_CLR_MASK_REG,
-		.reset_to_clear = 1,
-	},
 };
 
 static struct spear_shirq spear320_shirq_ras1 = {
@@ -166,8 +150,6 @@ static struct spear_shirq spear320_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
-		.clear_reg = SPEAR320_INT_CLR_MASK_REG,
-		.reset_to_clear = 1,
 	},
 };
 
@@ -178,8 +160,6 @@ static struct spear_shirq spear320_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
-		.clear_reg = SPEAR320_INT_CLR_MASK_REG,
-		.reset_to_clear = 1,
 	},
 };
 
@@ -190,8 +170,6 @@ static struct spear_shirq spear320_shirq
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
-		.clear_reg = SPEAR320_INT_CLR_MASK_REG,
-		.reset_to_clear = 1,
 	},
 };
 
@@ -246,7 +224,7 @@ static void shirq_handler(unsigned irq,
 	struct spear_shirq *shirq = irq_get_handler_data(irq);
 	struct irq_data *idata = irq_desc_get_irq_data(desc);
 	struct irq_chip *chip = irq_data_get_irq_chip(idata);
-	u32 i, j, val, mask, tmp;
+	u32 i, j, val, mask;
 
 	chip->irq_ack(idata);
 
@@ -261,17 +239,6 @@ static void shirq_handler(unsigned irq,
 				continue;
 
 			generic_handle_irq(shirq->virq_base + i);
-
-			/* clear interrupt */
-			if (shirq->regs.clear_reg == -1)
-				continue;
-
-			tmp = readl(shirq->base + shirq->regs.clear_reg);
-			if (shirq->regs.reset_to_clear)
-				tmp &= ~(j << shirq->offset);
-			else
-				tmp |= (j << shirq->offset);
-			writel(tmp, shirq->base + shirq->regs.clear_reg);
 		}
 	}
 	chip->irq_unmask(idata);

Re: [patch 09/13] irqchip: spear_shirq: Kill the clear_reg nonsense

[Viresh Kumar]

On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> None of the chips has a ACK register.

I need to recheck on this after looking at datasheets. Arranging for
them, will revert by tomorrow.

> The code brainlessly fiddles
> with the enable register, so it might even reenable a disabled
> interrupt at least on spear300.

Ack/Clear register is only configured for SPEAr320, how will it
make a difference to SPEAr300 ?

And for SPEAr320 as well, the offset mentioned in code for clear
register is different then ENABLE register.

> Index: linux/drivers/irqchip/spear-shirq.c
> ===================================================================
> --- linux.orig/drivers/irqchip/spear-shirq.c
> +++ linux/drivers/irqchip/spear-shirq.c
> @@ -33,15 +33,11 @@
>   * reset_to_enb: val 1 indicates, we need to clear bit for enabling interrupt
>   * status_reg: status register offset
>   * status_reg_mask: status register valid mask
> - * clear_reg: clear register offset
> - * reset_to_clear: val 1 indicates, we need to clear bit for clearing interrupt
>   */
>  struct shirq_regs {
>         u32 enb_reg;
>         u32 reset_to_enb;
>         u32 status_reg;
> -       u32 clear_reg;

> -       u32 reset_to_clear;

AFAIR, there was a revision for SPEAr320 which was actually using
reset_to_clear and so was present in code. But later revisions got rid
of it and code never got updated.

> @@ -150,13 +141,6 @@ static struct spear_shirq spear320_shirq
>         .nr_irqs        = 7,
>         .mask           = ((0x1 << 7) - 1) << 0,
>         .disabled       = 1,
> -       .regs = {
> -               .enb_reg = SPEAR320_INT_ENB_MASK_REG,
> -               .reset_to_enb = 1,
> -               .status_reg = SPEAR320_INT_STS_MASK_REG,
> -               .clear_reg = SPEAR320_INT_CLR_MASK_REG,
> -               .reset_to_clear = 1,
> -       },

Was removing .regs completely intentional?

I don't see these registers getting added again in later patches.

Re: [patch 09/13] irqchip: spear_shirq: Kill the clear_reg nonsense

[Thomas Gleixner]

On Fri, 20 Jun 2014, Viresh Kumar wrote:

> On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > None of the chips has a ACK register.
> 
> I need to recheck on this after looking at datasheets. Arranging for
> them, will revert by tomorrow.
> 
> > The code brainlessly fiddles
> > with the enable register, so it might even reenable a disabled
> > interrupt at least on spear300.
> 
> Ack/Clear register is only configured for SPEAr320, how will it
> make a difference to SPEAr300 ?

Sorry, my bad. misread the code. So this wants a different
changelog.

> And for SPEAr320 as well, the offset mentioned in code for clear
> register is different then ENABLE register.

I still don't see why you'd write something into the status register
on 320, which is RO according to documentation.

> > @@ -150,13 +141,6 @@ static struct spear_shirq spear320_shirq
> >         .nr_irqs        = 7,
> >         .mask           = ((0x1 << 7) - 1) << 0,
> >         .disabled       = 1,
> > -       .regs = {
> > -               .enb_reg = SPEAR320_INT_ENB_MASK_REG,
> > -               .reset_to_enb = 1,
> > -               .status_reg = SPEAR320_INT_STS_MASK_REG,
> > -               .clear_reg = SPEAR320_INT_CLR_MASK_REG,
> > -               .reset_to_clear = 1,
> > -       },
> 
> Was removing .regs completely intentional?
> 
> I don't see these registers getting added again in later patches.

Yes, because that block is NEVER used because disabled = 1

Thanks,

	tglx

[patch 10/13] irqchip: spear_shirq: Simplify chained handler

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-kill-loop-nonsense.patch --]
[-- Type: text/plain, Size: 1289 bytes --]

I don't know if there are less efficient ways to code that. Get rid of
the loop mess and use efficient code.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -224,23 +224,20 @@ static void shirq_handler(unsigned irq,
 	struct spear_shirq *shirq = irq_get_handler_data(irq);
 	struct irq_data *idata = irq_desc_get_irq_data(desc);
 	struct irq_chip *chip = irq_data_get_irq_chip(idata);
-	u32 i, j, val, mask;
+	u32 pend;
 
 	chip->irq_ack(idata);
 
-	mask = shirq->mask;
-	while ((val = readl(shirq->base + shirq->regs.status_reg) &
-				mask)) {
+	pend = readl(shirq->base + shirq->regs.status_reg) & shirq->mask;
+	pend >>= shirq->offset;
 
-		val >>= shirq->offset;
-		for (i = 0, j = 1; i < shirq->nr_irqs; i++, j <<= 1) {
+	while (pend) {
+		int irq = __ffs(pend);
 
-			if (!(j & val))
-				continue;
-
-			generic_handle_irq(shirq->virq_base + i);
-		}
+		pend &= ~(0x1 << irq);
+		generic_handle_irq(shirq->virq_base + irq);
 	}
+
 	chip->irq_unmask(idata);
 }
 

[patch 11/13] irqchip: spear_shirq: Remove the parent irq "ack"/unmask

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-kill-vic-ack-unmask-hackery.patch --]
[-- Type: text/plain, Size: 1158 bytes --]

"ack" is actually a mask in the parent irq. The demultiplexer and the
handlers run with interrupts disabled. No point in masking and
unmasking the parent.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |    6 ------
 1 file changed, 6 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -222,12 +222,8 @@ static struct irq_chip shirq_chip = {
 static void shirq_handler(unsigned irq, struct irq_desc *desc)
 {
 	struct spear_shirq *shirq = irq_get_handler_data(irq);
-	struct irq_data *idata = irq_desc_get_irq_data(desc);
-	struct irq_chip *chip = irq_data_get_irq_chip(idata);
 	u32 pend;
 
-	chip->irq_ack(idata);
-
 	pend = readl(shirq->base + shirq->regs.status_reg) & shirq->mask;
 	pend >>= shirq->offset;
 
@@ -237,8 +233,6 @@ static void shirq_handler(unsigned irq,
 		pend &= ~(0x1 << irq);
 		generic_handle_irq(shirq->virq_base + irq);
 	}
-
-	chip->irq_unmask(idata);
 }
 
 static void __init spear_shirq_register(struct spear_shirq *shirq,

[patch 12/13] irqchip: spear_shirq: Use proper irq chips for the different SoCs

[Thomas Gleixner]

[-- Attachment #1: spear-shirq-kill-enb-crap.patch --]
[-- Type: text/plain, Size: 6033 bytes --]

Only spear300 has an actual mask register for the RAS interrupts. Add
an irq chip pointer to the shirq struct and initialize spear300 with
the actual implementation and the others with dummy_irq_chip. The
disabled RAS3 block has no irq chip assigned, so we can check for this
and remove the disabled member.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   97 +++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 52 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -49,7 +49,8 @@ struct shirq_regs {
  * virq_base:	Base virtual interrupt number
  * nr_irqs:	Number of interrupts handled by this block
  * offset:	Bit offset of the first interrupt
- * disabled:	Group is disabled, but accounted
+ * irq_chip:	Interrupt controller chip used for this instance,
+ *		if NULL group is disabled, but accounted
  */
 struct spear_shirq {
 	void __iomem		*base;
@@ -58,19 +59,50 @@ struct spear_shirq {
 	u32			virq_base;
 	u32			nr_irqs;
 	u32			offset;
-	bool			disabled;
+	struct irq_chip		*irq_chip;
 };
 
-static DEFINE_SPINLOCK(lock);
-
 /* spear300 shared irq registers offsets and masks */
 #define SPEAR300_INT_ENB_MASK_REG	0x54
 #define SPEAR300_INT_STS_MASK_REG	0x58
 
+static DEFINE_RAW_SPINLOCK(shirq_lock);
+
+static void shirq_irq_mask(struct irq_data *d)
+{
+	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
+	u32 val, shift = d->irq - shirq->virq_base + shirq->offset;
+	u32 __iomem *reg = shirq->base + shirq->regs.enb_reg;
+
+	raw_spin_lock(&shirq_lock);
+	val = readl(reg) & ~(0x1 << shift);
+	writel(val, reg);
+	raw_spin_unlock(&shirq_lock);
+}
+
+static void shirq_irq_unmask(struct irq_data *d)
+{
+	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
+	u32 val, shift = d->irq - shirq->virq_base + shirq->offset;
+	u32 __iomem *reg = shirq->base + shirq->regs.enb_reg;
+
+	raw_spin_lock(&shirq_lock);
+	val = readl(reg) | (0x1 << shift);
+	writel(val, reg);
+	raw_spin_unlock(&shirq_lock);
+}
+
+static struct irq_chip shirq_chip = {
+	.name		= "spear-shirq",
+	.irq_mask	= shirq_irq_mask,
+	.irq_unmask	= shirq_irq_unmask,
+};
+
 static struct spear_shirq spear300_shirq_ras1 = {
 	.offset		= 0,
 	.nr_irqs	= 9,
 	.mask		= ((0x1 << 9) - 1) << 0,
+	.irq_chip	= &shirq_chip,
 	.regs = {
 		.enb_reg = SPEAR300_INT_ENB_MASK_REG,
 		.status_reg = SPEAR300_INT_STS_MASK_REG,
@@ -88,8 +120,8 @@ static struct spear_shirq spear310_shirq
 	.offset		= 0,
 	.nr_irqs	= 8,
 	.mask		= ((0x1 << 8) - 1) << 0,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
-		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
 	},
 };
@@ -98,6 +130,7 @@ static struct spear_shirq spear310_shirq
 	.offset		= 8,
 	.nr_irqs	= 5,
 	.mask		= ((0x1 << 5) - 1) << 8,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
 		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
@@ -108,8 +141,8 @@ static struct spear_shirq spear310_shirq
 	.offset		= 13,
 	.nr_irqs	= 1,
 	.mask		= ((0x1 << 1) - 1) << 13,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
-		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
 	},
 };
@@ -118,8 +151,8 @@ static struct spear_shirq spear310_shirq
 	.offset		= 14,
 	.nr_irqs	= 3,
 	.mask		= ((0x1 << 3) - 1) << 14,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
-		.enb_reg = -1,
 		.status_reg = SPEAR310_INT_STS_MASK_REG,
 	},
 };
@@ -140,15 +173,14 @@ static struct spear_shirq spear320_shirq
 	.offset		= 0,
 	.nr_irqs	= 7,
 	.mask		= ((0x1 << 7) - 1) << 0,
-	.disabled	= 1,
 };
 
 static struct spear_shirq spear320_shirq_ras1 = {
 	.offset		= 7,
 	.nr_irqs	= 3,
 	.mask		= ((0x1 << 3) - 1) << 7,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
-		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
 	},
 };
@@ -157,8 +189,8 @@ static struct spear_shirq spear320_shirq
 	.offset		= 10,
 	.nr_irqs	= 1,
 	.mask		= ((0x1 << 1) - 1) << 10,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
-		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
 	},
 };
@@ -167,8 +199,8 @@ static struct spear_shirq spear320_shirq
 	.offset		= 11,
 	.nr_irqs	= 11,
 	.mask		= ((0x1 << 11) - 1) << 11,
+	.irq_chip	= &dummy_irq_chip,
 	.regs = {
-		.enb_reg = -1,
 		.status_reg = SPEAR320_INT_STS_MASK_REG,
 	},
 };
@@ -180,45 +212,6 @@ static struct spear_shirq *spear320_shir
 	&spear320_shirq_intrcomm_ras,
 };
 
-static void shirq_irq_mask_unmask(struct irq_data *d, bool mask)
-{
-	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
-	u32 val, offset = d->irq - shirq->virq_base;
-	unsigned long flags;
-
-	if (shirq->regs.enb_reg == -1)
-		return;
-
-	spin_lock_irqsave(&lock, flags);
-	val = readl(shirq->base + shirq->regs.enb_reg);
-
-	if (mask ^ shirq->regs.reset_to_enb)
-		val &= ~(0x1 << shirq->offset << offset);
-	else
-		val |= 0x1 << shirq->offset << offset;
-
-	writel(val, shirq->base + shirq->regs.enb_reg);
-	spin_unlock_irqrestore(&lock, flags);
-
-}
-
-static void shirq_irq_mask(struct irq_data *d)
-{
-	shirq_irq_mask_unmask(d, 1);
-}
-
-static void shirq_irq_unmask(struct irq_data *d)
-{
-	shirq_irq_mask_unmask(d, 0);
-}
-
-static struct irq_chip shirq_chip = {
-	.name		= "spear-shirq",
-	.irq_ack	= shirq_irq_mask,
-	.irq_mask	= shirq_irq_mask,
-	.irq_unmask	= shirq_irq_unmask,
-};
-
 static void shirq_handler(unsigned irq, struct irq_desc *desc)
 {
 	struct spear_shirq *shirq = irq_get_handler_data(irq);
@@ -240,7 +233,7 @@ static void __init spear_shirq_register(
 {
 	int i;
 
-	if (shirq->disabled)
+	if (!shirq->irq_chip)
 		return;
 
 	irq_set_chained_handler(parent_irq, shirq_handler);
@@ -248,7 +241,7 @@ static void __init spear_shirq_register(
 
 	for (i = 0; i < shirq->nr_irqs; i++) {
 		irq_set_chip_and_handler(shirq->virq_base + i,
-					 &shirq_chip, handle_simple_irq);
+					 shirq->irq_chip, handle_simple_irq);
 		set_irq_flags(shirq->virq_base + i, IRQF_VALID);
 		irq_set_chip_data(shirq->virq_base + i, shirq);
 	}

[patch 13/13] irqchip: spear_shirq: Simplify register access code

[Thomas Gleixner]

[-- Attachment #1: spear-irqchip-optimize-register-access.patch --]
[-- Type: text/plain, Size: 5113 bytes --]

The extra register data structure is pointless. Move the offsets of
the status and the mask register into the shirq block structure.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/irqchip/spear-shirq.c |   61 +++++++++++-------------------------------
 1 file changed, 16 insertions(+), 45 deletions(-)

Index: linux/drivers/irqchip/spear-shirq.c
===================================================================
--- linux.orig/drivers/irqchip/spear-shirq.c
+++ linux/drivers/irqchip/spear-shirq.c
@@ -27,24 +27,11 @@
 #include "irqchip.h"
 
 /*
- * struct shirq_regs: shared irq register configuration
- *
- * enb_reg: enable register offset
- * reset_to_enb: val 1 indicates, we need to clear bit for enabling interrupt
- * status_reg: status register offset
- * status_reg_mask: status register valid mask
- */
-struct shirq_regs {
-	u32 enb_reg;
-	u32 reset_to_enb;
-	u32 status_reg;
-};
-
-/*
  * struct spear_shirq: shared irq structure
  *
  * base:	Base register address
- * regs:	Register configuration for shared irq block
+ * status_reg:	Status register offset for chained interrupt handler
+ * mask_reg:	Mask register offset for irq chip
  * mask:	Mask to apply to the status register
  * virq_base:	Base virtual interrupt number
  * nr_irqs:	Number of interrupts handled by this block
@@ -54,7 +41,8 @@ struct shirq_regs {
  */
 struct spear_shirq {
 	void __iomem		*base;
-	struct shirq_regs	regs;
+	u32			status_reg;
+	u32			mask_reg;
 	u32			mask;
 	u32			virq_base;
 	u32			nr_irqs;
@@ -72,7 +60,7 @@ static void shirq_irq_mask(struct irq_da
 {
 	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
 	u32 val, shift = d->irq - shirq->virq_base + shirq->offset;
-	u32 __iomem *reg = shirq->base + shirq->regs.enb_reg;
+	u32 __iomem *reg = shirq->base + shirq->mask_reg;
 
 	raw_spin_lock(&shirq_lock);
 	val = readl(reg) & ~(0x1 << shift);
@@ -84,7 +72,7 @@ static void shirq_irq_unmask(struct irq_
 {
 	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
 	u32 val, shift = d->irq - shirq->virq_base + shirq->offset;
-	u32 __iomem *reg = shirq->base + shirq->regs.enb_reg;
+	u32 __iomem *reg = shirq->base + shirq->mask_reg;
 
 	raw_spin_lock(&shirq_lock);
 	val = readl(reg) | (0x1 << shift);
@@ -103,10 +91,8 @@ static struct spear_shirq spear300_shirq
 	.nr_irqs	= 9,
 	.mask		= ((0x1 << 9) - 1) << 0,
 	.irq_chip	= &shirq_chip,
-	.regs = {
-		.enb_reg = SPEAR300_INT_ENB_MASK_REG,
-		.status_reg = SPEAR300_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR300_INT_STS_MASK_REG,
+	.mask_reg	= SPEAR300_INT_ENB_MASK_REG,
 };
 
 static struct spear_shirq *spear300_shirq_blocks[] = {
@@ -121,9 +107,7 @@ static struct spear_shirq spear310_shirq
 	.nr_irqs	= 8,
 	.mask		= ((0x1 << 8) - 1) << 0,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.status_reg = SPEAR310_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR310_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq spear310_shirq_ras2 = {
@@ -131,10 +115,7 @@ static struct spear_shirq spear310_shirq
 	.nr_irqs	= 5,
 	.mask		= ((0x1 << 5) - 1) << 8,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.enb_reg = -1,
-		.status_reg = SPEAR310_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR310_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq spear310_shirq_ras3 = {
@@ -142,9 +123,7 @@ static struct spear_shirq spear310_shirq
 	.nr_irqs	= 1,
 	.mask		= ((0x1 << 1) - 1) << 13,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.status_reg = SPEAR310_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR310_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq spear310_shirq_intrcomm_ras = {
@@ -152,9 +131,7 @@ static struct spear_shirq spear310_shirq
 	.nr_irqs	= 3,
 	.mask		= ((0x1 << 3) - 1) << 14,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.status_reg = SPEAR310_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR310_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq *spear310_shirq_blocks[] = {
@@ -180,9 +157,7 @@ static struct spear_shirq spear320_shirq
 	.nr_irqs	= 3,
 	.mask		= ((0x1 << 3) - 1) << 7,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.status_reg = SPEAR320_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR320_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq spear320_shirq_ras2 = {
@@ -190,9 +165,7 @@ static struct spear_shirq spear320_shirq
 	.nr_irqs	= 1,
 	.mask		= ((0x1 << 1) - 1) << 10,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.status_reg = SPEAR320_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR320_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq spear320_shirq_intrcomm_ras = {
@@ -200,9 +173,7 @@ static struct spear_shirq spear320_shirq
 	.nr_irqs	= 11,
 	.mask		= ((0x1 << 11) - 1) << 11,
 	.irq_chip	= &dummy_irq_chip,
-	.regs = {
-		.status_reg = SPEAR320_INT_STS_MASK_REG,
-	},
+	.status_reg	= SPEAR320_INT_STS_MASK_REG,
 };
 
 static struct spear_shirq *spear320_shirq_blocks[] = {
@@ -217,7 +188,7 @@ static void shirq_handler(unsigned irq,
 	struct spear_shirq *shirq = irq_get_handler_data(irq);
 	u32 pend;
 
-	pend = readl(shirq->base + shirq->regs.status_reg) & shirq->mask;
+	pend = readl(shirq->base + shirq->status_reg) & shirq->mask;
 	pend >>= shirq->offset;
 
 	while (pend) {

Re: [patch 13/13] irqchip: spear_shirq: Simplify register access code

[Viresh Kumar]

On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:

> Index: linux/drivers/irqchip/spear-shirq.c
> -struct shirq_regs {
> -       u32 enb_reg;
> -       u32 reset_to_enb;

I don't see something similar to 'reset_to_enb' is added again.
AFAICT, this field is being used by two blocks:

spear300_shirq_ras1: writes 0 to this. i.e. we need to write 0 to
mask the interrupt

and

spear320_shirq_ras3: writes 1 to this. i.e. we need to write 1 to
mask the interrupt.

And so the new code you have added breaks it for SPEAr300 ?

> -       u32 status_reg;
> -};

Re: [patch 13/13] irqchip: spear_shirq: Simplify register access code

[Thomas Gleixner]

On Fri, 20 Jun 2014, Viresh Kumar wrote:
> On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > Index: linux/drivers/irqchip/spear-shirq.c
> > -struct shirq_regs {
> > -       u32 enb_reg;
> > -       u32 reset_to_enb;
> 
> I don't see something similar to 'reset_to_enb' is added again.
> AFAICT, this field is being used by two blocks:
> 
> spear300_shirq_ras1: writes 0 to this. i.e. we need to write 0 to
> mask the interrupt
> 
> and
> 
> spear320_shirq_ras3: writes 1 to this. i.e. we need to write 1 to
> mask the interrupt.

AGAIN: spear320_shirq_ras3 is never instantiated as a chained irq. So
the chip for these interrupts is never set.

And if it ever is, you need a separate irq chip for it and not some
conditional hackery.

> And so the new code you have added breaks it for SPEAr300 ?

+static void shirq_irq_mask(struct irq_data *d)
+{
+	struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
+	u32 val, shift = d->irq - shirq->virq_base + shirq->offset;
+	u32 __iomem *reg = shirq->base + shirq->regs.enb_reg;
+
+	raw_spin_lock(&shirq_lock);
+	val = readl(reg) & ~(0x1 << shift);
+	writel(val, reg);
+	raw_spin_unlock(&shirq_lock);
+}

That's the mask function for 300 and it clears the bit, right?

Thanks,

	tglx

Re: [patch 13/13] irqchip: spear_shirq: Simplify register access code

[Viresh Kumar]

On 20 June 2014 13:35, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 20 Jun 2014, Viresh Kumar wrote:
>> On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> > Index: linux/drivers/irqchip/spear-shirq.c
>> > -struct shirq_regs {
>> > -       u32 enb_reg;
>> > -       u32 reset_to_enb;
>>
>> I don't see something similar to 'reset_to_enb' is added again.
>> AFAICT, this field is being used by two blocks:
>>
>> spear300_shirq_ras1: writes 0 to this. i.e. we need to write 0 to
>> mask the interrupt
>>
>> and
>>
>> spear320_shirq_ras3: writes 1 to this. i.e. we need to write 1 to
>> mask the interrupt.
>
> AGAIN: spear320_shirq_ras3 is never instantiated as a chained irq. So
> the chip for these interrupts is never set.

Didn't knew it, yes spear320_shirq_ras3 is never registered.

> And if it ever is, you need a separate irq chip for it and not some
> conditional hackery.

Yeah.

>> And so the new code you have added breaks it for SPEAr300 ?
>
> +static void shirq_irq_mask(struct irq_data *d)
> +{
> +       struct spear_shirq *shirq = irq_data_get_irq_chip_data(d);
> +       u32 val, shift = d->irq - shirq->virq_base + shirq->offset;
> +       u32 __iomem *reg = shirq->base + shirq->regs.enb_reg;
> +
> +       raw_spin_lock(&shirq_lock);
> +       val = readl(reg) & ~(0x1 << shift);
> +       writel(val, reg);
> +       raw_spin_unlock(&shirq_lock);
> +}
>
> That's the mask function for 300 and it clears the bit, right?

Yeah, that looks fine. Misread it.

Re: [patch 00/13] irqchip: spear_shirq: Cleanup the bitrot

[Viresh Kumar]

On Fri, Jun 20, 2014 at 3:04 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> The driver is broken for spear320 since commit 80515a5a(ARM: SPEAr3xx:
> shirq: simplify and move the shared irq multiplexor to DT). Clearly
> never tested on spear320.
>
> Aside of that it's an unreadable overengineered trainwreck with lots
> of obscure "functionality".
>
> Make it work and clean it up.
>
> Thanks,

For 1-7, 10 & 11

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Re: [patch 00/13] irqchip: spear_shirq: Cleanup the bitrot

[Viresh Kumar]

On Fri, Jun 20, 2014 at 2:50 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> For 1-7, 10 & 11
>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

This is applicable to 8-9, 12-13 as well. I couldn't find any
documentation which has Ack registers for SPEAr320..

Re: [patch 00/13] irqchip: spear_shirq: Cleanup the bitrot

[Jason Cooper]

On Thu, Jun 19, 2014 at 09:34:36PM -0000, Thomas Gleixner wrote:
> The driver is broken for spear320 since commit 80515a5a(ARM: SPEAr3xx:
> shirq: simplify and move the shared irq multiplexor to DT). Clearly
> never tested on spear320.
> 
> Aside of that it's an unreadable overengineered trainwreck with lots
> of obscure "functionality". 
> 
> Make it work and clean it up.

#2 - 13 applied to irqchip/core with Viresh's Ack and a dep on
irqchip/urgent for patch #1.

thx,

Jason.