arm smp support patch

arm smp support patch

[Scott Valentine]

On arm multi-core platforms that have a gic, the secondary cores fail to
wake if they are booted in WFI mode, as the gic_dist_init disables all
interrupts including IPI. I've included a simple patch to the
gic_dist_init function that enables interrupts 0-15 on SMP enabled
systems. This patch was made against linux-2.6-HEAD-151f52f.


diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
--- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
+++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
@@ -262,6 +262,13 @@
        for (i = 0; i < max_irq; i += 32)
                writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 /
32);

+#ifdef CONFIG_SMP
+       /*
+        * Enable IPI interrupts on SMP systems so we can wake secondary
cores
+        */
+       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
+#endif
+
        /*
         * Setup the Linux IRQ subsystem.
         */





NOTES:

After browsing through the Kconfigs, I am confident that this will only
affect the following platforms (which have ARM_GIC and SMP):
MACH_REALVIEW, ARCH_OMAP4, ARCH_S5PV310, ARCH_VEXPRESS_CA9X4, ARCH_U8500,
and ARCH_TEGRA_2x_SOC. The TEGRA_2x appears to use a diffent smp-boot
mechanism, however. The patch was tested successfully on MACH_CNS3XXX
using the bsp for the gateworks laguna platform
(http://svn.gateworks.com/bsp/laguna/trunk/openwrt_patches).


Scott Valentine

Concentris Systems LLC
Manoa Innovation Center, Suite #238
2800 Woodlawn Drive
Honolulu, HI  96822

http://www.Concentris-Systems.com

(808) 988-6100

arm smp support patch

[Sergei Shtylyov]

Hello.

On 10-11-2010 2:33, Scott Valentine wrote:

> On arm multi-core platforms that have a gic, the secondary cores fail to
> wake if they are booted in WFI mode, as the gic_dist_init disables all
> interrupts including IPI. I've included a simple patch to the
> gic_dist_init function that enables interrupts 0-15 on SMP enabled
> systems. This patch was made against linux-2.6-HEAD-151f52f.

    Your signoff is necessary for the patch to be applied.

> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
> @@ -262,6 +262,13 @@
>          for (i = 0; i<  max_irq; i += 32)
>                  writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 /
> 32);
>
> +#ifdef CONFIG_SMP
> +       /*
> +        * Enable IPI interrupts on SMP systems so we can wake secondary
> cores

   Your patch is line wrapped.

> +        */
> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
> +#endif
> +
>          /*
>           * Setup the Linux IRQ subsystem.
>           */

> Scott Valentine

WBR, Sergei

arm smp support patch

[Scott Valentine]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com] 
> Sent: Wednesday, November 10, 2010 1:22 AM
> To: svalentine at concentris-systems.com
> Cc: linux at arm.linux.org.uk; laguna at lists.gateworks.com; 
> linux-arm-kernel at lists.infradead.org
> Subject: Re: arm smp support patch
> 
> Hello.
> 
> On 10-11-2010 2:33, Scott Valentine wrote:
> 
> > On arm multi-core platforms that have a gic, the secondary 
> cores fail 
> > to wake if they are booted in WFI mode, as the 
> gic_dist_init disables 
> > all interrupts including IPI. I've included a simple patch to the 
> > gic_dist_init function that enables interrupts 0-15 on SMP enabled 
> > systems. This patch was made against linux-2.6-HEAD-151f52f.
> 
>     Your signoff is necessary for the patch to be applied.
> 
> > diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> > --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
> > +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
> > @@ -262,6 +262,13 @@
> >          for (i = 0; i<  max_irq; i += 32)
> >                  writel(0xffffffff, base + 
> GIC_DIST_ENABLE_CLEAR + i * 
> > 4 / 32);
> >
> > +#ifdef CONFIG_SMP
> > +       /*
> > +        * Enable IPI interrupts on SMP systems so we can wake 
> > +secondary
> > cores
> 
>    Your patch is line wrapped.
> 
> > +        */
> > +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET); #endif
> > +
> >          /*
> >           * Setup the Linux IRQ subsystem.
> >           */
> 
> > Scott Valentine
> 
> WBR, Sergei

