[PATCH V2 00/15] genirq endian fixes; bcm7120/brcmstb IRQ updates

[PATCH V2 00/15] genirq endian fixes; bcm7120/brcmstb IRQ updates

[Kevin Cernekee]

I don't know if this will make everyone 100% happy but hopefully we're
inching slightly closer to a solution...

V1->V2:

 - Rework big endian support per the discussion on the list
 - Get rid of the global compile-time irq_reg_{readl,writel} accessors
   and make them private to generic-chip.c, so that on multiplatform
   kernels, different irqchip drivers can specify different MMIO behavior
 - Rebase on Linus's head of tree

Patch 06/15 still feels a bit like premature optimization to me.  Perhaps
we can drop it if nobody reports a measurable performance advantage in
any known configuration?


Kevin Cernekee (15):
  irqchip: Replace irq_reg_{readl,writel} with {readl,writel}
  sh: Eliminate unused irq_reg_{readl,writel} accessors
  genirq: Generic chip: Move irq_reg_{readl,writel} accessors into
    generic-chip.c
  genirq: Generic chip: Change irq_reg_{readl,writel} arguments
  genirq: Generic chip: Add big endian I/O accessors
  genirq: Generic chip: Optimize for fixed-endian systems
  irqchip: brcmstb-l2: Eliminate dependency on ARM code
  irqchip: bcm7120-l2: Eliminate bad IRQ check
  irqchip: Remove ARM dependency for bcm7120-l2 and brcmstb-l2
  irqchip: bcm7120-l2: Make sure all register accesses use base+offset
  irqchip: bcm7120-l2: Fix missing nibble in gc->unused mask
  irqchip: bcm7120-l2: Use gc->mask_cache to simplify suspend/resume
    functions
  irqchip: bcm7120-l2: Extend driver to support 64+ bit controllers
  irqchip: Decouple bcm7120-l2 from brcmstb-l2
  irqchip: bcm7120-l2: Enable big endian register accesses on BE kernels

 .../interrupt-controller/brcm,bcm7120-l2-intc.txt  |  26 +++-
 arch/arm/mach-bcm/Kconfig                          |   1 +
 arch/sh/boards/mach-se/7343/irq.c                  |   3 -
 arch/sh/boards/mach-se/7722/irq.c                  |   3 -
 drivers/irqchip/Kconfig                            |   8 +-
 drivers/irqchip/Makefile                           |   4 +-
 drivers/irqchip/irq-atmel-aic.c                    |  40 ++---
 drivers/irqchip/irq-atmel-aic5.c                   |  63 ++++----
 drivers/irqchip/irq-bcm7120-l2.c                   | 169 +++++++++++++--------
 drivers/irqchip/irq-brcmstb-l2.c                   |   7 +-
 drivers/irqchip/irq-sunxi-nmi.c                    |   4 +-
 drivers/irqchip/irq-tb10x.c                        |   4 +-
 include/linux/irq.h                                |   8 +-
 kernel/irq/Kconfig                                 |   5 +
 kernel/irq/Makefile                                |   1 +
 kernel/irq/generic-chip.c                          |  51 +++++--
 16 files changed, 237 insertions(+), 160 deletions(-)

-- 
2.1.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 01/15] irqchip: Replace irq_reg_{readl,writel} with {readl,writel}

[Kevin Cernekee]

Nobody is overriding these definitions anyway, so get rid of the wrappers.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/irqchip/irq-atmel-aic.c  | 40 ++++++++++++-------------
 drivers/irqchip/irq-atmel-aic5.c | 65 +++++++++++++++++++---------------------
 drivers/irqchip/irq-sunxi-nmi.c  |  4 +--
 drivers/irqchip/irq-tb10x.c      |  4 +--
 4 files changed, 55 insertions(+), 58 deletions(-)

diff --git a/drivers/irqchip/irq-atmel-aic.c b/drivers/irqchip/irq-atmel-aic.c
index 9a2cf3c..cfa65f13 100644
--- a/drivers/irqchip/irq-atmel-aic.c
+++ b/drivers/irqchip/irq-atmel-aic.c
@@ -65,11 +65,11 @@ aic_handle(struct pt_regs *regs)
 	u32 irqnr;
 	u32 irqstat;
 
-	irqnr = irq_reg_readl(gc->reg_base + AT91_AIC_IVR);
-	irqstat = irq_reg_readl(gc->reg_base + AT91_AIC_ISR);
+	irqnr = readl(gc->reg_base + AT91_AIC_IVR);
+	irqstat = readl(gc->reg_base + AT91_AIC_ISR);
 
 	if (!irqstat)
-		irq_reg_writel(0, gc->reg_base + AT91_AIC_EOICR);
+		writel(0, gc->reg_base + AT91_AIC_EOICR);
 	else
 		handle_domain_irq(aic_domain, irqnr, regs);
 }
@@ -80,7 +80,7 @@ static int aic_retrigger(struct irq_data *d)
 
 	/* Enable interrupt on AIC5 */
 	irq_gc_lock(gc);
-	irq_reg_writel(d->mask, gc->reg_base + AT91_AIC_ISCR);
+	writel(d->mask, gc->reg_base + AT91_AIC_ISCR);
 	irq_gc_unlock(gc);
 
 	return 0;
@@ -92,12 +92,12 @@ static int aic_set_type(struct irq_data *d, unsigned type)
 	unsigned int smr;
 	int ret;
 
-	smr = irq_reg_readl(gc->reg_base + AT91_AIC_SMR(d->hwirq));
+	smr = readl(gc->reg_base + AT91_AIC_SMR(d->hwirq));
 	ret = aic_common_set_type(d, type, &smr);
 	if (ret)
 		return ret;
 
-	irq_reg_writel(smr, gc->reg_base + AT91_AIC_SMR(d->hwirq));
+	writel(smr, gc->reg_base + AT91_AIC_SMR(d->hwirq));
 
 	return 0;
 }
@@ -108,8 +108,8 @@ static void aic_suspend(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(gc->mask_cache, gc->reg_base + AT91_AIC_IDCR);
-	irq_reg_writel(gc->wake_active, gc->reg_base + AT91_AIC_IECR);
+	writel(gc->mask_cache, gc->reg_base + AT91_AIC_IDCR);
+	writel(gc->wake_active, gc->reg_base + AT91_AIC_IECR);
 	irq_gc_unlock(gc);
 }
 
@@ -118,8 +118,8 @@ static void aic_resume(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(gc->wake_active, gc->reg_base + AT91_AIC_IDCR);
-	irq_reg_writel(gc->mask_cache, gc->reg_base + AT91_AIC_IECR);
+	writel(gc->wake_active, gc->reg_base + AT91_AIC_IDCR);
+	writel(gc->mask_cache, gc->reg_base + AT91_AIC_IECR);
 	irq_gc_unlock(gc);
 }
 
@@ -128,8 +128,8 @@ static void aic_pm_shutdown(struct irq_data *d)
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 
 	irq_gc_lock(gc);
-	irq_reg_writel(0xffffffff, gc->reg_base + AT91_AIC_IDCR);
-	irq_reg_writel(0xffffffff, gc->reg_base + AT91_AIC_ICCR);
+	writel(0xffffffff, gc->reg_base + AT91_AIC_IDCR);
+	writel(0xffffffff, gc->reg_base + AT91_AIC_ICCR);
 	irq_gc_unlock(gc);
 }
 #else
@@ -148,24 +148,24 @@ static void __init aic_hw_init(struct irq_domain *domain)
 	 * will not Lock out nIRQ
 	 */
 	for (i = 0; i < 8; i++)
-		irq_reg_writel(0, gc->reg_base + AT91_AIC_EOICR);
+		writel(0, gc->reg_base + AT91_AIC_EOICR);
 
 	/*
 	 * Spurious Interrupt ID in Spurious Vector Register.
 	 * When there is no current interrupt, the IRQ Vector Register
 	 * reads the value stored in AIC_SPU
 	 */
-	irq_reg_writel(0xffffffff, gc->reg_base + AT91_AIC_SPU);
+	writel(0xffffffff, gc->reg_base + AT91_AIC_SPU);
 
 	/* No debugging in AIC: Debug (Protect) Control Register */
-	irq_reg_writel(0, gc->reg_base + AT91_AIC_DCR);
+	writel(0, gc->reg_base + AT91_AIC_DCR);
 
 	/* Disable and clear all interrupts initially */
-	irq_reg_writel(0xffffffff, gc->reg_base + AT91_AIC_IDCR);
-	irq_reg_writel(0xffffffff, gc->reg_base + AT91_AIC_ICCR);
+	writel(0xffffffff, gc->reg_base + AT91_AIC_IDCR);
+	writel(0xffffffff, gc->reg_base + AT91_AIC_ICCR);
 
 	for (i = 0; i < 32; i++)
-		irq_reg_writel(i, gc->reg_base + AT91_AIC_SVR(i));
+		writel(i, gc->reg_base + AT91_AIC_SVR(i));
 }
 
 static int aic_irq_domain_xlate(struct irq_domain *d,
@@ -195,10 +195,10 @@ static int aic_irq_domain_xlate(struct irq_domain *d,
 	gc = dgc->gc[idx];
 
 	irq_gc_lock(gc);
-	smr = irq_reg_readl(gc->reg_base + AT91_AIC_SMR(*out_hwirq));
+	smr = readl(gc->reg_base + AT91_AIC_SMR(*out_hwirq));
 	ret = aic_common_set_priority(intspec[2], &smr);
 	if (!ret)
-		irq_reg_writel(smr, gc->reg_base + AT91_AIC_SMR(*out_hwirq));
+		writel(smr, gc->reg_base + AT91_AIC_SMR(*out_hwirq));
 	irq_gc_unlock(gc);
 
 	return ret;
diff --git a/drivers/irqchip/irq-atmel-aic5.c b/drivers/irqchip/irq-atmel-aic5.c
index a11aae8..f6ef4da 100644
--- a/drivers/irqchip/irq-atmel-aic5.c
+++ b/drivers/irqchip/irq-atmel-aic5.c
@@ -75,11 +75,11 @@ aic5_handle(struct pt_regs *regs)
 	u32 irqnr;
 	u32 irqstat;
 
-	irqnr = irq_reg_readl(gc->reg_base + AT91_AIC5_IVR);
-	irqstat = irq_reg_readl(gc->reg_base + AT91_AIC5_ISR);
+	irqnr = readl(gc->reg_base + AT91_AIC5_IVR);
+	irqstat = readl(gc->reg_base + AT91_AIC5_ISR);
 
 	if (!irqstat)
-		irq_reg_writel(0, gc->reg_base + AT91_AIC5_EOICR);
+		writel(0, gc->reg_base + AT91_AIC5_EOICR);
 	else
 		handle_domain_irq(aic5_domain, irqnr, regs);
 }
@@ -92,8 +92,8 @@ static void aic5_mask(struct irq_data *d)
 
 	/* Disable interrupt on AIC5 */
 	irq_gc_lock(gc);
-	irq_reg_writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
-	irq_reg_writel(1, gc->reg_base + AT91_AIC5_IDCR);
+	writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
+	writel(1, gc->reg_base + AT91_AIC5_IDCR);
 	gc->mask_cache &= ~d->mask;
 	irq_gc_unlock(gc);
 }
