[PATCH v3] irqchip: mmp: add dt support for wakeup

[PATCH v3] irqchip: mmp: add dt support for wakeup

[Neil Zhang]

Some of the Marvell SoCs use GIC as its interrupt controller,and ICU
only used as wakeup logic. When AP subsystem is powered off, GIC will
lose its context, the PMU will need ICU to wakeup the AP subsystem.
So add wakeup entry for such kind of usage.

Signed-off-by: Neil Zhang <zhangwm@marvell.com>
---
 .../devicetree/bindings/arm/mrvl/intc.txt          |   12 +-
 drivers/irqchip/irq-mmp.c                          |  116 ++++++++++++++++++++
 include/linux/irqchip/mmp.h                        |   13 +++
 3 files changed, 140 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/mrvl/intc.txt b/Documentation/devicetree/bindings/arm/mrvl/intc.txt
index 8b53273..401be87 100644
--- a/Documentation/devicetree/bindings/arm/mrvl/intc.txt
+++ b/Documentation/devicetree/bindings/arm/mrvl/intc.txt
@@ -2,7 +2,7 @@
 
 Required properties:
 - compatible : Should be "mrvl,mmp-intc", "mrvl,mmp2-intc" or
-  "mrvl,mmp2-mux-intc"
+  "mrvl,mmp2-mux-intc", "mrvl,mmp-intc-wakeupgen"
 - reg : Address and length of the register set of the interrupt controller.
   If the interrupt controller is intc, address and length means the range
   of the whold interrupt controller. If the interrupt controller is mux-intc,
@@ -15,6 +15,9 @@ Required properties:
 - interrupt-controller : Identifies the node as an interrupt controller.
 - #interrupt-cells : Specifies the number of cells needed to encode an
   interrupt source.
+- mrvl,intc-gbl-mask : Specifies the address and value for global mask in the
+  interrupt controller.
+- mrvl,intc-for-cp : Specifies the irqs that will be routed to cp
 - mrvl,intc-nr-irqs : Specifies the number of interrupts in the interrupt
   controller.
 - mrvl,clr-mfp-irq : Specifies the interrupt that needs to clear MFP edge
@@ -39,6 +42,13 @@ Example:
 		mrvl,intc-nr-irqs = <2>;
 	};
 
+	intc: interrupt-controller@d4282000 {
+		compatible = "mrvl,mmp-intc";
+		reg = <0xd4282000 0x1000>;
+		mrvl,intc-wakeup = <0x114 0x3
+		                    0x144 0x3>;
+	};
+
 * Marvell Orion Interrupt controller
 
 Required properties
diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
index 470c5de..6a2f4d1 100644
--- a/drivers/irqchip/irq-mmp.c
+++ b/drivers/irqchip/irq-mmp.c
@@ -16,6 +16,8 @@
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/irqchip/mmp.h>
+#include <linux/irqchip/arm-gic.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/of_address.h>
@@ -58,6 +60,8 @@ struct mmp_intc_conf {
 static void __iomem *mmp_icu_base;
 static struct icu_chip_data icu_data[MAX_ICU_NR];
 static int max_icu_nr;
+static u32 irq_for_cp[64];
+static u32 irq_for_cp_nr;	/* How many irqs will be routed to cp */
 
 extern void mmp2_clear_pmic_int(void);
 
@@ -123,6 +127,50 @@ static void icu_unmask_irq(struct irq_data *d)
 	}
 }
 