Sorry, bad mail-client settings. Also, I'm not really a maintainer, but I will signoff as requested. Below is the non-wrapped patch:

Signed-off-by: Scott Valentine <svalentine@concentris-systems.com>

diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
--- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
+++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
@@ -262,6 +262,13 @@
        for (i = 0; i < max_irq; i += 32)
                writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);

+#ifdef CONFIG_SMP
+       /*
+        * Enable IPI interrupts on SMP systems so we can wake secondary cores
+        */
+       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
+#endif
+
        /*
         * Setup the Linux IRQ subsystem.
         */

arm smp support patch

[George G. Davis]

On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
> On arm multi-core platforms that have a gic, the secondary cores fail to
> wake if they are booted in WFI mode, as the gic_dist_init disables all
> interrupts including IPI. I've included a simple patch to the
> gic_dist_init function that enables interrupts 0-15 on SMP enabled
> systems. This patch was made against linux-2.6-HEAD-151f52f.
> 
> 
> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
> @@ -262,6 +262,13 @@
>         for (i = 0; i < max_irq; i += 32)
>                 writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 /
> 32);
> 
> +#ifdef CONFIG_SMP
> +       /*
> +        * Enable IPI interrupts on SMP systems so we can wake secondary
> cores
> +        */
> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);

The ARM11 MPCore TRM [1] states "Interrupts 0-15 fields are read as one,
that is, always enabled, and write to these fields have no effect."  So
it seems odd that this is needed.  Errata?  FWIW, I've peeked and poked
at GIC_DIST_ENABLE_SET and GIC_DIST_ENABLE_CLEAR via a debugger and the
16 LSBs of both of these registers are stuck-at-one on my ARM11 MPCore
r1p0.

--
Regards,
George

[1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0360f/CCHGFFFA.html

> +#endif
> +
>         /*
>          * Setup the Linux IRQ subsystem.
>          */
> 
> 
> 
> 
> 
> NOTES:
> 
> After browsing through the Kconfigs, I am confident that this will only
> affect the following platforms (which have ARM_GIC and SMP):
> MACH_REALVIEW, ARCH_OMAP4, ARCH_S5PV310, ARCH_VEXPRESS_CA9X4, ARCH_U8500,
> and ARCH_TEGRA_2x_SOC. The TEGRA_2x appears to use a diffent smp-boot
> mechanism, however. The patch was tested successfully on MACH_CNS3XXX
> using the bsp for the gateworks laguna platform
> (http://svn.gateworks.com/bsp/laguna/trunk/openwrt_patches).
> 
> 
> Scott Valentine
> 
> Concentris Systems LLC
> Manoa Innovation Center, Suite #238
> 2800 Woodlawn Drive
> Honolulu, HI  96822
> 
> http://www.Concentris-Systems.com
> 
> (808) 988-6100
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

arm smp support patch

[Sergei Shtylyov]

Hello.

On 10-11-2010 23:37, Scott Valentine wrote:

>>> On arm multi-core platforms that have a gic, the secondary cores fail
>>> to wake if they are booted in WFI mode, as the gic_dist_init disables
>>> all interrupts including IPI. I've included a simple patch to the
>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>> systems. This patch was made against linux-2.6-HEAD-151f52f.

>>      Your signoff is necessary for the patch to be applied.

>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>> @@ -262,6 +262,13 @@
>>>           for (i = 0; i<   max_irq; i += 32)
>>>                   writel(0xffffffff, base +
>> GIC_DIST_ENABLE_CLEAR + i *
>>> 4 / 32);
>>>
>>> +#ifdef CONFIG_SMP
>>> +       /*
>>> +        * Enable IPI interrupts on SMP systems so we can wake
>>> +secondary
>>> cores

>>     Your patch is line wrapped.

>>> +        */
>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET); #endif
>>> +
>>>           /*
>>>            * Setup the Linux IRQ subsystem.
>>>            */

>>> Scott Valentine

>> WBR, Sergei

> Sorry, bad mail-client settings. Also, I'm not really a maintainer, but I will signoff as requested.

    You don't need to be a maintainer to sign off on your own patches. :-)

> Below is the non-wrapped patch:

    You didn't provide the patch summary this time.

> Signed-off-by: Scott Valentine<svalentine@concentris-systems.com>

WBR, Sergei

arm smp support patch

[Rob Herring]

On 11/10/2010 10:41 PM, George G. Davis wrote:
> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>> On arm multi-core platforms that have a gic, the secondary cores fail to
>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>> interrupts including IPI. I've included a simple patch to the
>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>
>>
>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>> @@ -262,6 +262,13 @@
>>          for (i = 0; i<  max_irq; i += 32)
>>                  writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 /
>> 32);
>>
>> +#ifdef CONFIG_SMP
>> +       /*
>> +        * Enable IPI interrupts on SMP systems so we can wake secondary
>> cores
>> +        */
>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>
> The ARM11 MPCore TRM [1] states "Interrupts 0-15 fields are read as one,
> that is, always enabled, and write to these fields have no effect."  So
> it seems odd that this is needed.  Errata?  FWIW, I've peeked and poked
> at GIC_DIST_ENABLE_SET and GIC_DIST_ENABLE_CLEAR via a debugger and the
> 16 LSBs of both of these registers are stuck-at-one on my ARM11 MPCore
> r1p0.
>
The GIC arch spec states these bits are implementation defined. However, 
this register is also banked for SMP. So you cannot enable the interrupt 
for other cores from the primary core.