@@ -106,8 +106,8 @@ static void aic5_unmask(struct irq_data *d)
 
 	/* Enable interrupt on AIC5 */
 	irq_gc_lock(gc);
-	irq_reg_writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
-	irq_reg_writel(1, gc->reg_base + AT91_AIC5_IECR);
+	writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
+	writel(1, gc->reg_base + AT91_AIC5_IECR);
 	gc->mask_cache |= d->mask;
 	irq_gc_unlock(gc);
 }
@@ -120,8 +120,8 @@ static int aic5_retrigger(struct irq_data *d)
 
 	/* Enable interrupt on AIC5 */
 	irq_gc_lock(gc);
-	irq_reg_writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
-	irq_reg_writel(1, gc->reg_base + AT91_AIC5_ISCR);
+	writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
+	writel(1, gc->reg_base + AT91_AIC5_ISCR);
 	irq_gc_unlock(gc);
 
 	return 0;
@@ -136,11 +136,11 @@ static int aic5_set_type(struct irq_data *d, unsigned type)
 	int ret;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
-	smr = irq_reg_readl(gc->reg_base + AT91_AIC5_SMR);
+	writel(d->hwirq, gc->reg_base + AT91_AIC5_SSR);
+	smr = readl(gc->reg_base + AT91_AIC5_SMR);
 	ret = aic_common_set_type(d, type, &smr);
 	if (!ret)
-		irq_reg_writel(smr, gc->reg_base + AT91_AIC5_SMR);
+		writel(smr, gc->reg_base + AT91_AIC5_SMR);
 	irq_gc_unlock(gc);
 
 	return ret;
@@ -162,12 +162,11 @@ static void aic5_suspend(struct irq_data *d)
 		if ((mask & gc->mask_cache) == (mask & gc->wake_active))
 			continue;
 
-		irq_reg_writel(i + gc->irq_base,
-			       bgc->reg_base + AT91_AIC5_SSR);
+		writel(i + gc->irq_base, bgc->reg_base + AT91_AIC5_SSR);
 		if (mask & gc->wake_active)
-			irq_reg_writel(1, bgc->reg_base + AT91_AIC5_IECR);
+			writel(1, bgc->reg_base + AT91_AIC5_IECR);
 		else
-			irq_reg_writel(1, bgc->reg_base + AT91_AIC5_IDCR);
+			writel(1, bgc->reg_base + AT91_AIC5_IDCR);
 	}
 	irq_gc_unlock(bgc);
 }
@@ -187,12 +186,11 @@ static void aic5_resume(struct irq_data *d)
 		if ((mask & gc->mask_cache) == (mask & gc->wake_active))
 			continue;
 
-		irq_reg_writel(i + gc->irq_base,
-			       bgc->reg_base + AT91_AIC5_SSR);
+		writel(i + gc->irq_base, bgc->reg_base + AT91_AIC5_SSR);
 		if (mask & gc->mask_cache)
-			irq_reg_writel(1, bgc->reg_base + AT91_AIC5_IECR);
+			writel(1, bgc->reg_base + AT91_AIC5_IECR);
 		else
-			irq_reg_writel(1, bgc->reg_base + AT91_AIC5_IDCR);
+			writel(1, bgc->reg_base + AT91_AIC5_IDCR);
 	}
 	irq_gc_unlock(bgc);
 }
@@ -207,10 +205,9 @@ static void aic5_pm_shutdown(struct irq_data *d)
 
 	irq_gc_lock(bgc);
 	for (i = 0; i < dgc->irqs_per_chip; i++) {
-		irq_reg_writel(i + gc->irq_base,
-			       bgc->reg_base + AT91_AIC5_SSR);
-		irq_reg_writel(1, bgc->reg_base + AT91_AIC5_IDCR);
-		irq_reg_writel(1, bgc->reg_base + AT91_AIC5_ICCR);
+		writel(i + gc->irq_base, bgc->reg_base + AT91_AIC5_SSR);
+		writel(1, bgc->reg_base + AT91_AIC5_IDCR);
+		writel(1, bgc->reg_base + AT91_AIC5_ICCR);
 	}
 	irq_gc_unlock(bgc);
 }
@@ -230,24 +227,24 @@ static void __init aic5_hw_init(struct irq_domain *domain)
 	 * will not Lock out nIRQ
 	 */
 	for (i = 0; i < 8; i++)
-		irq_reg_writel(0, gc->reg_base + AT91_AIC5_EOICR);
+		writel(0, gc->reg_base + AT91_AIC5_EOICR);
 
 	/*
 	 * Spurious Interrupt ID in Spurious Vector Register.
 	 * When there is no current interrupt, the IRQ Vector Register
 	 * reads the value stored in AIC_SPU
 	 */
-	irq_reg_writel(0xffffffff, gc->reg_base + AT91_AIC5_SPU);
+	writel(0xffffffff, gc->reg_base + AT91_AIC5_SPU);
 
 	/* No debugging in AIC: Debug (Protect) Control Register */
-	irq_reg_writel(0, gc->reg_base + AT91_AIC5_DCR);
+	writel(0, gc->reg_base + AT91_AIC5_DCR);
 
 	/* Disable and clear all interrupts initially */
 	for (i = 0; i < domain->revmap_size; i++) {
-		irq_reg_writel(i, gc->reg_base + AT91_AIC5_SSR);
-		irq_reg_writel(i, gc->reg_base + AT91_AIC5_SVR);
-		irq_reg_writel(1, gc->reg_base + AT91_AIC5_IDCR);
-		irq_reg_writel(1, gc->reg_base + AT91_AIC5_ICCR);
+		writel(i, gc->reg_base + AT91_AIC5_SSR);
+		writel(i, gc->reg_base + AT91_AIC5_SVR);
+		writel(1, gc->reg_base + AT91_AIC5_IDCR);
+		writel(1, gc->reg_base + AT91_AIC5_ICCR);
 	}
 }
 