+static int irq_ignore_wakeup(struct icu_chip_data *data, int hwirq)
+{
+	int i;
+
+	if (hwirq < 0 || hwirq >= data->nr_irqs)
+		return 1;
+
+	for (i = 0; i < irq_for_cp_nr; i++)
+		if (irq_for_cp[i] == hwirq)
+			return 1;
+
+	return 0;
+}
+
+static void icu_mask_irq_wakeup(struct irq_data *d)
+{
+	struct icu_chip_data *data = &icu_data[0];
+	int hwirq = d->hwirq - data->virq_base;
+	u32 r;
+
+	if (irq_ignore_wakeup(data, hwirq))
+		return;
+
+	r = readl_relaxed(mmp_icu_base + (hwirq << 2));
+	r &= ~data->conf_mask;
+	r |= data->conf_disable;
+	writel_relaxed(r, mmp_icu_base + (hwirq << 2));
+}
+
+static void icu_unmask_irq_wakeup(struct irq_data *d)
+{
+	struct icu_chip_data *data = &icu_data[0];
+	int hwirq = d->irq - data->virq_base;
+	u32 r;
+
+	if (irq_ignore_wakeup(data, hwirq))
+		return;
+
+	r = readl_relaxed(mmp_icu_base + (hwirq << 2));
+	r &= ~data->conf_mask;
+	r |= data->conf_enable;
+	writel_relaxed(r, mmp_icu_base + (hwirq << 2));
+}
+
 struct irq_chip icu_irq_chip = {
 	.name		= "icu_irq",
 	.irq_mask	= icu_mask_irq,
@@ -491,5 +539,73 @@ err:
 	irq_domain_remove(icu_data[i].domain);
 	return -EINVAL;
 }
+
+void __init mmp_of_wakeup_init(void)
+{
+	struct device_node *node;
+	int ret, nr_irqs;
+	int irq, i = 0;
+
+	node = of_find_compatible_node(NULL, NULL, "mrvl,mmp-intc-wakeupgen");
+	if (!node) {
+		pr_err("Failed to find interrupt controller in arch-mmp\n");
+		return;
+	}
+
+	mmp_icu_base = of_iomap(node, 0);
+	if (!mmp_icu_base) {
+		pr_err("Failed to get interrupt controller register\n");
+		return;
+	}
+
+	ret = of_property_read_u32(node, "mrvl,intc-nr-irqs", &nr_irqs);
+	if (ret) {
+		pr_err("Not found mrvl,intc-nr-irqs property\n");
+		return;
+	}
+
+	/*
+	 * Config all the interrupt source be able to interrupt the cpu 0,
+	 * in IRQ mode, with priority 0 as masked by default.
+	 */
+	for (irq = 0; irq < nr_irqs; irq++)
+		__raw_writel(ICU_IRQ_CPU0_MASKED, mmp_icu_base + (irq << 2));
+
+	/* ICU is only used as wake up logic
+	  * disable the global irq/fiq in icu for all cores.
+	  */
+	i = 0;
+	while (1) {
+		u32 offset, val;
+		if (of_property_read_u32_index(node, "mrvl,intc-gbl-mask", i++, &offset))
+			break;
+		if (of_property_read_u32_index(node, "mrvl,intc-gbl-mask", i++, &val)) {
+			pr_warn("The params should keep pair!!!\n");
+			break;
+		}
+
+		writel_relaxed(val, mmp_icu_base + offset);
+	}
+
+	/* Get the irq lines for cp */
+	i = 0;
+	while (!of_property_read_u32_index(node, "mrvl,intc-for-cp", i, &irq_for_cp[i])) {
+		writel_relaxed(ICU_CONF_SEAGULL, mmp_icu_base + (irq_for_cp[i] << 2));
+		i++;
+	}
+	irq_for_cp_nr = i;
+
+	icu_data[0].conf_enable = mmp_conf.conf_enable;
+	icu_data[0].conf_disable = mmp_conf.conf_disable;
+	icu_data[0].conf_mask = mmp_conf.conf_mask;
+	icu_data[0].nr_irqs = nr_irqs;
+	icu_data[0].virq_base = 32;
+
+	gic_arch_extn.irq_mask = icu_mask_irq_wakeup;
+	gic_arch_extn.irq_unmask = icu_unmask_irq_wakeup;
+
+	return;
+}
+
 IRQCHIP_DECLARE(mmp2_mux_intc, "mrvl,mmp2-mux-intc", mmp2_mux_of_init);
 #endif