Rob

arm smp support patch

[Scott Valentine]

> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>> On arm multi-core platforms that have a gic, the secondary cores fail to
>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>> interrupts including IPI. I've included a simple patch to the
>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>
>>
>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>> @@ -262,6 +262,13 @@
>>         for (i = 0; i < max_irq; i += 32)
>>                 writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4
>> /
>> 32);
>>
>> +#ifdef CONFIG_SMP
>> +       /*
>> +        * Enable IPI interrupts on SMP systems so we can wake secondary
>> cores
>> +        */
>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>
> The ARM11 MPCore TRM [1] states "Interrupts 0-15 fields are read as one,
> that is, always enabled, and write to these fields have no effect."  So
> it seems odd that this is needed.  Errata?  FWIW, I've peeked and poked
> at GIC_DIST_ENABLE_SET and GIC_DIST_ENABLE_CLEAR via a debugger and the
> 16 LSBs of both of these registers are stuck-at-one on my ARM11 MPCore
> r1p0.

Okay, so I was wondering why this "bug" would be sitting around with so
many mpcore systems out in the field. It must be specific to the econa
3xxx processor (I'm working with a cns3420), as I can verify that the
second core fails to boot without the patch, but successfully boots with
it. The second core never gets past the wfi state in u-boot without this
patch.

Perhaps it the section should be wrapped with:

#if defined(MACH_CNS3XXX) && defined(SMP)

Can anyone else verify the behavior on the econa processor?

Thanks,
Scott V.

>
> --
> Regards,
> George
>
> [1]
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0360f/CCHGFFFA.html
>
>> +#endif
>> +
>>         /*
>>          * Setup the Linux IRQ subsystem.
>>          */
>>
>>
>>
>>
>>
>> NOTES:
>>
>> After browsing through the Kconfigs, I am confident that this will only
>> affect the following platforms (which have ARM_GIC and SMP):
>> MACH_REALVIEW, ARCH_OMAP4, ARCH_S5PV310, ARCH_VEXPRESS_CA9X4,
>> ARCH_U8500,
>> and ARCH_TEGRA_2x_SOC. The TEGRA_2x appears to use a diffent smp-boot
>> mechanism, however. The patch was tested successfully on MACH_CNS3XXX
>> using the bsp for the gateworks laguna platform
>> (http://svn.gateworks.com/bsp/laguna/trunk/openwrt_patches).
>>
>>
>> Scott Valentine
>>
>> Concentris Systems LLC
>> Manoa Innovation Center, Suite #238
>> 2800 Woodlawn Drive
>> Honolulu, HI  96822
>>
>> http://www.Concentris-Systems.com
>>
>> (808) 988-6100
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


Scott Valentine

Concentris Systems LLC
Manoa Innovation Center, Suite #238
2800 Woodlawn Drive
Honolulu, HI  96822

http://www.Concentris-Systems.com

(808) 988-6100

arm smp support patch

[Scott Valentine]

> On 11/10/2010 10:41 PM, George G. Davis wrote:
>> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>>> On arm multi-core platforms that have a gic, the secondary cores fail
>>> to
>>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>>> interrupts including IPI. I've included a simple patch to the
>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>>
>>>
>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>> @@ -262,6 +262,13 @@
>>>          for (i = 0; i<  max_irq; i += 32)
>>>                  writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i *
>>> 4 /
>>> 32);
>>>
>>> +#ifdef CONFIG_SMP
>>> +       /*
>>> +        * Enable IPI interrupts on SMP systems so we can wake
>>> secondary
>>> cores
>>> +        */
>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>>
>> The ARM11 MPCore TRM [1] states "Interrupts 0-15 fields are read as one,
>> that is, always enabled, and write to these fields have no effect."  So
>> it seems odd that this is needed.  Errata?  FWIW, I've peeked and poked
>> at GIC_DIST_ENABLE_SET and GIC_DIST_ENABLE_CLEAR via a debugger and the
>> 16 LSBs of both of these registers are stuck-at-one on my ARM11 MPCore
>> r1p0.
>>
> The GIC arch spec states these bits are implementation defined. However,
> this register is also banked for SMP. So you cannot enable the interrupt
> for other cores from the primary core.
>
> Rob
>
I couldn't find any documentation that states this explicitly; my best
understanding is that the IPI interrupts (0-15) are aliased for each core.
The behavior I'm seeing in the econa indicates that the write from core 0
does in fact enable the interrupts on core 1.

arm smp support patch

[Abhijeet Dharmapurikar]

Scott Valentine wrote:
>> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>>> On arm multi-core platforms that have a gic, the secondary cores fail to
>>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>>> interrupts including IPI. I've included a simple patch to the
>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>>
>>>
>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>> @@ -262,6 +262,13 @@
>>>         for (i = 0; i < max_irq; i += 32)
>>>                 writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4
>>> /
>>> 32);
>>>
>>> +#ifdef CONFIG_SMP
>>> +       /*
>>> +        * Enable IPI interrupts on SMP systems so we can wake secondary
>>> cores
>>> +        */
>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);