@@ -273,11 +270,11 @@ static int aic5_irq_domain_xlate(struct irq_domain *d,
 	gc = dgc->gc[0];
 
 	irq_gc_lock(gc);
-	irq_reg_writel(*out_hwirq, gc->reg_base + AT91_AIC5_SSR);
-	smr = irq_reg_readl(gc->reg_base + AT91_AIC5_SMR);
+	writel(*out_hwirq, gc->reg_base + AT91_AIC5_SSR);
+	smr = readl(gc->reg_base + AT91_AIC5_SMR);
 	ret = aic_common_set_priority(intspec[2], &smr);
 	if (!ret)
-		irq_reg_writel(intspec[2] | smr, gc->reg_base + AT91_AIC5_SMR);
+		writel(intspec[2] | smr, gc->reg_base + AT91_AIC5_SMR);
 	irq_gc_unlock(gc);
 
 	return ret;
diff --git a/drivers/irqchip/irq-sunxi-nmi.c b/drivers/irqchip/irq-sunxi-nmi.c
index 12f547a..776e3f0 100644
--- a/drivers/irqchip/irq-sunxi-nmi.c
+++ b/drivers/irqchip/irq-sunxi-nmi.c
@@ -50,12 +50,12 @@ static struct sunxi_sc_nmi_reg_offs sun6i_reg_offs = {
 static inline void sunxi_sc_nmi_write(struct irq_chip_generic *gc, u32 off,
 				      u32 val)
 {
-	irq_reg_writel(val, gc->reg_base + off);
+	writel(val, gc->reg_base + off);
 }
 
 static inline u32 sunxi_sc_nmi_read(struct irq_chip_generic *gc, u32 off)
 {
-	return irq_reg_readl(gc->reg_base + off);
+	return readl(gc->reg_base + off);
 }
 
 static void sunxi_sc_nmi_handle_irq(unsigned int irq, struct irq_desc *desc)
diff --git a/drivers/irqchip/irq-tb10x.c b/drivers/irqchip/irq-tb10x.c
index 7c44c99..8cd995d 100644
--- a/drivers/irqchip/irq-tb10x.c
+++ b/drivers/irqchip/irq-tb10x.c
@@ -43,12 +43,12 @@
 static inline void ab_irqctl_writereg(struct irq_chip_generic *gc, u32 reg,
 	u32 val)
 {
-	irq_reg_writel(val, gc->reg_base + reg);
+	writel(val, gc->reg_base + reg);
 }
 
 static inline u32 ab_irqctl_readreg(struct irq_chip_generic *gc, u32 reg)
 {
-	return irq_reg_readl(gc->reg_base + reg);
+	return readl(gc->reg_base + reg);
 }
 
 static int tb10x_irq_set_type(struct irq_data *data, unsigned int flow_type)
-- 
2.1.1

[PATCH V2 02/15] sh: Eliminate unused irq_reg_{readl,writel} accessors

[Kevin Cernekee]

Defining these macros way down in arch/sh/.../irq.c doesn't cause
kernel/irq/generic-chip.c to use them.  As far as I can tell this code
has no effect.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 arch/sh/boards/mach-se/7343/irq.c | 3 ---
 arch/sh/boards/mach-se/7722/irq.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/arch/sh/boards/mach-se/7343/irq.c b/arch/sh/boards/mach-se/7343/irq.c
index 7646bf0..1087dba 100644
--- a/arch/sh/boards/mach-se/7343/irq.c
+++ b/arch/sh/boards/mach-se/7343/irq.c
@@ -14,9 +14,6 @@
 #define DRV_NAME "SE7343-FPGA"
 #define pr_fmt(fmt) DRV_NAME ": " fmt
 
-#define irq_reg_readl	ioread16
-#define irq_reg_writel	iowrite16
-
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
diff --git a/arch/sh/boards/mach-se/7722/irq.c b/arch/sh/boards/mach-se/7722/irq.c
index f5e2af1..00e6992 100644
--- a/arch/sh/boards/mach-se/7722/irq.c
+++ b/arch/sh/boards/mach-se/7722/irq.c
@@ -11,9 +11,6 @@
 #define DRV_NAME "SE7722-FPGA"
 #define pr_fmt(fmt) DRV_NAME ": " fmt
 
-#define irq_reg_readl	ioread16
-#define irq_reg_writel	iowrite16
-
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/interrupt.h>
-- 
2.1.1

Re: [PATCH V2 02/15] sh: Eliminate unused irq_reg_{readl,writel} accessors

[Arnd Bergmann]

On Wednesday 29 October 2014 19:17:55 Kevin Cernekee wrote:
> Defining these macros way down in arch/sh/.../irq.c doesn't cause
> kernel/irq/generic-chip.c to use them.  As far as I can tell this code
> has no effect.
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>

Actually it overrides the 32-bit accessors with 16-bit accessors,
which does seem intentional and certainly has an effect.

	Arnd

>  arch/sh/boards/mach-se/7343/irq.c | 3 ---
>  arch/sh/boards/mach-se/7722/irq.c | 3 ---
>  2 files changed, 6 deletions(-)
> 
> diff --git a/arch/sh/boards/mach-se/7343/irq.c b/arch/sh/boards/mach-se/7343/irq.c
> index 7646bf0..1087dba 100644
> --- a/arch/sh/boards/mach-se/7343/irq.c
> +++ b/arch/sh/boards/mach-se/7343/irq.c
> @@ -14,9 +14,6 @@
>  #define DRV_NAME "SE7343-FPGA"
>  #define pr_fmt(fmt) DRV_NAME ": " fmt
>  
> -#define irq_reg_readl  ioread16
> -#define irq_reg_writel iowrite16
> -
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/interrupt.h>

Re: [PATCH V2 02/15] sh: Eliminate unused irq_reg_{readl,writel} accessors

[Thomas Gleixner]

On Thu, 30 Oct 2014, Arnd Bergmann wrote:

> On Wednesday 29 October 2014 19:17:55 Kevin Cernekee wrote:
> > Defining these macros way down in arch/sh/.../irq.c doesn't cause
> > kernel/irq/generic-chip.c to use them.  As far as I can tell this code
> > has no effect.
> > 
> > Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> 
> Actually it overrides the 32-bit accessors with 16-bit accessors,
> which does seem intentional and certainly has an effect.

Not really. Neither arch/sh/boards/mach-se/7343/irq.c nor
arch/sh/boards/mach-se/7722/irq.c actually use
irq_reg_readl/writel. They simply define it.
 
Thanks,

	tglx

Re: [PATCH V2 02/15] sh: Eliminate unused irq_reg_{readl,writel} accessors

[Arnd Bergmann]

On Thursday 30 October 2014 11:43:00 Thomas Gleixner wrote:
> On Thu, 30 Oct 2014, Arnd Bergmann wrote:
> 
> > On Wednesday 29 October 2014 19:17:55 Kevin Cernekee wrote:
> > > Defining these macros way down in arch/sh/.../irq.c doesn't cause
> > > kernel/irq/generic-chip.c to use them.  As far as I can tell this code
> > > has no effect.
> > > 
> > > Signed-off-by: Kevin Cernekee <cernekee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > Actually it overrides the 32-bit accessors with 16-bit accessors,
> > which does seem intentional and certainly has an effect.
> 
> Not really. Neither arch/sh/boards/mach-se/7343/irq.c nor
> arch/sh/boards/mach-se/7722/irq.c actually use
> irq_reg_readl/writel. They simply define it.

Ah, that makes things easier. I looked at the commits that introduced
them, and even then they were unused. Probably an artifact from an
earlier version of the patch which did not get merged.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 02/15] sh: Eliminate unused irq_reg_{readl,writel} accessors

[Kevin Cernekee]

On Thu, Oct 30, 2014 at 3:48 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thursday 30 October 2014 11:43:00 Thomas Gleixner wrote:
>> On Thu, 30 Oct 2014, Arnd Bergmann wrote:
>>
>> > On Wednesday 29 October 2014 19:17:55 Kevin Cernekee wrote:
>> > > Defining these macros way down in arch/sh/.../irq.c doesn't cause
>> > > kernel/irq/generic-chip.c to use them.  As far as I can tell this code
>> > > has no effect.
>> > >
>> > > Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
>> >
>> > Actually it overrides the 32-bit accessors with 16-bit accessors,
>> > which does seem intentional and certainly has an effect.
>>
>> Not really. Neither arch/sh/boards/mach-se/7343/irq.c nor
>> arch/sh/boards/mach-se/7722/irq.c actually use
>> irq_reg_readl/writel. They simply define it.
>
> Ah, that makes things easier. I looked at the commits that introduced
> them, and even then they were unused. Probably an artifact from an
> earlier version of the patch which did not get merged.

I suspect that the intention was to put them into the machine's
<irq.h> so that generic-chip.c would pick them up.  The sh irq.c files
do call ioread16/iowrite16 themselves, and like bcm7120-l2, they
utilize irq_gc_mask_{set,clr}_bit from generic-chip.c.

This may be something for the maintainers (Paul?) to double-check, but
if the submission worked as-is, maybe overriding the I/O accessors
wasn't actually needed.  Or maybe there are subtle and unfortunate
side effects on adjacent registers.

[PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c

[Kevin Cernekee]

This allows us to implement per-irqchip behavior when necessary, instead
of hardcoding the behavior for all irqchip drivers at compile time.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 include/linux/irq.h       |  7 -------
 kernel/irq/generic-chip.c | 10 ++++++++++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 03f48d9..8049e93 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq);
 void irq_init_desc(unsigned int irq);
 #endif
 
-#ifndef irq_reg_writel
-# define irq_reg_writel(val, addr)	writel(val, addr)
-#endif
-#ifndef irq_reg_readl
-# define irq_reg_readl(addr)		readl(addr)
-#endif
-
 /**
  * struct irq_chip_regs - register offsets for struct irq_gci
  * @enable:	Enable register offset to reg_base
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index cf80e7b..380595f 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -17,6 +17,16 @@
 static LIST_HEAD(gc_list);
 static DEFINE_RAW_SPINLOCK(gc_lock);
 
+static void irq_reg_writel(u32 val, void __iomem *addr)
+{
+	writel(val, addr);
+}
+
+static u32 irq_reg_readl(void __iomem *addr)
+{
+	return readl(addr);
+}
+
 /**
  * irq_gc_noop - NOOP function
  * @d: irq_data
-- 
2.1.1

Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c

[Thomas Gleixner]

On Wed, 29 Oct 2014, Kevin Cernekee wrote:

> This allows us to implement per-irqchip behavior when necessary, instead
> of hardcoding the behavior for all irqchip drivers at compile time.
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  include/linux/irq.h       |  7 -------
>  kernel/irq/generic-chip.c | 10 ++++++++++
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 03f48d9..8049e93 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq);
>  void irq_init_desc(unsigned int irq);
>  #endif
>  
> -#ifndef irq_reg_writel
> -# define irq_reg_writel(val, addr)	writel(val, addr)
> -#endif
> -#ifndef irq_reg_readl
> -# define irq_reg_readl(addr)		readl(addr)
> -#endif
> -

Brilliant patch that.

# git grep -l irq_reg_readl drivers/irqchip/
drivers/irqchip/irq-atmel-aic.c
drivers/irqchip/irq-atmel-aic5.c
drivers/irqchip/irq-sunxi-nmi.c
drivers/irqchip/irq-tb10x.c

Thanks,

	tglx

Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c

[Arnd Bergmann]

On Thursday 30 October 2014 09:43:02 Thomas Gleixner wrote:
> > diff --git a/include/linux/irq.h b/include/linux/irq.h
> > index 03f48d9..8049e93 100644
> > --- a/include/linux/irq.h
> > +++ b/include/linux/irq.h
> > @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq);
> >  void irq_init_desc(unsigned int irq);
> >  #endif
> >  
> > -#ifndef irq_reg_writel
> > -# define irq_reg_writel(val, addr)   writel(val, addr)
> > -#endif
> > -#ifndef irq_reg_readl
> > -# define irq_reg_readl(addr)         readl(addr)
> > -#endif
> > -
> 
> Brilliant patch that.
> 
> # git grep -l irq_reg_readl drivers/irqchip/
> drivers/irqchip/irq-atmel-aic.c
> drivers/irqchip/irq-atmel-aic5.c
> drivers/irqchip/irq-sunxi-nmi.c
> drivers/irqchip/irq-tb10x.c

Patch 1/15 changes these all. I think it's still broken because patch 2/15
is wrong, but it's not /that/ obviously broken.

	Arnd

Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c

[Thomas Gleixner]

On Thu, 30 Oct 2014, Arnd Bergmann wrote:
> On Thursday 30 October 2014 09:43:02 Thomas Gleixner wrote:
> > > diff --git a/include/linux/irq.h b/include/linux/irq.h
> > > index 03f48d9..8049e93 100644
> > > --- a/include/linux/irq.h
> > > +++ b/include/linux/irq.h
> > > @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq);
> > >  void irq_init_desc(unsigned int irq);
> > >  #endif
> > >  
> > > -#ifndef irq_reg_writel
> > > -# define irq_reg_writel(val, addr)   writel(val, addr)
> > > -#endif
> > > -#ifndef irq_reg_readl
> > > -# define irq_reg_readl(addr)         readl(addr)
> > > -#endif
> > > -
> > 
> > Brilliant patch that.
> > 
> > # git grep -l irq_reg_readl drivers/irqchip/
> > drivers/irqchip/irq-atmel-aic.c
> > drivers/irqchip/irq-atmel-aic5.c
> > drivers/irqchip/irq-sunxi-nmi.c
> > drivers/irqchip/irq-tb10x.c
> 
> Patch 1/15 changes these all. I think it's still broken because patch 2/15
> is wrong, but it's not /that/ obviously broken.

Oops. Missed that.

Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c

[Thomas Gleixner]

On Thu, 30 Oct 2014, Thomas Gleixner wrote:
> On Thu, 30 Oct 2014, Arnd Bergmann wrote:
> > On Thursday 30 October 2014 09:43:02 Thomas Gleixner wrote:
> > > > diff --git a/include/linux/irq.h b/include/linux/irq.h
> > > > index 03f48d9..8049e93 100644
> > > > --- a/include/linux/irq.h
> > > > +++ b/include/linux/irq.h
> > > > @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq);
> > > >  void irq_init_desc(unsigned int irq);
> > > >  #endif
> > > >  
> > > > -#ifndef irq_reg_writel
> > > > -# define irq_reg_writel(val, addr)   writel(val, addr)
> > > > -#endif
> > > > -#ifndef irq_reg_readl
> > > > -# define irq_reg_readl(addr)         readl(addr)
> > > > -#endif
> > > > -
> > > 
> > > Brilliant patch that.
> > > 
> > > # git grep -l irq_reg_readl drivers/irqchip/
> > > drivers/irqchip/irq-atmel-aic.c
> > > drivers/irqchip/irq-atmel-aic5.c
> > > drivers/irqchip/irq-sunxi-nmi.c
> > > drivers/irqchip/irq-tb10x.c
> > 
> > Patch 1/15 changes these all. I think it's still broken because patch 2/15
> > is wrong, but it's not /that/ obviously broken.
> 
> Oops. Missed that.

But looking at: drivers/irqchip/irq-sunxi-nmi.c

It's using the generic chip. So while the generic chip uses the
accessor function, the driver implementation which implements extra
functionality will use something hardcoded. While it does not break
the current setup, it's wrong nevertheless.

Thanks,

	tglx

[PATCH V2 04/15] genirq: Generic chip: Change irq_reg_{readl,writel} arguments

[Kevin Cernekee]

Instead of taking a raw register virtual address, we will take an
irq_chip_generic struct and a register offset.  This makes it possible to
implement different behavior on different irqchips.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 kernel/irq/generic-chip.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index 380595f..b2ee65d 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -17,14 +17,16 @@
 static LIST_HEAD(gc_list);
 static DEFINE_RAW_SPINLOCK(gc_lock);
 
-static void irq_reg_writel(u32 val, void __iomem *addr)
+static void irq_reg_writel(struct irq_chip_generic *gc,
+			   u32 val, int reg_offset)
 {
-	writel(val, addr);
+	writel(val, gc->reg_base + reg_offset);
 }
 
-static u32 irq_reg_readl(void __iomem *addr)
+static u32 irq_reg_readl(struct irq_chip_generic *gc,
+			 int reg_offset)
 {
-	return readl(addr);
+	return readl(gc->reg_base + reg_offset);
 }
 
 /**
@@ -49,7 +51,7 @@ void irq_gc_mask_disable_reg(struct irq_data *d)
 	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.disable);
+	irq_reg_writel(gc, mask, ct->regs.disable);
 	*ct->mask_cache &= ~mask;
 	irq_gc_unlock(gc);
 }
@@ -69,7 +71,7 @@ void irq_gc_mask_set_bit(struct irq_data *d)
 
 	irq_gc_lock(gc);
 	*ct->mask_cache |= mask;
-	irq_reg_writel(*ct->mask_cache, gc->reg_base + ct->regs.mask);
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
 	irq_gc_unlock(gc);
 }
 EXPORT_SYMBOL_GPL(irq_gc_mask_set_bit);
@@ -89,7 +91,7 @@ void irq_gc_mask_clr_bit(struct irq_data *d)
 
 	irq_gc_lock(gc);
 	*ct->mask_cache &= ~mask;
-	irq_reg_writel(*ct->mask_cache, gc->reg_base + ct->regs.mask);
+	irq_reg_writel(gc, *ct->mask_cache, ct->regs.mask);
 	irq_gc_unlock(gc);
 }
 EXPORT_SYMBOL_GPL(irq_gc_mask_clr_bit);
@@ -108,7 +110,7 @@ void irq_gc_unmask_enable_reg(struct irq_data *d)
 	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.enable);
+	irq_reg_writel(gc, mask, ct->regs.enable);
 	*ct->mask_cache |= mask;
 	irq_gc_unlock(gc);
 }
@@ -124,7 +126,7 @@ void irq_gc_ack_set_bit(struct irq_data *d)
 	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.ack);
+	irq_reg_writel(gc, mask, ct->regs.ack);
 	irq_gc_unlock(gc);
 }
 EXPORT_SYMBOL_GPL(irq_gc_ack_set_bit);
@@ -140,7 +142,7 @@ void irq_gc_ack_clr_bit(struct irq_data *d)
 	u32 mask = ~d->mask;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.ack);
+	irq_reg_writel(gc, mask, ct->regs.ack);
 	irq_gc_unlock(gc);
 }
 
@@ -155,8 +157,8 @@ void irq_gc_mask_disable_reg_and_ack(struct irq_data *d)
 	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.mask);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.ack);
+	irq_reg_writel(gc, mask, ct->regs.mask);
+	irq_reg_writel(gc, mask, ct->regs.ack);
 	irq_gc_unlock(gc);
 }
 
@@ -171,7 +173,7 @@ void irq_gc_eoi(struct irq_data *d)
 	u32 mask = d->mask;
 
 	irq_gc_lock(gc);
-	irq_reg_writel(mask, gc->reg_base + ct->regs.eoi);
+	irq_reg_writel(gc, mask, ct->regs.eoi);
 	irq_gc_unlock(gc);
 }
 
@@ -255,7 +257,7 @@ irq_gc_init_mask_cache(struct irq_chip_generic *gc, enum irq_gc_flags flags)
 		}
 		ct[i].mask_cache = mskptr;
 		if (flags & IRQ_GC_INIT_MASK_CACHE)
-			*mskptr = irq_reg_readl(gc->reg_base + mskreg);
+			*mskptr = irq_reg_readl(gc, mskreg);
 	}
 }
 
-- 
2.1.1

[PATCH V2 05/15] genirq: Generic chip: Add big endian I/O accessors

[Kevin Cernekee]

Use io{read,write}32be if the caller specified IRQ_GC_BE_IO when creating
the irqchip.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 include/linux/irq.h       |  1 +
 kernel/irq/generic-chip.c | 15 +++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 8049e93..e69b7b2 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -739,6 +739,7 @@ enum irq_gc_flags {
 	IRQ_GC_INIT_NESTED_LOCK		= 1 << 1,
 	IRQ_GC_MASK_CACHE_PER_TYPE	= 1 << 2,
 	IRQ_GC_NO_MASK			= 1 << 3,
+	IRQ_GC_BE_IO			= 1 << 4,
 };
 
 /*
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index b2ee65d..c1890bb 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -17,16 +17,27 @@
 static LIST_HEAD(gc_list);
 static DEFINE_RAW_SPINLOCK(gc_lock);
 
+static int is_big_endian(struct irq_chip_generic *gc)
+{
+	return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
+}
+
 static void irq_reg_writel(struct irq_chip_generic *gc,
 			   u32 val, int reg_offset)
 {
-	writel(val, gc->reg_base + reg_offset);
+	if (is_big_endian(gc))
+		iowrite32be(val, gc->reg_base + reg_offset);
+	else
+		writel(val, gc->reg_base + reg_offset);
 }
 
 static u32 irq_reg_readl(struct irq_chip_generic *gc,
 			 int reg_offset)
 {
-	return readl(gc->reg_base + reg_offset);
+	if (is_big_endian(gc))
+		return ioread32be(gc->reg_base + reg_offset);
+	else
+		return readl(gc->reg_base + reg_offset);
 }
 
 /**
-- 
2.1.1

Re: [PATCH V2 05/15] genirq: Generic chip: Add big endian I/O accessors

[Arnd Bergmann]

On Wednesday 29 October 2014 19:17:58 Kevin Cernekee wrote:
>  static LIST_HEAD(gc_list);
>  static DEFINE_RAW_SPINLOCK(gc_lock);
>  
> +static int is_big_endian(struct irq_chip_generic *gc)
> +{
> +       return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
> +}
> +
>  static void irq_reg_writel(struct irq_chip_generic *gc,
>                            u32 val, int reg_offset)
>  {
> -       writel(val, gc->reg_base + reg_offset);
> +       if (is_big_endian(gc))
> +               iowrite32be(val, gc->reg_base + reg_offset);
> +       else
> +               writel(val, gc->reg_base + reg_offset);
>  }
>  

What I had in mind was to use indirect function calls instead, like

#ifndef irq_reg_writel
static inline void irq_reg_writel_le(u32 val, void __iomem *addr)
{
	return writel(val, addr);
}
#endif

#ifndef irq_reg_writel_be
static inline void irq_reg_writel_be(u32 val, void __iomem *addr)
{
	return iowrite32_be(val, addr);
}
#endif


static inline void irq_reg_writel(struct irq_chip_generic *gc, u32 val, int reg_offset)
{
       if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
           !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
		return irq_reg_writel_le(val, gc->reg_base + reg_offset);

       if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
           !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
		return irq_reg_writel_be(val, gc->reg_base + reg_offset);

	return gc->writel(val, gc->reg_base + reg_offset);
}

This would take the condition out of the callers.

	Arnd

Re: [PATCH V2 05/15] genirq: Generic chip: Add big endian I/O accessors

[Thomas Gleixner]

On Thu, 30 Oct 2014, Arnd Bergmann wrote:
> On Wednesday 29 October 2014 19:17:58 Kevin Cernekee wrote:
> >  static LIST_HEAD(gc_list);
> >  static DEFINE_RAW_SPINLOCK(gc_lock);
> >  
> > +static int is_big_endian(struct irq_chip_generic *gc)
> > +{
> > +       return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
> > +}
> > +
> >  static void irq_reg_writel(struct irq_chip_generic *gc,
> >                            u32 val, int reg_offset)
> >  {
> > -       writel(val, gc->reg_base + reg_offset);
> > +       if (is_big_endian(gc))
> > +               iowrite32be(val, gc->reg_base + reg_offset);
> > +       else
> > +               writel(val, gc->reg_base + reg_offset);
> >  }
> >  
> 
> What I had in mind was to use indirect function calls instead, like
> 
> #ifndef irq_reg_writel
> static inline void irq_reg_writel_le(u32 val, void __iomem *addr)
> {
> 	return writel(val, addr);
> }
> #endif
> 
> #ifndef irq_reg_writel_be
> static inline void irq_reg_writel_be(u32 val, void __iomem *addr)
> {
> 	return iowrite32_be(val, addr);
> }
> #endif
> 
> 
> static inline void irq_reg_writel(struct irq_chip_generic *gc, u32 val, int reg_offset)
> {
>        if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&

That's inside of the generic irq chip, so CONFIG_GENERIC_IRQ_CHIP is
always set when this is compiled.

>            !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
> 		return irq_reg_writel_le(val, gc->reg_base + reg_offset);
> 
>        if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
>            !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))

	     s/!// ?

> 		return irq_reg_writel_be(val, gc->reg_base + reg_offset);

I don't think the above will cover all combinations.

..._CHIP_BE	...CHIP_LE
N		N			; Default behaviour: readl/writel
Y		N			; ioread/write32be
N		Y			; Default behaviour: readl/writel
Y		Y			; Runtime selected

> 	return gc->writel(val, gc->reg_base + reg_offset);
> }
> 
> This would take the condition out of the callers.

So you trade a conditional for an indirect call. Not sure what's more
expensive. The indirect call is definitely a smaller text footprint,
so we should opt for this.

Thanks,

	tglx

Re: [PATCH V2 05/15] genirq: Generic chip: Add big endian I/O accessors

[Arnd Bergmann]

On Thursday 30 October 2014 13:30:04 Thomas Gleixner wrote:
> On Thu, 30 Oct 2014, Arnd Bergmann wrote:
> > On Wednesday 29 October 2014 19:17:58 Kevin Cernekee wrote:
> > >  static LIST_HEAD(gc_list);
> > >  static DEFINE_RAW_SPINLOCK(gc_lock);
> > >  
> > > +static int is_big_endian(struct irq_chip_generic *gc)
> > > +{
> > > +       return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
> > > +}
> > > +
> > >  static void irq_reg_writel(struct irq_chip_generic *gc,
> > >                            u32 val, int reg_offset)
> > >  {
> > > -       writel(val, gc->reg_base + reg_offset);
> > > +       if (is_big_endian(gc))
> > > +               iowrite32be(val, gc->reg_base + reg_offset);
> > > +       else
> > > +               writel(val, gc->reg_base + reg_offset);
> > >  }
> > >  
> > 
> > What I had in mind was to use indirect function calls instead, like
> > 
> > #ifndef irq_reg_writel
> > static inline void irq_reg_writel_le(u32 val, void __iomem *addr)
> > {
> > 	return writel(val, addr);
> > }
> > #endif
> > 
> > #ifndef irq_reg_writel_be
> > static inline void irq_reg_writel_be(u32 val, void __iomem *addr)
> > {
> > 	return iowrite32_be(val, addr);
> > }
> > #endif
> > 
> > 
> > static inline void irq_reg_writel(struct irq_chip_generic *gc, u32 val, int reg_offset)
> > {
> >        if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
> 
> That's inside of the generic irq chip, so CONFIG_GENERIC_IRQ_CHIP is
> always set when this is compiled.

The part that I mentioned in the other mail and omitted here is that
I'd then build the kernel/irq/generic-chip.c file when one or both of
CONFIG_GENERIC_IRQ_CHIP or CONFIG_GENERIC_IRQ_CHIP_BE is set.

The alternative would be to introduce CONFIG_GENERIC_IRQ_CHIP_LE along
with CONFIG_GENERIC_IRQ_CHIP_BE, which might be cleaner, but requires
all existing 39 'select GENERIC_IRQ_CHIP' statements to be changed to
'GENERIC_IRQ_CHIP_LE'.

Either way would work.

> >            !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
> > 		return irq_reg_writel_le(val, gc->reg_base + reg_offset);
> > 
> >        if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
> >            !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
> 
> 	     s/!// ?

typo: I put the ! in the wrong line, sorry.

> > 		return irq_reg_writel_be(val, gc->reg_base + reg_offset);
> 
> I don't think the above will cover all combinations.
> 
> ..._CHIP_BE	...CHIP_LE
> N		N			; Default behaviour: readl/writel

that would not be allowed with my approach. It should probably cause
a compile-error if we introduce all three symbols.

> Y		N			; ioread/write32be
> N		Y			; Default behaviour: readl/writel
> Y		Y			; Runtime selected



> > 	return gc->writel(val, gc->reg_base + reg_offset);
> > }
> > 
> > This would take the condition out of the callers.
> 
> So you trade a conditional for an indirect call. Not sure what's more
> expensive. The indirect call is definitely a smaller text footprint,
> so we should opt for this.

It depends on the register pressure in the caller and on the pipeline
of the CPU. My guess was that indirect call is generally more efficient,
but you are right that this is not obvious, and I have no reliable data
to back up my guess.

If we do the conditional, we could also just add an extra byte swap,
instead of choosing between two function calls.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 06/15] genirq: Generic chip: Optimize for fixed-endian systems

[Kevin Cernekee]

Allow the compiler to inline an LE MMIO access if the configuration only
supports LE registers, or a BE MMIO access if the configuration only
supports BE registers.  If the configuration supports both (possibly
a multiplatform kernel) then make the decision at runtime.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 kernel/irq/Kconfig        |  5 +++++
 kernel/irq/Makefile       |  1 +
 kernel/irq/generic-chip.c | 10 +++++++++-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 225086b..6283c8c 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -51,6 +51,11 @@ config GENERIC_IRQ_CHIP
        bool
        select IRQ_DOMAIN
 
+# Same as above, but use big-endian MMIO accesses
+config GENERIC_IRQ_CHIP_BE
+       bool
+       select IRQ_DOMAIN
+
 # Generic irq_domain hw <--> linux irq number translation
 config IRQ_DOMAIN
 	bool
diff --git a/kernel/irq/Makefile b/kernel/irq/Makefile
index fff1738..34c2b0f 100644
--- a/kernel/irq/Makefile
+++ b/kernel/irq/Makefile
@@ -1,6 +1,7 @@
 
 obj-y := irqdesc.o handle.o manage.o spurious.o resend.o chip.o dummychip.o devres.o
 obj-$(CONFIG_GENERIC_IRQ_CHIP) += generic-chip.o
+obj-$(CONFIG_GENERIC_IRQ_CHIP_BE) += generic-chip.o
 obj-$(CONFIG_GENERIC_IRQ_PROBE) += autoprobe.o
 obj-$(CONFIG_IRQ_DOMAIN) += irqdomain.o
 obj-$(CONFIG_PROC_FS) += proc.o
diff --git a/kernel/irq/generic-chip.c b/kernel/irq/generic-chip.c
index c1890bb..457ea48 100644
--- a/kernel/irq/generic-chip.c
+++ b/kernel/irq/generic-chip.c
@@ -9,6 +9,7 @@
 #include <linux/export.h>
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
+#include <linux/kconfig.h>
 #include <linux/kernel_stat.h>
 #include <linux/syscore_ops.h>
 
@@ -19,7 +20,14 @@ static DEFINE_RAW_SPINLOCK(gc_lock);
 
 static int is_big_endian(struct irq_chip_generic *gc)
 {
-	return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
+	if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
+	    !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
+		return 0;
+	else if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE) &&
+		 !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP))
+		return 1;
+	else
+		return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
 }
 
 static void irq_reg_writel(struct irq_chip_generic *gc,
-- 
2.1.1

Re: [PATCH V2 06/15] genirq: Generic chip: Optimize for fixed-endian systems

[Brian Norris]

On Wed, Oct 29, 2014 at 07:17:59PM -0700, Kevin Cernekee wrote:
> @@ -19,7 +20,14 @@ static DEFINE_RAW_SPINLOCK(gc_lock);
>  
>  static int is_big_endian(struct irq_chip_generic *gc)
>  {
> -	return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
> +	if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
> +	    !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
> +		return 0;
> +	else if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE) &&
> +		 !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP))
> +		return 1;

Would XOR make this any easier to read? e.g.:

	if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) ^
	    IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
		return IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE);
	else
		...

> +	else
> +		return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
>  }
>  
>  static void irq_reg_writel(struct irq_chip_generic *gc,

Brian

Re: [PATCH V2 06/15] genirq: Generic chip: Optimize for fixed-endian systems

[Arnd Bergmann]

On Wednesday 29 October 2014 21:16:58 Brian Norris wrote:
> >  static int is_big_endian(struct irq_chip_generic *gc)
> >  {
> > -     return !!(gc->domain->gc->gc_flags & IRQ_GC_BE_IO);
> > +     if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) &&
> > +         !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
> > +             return 0;
> > +     else if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE) &&
> > +              !IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP))
> > +             return 1;
> 
> Would XOR make this any easier to read? e.g.:
> 
>         if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP) ^
>             IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
>                 return IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE);
>         else
>                 ...
> 

I think that would only be easier to read for the compiler, not for
for a human. ;-)

	Arnd

[PATCH V2 07/15] irqchip: brcmstb-l2: Eliminate dependency on ARM code

[Kevin Cernekee]

The irq-brcmstb-l2 driver has a single dependency on the ARM code, the
do_bad_IRQ macro.  Expand this macro in-place so that the driver can be
built on non-ARM platforms.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/irq-brcmstb-l2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
index c15c840..c9bdf20 100644
--- a/drivers/irqchip/irq-brcmstb-l2.c
+++ b/drivers/irqchip/irq-brcmstb-l2.c
@@ -19,6 +19,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/spinlock.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
@@ -30,8 +31,6 @@
 #include <linux/irqchip.h>
 #include <linux/irqchip/chained_irq.h>
 
-#include <asm/mach/irq.h>
-
 #include "irqchip.h"
 
 /* Register offsets in the L2 interrupt controller */
@@ -63,7 +62,9 @@ static void brcmstb_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
 		~(__raw_readl(b->base + CPU_MASK_STATUS));
 
 	if (status == 0) {
-		do_bad_IRQ(irq, desc);
+		raw_spin_lock(&desc->lock);
+		handle_bad_irq(irq, desc);
+		raw_spin_unlock(&desc->lock);
 		goto out;
 	}
 
-- 
2.1.1

[PATCH V2 08/15] irqchip: bcm7120-l2: Eliminate bad IRQ check

[Kevin Cernekee]

This check may be prone to race conditions, e.g.

1) Some external event (e.g. GPIO level) causes an IRQ to become pending
2) Peripheral asserts the L2 IRQ
3) CPU takes an interrupt
4) The event from #1 goes away
5) bcm7120_l2_intc_irq_handle() reads back a 0 status