diff --git a/include/linux/irqchip/mmp.h b/include/linux/irqchip/mmp.h
index c78a892..93b05ad 100644
--- a/include/linux/irqchip/mmp.h
+++ b/include/linux/irqchip/mmp.h
@@ -1,6 +1,19 @@
 #ifndef	__IRQCHIP_MMP_H
 #define	__IRQCHIP_MMP_H
 
+#define ICU_CONF_CPU3      (1 << 9)
+#define ICU_CONF_CPU2      (1 << 8)
+#define ICU_CONF_CPU1      (1 << 7)
+#define ICU_CONF_CPU0      (1 << 6)
+#define ICU_CONF_AP(n)     (1 << (6 + (n & 0x3)))
+#define ICU_CONF_AP_MASK   (0xF << 6)
+#define ICU_CONF_SEAGULL   (1 << 5)
+#define ICU_CONF_IRQ_FIQ   (1 << 4)
+#define ICU_CONF_PRIO(n)   (n & 0xF)
+
+#define ICU_IRQ_CPU0_MASKED    (ICU_CONF_IRQ_FIQ | ICU_CONF_CPU0)
+
 extern struct irq_chip icu_irq_chip;
 
+extern void __init mmp_of_wakeup_init(void);
 #endif	/* __IRQCHIP_MMP_H */
-- 
1.7.9.5

Re: [PATCH v3] irqchip: mmp: add dt support for wakeup

[Thomas Gleixner]

On Wed, 4 Dec 2013, Neil Zhang wrote:

> Some of the Marvell SoCs use GIC as its interrupt controller,and ICU
> only used as wakeup logic. When AP subsystem is powered off, GIC will
> lose its context, the PMU will need ICU to wakeup the AP subsystem.
> So add wakeup entry for such kind of usage.

This changelog is useless.

What the heck means: "So add wakeup entry for such kind of usage" ?

To me nothing, and I'm quite familiar with this kinds of problems. So
please explain how someone less familiar with that can decode this?

> @@ -58,6 +60,8 @@ struct mmp_intc_conf {
>  static void __iomem *mmp_icu_base;
>  static struct icu_chip_data icu_data[MAX_ICU_NR];
>  static int max_icu_nr;
> +static u32 irq_for_cp[64];

64 is a magic number pulled out of thin air, right?

> +static u32 irq_for_cp_nr;	/* How many irqs will be routed to cp */

What kind of magic nonsense is that?
  
>  extern void mmp2_clear_pmic_int(void);
>  
> @@ -123,6 +127,50 @@ static void icu_unmask_irq(struct irq_data *d)
>  	}
>  }
>  
> +static int irq_ignore_wakeup(struct icu_chip_data *data, int hwirq)
> +{
> +	int i;
> +
> +	if (hwirq < 0 || hwirq >= data->nr_irqs)
> +		return 1;
> +
> +	for (i = 0; i < irq_for_cp_nr; i++)
> +		if (irq_for_cp[i] == hwirq)
> +			return 1;
> +
> +	return 0;
> +}

Oh, I see. A brilliant workaround for something which is missing at
the caller level.

Why the heck do you add a totally braindead function to your driver,
if you can avoid the call to icu_[un]mask_wakeup() in the first place?

Why on earth is something calling this function, if it's a not
supported property of that particular interrupt line?

  Simply because the call site does not have an indicator to avoid
  that call in the first place.

So why are you not adding a proper mechanism to the caller level to
avoid the call? This problem is not unique to magic marvell chips.

And going through a loop over and over again is very efficient in
terms of code size, cache footprint and power consumption, right?