Scott, you might want to try doing this first thing in gic_cpu_init. 
That seems to be the right place since the ENABLE_SET is banked per
cpu.

Abhijeet

arm smp support patch

[Russell King - ARM Linux]

On Thu, Nov 11, 2010 at 02:03:29PM -0600, Rob Herring wrote:
> On 11/10/2010 10:41 PM, George G. Davis wrote:
>> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>>> On arm multi-core platforms that have a gic, the secondary cores fail to
>>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>>> interrupts including IPI. I've included a simple patch to the
>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>>
>>>
>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>> @@ -262,6 +262,13 @@
>>>          for (i = 0; i<  max_irq; i += 32)
>>>                  writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 /
>>> 32);
>>>
>>> +#ifdef CONFIG_SMP
>>> +       /*
>>> +        * Enable IPI interrupts on SMP systems so we can wake secondary
>>> cores
>>> +        */
>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>>
>> The ARM11 MPCore TRM [1] states "Interrupts 0-15 fields are read as one,
>> that is, always enabled, and write to these fields have no effect."  So
>> it seems odd that this is needed.  Errata?  FWIW, I've peeked and poked
>> at GIC_DIST_ENABLE_SET and GIC_DIST_ENABLE_CLEAR via a debugger and the
>> 16 LSBs of both of these registers are stuck-at-one on my ARM11 MPCore
>> r1p0.
>>
> The GIC arch spec states these bits are implementation defined. However,  
> this register is also banked for SMP. So you cannot enable the interrupt  
> for other cores from the primary core.

Can you point out the ARM doc number/section which specifies that.

My GIC TRM (which may be outdated) shows that although the SGI interrupt
(0:15) enable register is banked, it goes on to say "The distributor does
not provide registers for INTIDs < 16 because SGIs are always enabled".

arm smp support patch

[Rob Herring]