Unlike the hardware supported by brcmstb-l2, the bcm7120-l2 controller
does not latch the IRQ status.  Bits can change if the inputs to the
controller change.  Also, do_bad_IRQ() is an ARM-specific macro.

So let's just nuke it.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/irq-bcm7120-l2.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index b9f4fb8..49d8f3d 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -27,8 +27,6 @@
 
 #include "irqchip.h"
 
-#include <asm/mach/irq.h>
-
 /* Register offset in the L2 interrupt controller */
 #define IRQEN		0x00
 #define IRQSTAT		0x04
@@ -51,19 +49,12 @@ static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
 	chained_irq_enter(chip, desc);
 
 	status = __raw_readl(b->base + IRQSTAT);
-
-	if (status == 0) {
-		do_bad_IRQ(irq, desc);
-		goto out;
-	}
-
 	do {
 		irq = ffs(status) - 1;
 		status &= ~(1 << irq);
 		generic_handle_irq(irq_find_mapping(b->domain, irq));
 	} while (status);
 
-out:
 	chained_irq_exit(chip, desc);
 }
 
-- 
2.1.1

Re: [PATCH V2 08/15] irqchip: bcm7120-l2: Eliminate bad IRQ check

[Sergei Shtylyov]

Hello.

On 10/30/2014 5:18 AM, Kevin Cernekee wrote:

> This check may be prone to race conditions, e.g.

> 1) Some external event (e.g. GPIO level) causes an IRQ to become pending
> 2) Peripheral asserts the L2 IRQ
> 3) CPU takes an interrupt
> 4) The event from #1 goes away
> 5) bcm7120_l2_intc_irq_handle() reads back a 0 status

> Unlike the hardware supported by brcmstb-l2, the bcm7120-l2 controller
> does not latch the IRQ status.  Bits can change if the inputs to the
> controller change.  Also, do_bad_IRQ() is an ARM-specific macro.

> So let's just nuke it.

> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/irqchip/irq-bcm7120-l2.c | 9 ---------
>   1 file changed, 9 deletions(-)

> diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
> index b9f4fb8..49d8f3d 100644
> --- a/drivers/irqchip/irq-bcm7120-l2.c
> +++ b/drivers/irqchip/irq-bcm7120-l2.c
[...]
> @@ -51,19 +49,12 @@ static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
>   	chained_irq_enter(chip, desc);
>
>   	status = __raw_readl(b->base + IRQSTAT);
> -
> -	if (status == 0) {
> -		do_bad_IRQ(irq, desc);
> -		goto out;
> -	}
> -
>   	do {

    I think this needs to now become:

	while (status) {

>   		irq = ffs(status) - 1;
>   		status &= ~(1 << irq);

    In case 'status' is 0, 'irq' will be equal to -1. How does the shift by 
negative value work?

>   		generic_handle_irq(irq_find_mapping(b->domain, irq));
>   	} while (status);

WBR, Sergei

Re: [PATCH V2 08/15] irqchip: bcm7120-l2: Eliminate bad IRQ check

[Kevin Cernekee]

On Thu, Oct 30, 2014 at 4:09 AM, Sergei Shtylyov wrote:
>> diff --git a/drivers/irqchip/irq-bcm7120-l2.c
>> b/drivers/irqchip/irq-bcm7120-l2.c
>> index b9f4fb8..49d8f3d 100644
>> --- a/drivers/irqchip/irq-bcm7120-l2.c
>> +++ b/drivers/irqchip/irq-bcm7120-l2.c
>
> [...]
>>
>> @@ -51,19 +49,12 @@ static void bcm7120_l2_intc_irq_handle(unsigned int
>> irq, struct irq_desc *desc)
>>         chained_irq_enter(chip, desc);
>>
>>         status = __raw_readl(b->base + IRQSTAT);
>> -
>> -       if (status == 0) {
>> -               do_bad_IRQ(irq, desc);
>> -               goto out;
>> -       }
>> -
>>         do {
>
>
>    I think this needs to now become:
>
>         while (status) {
>
>>                 irq = ffs(status) - 1;
>>                 status &= ~(1 << irq);
>
>
>    In case 'status' is 0, 'irq' will be equal to -1. How does the shift by
> negative value work?
>
>>                 generic_handle_irq(irq_find_mapping(b->domain, irq));
>>         } while (status);

That's a good point.  We need to check for 0 somehow.

This code gets replaced in patch 13/15 anyway, but I'll fix it in the
next round.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2 09/15] irqchip: Remove ARM dependency for bcm7120-l2 and brcmstb-l2

[Kevin Cernekee]

This can compile for MIPS (or anything else) now.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b21f12f..09c79d1 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -50,7 +50,6 @@ config ATMEL_AIC5_IRQ
 
 config BRCMSTB_L2_IRQ
 	bool
-	depends on ARM
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
-- 
2.1.1

Re: [PATCH V2 09/15] irqchip: Remove ARM dependency for bcm7120-l2 and brcmstb-l2

[Sergei Shtylyov]

On 10/30/2014 5:18 AM, Kevin Cernekee wrote:

> This can compile for MIPS (or anything else) now.

> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>   drivers/irqchip/Kconfig | 1 -
>   1 file changed, 1 deletion(-)

> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index b21f12f..09c79d1 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -50,7 +50,6 @@ config ATMEL_AIC5_IRQ
>
>   config BRCMSTB_L2_IRQ
>   	bool
> -	depends on ARM

    How about the following?

	depends on ARM || MIPS || COMPILE_TEST

[...]

WBR, Sergei

Re: [PATCH V2 09/15] irqchip: Remove ARM dependency for bcm7120-l2 and brcmstb-l2

[Arnd Bergmann]

On Thursday 30 October 2014 14:10:30 Sergei Shtylyov wrote:
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index b21f12f..09c79d1 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -50,7 +50,6 @@ config ATMEL_AIC5_IRQ
> >
> >   config BRCMSTB_L2_IRQ
> >       bool
> > -     depends on ARM
> 
>     How about the following?
> 
>         depends on ARM || MIPS || COMPILE_TEST
> 

Makes no sense when the driver isn't user-selectable.

	Arnd

Re: [PATCH V2 09/15] irqchip: Remove ARM dependency for bcm7120-l2 and brcmstb-l2

[Sergei Shtylyov]

Hello.

On 10/30/2014 2:24 PM, Arnd Bergmann wrote:

>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>> index b21f12f..09c79d1 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -50,7 +50,6 @@ config ATMEL_AIC5_IRQ
>>>
>>>    config BRCMSTB_L2_IRQ
>>>        bool
>>> -     depends on ARM

>>      How about the following?

>>          depends on ARM || MIPS || COMPILE_TEST

> Makes no sense when the driver isn't user-selectable.

    Ah, I was just blind. :-/

> 	Arnd

WBR, Sergei

[PATCH V2 10/15] irqchip: bcm7120-l2: Make sure all register accesses use base+offset

[Kevin Cernekee]

A couple of accesses to IRQEN (base+0x00) just used "base" directly, so
they would break if IRQEN ever became nonzero.  Make sure that all
reads/writes specify the register offset constant.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/irq-bcm7120-l2.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index 49d8f3d..6472b71 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -66,10 +66,10 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 
 	irq_gc_lock(gc);
 	/* Save the current mask and the interrupt forward mask */
-	b->saved_mask = __raw_readl(b->base) | b->irq_fwd_mask;
+	b->saved_mask = __raw_readl(b->base + IRQEN) | b->irq_fwd_mask;
 	if (b->can_wake) {
 		reg = b->saved_mask | gc->wake_active;
-		__raw_writel(reg, b->base);
+		__raw_writel(reg, b->base + IRQEN);
 	}
 	irq_gc_unlock(gc);
 }