> +static void icu_mask_irq_wakeup(struct irq_data *d)
> +{
> +	struct icu_chip_data *data = &icu_data[0];
> +	int hwirq = d->hwirq - data->virq_base;
> +	u32 r;
> +
> +	if (irq_ignore_wakeup(data, hwirq))
> +		return;

And you call that loop for every invocation of icu_[un]mask_wakeup().

Intelligent design, right?

Now lets have a look at how this magic loop data is set up:

> +void __init mmp_of_wakeup_init(void)
> +{
	....

> +	/* Get the irq lines for cp */
> +	i = 0;
> +	while (!of_property_read_u32_index(node, "mrvl,intc-for-cp", i, &irq_for_cp[i])) {
> +		writel_relaxed(ICU_CONF_SEAGULL, mmp_icu_base + (irq_for_cp[i] << 2));
> +		i++;
> +	}
> +	irq_for_cp_nr = i;

Amazing.

I can understand the part where you setup the mmp_icu registers for
this.

But recording that information in your own magic array is beyond my
comprehension.

Now lets look at why you are doing this at all.

> +	gic_arch_extn.irq_mask = icu_mask_irq_wakeup;
> +	gic_arch_extn.irq_unmask = icu_unmask_irq_wakeup;

Neil, please do not misunderstand me. You are not responsible for the
gic_arch_extn implementation, but you should have noticed that this
gic_arch_extn thing is ass backwards to begin with.

I'm going to reply in a separate mail on this, because you have
brought this to my attention, but you are not responsible in the first
place for this brainfart.

Thanks,

	tglx

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Thomas Gleixner]

@all who feel responsible for gic_arch_extn

On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> I'm going to reply in a separate mail on this, because you have
> brought this to my attention, but you are not responsible in the first
> place for this brainfart.

Who came up with that gic_arch_extn concept in the first place?

It forces all GIC hotpath users to do:

   hotpath_function(x)
   {
   	do_hotpath_work();

	if (random_arch_wants_crap())
	   random_arch_crap(x);
   }

Brilliant design that. Even more so that we have only a few lonely
lusers of this brainfart. Lets look at these ordered by the output of

$ git grep -l gic_arch_extn

arch/arm/mach-imx/gpc.c
arch/arm/mach-omap2/omap-wakeupgen.c
arch/arm/mach-shmobile/intc-sh73a0.c
arch/arm/mach-shmobile/setup-r8a7779.c
arch/arm/mach-tegra/irq.c
arch/arm/mach-ux500/cpu.c

So looking at the first instance makes me go berserk already 

arch/arm/mach-imx/gpc.c

  This has the following repeating pattern:

  imx_gpc_irq_XXX(struct irq_data *d)
  {
	if (d->irq < 32)
	   return;  

  So the person who comitted that crime did notice, that the upper
  layer calls this for all interrupts even those < 32, but he could
  not be arsed to sit down and avoid that.

  Even worse this resulted in the following totaly misleading comment
  above the irq number < 32 check:

  	/* Sanity check for SPI irq */
	if (d->irq < 32)

  This has nothing to do with sanity.

       A sanity check is applied in case that something is expected to
       be always correct, but where we want to catch the corner case
       which we did not imagine yet.

  So what is this (d->irq < 32) check about?

      It's a proof of incompetence because the only lame excuse of
      implementing this nonsense is:

	 /*
	  * We are lazy and do that check on all irqs, but we could
	  * avoid that if we would register a different irq_chip for
	  * these irq lines.
	  */

And I really stop here, because all other places using that nonsense
are more or less equally braindamaged.

I leave that as a an exercise to those who are responsible for the
initial implementation of gic_arch_extn and those who blindly used it.

FYI, this made me even more alert of drivers/irqchip/ being used as a
dump ground for random nonsense. It's on my high prio watch list now
and you better get your gear together and clean up the mess before I
go berserk on you.

Non-Subtle-Hint: Get rid of gic_arch_extn

Thanks,

	tglx

Re: ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 01:41:53AM +0100, Thomas Gleixner wrote:
> @all who feel responsible for gic_arch_extn
> 
> On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> > I'm going to reply in a separate mail on this, because you have
> > brought this to my attention, but you are not responsible in the first
> > place for this brainfart.
> 
> Who came up with that gic_arch_extn concept in the first place?

If you'd spend more time reviewing IRQ patches then maybe you'd catch
this at review time.  So please stop your rediculous whinging when
most of the problem is your own lack of time.

If you must know, it was introduced by TI to work around the power
management shortcomings of the architecture mandated GIC.  No it doesn't
get called for IPIs, but it damned well needs to be called for normal
IRQs.

At the point it was created, it wasn't clear whether this also applied
to local IRQs.  Since I *no* *longer* have visibility of what SoC stuff
is doing with it, of course it's not going to get fixed when a common
pattern emerges.

So... congratulations, you've found something which can be improved,
which has come to light as the code has evolved and a better
understanding of what is required has been discovered.

Re: ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Thomas Gleixner]

Russell,

On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
> On Thu, Dec 05, 2013 at 01:41:53AM +0100, Thomas Gleixner wrote:
> > @all who feel responsible for gic_arch_extn
> > 
> > On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> > > I'm going to reply in a separate mail on this, because you have
> > > brought this to my attention, but you are not responsible in the first
> > > place for this brainfart.
> > 
> > Who came up with that gic_arch_extn concept in the first place?
> 
> If you'd spend more time reviewing IRQ patches then maybe you'd catch
> this at review time.  So please stop your rediculous whinging when
> most of the problem is your own lack of time.

I'm not a native english speaker, so I want to make sure in the first
place that you meant:

    "ridiculous whingeing" 

Assumed that you meant that, let me ridicule you a bit.

The gic_arch_extn concept got merged with:

    commit d7ed36a4ea84e3a850f9932e2058ceef987d1acd
    Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
    Date:   Wed Mar 2 08:03:22 2011 +0100

    ARM: 6777/1: gic: Add hooks for architecture specific extensions

    <SNIP>

    Cc: Russell King <rmk+kernel@arm.linux.org.uk>
    Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
    Acked-by: Colin Cross <ccross@android.com>
    Tested-by: Colin Cross <ccross@android.com>
    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

    ---
    arch/arm/common/gic.c		|   47 ++++++++++++....
    arch/arm/include/asm/hardware/gic.h |    1

The patch in question was never cc'ed to me and you merged it on your
own.

So now you have the chuzpe to blame me for that, just because this
code moved to drivers/irqchip with

     commit 81243e444c6e9d1625073e4a3d3bc244c8a545f0
     Author: Rob Herring <rob.herring@calxeda.com>
     Date:   Tue Nov 20 21:21:40 2012 -0600

     irqchip: Move ARM GIC to drivers/irqchip
    
almost two years later?

The code move neither exempts you from the responsibility of merging
it nor does it imply a retroactive responsibility for me to review all
patches which went into that code prior to the move.

Thanks,

	tglx

Re: ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 03:12:55AM +0100, Thomas Gleixner wrote:
> Russell,
> 
> On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
> > On Thu, Dec 05, 2013 at 01:41:53AM +0100, Thomas Gleixner wrote:
> > > @all who feel responsible for gic_arch_extn
> > > 
> > > On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> > > > I'm going to reply in a separate mail on this, because you have
> > > > brought this to my attention, but you are not responsible in the first
> > > > place for this brainfart.
> > > 
> > > Who came up with that gic_arch_extn concept in the first place?
> > 
> > If you'd spend more time reviewing IRQ patches then maybe you'd catch
> > this at review time.  So please stop your rediculous whinging when
> > most of the problem is your own lack of time.
> 
> I'm not a native english speaker, so I want to make sure in the first
> place that you meant:
> 
>     "ridiculous whingeing" 
> 
> Assumed that you meant that, let me ridicule you a bit.
> 
> The gic_arch_extn concept got merged with:
> 
>     commit d7ed36a4ea84e3a850f9932e2058ceef987d1acd
>     Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
>     Date:   Wed Mar 2 08:03:22 2011 +0100
> 
>     ARM: 6777/1: gic: Add hooks for architecture specific extensions
> 
>     <SNIP>
> 
>     Cc: Russell King <rmk+kernel@arm.linux.org.uk>
>     Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>     Acked-by: Colin Cross <ccross@android.com>
>     Tested-by: Colin Cross <ccross@android.com>
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
>     ---
>     arch/arm/common/gic.c		|   47 ++++++++++++....
>     arch/arm/include/asm/hardware/gic.h |    1
> 
> The patch in question was never cc'ed to me and you merged it on your
> own.
> 
> So now you have the chuzpe to blame me for that, just because this
> code moved to drivers/irqchip with
> 
>      commit 81243e444c6e9d1625073e4a3d3bc244c8a545f0
>      Author: Rob Herring <rob.herring@calxeda.com>
>      Date:   Tue Nov 20 21:21:40 2012 -0600
> 
>      irqchip: Move ARM GIC to drivers/irqchip
>     
> almost two years later?
> 
> The code move neither exempts you from the responsibility of merging
> it nor does it imply a retroactive responsibility for me to review all
> patches which went into that code prior to the move.

And neither does it give you permission to send such an idiotic and
rediculous email.

I'm not going to do anything about it because "Thomas Glexiner" has
suddenly decided he doesn't like it.

As for your definition of "hotpath", you're really screwed on that
because you don't seem to understand what is or isn't the hotpath in
this code.

So there's not much point discussing this with you until you:

(a) calm down
(b) analyse it properly and work out the frequency under which each
class of IRQ (those >= 32 and those < 32) call into these functions.

To put it bluntly, you're wrong.

Re: ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Thomas Gleixner]

