[PATCH] ARM: ux500: Initialize irq affinity

[PATCH] ARM: ux500: Initialize irq affinity

[Per Fransson]

Use irq_set_affinity() to initialize the kernel view of irq affinity
when programming the GIC registers.

Signed-off-by: Per Fransson <per.xx.fransson@stericsson.com>
---
 arch/arm/mach-ux500/cpu.c                      |    7 +++++++
 arch/arm/mach-ux500/include/mach/irqs-db8500.h |    4 ++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ux500/cpu.c b/arch/arm/mach-ux500/cpu.c
index f418574..164347f 100644
--- a/arch/arm/mach-ux500/cpu.c
+++ b/arch/arm/mach-ux500/cpu.c
@@ -28,6 +28,7 @@ void __init ux500_init_irq(void)
 {
 	void __iomem *dist_base;
 	void __iomem *cpu_base;
+	int i;
 
 	if (cpu_is_u5500()) {
 		dist_base = __io_address(U5500_GIC_DIST_BASE);
@@ -40,6 +41,12 @@ void __init ux500_init_irq(void)
 
 	gic_init(0, 29, dist_base, cpu_base);
 
+	for (i = IRQ_SHPI_START; i <= IRQ_SHPI_END; i++) {
+		if (i >= IRQ_MODEM_START && i <= IRQ_MODEM_END)
+			continue;
+		irq_set_affinity(i, cpumask_of(0));
+	}
+
 	/*
 	 * Init clocks here so that they are available for system timer
 	 * initialization.
diff --git a/arch/arm/mach-ux500/include/mach/irqs-db8500.h b/arch/arm/mach-ux500/include/mach/irqs-db8500.h
index 68bc149..3279b0e 100644
--- a/arch/arm/mach-ux500/include/mach/irqs-db8500.h
+++ b/arch/arm/mach-ux500/include/mach/irqs-db8500.h
@@ -8,6 +8,10 @@
 #ifndef __MACH_IRQS_DB8500_H
 #define __MACH_IRQS_DB8500_H
 
+#define IRQ_SHPI_END (IRQ_SHPI_START + 127)
+#define IRQ_MODEM_START (IRQ_SHPI_START + 64)
+#define IRQ_MODEM_END (IRQ_SHPI_START + 83)
+
 #define IRQ_DB8500_MTU0			(IRQ_SHPI_START + 4)
 #define IRQ_DB8500_SPI2			(IRQ_SHPI_START + 6)
 #define IRQ_DB8500_PMU			(IRQ_SHPI_START + 7)
-- 
1.7.8.3

[PATCH] ARM: ux500: Initialize irq affinity

[Russell King - ARM Linux]

On Fri, Jan 20, 2012 at 01:59:16PM +0100, Per Fransson wrote:
> Use irq_set_affinity() to initialize the kernel view of irq affinity
> when programming the GIC registers.

Why is this necessary?  And if it is necessary, why do you think it
should be ux500 specific?

[PATCH] ARM: ux500: Initialize irq affinity

[Linus Walleij]

On Fri, Jan 20, 2012 at 2:03 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 20, 2012 at 01:59:16PM +0100, Per Fransson wrote:
>> Use irq_set_affinity() to initialize the kernel view of irq affinity
>> when programming the GIC registers.
>
> Why is this necessary?

Necessary: we don't quite know. (Maybe should have been RFC, but
this patch is better than being silent of it...)

On the Ux500 all shared peripheral IRQs (32 thru 127) are delivered
to CPU0 (hardwired, will not work on CPU1), I have heard that this
is sort of what everybody does but don't know for sure. (Marc?)

Currently the gic_set_affinity() in common/gic.c isn't called for any
IRQs so they are left in their power-on state (from U-Boot or
similar) which is essentially correct (we checked dumps of the
registers).

However when we look at the irq descriptors in e.g. crash:

    crash> irq -s
    IRQ NAME                 AFFINITY
     36 Nomadik Timer Tick   0
     43 uart-pl011           0-1
     44 nmk-i2c              0-1
     46 pl022                0-1
     53 nmk-i2c              0-1
     54 nmk-i2c              0-1
    (... etc ...)

So the descriptors think the IRQs are shared except for the
timer. Which isn't true. So this patch makes the map match
reality.

> And if it is necessary, why do you think it
> should be ux500 specific?

Good question. If this is the case for all SMP gic users maybe we
should make the code #ifdef CONFIG_SMP inside gic.c instead?
(Sans the check for the modem IRQs, they should probably also
be made affine to CPU0.)

Yours,
Linus Walleij

[PATCH] ARM: ux500: Initialize irq affinity

[Marc Zyngier]

On 20/01/12 15:45, Linus Walleij wrote:
> On Fri, Jan 20, 2012 at 2:03 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Fri, Jan 20, 2012 at 01:59:16PM +0100, Per Fransson wrote:
>>> Use irq_set_affinity() to initialize the kernel view of irq affinity
>>> when programming the GIC registers.
>>
>> Why is this necessary?
> 
> Necessary: we don't quite know. (Maybe should have been RFC, but
> this patch is better than being silent of it...)
> 
> On the Ux500 all shared peripheral IRQs (32 thru 127) are delivered
> to CPU0 (hardwired, will not work on CPU1), I have heard that this
> is sort of what everybody does but don't know for sure. (Marc?)

Not as far as I know. My platforms seems to happily deliver interrupts
on other cores by setting the SMP affinity (just tested on my EB11MP).

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH] ARM: ux500: Initialize irq affinity

[Will Deacon]

Hi Linus,

On Fri, Jan 20, 2012 at 03:45:40PM +0000, Linus Walleij wrote:
> On Fri, Jan 20, 2012 at 2:03 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Jan 20, 2012 at 01:59:16PM +0100, Per Fransson wrote:
> >> Use irq_set_affinity() to initialize the kernel view of irq affinity
> >> when programming the GIC registers.
> >
> > Why is this necessary?
> 
> Necessary: we don't quite know. (Maybe should have been RFC, but
> this patch is better than being silent of it...)
> 
> On the Ux500 all shared peripheral IRQs (32 thru 127) are delivered
> to CPU0 (hardwired, will not work on CPU1), I have heard that this
> is sort of what everybody does but don't know for sure. (Marc?)

Oo-er, I've not heard of any platforms doing this before, but it is
permitted by the GIC spec so we should deal with it.

> Currently the gic_set_affinity() in common/gic.c isn't called for any
> IRQs so they are left in their power-on state (from U-Boot or
> similar) which is essentially correct (we checked dumps of the
> registers).
> 
> However when we look at the irq descriptors in e.g. crash:
> 
>     crash> irq -s
>     IRQ NAME                 AFFINITY
>      36 Nomadik Timer Tick   0
>      43 uart-pl011           0-1
>      44 nmk-i2c              0-1
>      46 pl022                0-1
>      53 nmk-i2c              0-1
>      54 nmk-i2c              0-1
>     (... etc ...)
> 
> So the descriptors think the IRQs are shared except for the
> timer. Which isn't true. So this patch makes the map match
> reality.

Right. So these are SPIs that are configured statically in hardware. They
should therefore be identifiable by writing zeros to the ICDIPTRn registers
at the distributor and then reading back to see if any of the bits are set.
If they are, we can create a cpumask for each interrupt and set the affinity
to that.

The next question is, is gic_dist_init the right place to do this or is it
too early (we need to update the irq_desc)?

> > And if it is necessary, why do you think it
> > should be ux500 specific?
> 
> Good question. If this is the case for all SMP gic users maybe we
> should make the code #ifdef CONFIG_SMP inside gic.c instead?
> (Sans the check for the modem IRQs, they should probably also
> be made affine to CPU0.)

Although I think you're the only platform doing this, we should aim to
handle it in the GIC driver at some initialisation stage.

Will

[PATCH] ARM: ux500: Initialize irq affinity

[Russell King - ARM Linux]

On Fri, Jan 20, 2012 at 04:45:40PM +0100, Linus Walleij wrote:
> On Fri, Jan 20, 2012 at 2:03 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Fri, Jan 20, 2012 at 01:59:16PM +0100, Per Fransson wrote:
> >> Use irq_set_affinity() to initialize the kernel view of irq affinity
> >> when programming the GIC registers.
> >
> > Why is this necessary?
> 
> Necessary: we don't quite know. (Maybe should have been RFC, but
> this patch is better than being silent of it...)

I'm afraid Per Fransson took the thread off the mailing lists,
so my reply to him also wasn't public.  Here's what I said:

| The affinity starts off as "all CPUs", and the implementation is free
| to route the IRQ to any of those CPUs.  On x86, some hardware is smart
| enough to route the IRQ to one of a set of CPUs, choosing which CPU
| to target in hardware.
| 
| We don't have that ability, and trying to do that in software proved to
| be very problematic to get it to work satisfactorily.  Given that there
| is a userspace daemon which helps to solve the problem, it was decided
| that we wouldn't bother with this i the kernel.
| 
| So, we have to choose a CPU to route the IRQ to from the mask being
| requested - and the requested mask may contain anything from a single CPU
| to multiple CPUs.
| 
| We choose to route it to the first online CPU in the set.  That fits with
| the model, and does not require us to report back which CPU it's currently
| routed to.  Indeed, architectures such as x86 can't tell you which CPU
| the next interrupt will hit.
| 
| So we shouldn't even try to do this on ARM.  Yes, we end up with all IRQs
| targetting CPU0 at boot, and with the masks showing 'all CPUs' but that's
| not a problem.  Really not a problem.

[PATCH] ARM: ux500: Initialize irq affinity

[Russell King - ARM Linux]

On Fri, Jan 20, 2012 at 04:45:40PM +0100, Linus Walleij wrote:
> On the Ux500 all shared peripheral IRQs (32 thru 127) are delivered
> to CPU0 (hardwired, will not work on CPU1), I have heard that this
> is sort of what everybody does but don't know for sure. (Marc?)

BTW, if you can't route IRQs to a CPU, then we'll need a hook to
refuse the affinity call.

> Currently the gic_set_affinity() in common/gic.c isn't called for any
> IRQs so they are left in their power-on state (from U-Boot or
> similar) which is essentially correct (we checked dumps of the
> registers).

No they aren't.  See the gic code:

        cpu = cpu_logical_map(smp_processor_id());
        cpumask = 1 << cpu;
        cpumask |= cpumask << 8;
        cpumask |= cpumask << 16;

        /*
         * Set all global interrupts to this CPU only.
         */
        for (i = 32; i < gic_irqs; i += 4)
                writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);

At boot, we program _all_ the GIC IRQs to point@the boot CPU as part
of the initialization, which will be logical CPU 0.

> However when we look at the irq descriptors in e.g. crash:
> 
>     crash> irq -s
>     IRQ NAME                 AFFINITY
>      36 Nomadik Timer Tick   0
>      43 uart-pl011           0-1
>      44 nmk-i2c              0-1
>      46 pl022                0-1
>      53 nmk-i2c              0-1
>      54 nmk-i2c              0-1
>     (... etc ...)

Yes, and as I said in my previous email, having the affinity mask report
more CPUs than we set the hardware to is perfectly legal.  What you've
pointed out above is that setting an affinity mask of _just_ CPU1 should
be made fail on Ux500.

In other words, when setting an affinity, if:

	affinity & online_mask & ~invalid_cpus

is zero, then we should fail the request to set the affinity.

But that doesn't mean that setting the affinity to ~0 (all CPUs, including
the invalid CPUs) should be failed.