@@ -81,7 +81,7 @@ static void bcm7120_l2_intc_resume(struct irq_data *d)
 
 	/* Restore the saved mask */
 	irq_gc_lock(gc);
-	__raw_writel(b->saved_mask, b->base);
+	__raw_writel(b->saved_mask, b->base + IRQEN);
 	irq_gc_unlock(gc);
 }
 
-- 
2.1.1

Re: [PATCH V2 10/15] irqchip: bcm7120-l2: Make sure all register accesses use base+offset

[Arnd Bergmann]

On Wednesday 29 October 2014 19:18:03 Kevin Cernekee wrote:
> A couple of accesses to IRQEN (base+0x00) just used "base" directly, so
> they would break if IRQEN ever became nonzero.  Make sure that all
> reads/writes specify the register offset constant.
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> 

Now you no longer convert them this driver to use irq_reg_{readl,writel}, which
breaks support for big-endian ARM.

	Arnd

[PATCH V2 11/15] irqchip: bcm7120-l2: Fix missing nibble in gc->unused mask

[Kevin Cernekee]

This mask should have been 0xffff_ffff, not 0x0fff_ffff.

The change should not have an effect on current users (STB) because bits
31:27 are unused.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/irq-bcm7120-l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index 6472b71..e3cfff5 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -171,7 +171,7 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 	}
 
 	gc = irq_get_domain_generic_chip(data->domain, 0);
-	gc->unused = 0xfffffff & ~data->irq_map_mask;
+	gc->unused = 0xffffffff & ~data->irq_map_mask;
 	gc->reg_base = data->base;
 	gc->private = data;
 	ct = gc->chip_types;
-- 
2.1.1

[PATCH V2 12/15] irqchip: bcm7120-l2: Use gc->mask_cache to simplify suspend/resume functions