On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
> So there's not much point discussing this with you until you:
> 
> (a) calm down

Done so :)

> (b) analyse it properly and work out the frequency under which each
> class of IRQ (those >= 32 and those < 32) call into these functions.

Here you go:

The frequency of invoking the gic_arch_extn callbacks is exactly equal
to the frequency of interrupts in the system which go through the GIC
at least for mask/unmask/eoi. The frequency of calls per interrupt
depends on the interrupt type, but at minimum it is one.

So basically it does for any interrupt independent of >= 32 or < 32:

irq_fn(d)
{
	do_something_common(d);
	if (gic_arch_extn.fn)
	   gic_arch_extn.fn(d);
	do_something_common(d);
}

which then does:

extn_fn(d)
{
	if (this_irq_is_affected(d))
	   do_some_more(d);
}      

So when this_irq_is_affected(d) boils down to

   if (d->irq [<>] n)

then I agree, that it's debatable, whether the conditonal function
call and the simple this_irq_is_affected(d) check is worth to worry
about. Though there are people who care about two pointless
conditonals for various reasons.

But at the point when this_irq_is_affected(d) is doing a loop lookup,
then this really starts to smell badly.

Sure you might argue, that it's the problem of that particular patch
author to put a loop lookup into this_irq_is_affected().

Fair enough, though you have to admit that the design of the
gic_arch_extn actually enforces that, if you can't do a simple "if irq
[<>] n" check for whatever reason.