[PATCH] ARM: ux500: Initialize irq affinity

[Linus Walleij]

On Fri, Jan 20, 2012 at 6:50 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 20, 2012 at 04:45:40PM +0100, Linus Walleij wrote:

>> Currently the gic_set_affinity() in common/gic.c isn't called for any
>> IRQs so they are left in their power-on state (from U-Boot or
>> similar) which is essentially correct (we checked dumps of the
>> registers).
>
> No they aren't. ?See the gic code:
>(...)
> At boot, we program _all_ the GIC IRQs to point at the boot CPU as part
> of the initialization, which will be logical CPU 0.

Yes, I was dead wrong. And I even read that code just hours ago :-(

>> However when we look at the irq descriptors in e.g. crash:
>>
>> ? ? crash> irq -s
>> ? ? IRQ NAME ? ? ? ? ? ? ? ? AFFINITY
>> ? ? ?36 Nomadik Timer Tick ? 0
>> ? ? ?43 uart-pl011 ? ? ? ? ? 0-1
>> ? ? ?44 nmk-i2c ? ? ? ? ? ? ?0-1
>> ? ? ?46 pl022 ? ? ? ? ? ? ? ?0-1
>> ? ? ?53 nmk-i2c ? ? ? ? ? ? ?0-1
>> ? ? ?54 nmk-i2c ? ? ? ? ? ? ?0-1
>> ? ? (... etc ...)
>
> Yes, and as I said in my previous email, having the affinity mask report
> more CPUs than we set the hardware to is perfectly legal.

OK I see, then it's no problem.

Yours,
Linus Walleij

[PATCH] ARM: ux500: Initialize irq affinity

[Per Fransson]

On Fri, Jan 20, 2012 at 6:44 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jan 20, 2012 at 04:45:40PM +0100, Linus Walleij wrote:
>> On Fri, Jan 20, 2012 at 2:03 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Fri, Jan 20, 2012 at 01:59:16PM +0100, Per Fransson wrote:
>> >> Use irq_set_affinity() to initialize the kernel view of irq affinity
>> >> when programming the GIC registers.
>> >
>> > Why is this necessary?
>>
>> Necessary: we don't quite know. (Maybe should have been RFC, but
>> this patch is better than being silent of it...)
>
> I'm afraid Per Fransson took the thread off the mailing lists,
> so my reply to him also wasn't public. ?Here's what I said:
>