[Kevin Cernekee]

The cached value already incorporates irq_fwd_mask, and was saved the
last time an IRQ was enabled/disabled.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/irqchip/irq-bcm7120-l2.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index e3cfff5..ec84f65 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -37,7 +37,6 @@ struct bcm7120_l2_intc_data {
 	bool can_wake;
 	u32 irq_fwd_mask;
 	u32 irq_map_mask;
-	u32 saved_mask;
 };
 
 static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
@@ -62,14 +61,11 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct bcm7120_l2_intc_data *b = gc->private;
-	u32 reg;
 
 	irq_gc_lock(gc);
-	/* Save the current mask and the interrupt forward mask */
-	b->saved_mask = __raw_readl(b->base + IRQEN) | b->irq_fwd_mask;
 	if (b->can_wake) {
-		reg = b->saved_mask | gc->wake_active;
-		__raw_writel(reg, b->base + IRQEN);
+		__raw_writel(gc->mask_cache | gc->wake_active,
+			     b->base + IRQEN);
 	}
 	irq_gc_unlock(gc);
 }
@@ -77,11 +73,10 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 static void bcm7120_l2_intc_resume(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
-	struct bcm7120_l2_intc_data *b = gc->private;
 
 	/* Restore the saved mask */
 	irq_gc_lock(gc);
-	__raw_writel(b->saved_mask, b->base + IRQEN);
+	__raw_writel(gc->mask_cache, b->base + IRQEN);
 	irq_gc_unlock(gc);
 }
 
-- 
2.1.1

[PATCH V2 13/15] irqchip: bcm7120-l2: Extend driver to support 64+ bit controllers

[Kevin Cernekee]

Most implementations of the bcm7120-l2 controller only have a single
32-bit enable word + 32-bit status word.  But some instances have added
more enable/status pairs in order to support 64+ IRQs (which are all
ORed into one parent IRQ input).  Make the following changes to allow
the driver to support this:

 - Extend DT bindings so that multiple words can be specified for the
   reg property, various masks, etc.

 - Add loops to the probe/handle functions to deal with each word
   separately

 - Allocate 1 generic-chip for every 32 IRQs, so we can still use the
   clr/set helper functions

 - Update the documentation

This uses one domain per bcm7120-l2 DT node.  If the DT node defines
multiple enable/status pairs (i.e. >=64 IRQs) then the driver will
create a single IRQ domain with 2+ generic chips.  Multiple generic chips
are required because the generic-chip code can only handle one
enable/status register pair per instance.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 .../interrupt-controller/brcm,bcm7120-l2-intc.txt  |  26 ++--
 drivers/irqchip/irq-bcm7120-l2.c                   | 146 ++++++++++++++-------
 2 files changed, 114 insertions(+), 58 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
index ff812a8..bae1f21 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
@@ -13,7 +13,12 @@ Such an interrupt controller has the following hardware design:
   or if they will output an interrupt signal at this 2nd level interrupt
   controller, in particular for UARTs
 
-- not all 32-bits within the interrupt controller actually map to an interrupt
+- typically has one 32-bit enable word and one 32-bit status word, but on
+  some hardware may have more than one enable/status pair
+
+- no atomic set/clear operations
+
+- not all bits within the interrupt controller actually map to an interrupt
 
 The typical hardware layout for this controller is represented below:
 
@@ -48,7 +53,9 @@ The typical hardware layout for this controller is represented below:
 Required properties:
 
 - compatible: should be "brcm,bcm7120-l2-intc"
-- reg: specifies the base physical address and size of the registers
+- reg: specifies the base physical address and size of the registers;
+  multiple pairs may be specified, with the first pair handling IRQ offsets
+  0..31 and the second pair handling 32..63
 - interrupt-controller: identifies the node as an interrupt controller
 - #interrupt-cells: specifies the number of cells needed to encode an interrupt
   source, should be 1.
@@ -59,18 +66,21 @@ Required properties:
 - brcm,int-map-mask: 32-bits bit mask describing how many and which interrupts
   are wired to this 2nd level interrupt controller, and how they match their
   respective interrupt parents. Should match exactly the number of interrupts
-  specified in the 'interrupts' property.
+  specified in the 'interrupts' property, multiplied by the number of
+  enable/status register pairs implemented by this controller.  For
+  multiple parent IRQs with multiple enable/status words, this looks like:
+  <irq0_w0 irq0_w1 irq1_w0 irq1_w1 ...>
 
 Optional properties:
 
 - brcm,irq-can-wake: if present, this means the L2 controller can be used as a
   wakeup source for system suspend/resume.
 
-- brcm,int-fwd-mask: if present, a 32-bits bit mask to configure for the
-  interrupts which have a mux gate, typically UARTs. Setting these bits will
-  make their respective interrupts outputs bypass this 2nd level interrupt
-  controller completely, it completely transparent for the interrupt controller
-  parent
+- brcm,int-fwd-mask: if present, a bit mask to configure the interrupts which
+  have a mux gate, typically UARTs. Setting these bits will make their
+  respective interrupt outputs bypass this 2nd level interrupt controller
+  completely; it is completely transparent for the interrupt controller
+  parent. This should have one 32-bit word per enable/status pair.
 
 Example:
 
diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index ec84f65..ef4d32c 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -23,6 +23,7 @@
 #include <linux/io.h>
 #include <linux/irqdomain.h>
 #include <linux/reboot.h>
+#include <linux/bitops.h>
 #include <linux/irqchip/chained_irq.h>
 
 #include "irqchip.h"
@@ -31,28 +32,43 @@
 #define IRQEN		0x00
 #define IRQSTAT		0x04
 
+#define MAX_WORDS	4
+#define IRQS_PER_WORD	32
+
 struct bcm7120_l2_intc_data {
-	void __iomem *base;
+	unsigned int n_words;
+	void __iomem *base[MAX_WORDS];
 	struct irq_domain *domain;
 	bool can_wake;
-	u32 irq_fwd_mask;
-	u32 irq_map_mask;
+	u32 irq_fwd_mask[MAX_WORDS];
+	u32 irq_map_mask[MAX_WORDS];
 };
 
 static void bcm7120_l2_intc_irq_handle(unsigned int irq, struct irq_desc *desc)
 {
 	struct bcm7120_l2_intc_data *b = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	u32 status;
+	unsigned int idx;
 
 	chained_irq_enter(chip, desc);
 
-	status = __raw_readl(b->base + IRQSTAT);
-	do {
-		irq = ffs(status) - 1;
-		status &= ~(1 << irq);
-		generic_handle_irq(irq_find_mapping(b->domain, irq));
-	} while (status);
+	for (idx = 0; idx < b->n_words; idx++) {
+		int base = idx * IRQS_PER_WORD;
+		struct irq_chip_generic *gc =
+			irq_get_domain_generic_chip(b->domain, base);
+		unsigned long pending;
+		int hwirq;
+
+		irq_gc_lock(gc);
+		pending = __raw_readl(b->base[idx] + IRQSTAT) &
+			  gc->mask_cache;
+		irq_gc_unlock(gc);
+
+		for_each_set_bit(hwirq, &pending, IRQS_PER_WORD) {
+			generic_handle_irq(irq_find_mapping(b->domain,
+					   base + hwirq));
+		}
+	}
 
 	chained_irq_exit(chip, desc);
 }
@@ -65,7 +81,7 @@ static void bcm7120_l2_intc_suspend(struct irq_data *d)
 	irq_gc_lock(gc);
 	if (b->can_wake) {
 		__raw_writel(gc->mask_cache | gc->wake_active,
-			     b->base + IRQEN);
+			     gc->reg_base + IRQEN);
 	}
 	irq_gc_unlock(gc);
 }
@@ -76,7 +92,7 @@ static void bcm7120_l2_intc_resume(struct irq_data *d)
 
 	/* Restore the saved mask */
 	irq_gc_lock(gc);
-	__raw_writel(gc->mask_cache, b->base + IRQEN);
+	__raw_writel(gc->mask_cache, gc->reg_base + IRQEN);
 	irq_gc_unlock(gc);
 }
 