On 11/11/2010 04:46 PM, Russell King - ARM Linux wrote:
> On Thu, Nov 11, 2010 at 02:03:29PM -0600, Rob Herring wrote:
>> On 11/10/2010 10:41 PM, George G. Davis wrote:
>>> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>>>> On arm multi-core platforms that have a gic, the secondary cores fail to
>>>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>>>> interrupts including IPI. I've included a simple patch to the
>>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>>>
>>>>
>>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>>> @@ -262,6 +262,13 @@
>>>>           for (i = 0; i<   max_irq; i += 32)
>>>>                   writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 /
>>>> 32);
>>>>
>>>> +#ifdef CONFIG_SMP
>>>> +       /*
>>>> +        * Enable IPI interrupts on SMP systems so we can wake secondary
>>>> cores
>>>> +        */
>>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>>>
>>> The ARM11 MPCore TRM [1] states "Interrupts 0-15 fields are read as one,
>>> that is, always enabled, and write to these fields have no effect."  So
>>> it seems odd that this is needed.  Errata?  FWIW, I've peeked and poked
>>> at GIC_DIST_ENABLE_SET and GIC_DIST_ENABLE_CLEAR via a debugger and the
>>> 16 LSBs of both of these registers are stuck-at-one on my ARM11 MPCore
>>> r1p0.
>>>
>> The GIC arch spec states these bits are implementation defined. However,
>> this register is also banked for SMP. So you cannot enable the interrupt
>> for other cores from the primary core.
>
> Can you point out the ARM doc number/section which specifies that.
>
> My GIC TRM (which may be outdated) shows that although the SGI interrupt
> (0:15) enable register is banked, it goes on to say "The distributor does
> not provide registers for INTIDs<  16 because SGIs are always enabled".
>

Section 4.3.5:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0048a/index.html

Rob

arm smp support patch

[Russell King - ARM Linux]

On Thu, Nov 11, 2010 at 04:50:14PM -0600, Rob Herring wrote:
> On 11/11/2010 04:46 PM, Russell King - ARM Linux wrote:
>> Can you point out the ARM doc number/section which specifies that.
>>
>> My GIC TRM (which may be outdated) shows that although the SGI interrupt
>> (0:15) enable register is banked, it goes on to say "The distributor does
>> not provide registers for INTIDs<  16 because SGIs are always enabled".
>>
>
> Section 4.3.5:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ihi0048a/index.html

Yea, just found that... ;)

arm smp support patch

[Russell King - ARM Linux]

On Thu, Nov 11, 2010 at 01:44:09PM -0800, Abhijeet Dharmapurikar wrote:
> Scott Valentine wrote:
>>> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>>>> On arm multi-core platforms that have a gic, the secondary cores fail to
>>>> wake if they are booted in WFI mode, as the gic_dist_init disables all
>>>> interrupts including IPI. I've included a simple patch to the
>>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>>>
>>>>
>>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>>> @@ -262,6 +262,13 @@
>>>>         for (i = 0; i < max_irq; i += 32)
>>>>                 writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4
>>>> /
>>>> 32);
>>>>
>>>> +#ifdef CONFIG_SMP
>>>> +       /*
>>>> +        * Enable IPI interrupts on SMP systems so we can wake secondary
>>>> cores
>>>> +        */
>>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>
> Scott, you might want to try doing this first thing in gic_cpu_init.  
> That seems to be the right place since the ENABLE_SET is banked per
> cpu.

Secondary CPUs are placed into WFI mode by whatever preceeds the kernel
as part of the boot protocol, and it is expected that they are sent some
kind of interrupt to wake them from WFI mode.  While they are in this
state, they have no knowledge of the kernel, so they haven't run any
kernel code.

The GIC TRM does imply that these bits are banked - the upper 16 bits
are used for PPIs (private peripheral interrupts, private to the CPU)
while the lower 16 bits are used for the SGI (software generated
interrupts.)  According to the online docs, ARM IHI 0048A (Generic
Interrupt Controller Architecture Specification) it is _implementation
defined_ whether the SGI enable bits are implemented.

If your pre-kernel boot software places your secondary CPUs into WFI
mode with all sources of software interrupts disabled, then it's fairly
clear that there's no way to wake them from WFI mode via software
generated GIC interrupts.

The only way to do it is to route a real hardware IRQ to your desired
secondary CPU, and provoke the hardware into generating an IRQ.  That's
hardly a good way to go about bringing secondary CPUs online.