Oh, I did do that. My apologies.

[PATCH] ARM: ux500: Initialize irq affinity

[Will Deacon]

Hi Russell,

On Fri, Jan 20, 2012 at 05:44:57PM +0000, Russell King - ARM Linux wrote:
> | So, we have to choose a CPU to route the IRQ to from the mask being
> | requested - and the requested mask may contain anything from a single CPU
> | to multiple CPUs.
> | 
> | We choose to route it to the first online CPU in the set.  That fits with
> | the model, and does not require us to report back which CPU it's currently
> | routed to.  Indeed, architectures such as x86 can't tell you which CPU
> | the next interrupt will hit.
> | 
> | So we shouldn't even try to do this on ARM.  Yes, we end up with all IRQs
> | targetting CPU0 at boot, and with the masks showing 'all CPUs' but that's
> | not a problem.  Really not a problem.

I think this *could* be a problem if, as in Linus's case, some interrupts
have a hardcoded affinity enforced by the hardware (i.e. there are sticky bits
in the GIC distributor CPU targets registers). If we boot on a CPU that is
different from the hardcoded affinity, we could end up initialising an
interrupt with multiple targets. I suspect this will just lead to spurious
interrupts at the end of the day, but there's a potential performance impact
too.

So we could change gic_dist_init to ignore interrupts with sticky bits when
setting up the targets (assuming that the hardcoded target is limited to a
single CPU...) although we also need a way to prevent those interrupts being
moved later on.