The alternative approach to that is to use different irq chip
implementations for interrupts affected by gic_arch_extn and those
which are not affected as we do in lot of other places do deal with
the subtle differences of particular interrupt lines. That definitely
would avoid that people try to stick more complex decision functions
than "irq [<>] n" into this_irq_is_affected().

Now looking at the locking szenario of GIC, it might eventually create
quite some duplicated code, which is undesirable as well. OTOH, the
locking requirements especially if I look at gic_[un]mask_irq and
gic_eoi_irq are not entirely clear to me from the code.

gic_[un]mask_irq(d)
{
	mask = 1 << SFT(gic_irq(d));

	lock(controller_lock);
	if (gic_arch_extn.irq_[un]mask)
	   gic_arch_extn.irq_[un]mask(d);
	writel(mask, GIC_ENABLE_[CLEAR|SET]);
	unlock(controller_lock);
}

while

gic_eoi_irq(d)
{
	if (gic_arch_extn.irq_eoi) {
	   lock(controller_lock);
	   gic_arch_extn.irq_eoi(d);
	   unlock(controller_lock);
	}
	writel(gic_irq(d), GIC_EOI);
}

So is there a requirement to serialize the mask/unmask operations for
a particular interrupt line against mask/unmask operations on a
different core on some other interrupt line?

  The operations for a particular interrupt line are already
  serialized at the core code level.

  The CLEAR/SET registers are designed to avoid locking in contrary to
  the classic ENABLE reg, where you have to maintain consistency of
  the full set of interrupts affected by that register.

  So for the case where gic_arch_extn is not used, the locking is
  completely pointless, right?