It seems that we have one of the first implementations which has enable
register bits provided for the SGIs.  As the enable register bits are
banked on a per-CPU basis, there's nothing that the boot CPU can do to
set those enable bits - the only thing that can enable those is the
pre-kernel boot software.

However, we also need to ensure that the kernel doesn't clear those
bits.  So:

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index ada6359..a333811 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -257,9 +257,10 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
 		writel(0xa0a0a0a0, base + GIC_DIST_PRI + i * 4 / 4);
 
 	/*
-	 * Disable all interrupts.
+	 * Disable all interrupts, except the software generated interrupts.
 	 */
-	for (i = 0; i < max_irq; i += 32)
+	writel(0xffff0000, base + GIC_DIST_ENABLE_CLEAR);
+	for (i = 32; i < max_irq; i += 32)
 		writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
 
 	/*

and we need to have the boot software on this platform to set the
enable bits.

arm smp support patch

[Russell King - ARM Linux]

On Thu, Nov 11, 2010 at 11:06:52PM +0000, Russell King - ARM Linux wrote:
> It seems that we have one of the first implementations which has enable
> register bits provided for the SGIs.  As the enable register bits are
> banked on a per-CPU basis, there's nothing that the boot CPU can do to
> set those enable bits - the only thing that can enable those is the
> pre-kernel boot software.

Reading a bit deeper, not only are the enable registers banked, but also
the priority registers.  So let's do this properly and move the code
accessing these banked registers to the per-CPU initialization function.

I've also decided that we should explicitly enable all the SGI interrupts
so that we're compatible with what happens on the previous GICs if these
enable bits are programmable.

Note that as gic_cpu_init() now accesses distributor registers, it
requires that gic_dist_init() has already been called - which appears
to be the case already by everyone using the GIC code.

From: Russell King - ARM Linux <linux@arm.linux.org.uk>

ARM: GIC: don't disable software generated interrupts

Software generated interrupts (SGI) are used for IPIs by the kernel.
While previous revisions of the GIC hardware were specified not to
implement enable bits for SGIs, more recent hardware is now permitted
to implement these bits in a per-CPU banked register.

The priority registers for the PPI and SGIs are also per-CPU banked
registers, so ensure that these are also appropriately initialized.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/common/gic.c |   28 ++++++++++++++++++++++++----
 1 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index ada6359..772f95f 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -251,15 +251,16 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
 		writel(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
 
 	/*
-	 * Set priority on all interrupts.
+	 * Set priority on all global interrupts.
 	 */
-	for (i = 0; i < max_irq; i += 4)
+	for (i = 32; i < max_irq; i += 4)
 		writel(0xa0a0a0a0, base + GIC_DIST_PRI + i * 4 / 4);
 
 	/*
-	 * Disable all interrupts.
+	 * Disable all interrupts.  Leave the PPI and SGIs alone
+	 * as these enables are banked registers.
 	 */
-	for (i = 0; i < max_irq; i += 32)
+	for (i = 32; i < max_irq; i += 32)
 		writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
 
 	/*
@@ -277,11 +278,30 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
 
 void __cpuinit gic_cpu_init(unsigned int gic_nr, void __iomem *base)
 {
+	void __iomem *dist_base;
+	int i;
+
 	if (gic_nr >= MAX_GIC_NR)
 		BUG();
 
+	dist_base = gic_data[gic_nr].dist_base;
+	BUG_ON(!dist_base);
+
 	gic_data[gic_nr].cpu_base = base;
 
+	/*
+	 * Deal with the banked PPI and SGI interrupts - disable all
+	 * PPI interrupts, ensure all SGI interrupts are enabled.
+	 */
+	writel(0xffff0000, dist_base + GIC_DIST_ENABLE_CLEAR);
+	writel(0x0000ffff, dist_base + GIC_DIST_ENABLE_SET);
+
+	/*
+	 * Set priority on PPI and SGI interrupts
+	 */
+	for (i = 0; i < 32; i += 4)
+		writel(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4);
+
 	writel(0xf0, base + GIC_CPU_PRIMASK);
 	writel(1, base + GIC_CPU_CTRL);
 }