@@ -85,6 +101,7 @@ static int bcm7120_l2_intc_init_one(struct device_node *dn,
 					int irq, const __be32 *map_mask)
 {
 	int parent_irq;
+	unsigned int idx;
 
 	parent_irq = irq_of_parse_and_map(dn, irq);
 	if (parent_irq < 0) {
@@ -92,7 +109,12 @@ static int bcm7120_l2_intc_init_one(struct device_node *dn,
 		return parent_irq;
 	}
 
-	data->irq_map_mask |= be32_to_cpup(map_mask + irq);
+	/* For multiple parent IRQs with multiple words, this looks like:
+	 * <irq0_w0 irq0_w1 irq1_w0 irq1_w1 ...>
+	 */
+	for (idx = 0; idx < data->n_words; idx++)
+		data->irq_map_mask[idx] |=
+			be32_to_cpup(map_mask + irq * data->n_words + idx);
 
 	irq_set_handler_data(parent_irq, data);
 	irq_set_chained_handler(parent_irq, bcm7120_l2_intc_irq_handle);
@@ -109,26 +131,41 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 	struct irq_chip_type *ct;
 	const __be32 *map_mask;
 	int num_parent_irqs;
-	int ret = 0, len, irq;
+	int ret = 0, len;
+	unsigned int idx, irq;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	data->base = of_iomap(dn, 0);
-	if (!data->base) {
+	for (idx = 0; idx < MAX_WORDS; idx++) {
+		data->base[idx] = of_iomap(dn, idx);
+		if (!data->base[idx])
+			break;
+		data->n_words = idx + 1;
+	}
+	if (!data->n_words) {
 		pr_err("failed to remap intc L2 registers\n");
 		ret = -ENOMEM;
-		goto out_free;
+		goto out_unmap;
 	}
 
-	if (of_property_read_u32(dn, "brcm,int-fwd-mask", &data->irq_fwd_mask))
-		data->irq_fwd_mask = 0;
-
-	/* Enable all interrupt specified in the interrupt forward mask and have
-	 * the other disabled
+	/* Enable all interrupts specified in the interrupt forward mask;
+	 * disable all others.  If the property doesn't exist (-EINVAL),
+	 * assume all zeroes.
 	 */
-	__raw_writel(data->irq_fwd_mask, data->base + IRQEN);
+	ret = of_property_read_u32_array(dn, "brcm,int-fwd-mask",
+					 data->irq_fwd_mask, data->n_words);
+	if (ret == 0 || ret == -EINVAL) {
+		for (idx = 0; idx < data->n_words; idx++)
+			__raw_writel(data->irq_fwd_mask[idx],
+				     data->base[idx] + IRQEN);
+	} else {
+		/* property exists but has the wrong number of words */
+		pr_err("invalid int-fwd-mask property\n");
+		ret = -EINVAL;
+		goto out_unmap;
+	}
 
 	num_parent_irqs = of_irq_count(dn);
 	if (num_parent_irqs <= 0) {
@@ -138,7 +175,8 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 	}
 
 	map_mask = of_get_property(dn, "brcm,int-map-mask", &len);
-	if (!map_mask || (len != (sizeof(*map_mask) * num_parent_irqs))) {
+	if (!map_mask ||
+	    (len != (sizeof(*map_mask) * num_parent_irqs * data->n_words))) {
 		pr_err("invalid brcm,int-map-mask property\n");
 		ret = -EINVAL;
 		goto out_unmap;
@@ -150,14 +188,14 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 			goto out_unmap;
 	}
 
-	data->domain = irq_domain_add_linear(dn, 32,
-					&irq_generic_chip_ops, NULL);
+	data->domain = irq_domain_add_linear(dn, IRQS_PER_WORD * data->n_words,
+					     &irq_generic_chip_ops, NULL);
 	if (!data->domain) {
 		ret = -ENOMEM;
 		goto out_unmap;
 	}
 
-	ret = irq_alloc_domain_generic_chips(data->domain, 32, 1,
+	ret = irq_alloc_domain_generic_chips(data->domain, IRQS_PER_WORD, 1,
 				dn->full_name, handle_level_irq, clr, 0,
 				IRQ_GC_INIT_MASK_CACHE);
 	if (ret) {
@@ -165,39 +203,47 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 		goto out_free_domain;
 	}
 
-	gc = irq_get_domain_generic_chip(data->domain, 0);
-	gc->unused = 0xffffffff & ~data->irq_map_mask;
-	gc->reg_base = data->base;
-	gc->private = data;
-	ct = gc->chip_types;
-
-	ct->regs.mask = IRQEN;
-	ct->chip.irq_mask = irq_gc_mask_clr_bit;
-	ct->chip.irq_unmask = irq_gc_mask_set_bit;
-	ct->chip.irq_ack = irq_gc_noop;
-	ct->chip.irq_suspend = bcm7120_l2_intc_suspend;
-	ct->chip.irq_resume = bcm7120_l2_intc_resume;
-
-	if (of_property_read_bool(dn, "brcm,irq-can-wake")) {
+	if (of_property_read_bool(dn, "brcm,irq-can-wake"))
 		data->can_wake = true;
-		/* This IRQ chip can wake the system, set all relevant child
-		 * interupts in wake_enabled mask
-		 */
-		gc->wake_enabled = 0xffffffff;
-		gc->wake_enabled &= ~gc->unused;
-		ct->chip.irq_set_wake = irq_gc_set_wake;
+
+	for (idx = 0; idx < data->n_words; idx++) {
+		irq = idx * IRQS_PER_WORD;
+		gc = irq_get_domain_generic_chip(data->domain, irq);
+
+		gc->unused = 0xffffffff & ~data->irq_map_mask[idx];
+		gc->reg_base = data->base[idx];
+		gc->private = data;
+		ct = gc->chip_types;
+
+		ct->regs.mask = IRQEN;
+		ct->chip.irq_mask = irq_gc_mask_clr_bit;
+		ct->chip.irq_unmask = irq_gc_mask_set_bit;
+		ct->chip.irq_ack = irq_gc_noop;
+		ct->chip.irq_suspend = bcm7120_l2_intc_suspend;
+		ct->chip.irq_resume = bcm7120_l2_intc_resume;
+
+		if (data->can_wake) {
+			/* This IRQ chip can wake the system, set all
+			 * relevant child interupts in wake_enabled mask
+			 */
+			gc->wake_enabled = 0xffffffff;
+			gc->wake_enabled &= ~gc->unused;
+			ct->chip.irq_set_wake = irq_gc_set_wake;
+		}
 	}
 
 	pr_info("registered BCM7120 L2 intc (mem: 0x%p, parent IRQ(s): %d)\n",
-			data->base, num_parent_irqs);
+			data->base[0], num_parent_irqs);
 
 	return 0;
 
 out_free_domain:
 	irq_domain_remove(data->domain);
 out_unmap:
-	iounmap(data->base);
-out_free:
+	for (idx = 0; idx < MAX_WORDS; idx++) {
+		if (data->base[idx])
+			iounmap(data->base[idx]);
+	}
 	kfree(data);
 	return ret;
 }
-- 
2.1.1

[PATCH V2 14/15] irqchip: Decouple bcm7120-l2 from brcmstb-l2

[Kevin Cernekee]

Some chips, such as BCM6328, only require the former driver.  Some
BCM7xxx STB configurations only require the latter driver.  Treat them
as two separate entities, and update the mach-bcm dependencies to
reflect the change.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/mach-bcm/Kconfig        | 1 +
 drivers/irqchip/Kconfig          | 5 +++++
 drivers/irqchip/Makefile         | 4 ++--
 drivers/irqchip/irq-bcm7120-l2.c | 2 +-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig
index 2abad74..bf47eb0 100644
--- a/arch/arm/mach-bcm/Kconfig
+++ b/arch/arm/mach-bcm/Kconfig
@@ -125,6 +125,7 @@ config ARCH_BRCMSTB
 	select HAVE_ARM_ARCH_TIMER
 	select BRCMSTB_GISB_ARB
 	select BRCMSTB_L2_IRQ
+	select BCM7120_L2_IRQ
 	help
 	  Say Y if you intend to run the kernel on a Broadcom ARM-based STB
 	  chipset.
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 09c79d1..afdc1f3 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -48,6 +48,11 @@ config ATMEL_AIC5_IRQ
 	select MULTI_IRQ_HANDLER
 	select SPARSE_IRQ
 
+config BCM7120_L2_IRQ
+	bool
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
+
 config BRCMSTB_L2_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 173bb5f..f0909d0 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -35,6 +35,6 @@ obj-$(CONFIG_TB10X_IRQC)		+= irq-tb10x.o
 obj-$(CONFIG_XTENSA)			+= irq-xtensa-pic.o
 obj-$(CONFIG_XTENSA_MX)			+= irq-xtensa-mx.o
 obj-$(CONFIG_IRQ_CROSSBAR)		+= irq-crossbar.o
-obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o \
-					   irq-bcm7120-l2.o
+obj-$(CONFIG_BCM7120_L2_IRQ)		+= irq-bcm7120-l2.o
+obj-$(CONFIG_BRCMSTB_L2_IRQ)		+= irq-brcmstb-l2.o
 obj-$(CONFIG_KEYSTONE_IRQ)		+= irq-keystone.o
diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index ef4d32c..e53a3a6 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -247,5 +247,5 @@ out_unmap:
 	kfree(data);
 	return ret;
 }
-IRQCHIP_DECLARE(brcmstb_l2_intc, "brcm,bcm7120-l2-intc",
+IRQCHIP_DECLARE(bcm7120_l2_intc, "brcm,bcm7120-l2-intc",
 		bcm7120_l2_intc_of_init);
-- 
2.1.1

[PATCH V2 15/15] irqchip: bcm7120-l2: Enable big endian register accesses on BE kernels

[Kevin Cernekee]

On all supported SoCs, the kernel will be built with CONFIG_CPU_BIG_ENDIAN
iff the CPU is running in BE mode.  Leverage this fact to autodetect
the MMIO byte ordering to use in generic-chip.c.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 drivers/irqchip/Kconfig          | 2 ++
 drivers/irqchip/irq-bcm7120-l2.c | 9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index afdc1f3..db44694 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -51,11 +51,13 @@ config ATMEL_AIC5_IRQ
 config BCM7120_L2_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
+	select GENERIC_IRQ_CHIP_BE
 	select IRQ_DOMAIN
 
 config BRCMSTB_L2_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
+	select GENERIC_IRQ_CHIP_BE
 	select IRQ_DOMAIN
 
 config DW_APB_ICTL
diff --git a/drivers/irqchip/irq-bcm7120-l2.c b/drivers/irqchip/irq-bcm7120-l2.c
index e53a3a6..5324249 100644
--- a/drivers/irqchip/irq-bcm7120-l2.c
+++ b/drivers/irqchip/irq-bcm7120-l2.c
@@ -132,7 +132,7 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 	const __be32 *map_mask;
 	int num_parent_irqs;
 	int ret = 0, len;
-	unsigned int idx, irq;
+	unsigned int idx, irq, flags;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -195,9 +195,12 @@ int __init bcm7120_l2_intc_of_init(struct device_node *dn,
 		goto out_unmap;
 	}
 
+	flags = IRQ_GC_INIT_MASK_CACHE;
+	if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		flags |= IRQ_GC_BE_IO;
+
 	ret = irq_alloc_domain_generic_chips(data->domain, IRQS_PER_WORD, 1,
-				dn->full_name, handle_level_irq, clr, 0,
-				IRQ_GC_INIT_MASK_CACHE);
+				dn->full_name, handle_level_irq, clr, 0, flags);
 	if (ret) {
 		pr_err("failed to allocate generic irq chip\n");
 		goto out_free_domain;
-- 
2.1.1

Re: [PATCH V2 15/15] irqchip: bcm7120-l2: Enable big endian register accesses on BE kernels

[Arnd Bergmann]

On Wednesday 29 October 2014 19:18:08 Kevin Cernekee wrote:
> 
> +       flags = IRQ_GC_INIT_MASK_CACHE;
> +       if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +               flags |= IRQ_GC_BE_IO;
> +
> 

As I said before, I think you should take this from a DT property instead
of making it dependent on the CPU endianess. Otherwise things go horribly
wrong e.g. when someone runs a big-endian kernel on one of the ARM
based chips.

	Arnd