Or is this locking required to maintain consistency between the
gic_arch_extn.[un]mask and the GIC.[un]mask itself?

And even if the locking is required for this, then having two separate
chips with two different callbacks makes sense.

gic_mask_irq()
{
	writel(mask, GIC_ENABLE_CLEAR);
}

gic_mask_extn_irq(d)
{
	lock(controller_lock);
	gic_arch_extn.irq_mask(d);
	gic_mask_irq(d);
	unlock(controller_lock);
}

And then have the gic_chip and gic_extn_chip set for the various
interrupt lines.

That avoids several things:

1) The locking for non gic_arch_extn interrupts

2) Two conditionals for gic_arch_extn interrupts

3) The enforcement of adding complex decision functions into the
   gic_extn functions if there is no simple x < N check possible.

I might have missed something as always, but at least I did a proper
analysis of the code as it is understandable to a mere mortal.

Thoughts?

Thanks,

	tglx

Re: ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Russell King - ARM Linux]

On Fri, Dec 06, 2013 at 10:25:49PM +0100, Thomas Gleixner wrote:
> The frequency of invoking the gic_arch_extn callbacks is exactly equal
> to the frequency of interrupts in the system which go through the GIC
> at least for mask/unmask/eoi. The frequency of calls per interrupt
> depends on the interrupt type, but at minimum it is one.
> 
> So basically it does for any interrupt independent of >= 32 or < 32:
> 
> irq_fn(d)
> {
> 	do_something_common(d);
> 	if (gic_arch_extn.fn)
> 	   gic_arch_extn.fn(d);
> 	do_something_common(d);
> }
> 
> which then does:
> 
> extn_fn(d)
> {
> 	if (this_irq_is_affected(d))
> 	   do_some_more(d);
> }      
> 
> So when this_irq_is_affected(d) boils down to
> 
>    if (d->irq [<>] n)
> 
> then I agree, that it's debatable, whether the conditonal function
> call and the simple this_irq_is_affected(d) check is worth to worry
> about. Though there are people who care about two pointless
> conditonals for various reasons.


Right - and as you correctly identify, it has become clear through code
evolution that there is a difference between the extended handling of
IRQs above and below the 32 breakpoint.

Now, what you previously termed the hotpath - IRQs less than 32 - isn't
the path we want to optimise for, unless it is our intention to optimise
for an idle system.  Why?