-- 
1.6.2.5

arm smp support patch

[Abhijeet Dharmapurikar]

> From: Russell King - ARM Linux <linux@arm.linux.org.uk>
> 
> ARM: GIC: don't disable software generated interrupts
> 
> Software generated interrupts (SGI) are used for IPIs by the kernel.
> While previous revisions of the GIC hardware were specified not to
> implement enable bits for SGIs, more recent hardware is now permitted
> to implement these bits in a per-CPU banked register.
> 
> The priority registers for the PPI and SGIs are also per-CPU banked
> registers, so ensure that these are also appropriately initialized.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/common/gic.c |   28 ++++++++++++++++++++++++----
>  1 files changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index ada6359..772f95f 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -251,15 +251,16 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
>  		writel(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
>  
>  	/*
> -	 * Set priority on all interrupts.
> +	 * Set priority on all global interrupts.
>  	 */
> -	for (i = 0; i < max_irq; i += 4)
> +	for (i = 32; i < max_irq; i += 4)
>  		writel(0xa0a0a0a0, base + GIC_DIST_PRI + i * 4 / 4);
>  
>  	/*
> -	 * Disable all interrupts.
> +	 * Disable all interrupts.  Leave the PPI and SGIs alone
> +	 * as these enables are banked registers.
>  	 */
> -	for (i = 0; i < max_irq; i += 32)
> +	for (i = 32; i < max_irq; i += 32)
>  		writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
>  
>  	/*
> @@ -277,11 +278,30 @@ void __init gic_dist_init(unsigned int gic_nr, void __iomem *base,
>  
>  void __cpuinit gic_cpu_init(unsigned int gic_nr, void __iomem *base)
>  {
> +	void __iomem *dist_base;
> +	int i;
> +
>  	if (gic_nr >= MAX_GIC_NR)
>  		BUG();
>  
> +	dist_base = gic_data[gic_nr].dist_base;
> +	BUG_ON(!dist_base);
> +
>  	gic_data[gic_nr].cpu_base = base;
>  
> +	/*
> +	 * Deal with the banked PPI and SGI interrupts - disable all
> +	 * PPI interrupts, ensure all SGI interrupts are enabled.
> +	 */
> +	writel(0xffff0000, dist_base + GIC_DIST_ENABLE_CLEAR);
> +	writel(0x0000ffff, dist_base + GIC_DIST_ENABLE_SET);
> +
> +	/*
> +	 * Set priority on PPI and SGI interrupts
> +	 */
> +	for (i = 0; i < 32; i += 4)
> +		writel(0xa0a0a0a0, dist_base + GIC_DIST_PRI + i * 4 / 4);
> +
>  	writel(0xf0, base + GIC_CPU_PRIMASK);
>  	writel(1, base + GIC_CPU_CTRL);
>  }

Tested on msm 8660.

Acked-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>

--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

arm smp support patch

[Scott Valentine]