For the time being we can just limit ux500 to booting on (physical) CPU 0
and wait and see if anybody hardcodes their targets to point at secondary
CPUs in the future.

Will

[PATCH] ARM: ux500: Initialize irq affinity

[Russell King - ARM Linux]

On Sun, Jan 22, 2012 at 12:54:11PM +0000, Will Deacon wrote:
> I think this *could* be a problem if, as in Linus's case, some interrupts
> have a hardcoded affinity enforced by the hardware (i.e. there are sticky bits
> in the GIC distributor CPU targets registers). If we boot on a CPU that is
> different from the hardcoded affinity, we could end up initialising an
> interrupt with multiple targets. I suspect this will just lead to spurious
> interrupts at the end of the day, but there's a potential performance impact
> too.

If we boot on a CPU which is incapable of receiving interrupts from
attached devices, we have a much larger problem than just affinity.
We potentially can't initialize those devices until their dependent
CPU comes online - we certainly can't start to use those devices until
that happens.

That's something we just don't deal with - and as far as userspace goes,
trying to set an affinity to just the offline CPU will return an error
code.

So, if you have interrupts which can only be serviced by a group of CPUs,
at least one of those CPUs must remain online at all times that those
dependent devices could produce an interrupt.

I think what we want instead is for the IRQ subsystem to record on a
per-IRQ basis which CPUs the interrupt /can/ be routed to, and if:

	affinity & cpus_online & allowable_cpus