IRQs 0-15 won't be seen in this path - they're the IPIs which are handled
completely outside of genirq.  So that leaves IRQs 16 to 31.  Of course,
only one or two are normally used (the watchdog driver has been removed,
so we're really down to just the local timer interrupt.)  In a system
doing work, you're going to have normal IRQs (IRQs >= 32) occuring,
probably at a much faster rate than the local timer interrupt.

Hence, there are two cases to optimise for: the case without the
extension hooks, and the case with the extension hook for IRQs >= 32.
Optimising the case for IRQ < 32 makes no sense because we're
effectively optimising for one IRQ vs all the rest.

Moving the test for IRQs >= 32 to the GIC code puts extra code and
overhead into that path, perturbing the case without extension - and
that's the use case we want to encourage.  Hence, having that test in
the code where the extension is needed makes total sense.

> But at the point when this_irq_is_affected(d) is doing a loop lookup,
> then this really starts to smell badly.

And why would that be a loop?  I can't speak for how it's been used
since it was introduced for OMAP - and it's well known why - remember,
we have arm-soc, and arm-soc deals with the SoC specific code, and as
such I no longer know what SoCs are doing with this stuff.

> The alternative approach to that is to use different irq chip
> implementations for interrupts affected by gic_arch_extn and those
> which are not affected as we do in lot of other places do deal with
> the subtle differences of particular interrupt lines. That definitely
> would avoid that people try to stick more complex decision functions
> than "irq [<>] n" into this_irq_is_affected().

... which results in more complexity.  Do we need complex code?  Do
we have implementations which loop?  Why make the thing complex in this
way if it's not actually required.  What you seem to be asking is for
overdesign to happen.  That's completely contary to the Linux philosophy.

Now, it may be that _since_ this code was originally merged, it does
need this complexity, but I'm not aware of it (see above for _why_), and
no one else has spotted this.  So to rant about it as if it's the worst
thing on the planet is a total over-reaction.

You may also like to note that I'm not Cc'd on GIC patches anymore.

> Now looking at the locking szenario of GIC, it might eventually create
> quite some duplicated code, which is undesirable as well. OTOH, the
> locking requirements especially if I look at gic_[un]mask_irq and
> gic_eoi_irq are not entirely clear to me from the code.
> 
> gic_[un]mask_irq(d)
> {
> 	mask = 1 << SFT(gic_irq(d));
> 
> 	lock(controller_lock);
> 	if (gic_arch_extn.irq_[un]mask)
> 	   gic_arch_extn.irq_[un]mask(d);
> 	writel(mask, GIC_ENABLE_[CLEAR|SET]);
> 	unlock(controller_lock);
> }

Do you really want to know what's absolutely hillarious here?  You're
now complaining about the locking in here...

commit c4bfa28aec58c588de55babe99f4c172ec534704
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Sat Jul 1 22:32:14 2006 +0100

    [ARM] 3686/1: ARM: arm/common: convert irq handling
    
    Patch from Thomas Gleixner
    
    From: Thomas Gleixner <tglx@linutronix.de>
    
    Convert the files in arch/arm/common to use the generic
    irq handling functions.
    
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index c02dc8116a18..f3c1ebfdd0aa 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -33,6 +33,7 @@
 
 static void __iomem *gic_dist_base;
 static void __iomem *gic_cpu_base;
+static DEFINE_SPINLOCK(irq_controller_lock);
 
 /*
  * Routines to acknowledge, disable and enable interrupts
@@ -52,32 +53,45 @@ static void __iomem *gic_cpu_base;
 static void gic_ack_irq(unsigned int irq)
 {
        u32 mask = 1 << (irq % 32);
+
+       spin_lock(&irq_controller_lock);
        writel(mask, gic_dist_base + GIC_DIST_ENABLE_CLEAR + (irq / 32) * 4);
        writel(irq, gic_cpu_base + GIC_CPU_EOI);
+       spin_unlock(&irq_controller_lock);
 }
 
 static void gic_mask_irq(unsigned int irq)
 {
        u32 mask = 1 << (irq % 32);
+
+       spin_lock(&irq_controller_lock);
        writel(mask, gic_dist_base + GIC_DIST_ENABLE_CLEAR + (irq / 32) * 4);
+       spin_unlock(&irq_controller_lock);
 }
 
 static void gic_unmask_irq(unsigned int irq)
 {
        u32 mask = 1 << (irq % 32);
+
+       spin_lock(&irq_controller_lock);
        writel(mask, gic_dist_base + GIC_DIST_ENABLE_SET + (irq / 32) * 4);
+       spin_unlock(&irq_controller_lock);
 }

... which is something you added in the first place, when you converted
the GIC to your genirq stuff. :)

> And even if the locking is required for this, then having two separate
> chips with two different callbacks makes sense.
> 
> gic_mask_irq()
> {
> 	writel(mask, GIC_ENABLE_CLEAR);
> }
> 
> gic_mask_extn_irq(d)
> {
> 	lock(controller_lock);
> 	gic_arch_extn.irq_mask(d);
> 	gic_mask_irq(d);
> 	unlock(controller_lock);
> }
> 
> And then have the gic_chip and gic_extn_chip set for the various
> interrupt lines.

Since knowledge of what platforms do with this extension has been long
lost (well, no one person ever had it because responsibility for this
stuff has been devolved...) I doubt we can safely get rid of that lock
without someone auditing this stuff.

Since I'm apparantly no longer responsible for the GIC - and I'm rarely
copied with patches for it anymore, I've basically washed my hands of
it - it seems everyone else maintains it now.