> On Thu, Nov 11, 2010 at 01:44:09PM -0800, Abhijeet Dharmapurikar wrote:
>> Scott Valentine wrote:
>>>> On Tue, Nov 09, 2010 at 01:33:20PM -1000, Scott Valentine wrote:
>>>>> On arm multi-core platforms that have a gic, the secondary cores fail
>>>>> to
>>>>> wake if they are booted in WFI mode, as the gic_dist_init disables
>>>>> all
>>>>> interrupts including IPI. I've included a simple patch to the
>>>>> gic_dist_init function that enables interrupts 0-15 on SMP enabled
>>>>> systems. This patch was made against linux-2.6-HEAD-151f52f.
>>>>>
>>>>>
>>>>> diff -uNr a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>>>> --- a/arch/arm/common/gic.c     2010-11-05 15:57:04.000000000 -1000
>>>>> +++ b/arch/arm/common/gic.c     2010-11-09 13:08:33.000000000 -1000
>>>>> @@ -262,6 +262,13 @@
>>>>>         for (i = 0; i < max_irq; i += 32)
>>>>>                 writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i *
>>>>> 4
>>>>> /
>>>>> 32);
>>>>>
>>>>> +#ifdef CONFIG_SMP
>>>>> +       /*
>>>>> +        * Enable IPI interrupts on SMP systems so we can wake
>>>>> secondary
>>>>> cores
>>>>> +        */
>>>>> +       writel(0x0000ffff, base + GIC_DIST_ENABLE_SET);
>>
>> Scott, you might want to try doing this first thing in gic_cpu_init.
>> That seems to be the right place since the ENABLE_SET is banked per
>> cpu.
>
> Secondary CPUs are placed into WFI mode by whatever preceeds the kernel
> as part of the boot protocol, and it is expected that they are sent some
> kind of interrupt to wake them from WFI mode.  While they are in this
> state, they have no knowledge of the kernel, so they haven't run any
> kernel code.
>
> The GIC TRM does imply that these bits are banked - the upper 16 bits
> are used for PPIs (private peripheral interrupts, private to the CPU)
> while the lower 16 bits are used for the SGI (software generated
> interrupts.)  According to the online docs, ARM IHI 0048A (Generic
> Interrupt Controller Architecture Specification) it is _implementation
> defined_ whether the SGI enable bits are implemented.
>
> If your pre-kernel boot software places your secondary CPUs into WFI
> mode with all sources of software interrupts disabled, then it's fairly
> clear that there's no way to wake them from WFI mode via software
> generated GIC interrupts.
>
> The only way to do it is to route a real hardware IRQ to your desired
> secondary CPU, and provoke the hardware into generating an IRQ.  That's
> hardly a good way to go about bringing secondary CPUs online.
>
> It seems that we have one of the first implementations which has enable
> register bits provided for the SGIs.  As the enable register bits are
> banked on a per-CPU basis, there's nothing that the boot CPU can do to
> set those enable bits - the only thing that can enable those is the
> pre-kernel boot software.
>
> However, we also need to ensure that the kernel doesn't clear those
> bits.  So:
>
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index ada6359..a333811 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -257,9 +257,10 @@ void __init gic_dist_init(unsigned int gic_nr, void
> __iomem *base,
>  		writel(0xa0a0a0a0, base + GIC_DIST_PRI + i * 4 / 4);
>
>  	/*
> -	 * Disable all interrupts.
> +	 * Disable all interrupts, except the software generated interrupts.
>  	 */
> -	for (i = 0; i < max_irq; i += 32)
> +	writel(0xffff0000, base + GIC_DIST_ENABLE_CLEAR);
> +	for (i = 32; i < max_irq; i += 32)
>  		writel(0xffffffff, base + GIC_DIST_ENABLE_CLEAR + i * 4 / 32);
>
>  	/*
>
> and we need to have the boot software on this platform to set the
> enable bits.
>

Thanks Russel,

I've been gone for the weekend, but I had given this some thought and this
seems like the proper approach. The power on state should be correct, so
the boot loader shouldn't have to really do anything. In any case, it
seems appropriate that the boot loader should ensure that the core is in
the appropriate state to allow the processor to wake up, and the kernel
should avoid messing with it.

So, I basically came to the conclusion as well that the OS should not
write ones to the lower 16 bits of the first GIC_DIST_ENABLE_CLEAR
register, as newer implementations apparently allow this to disable the
interrupts, but it has no effect on older implementations.

It is correct though, that the secondary cores really can't be manipulated
by the kernel init code, as they are in WFI state.

The gic_cpu_init code is called per core, but for secondary cores, this
only happens after it is awoken by an interrupt, so as you pointed out, it
is irrelavant to do any setup there.

It still seems odd to me, with respect to some of the comments, that a
write to GIC_DIST_ENABLE_SET from the primary core (per my previous patch
submission) actually works (which I can verify), or, conversly, the fact
that GIC_DIST_ENABLE_CLEAR was being written to causes the secondary cores
to get stuck in WFI. In the grand scheme of things, I suppose it doesn't
matter, but I wonder, are those registers actually "banked" as some have
commented? The behavior I am seeing indicates otherwise. On the other
hand, the documentation mentions that the priority of an IPI is that of
the interrupting cpu, so perhaps that has something to do with it?

I'll do a quick verification of this patch tomorrow on the econa processor
and provide verification that the issue is correctly resolved. Thanks for
the support.

Thanks again,
-Scott V.