becomes empty, the attempt to set the affinity is failed.  Also, a callback
in the hotplug code to error out if

	cpus_online & allowable_cpus & ~offlining_cpu

would become an empty set.

[PATCH] ARM: ux500: Initialize irq affinity

[Will Deacon]

On Sun, Jan 22, 2012 at 03:27:10PM +0000, Russell King - ARM Linux wrote:
> On Sun, Jan 22, 2012 at 12:54:11PM +0000, Will Deacon wrote:
> > I think this *could* be a problem if, as in Linus's case, some interrupts
> > have a hardcoded affinity enforced by the hardware (i.e. there are sticky bits
> > in the GIC distributor CPU targets registers). If we boot on a CPU that is
> > different from the hardcoded affinity, we could end up initialising an
> > interrupt with multiple targets. I suspect this will just lead to spurious
> > interrupts at the end of the day, but there's a potential performance impact
> > too.
> 
> If we boot on a CPU which is incapable of receiving interrupts from
> attached devices, we have a much larger problem than just affinity.
> We potentially can't initialize those devices until their dependent
> CPU comes online - we certainly can't start to use those devices until
> that happens.

My initial reading of the GIC spec was that you would always be able to
augment the set of targets, but taking another look this doesn't appear to
be the case. So yes, if/when we encounter a platform doing this for a CPU
other than the boot CPU then we'll be in trouble.

> That's something we just don't deal with - and as far as userspace goes,
> trying to set an affinity to just the offline CPU will return an error
> code.

Although we can currently end up reporting that an interrupt is affine only
to an offline CPU (patch 7292/1 fixes this).

> So, if you have interrupts which can only be serviced by a group of CPUs,
> at least one of those CPUs must remain online at all times that those
> dependent devices could produce an interrupt.
> 
> I think what we want instead is for the IRQ subsystem to record on a
> per-IRQ basis which CPUs the interrupt /can/ be routed to, and if:
> 
> 	affinity & cpus_online & allowable_cpus
> 
> becomes empty, the attempt to set the affinity is failed.  Also, a callback
> in the hotplug code to error out if
> 
> 	cpus_online & allowable_cpus & ~offlining_cpu
> 
> would become an empty set.

That looks like a good idea to me. I suggest we wait until somebody builds
such a monster and get them to implement the solution :) (unless the ux500 is
capable of booting on a CPU other than 0?).

Will

[PATCH] ARM: ux500: Initialize irq affinity

[Linus Walleij]

On Tue, Jan 24, 2012 at 12:04 PM, Will Deacon <will.deacon@arm.com> wrote:
> [Russell]
>> So, if you have interrupts which can only be serviced by a group of CPUs,
>> at least one of those CPUs must remain online at all times that those
>> dependent devices could produce an interrupt.
>>
>> I think what we want instead is for the IRQ subsystem to record on a
>> per-IRQ basis which CPUs the interrupt /can/ be routed to, and if:
>>
>> ? ? ? affinity & cpus_online & allowable_cpus
>>
>> becomes empty, the attempt to set the affinity is failed. ?Also, a callback
>> in the hotplug code to error out if
>>
>> ? ? ? cpus_online & allowable_cpus & ~offlining_cpu
>>
>> would become an empty set.
>
> That looks like a good idea to me. I suggest we wait until somebody builds
> such a monster and get them to implement the solution :) (unless the ux500 is
> capable of booting on a CPU other than 0?).

No it needs to boot on 0. So let's let this thread sleep ...

Thanks,
Linus Walleij