[PATCH] keystone: adds cpu_die implementation

[PATCH] keystone: adds cpu_die implementation

[Vitaly Andrianov]

This commit add cpu_die implementation

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
---

The discussion of the "keystone: psci: adds cpu_die implementation" commit
shows that if PCSI is enabled platform code doesn't need that implementation
at all. Having PSCI commands in DTB should be sufficient. Unfortunately 
Keystone with LPAE enable requires some additional development.

To support HOTPLUG_CPU w/o PSCI we need platform implementation of
the cpu_die(), which is added by this patch.

 arch/arm/mach-keystone/keystone.h |  1 +
 arch/arm/mach-keystone/platsmp.c  | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
index cd04a1c..93549cf 100644
--- a/arch/arm/mach-keystone/keystone.h
+++ b/arch/arm/mach-keystone/keystone.h
@@ -12,6 +12,7 @@
 #define __KEYSTONE_H__
 
 #define KEYSTONE_MON_CPU_UP_IDX		0x00
+#define KEYSTONE_MON_CPU_DIE_IDX	0x84000002
 
 #ifndef __ASSEMBLER__
 
diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
index 5f46a7c..4e5bdde 100644
--- a/arch/arm/mach-keystone/platsmp.c
+++ b/arch/arm/mach-keystone/platsmp.c
@@ -51,7 +51,31 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
 {}
 #endif
 
+
+#ifdef CONFIG_HOTPLUG_CPU
+static void keystone_cpu_die(unsigned int cpu)
+{
+	int error;
+
+	pr_debug("keystone-smp: cpu_die %d\n", cpu);
+
+	error = keystone_cpu_smc(KEYSTONE_MON_CPU_DIE_IDX, cpu, 0);
+	if (error)
+		pr_err("CPU %d die failed with %d\n", cpu, error);
+
+	/*
+	 * we shouldn't come here. But in case something went
+	 * wrong the code below prevents kernel from crush
+	 */
+	while (1)
+		cpu_do_idle();
+}
+#endif
+
 struct smp_operations keystone_smp_ops __initdata = {
 	.smp_boot_secondary	= keystone_smp_boot_secondary,
 	.smp_secondary_init     = keystone_smp_secondary_initmem,
+#ifdef CONFIG_HOTPLUG_CPU
+	.cpu_die		= keystone_cpu_die,
+#endif
 };
-- 
1.9.1

[PATCH] keystone: adds cpu_die implementation

[Mark Rutland]

On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote:
> This commit add cpu_die implementation
> 
> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> ---
> 
> The discussion of the "keystone: psci: adds cpu_die implementation" commit
> shows that if PCSI is enabled platform code doesn't need that implementation
> at all. Having PSCI commands in DTB should be sufficient. Unfortunately 
> Keystone with LPAE enable requires some additional development.

I don't follow.

What do you need to implement for LPAE?

Why not implement that rather than adding more platform code that you'll
likely want to remove later anyway?

Thanks,
Mark.

> To support HOTPLUG_CPU w/o PSCI we need platform implementation of
> the cpu_die(), which is added by this patch.
> 
>  arch/arm/mach-keystone/keystone.h |  1 +
>  arch/arm/mach-keystone/platsmp.c  | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
> index cd04a1c..93549cf 100644
> --- a/arch/arm/mach-keystone/keystone.h
> +++ b/arch/arm/mach-keystone/keystone.h
> @@ -12,6 +12,7 @@
>  #define __KEYSTONE_H__
>  
>  #define KEYSTONE_MON_CPU_UP_IDX		0x00
> +#define KEYSTONE_MON_CPU_DIE_IDX	0x84000002
>  
>  #ifndef __ASSEMBLER__
>  
> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
> index 5f46a7c..4e5bdde 100644
> --- a/arch/arm/mach-keystone/platsmp.c
> +++ b/arch/arm/mach-keystone/platsmp.c
> @@ -51,7 +51,31 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
>  {}
>  #endif
>  
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +static void keystone_cpu_die(unsigned int cpu)
> +{
> +	int error;
> +
> +	pr_debug("keystone-smp: cpu_die %d\n", cpu);
> +
> +	error = keystone_cpu_smc(KEYSTONE_MON_CPU_DIE_IDX, cpu, 0);
> +	if (error)
> +		pr_err("CPU %d die failed with %d\n", cpu, error);
> +
> +	/*
> +	 * we shouldn't come here. But in case something went
> +	 * wrong the code below prevents kernel from crush
> +	 */
> +	while (1)
> +		cpu_do_idle();
> +}
> +#endif
> +
>  struct smp_operations keystone_smp_ops __initdata = {
>  	.smp_boot_secondary	= keystone_smp_boot_secondary,
>  	.smp_secondary_init     = keystone_smp_secondary_initmem,
> +#ifdef CONFIG_HOTPLUG_CPU
> +	.cpu_die		= keystone_cpu_die,
> +#endif
>  };
> -- 
> 1.9.1
> 

[PATCH] keystone: adds cpu_die implementation

[Vitaly Andrianov]

On 06/29/2015 01:52 PM, Mark Rutland wrote:
> On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote:
>> This commit add cpu_die implementation
>>
>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>> ---
>>
>> The discussion of the "keystone: psci: adds cpu_die implementation" commit
>> shows that if PCSI is enabled platform code doesn't need that implementation
>> at all. Having PSCI commands in DTB should be sufficient. Unfortunately
>> Keystone with LPAE enable requires some additional development.
>
> I don't follow.
>
> What do you need to implement for LPAE?
Hi Mark,

The Keystone platform needs to set ttbr1 when it boots secondary core.
It is done in the keystone_smp_secondary_initmem(), which is 
.smp_secondary_init member of the keystone_smp_ops. I couldn't find a 
way how I can add similar function to psci_smp_ops.

Do you have any idea?

>
> Why not implement that rather than adding more platform code that you'll
> likely want to remove later anyway?

If I can solve the above problem, I will not add this code.

Thanks,
Vitaly

>
> Thanks,
> Mark.
>
>> To support HOTPLUG_CPU w/o PSCI we need platform implementation of
>> the cpu_die(), which is added by this patch.
>>
>>   arch/arm/mach-keystone/keystone.h |  1 +
>>   arch/arm/mach-keystone/platsmp.c  | 24 ++++++++++++++++++++++++
>>   2 files changed, 25 insertions(+)
>>
>> diff --git a/arch/arm/mach-keystone/keystone.h b/arch/arm/mach-keystone/keystone.h
>> index cd04a1c..93549cf 100644
>> --- a/arch/arm/mach-keystone/keystone.h
>> +++ b/arch/arm/mach-keystone/keystone.h
>> @@ -12,6 +12,7 @@
>>   #define __KEYSTONE_H__
>>
>>   #define KEYSTONE_MON_CPU_UP_IDX		0x00
>> +#define KEYSTONE_MON_CPU_DIE_IDX	0x84000002
>>
>>   #ifndef __ASSEMBLER__
>>
>> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
>> index 5f46a7c..4e5bdde 100644
>> --- a/arch/arm/mach-keystone/platsmp.c
>> +++ b/arch/arm/mach-keystone/platsmp.c
>> @@ -51,7 +51,31 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
>>   {}
>>   #endif
>>
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +static void keystone_cpu_die(unsigned int cpu)
>> +{
>> +	int error;
>> +
>> +	pr_debug("keystone-smp: cpu_die %d\n", cpu);
>> +
>> +	error = keystone_cpu_smc(KEYSTONE_MON_CPU_DIE_IDX, cpu, 0);
>> +	if (error)
>> +		pr_err("CPU %d die failed with %d\n", cpu, error);
>> +
>> +	/*
>> +	 * we shouldn't come here. But in case something went
>> +	 * wrong the code below prevents kernel from crush
>> +	 */
>> +	while (1)
>> +		cpu_do_idle();
>> +}
>> +#endif
>> +
>>   struct smp_operations keystone_smp_ops __initdata = {
>>   	.smp_boot_secondary	= keystone_smp_boot_secondary,
>>   	.smp_secondary_init     = keystone_smp_secondary_initmem,
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +	.cpu_die		= keystone_cpu_die,
>> +#endif
>>   };
>> --
>> 1.9.1
>>

[PATCH] keystone: adds cpu_die implementation

[santosh.shilimkar at oracle.com]

On 6/29/15 11:43 AM, Vitaly Andrianov wrote:
>
>
> On 06/29/2015 01:52 PM, Mark Rutland wrote:
>> On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote:
>>> This commit add cpu_die implementation
>>>
>>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>>> ---
>>>
>>> The discussion of the "keystone: psci: adds cpu_die implementation"
>>> commit
>>> shows that if PCSI is enabled platform code doesn't need that
>>> implementation
>>> at all. Having PSCI commands in DTB should be sufficient. Unfortunately
>>> Keystone with LPAE enable requires some additional development.
>>
>> I don't follow.
>>
>> What do you need to implement for LPAE?
> Hi Mark,
>
> The Keystone platform needs to set ttbr1 when it boots secondary core.
> It is done in the keystone_smp_secondary_initmem(), which is
> .smp_secondary_init member of the keystone_smp_ops. I couldn't find a
> way how I can add similar function to psci_smp_ops.
>
> Do you have any idea?
>
>>
>> Why not implement that rather than adding more platform code that you'll
>> likely want to remove later anyway?
>
> If I can solve the above problem, I will not add this code.
>
This problem is already solved. :-)

After [1], there is no longer need to fiddle with TTBR1 setup
for secondary bringup.  I believe its already in RMK's queue and it
should show up in linus's tree after 4.2 merge window.


Regards,
Santosh

[1] http://www.spinics.net/lists/arm-kernel/msg416129.html

[PATCH] keystone: adds cpu_die implementation

[Russell King - ARM Linux]

On Mon, Jun 29, 2015 at 02:43:44PM -0400, Vitaly Andrianov wrote:
> 
> 
> On 06/29/2015 01:52 PM, Mark Rutland wrote:
> >On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote:
> >>This commit add cpu_die implementation
> >>
> >>Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> >>---
> >>
> >>The discussion of the "keystone: psci: adds cpu_die implementation" commit
> >>shows that if PCSI is enabled platform code doesn't need that implementation
> >>at all. Having PSCI commands in DTB should be sufficient. Unfortunately
> >>Keystone with LPAE enable requires some additional development.
> >
> >I don't follow.
> >
> >What do you need to implement for LPAE?
> Hi Mark,
> 
> The Keystone platform needs to set ttbr1 when it boots secondary core.
> It is done in the keystone_smp_secondary_initmem(), which is
> .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way
> how I can add similar function to psci_smp_ops.

TTBR1 will be set by generic code.  You don't need to do anything special
now that my fixes for TI's horrid physical address space switch are in.
(you may remember, you tested the patches...)

secondary_startup now loads r4/r5 with the 64-bit physical swapper page
table address, and r8 contains the _pfn_ of the swapper_pg_dir.

It then calls through to (eventually) __v7_setup, which passes these
registers through to the v7_ttb_setup asm macro - r4 as ttbr0l, r5 as
ttbr0h, and r8 as ttbr1.

v7_ttb_setup converts the PFN to a physical address and programs it
into TTBR1.  TTBR0 is set directly from r4/r5 in generic code paths in
head.S::__enable_mmu.

There is no need to do any messing around when starting up a secondary
core on Keystone2.  In fact, my patches that I pointed you at a few
months ago ripped out all that code.  The entire keystone2 platsmp.c
file now contains only this code:

static int keystone_smp_boot_secondary(unsigned int cpu,
                                                struct task_struct *idle)
{
        unsigned long start = virt_to_idmap(&secondary_startup);
        int error;

        pr_debug("keystone-smp: booting cpu %d, vector %08lx\n",
                 cpu, start);

        error = keystone_cpu_smc(KEYSTONE_MON_CPU_UP_IDX, cpu, start);
        if (error)
                pr_err("CPU %d bringup failed with %d\n", cpu, error);

        return error;
}

struct smp_operations keystone_smp_ops __initdata = {
        .smp_boot_secondary     = keystone_smp_boot_secondary,
};

which is a lot simpler than it was.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] keystone: adds cpu_die implementation

[Russell King - ARM Linux]

On Mon, Jun 29, 2015 at 10:28:14PM +0100, Russell King - ARM Linux wrote:
> On Mon, Jun 29, 2015 at 02:43:44PM -0400, Vitaly Andrianov wrote:
> > 
> > 
> > On 06/29/2015 01:52 PM, Mark Rutland wrote:
> > >On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote:
> > >>This commit add cpu_die implementation
> > >>
> > >>Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> > >>---
> > >>
> > >>The discussion of the "keystone: psci: adds cpu_die implementation" commit
> > >>shows that if PCSI is enabled platform code doesn't need that implementation
> > >>at all. Having PSCI commands in DTB should be sufficient. Unfortunately
> > >>Keystone with LPAE enable requires some additional development.
> > >
> > >I don't follow.
> > >
> > >What do you need to implement for LPAE?
> > Hi Mark,
> > 
> > The Keystone platform needs to set ttbr1 when it boots secondary core.
> > It is done in the keystone_smp_secondary_initmem(), which is
> > .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way
> > how I can add similar function to psci_smp_ops.
> 
> TTBR1 will be set by generic code.  You don't need to do anything special
> now that my fixes for TI's horrid physical address space switch are in.
> (you may remember, you tested the patches...)

Oh, it was Murali who tested it, not yourself.  Sorry.  Suggest you
dig out the patches either from mainline (they're in Linus' tip) or
ask Murali for them...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] keystone: adds cpu_die implementation

[Vitaly Andrianov]

On 06/29/2015 05:37 PM, Russell King - ARM Linux wrote:
> On Mon, Jun 29, 2015 at 10:28:14PM +0100, Russell King - ARM Linux wrote:
>> On Mon, Jun 29, 2015 at 02:43:44PM -0400, Vitaly Andrianov wrote:
>>>
>>>
>>> On 06/29/2015 01:52 PM, Mark Rutland wrote:
>>>> On Mon, Jun 29, 2015 at 06:52:32PM +0100, Vitaly Andrianov wrote:
>>>>> This commit add cpu_die implementation
>>>>>
>>>>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>>>>> ---
>>>>>
>>>>> The discussion of the "keystone: psci: adds cpu_die implementation" commit
>>>>> shows that if PCSI is enabled platform code doesn't need that implementation
>>>>> at all. Having PSCI commands in DTB should be sufficient. Unfortunately
>>>>> Keystone with LPAE enable requires some additional development.
>>>>
>>>> I don't follow.
>>>>
>>>> What do you need to implement for LPAE?
>>> Hi Mark,
>>>
>>> The Keystone platform needs to set ttbr1 when it boots secondary core.
>>> It is done in the keystone_smp_secondary_initmem(), which is
>>> .smp_secondary_init member of the keystone_smp_ops. I couldn't find a way
>>> how I can add similar function to psci_smp_ops.
>>
>> TTBR1 will be set by generic code.  You don't need to do anything special
>> now that my fixes for TI's horrid physical address space switch are in.
>> (you may remember, you tested the patches...)
>
> Oh, it was Murali who tested it, not yourself.  Sorry.  Suggest you
> dig out the patches either from mainline (they're in Linus' tip) or
> ask Murali for them...
>
Thanks Russell,

Excellent. I'll test how it will work using PSCI framework.

Regards,
Vitaly

[PATCH] keystone: adds cpu_die implementation

[Russell King - ARM Linux]

On Tue, Jun 30, 2015 at 09:47:55AM -0400, Vitaly Andrianov wrote:
> 
> 
> On 06/29/2015 05:37 PM, Russell King - ARM Linux wrote:
> >Oh, it was Murali who tested it, not yourself.  Sorry.  Suggest you
> >dig out the patches either from mainline (they're in Linus' tip) or
> >ask Murali for them...
> >
> Thanks Russell,
> 
> Excellent. I'll test how it will work using PSCI framework.

The commits in mainline are:

b2c3e38a5471 ARM: redo TTBR setup code for LPAE
1221ed10f2a5 ARM: cleanup early_paging_init() calling
d8dc7fbd53ee ARM: re-implement physical address space switching
c0b759d87eab ARM: keystone2: rename init_meminfo to pv_fixup
39b74fe82f73 ARM: keystone2: move address space switch printk into generic code
c8ca2b4b2928 ARM: keystone2: move update of the phys-to-virt constants into generic code
30b5f4d6128e ARM: keystone2: move platform notifier initialisation into platform init

There are an additional three which are worthwhile testing with them as
they clean up this same code (and fix a subtle but very minor bug in the
errata code paths):

c76f238e261b ARM: proc-v7: sanitise and document registers around errata
4419496884ed ARM: proc-v7: clean up MIDR access
17e7bf86690e ARM: proc-v7: move CPU errata out of line

So, picking the range b787f68c36d4..c76f238e261b will get all of them.
(That's v4.1-rc1 to the tip of the branch holding those commits.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH] keystone: adds cpu_die implementation

[Vitaly Andrianov]

On 06/30/2015 10:54 AM, Russell King - ARM Linux wrote:
> On Tue, Jun 30, 2015 at 09:47:55AM -0400, Vitaly Andrianov wrote:
>>
>>
>> On 06/29/2015 05:37 PM, Russell King - ARM Linux wrote:
>>> Oh, it was Murali who tested it, not yourself.  Sorry.  Suggest you
>>> dig out the patches either from mainline (they're in Linus' tip) or
>>> ask Murali for them...
>>>
>> Thanks Russell,
>>
>> Excellent. I'll test how it will work using PSCI framework.
>
> The commits in mainline are:
>
> b2c3e38a5471 ARM: redo TTBR setup code for LPAE
> 1221ed10f2a5 ARM: cleanup early_paging_init() calling
> d8dc7fbd53ee ARM: re-implement physical address space switching
> c0b759d87eab ARM: keystone2: rename init_meminfo to pv_fixup
> 39b74fe82f73 ARM: keystone2: move address space switch printk into generic code
> c8ca2b4b2928 ARM: keystone2: move update of the phys-to-virt constants into generic code
> 30b5f4d6128e ARM: keystone2: move platform notifier initialisation into platform init
>
> There are an additional three which are worthwhile testing with them as
> they clean up this same code (and fix a subtle but very minor bug in the
> errata code paths):
>
> c76f238e261b ARM: proc-v7: sanitise and document registers around errata
> 4419496884ed ARM: proc-v7: clean up MIDR access
> 17e7bf86690e ARM: proc-v7: move CPU errata out of line
>
> So, picking the range b787f68c36d4..c76f238e261b will get all of them.
> (That's v4.1-rc1 to the tip of the branch holding those commits.)
>
I just finished testing. Here is what I did.

1) checkout commit c76f238e261b
2) applied Grygorii's "ARM: psci: boot_secondary: replace __pa with 
virt_to_idmap"
3) applied "keystone: dts: add psci command definition" commit.
This commit is not posted yet. So, here it's content:

diff --git a/arch/arm/boot/dts/keystone.dtsi 
b/arch/arm/boot/dts/keystone.dtsi
index c06542b..ab60fca 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -58,6 +58,14 @@
                              <GIC_SPI 23 IRQ_TYPE_EDGE_RISING>;
         };

+       psci {
+               compatible      = "arm,psci";
+               method          = "smc";
+               cpu_suspend     = <0x84000001>;
+               cpu_off         = <0x84000002>;
+               cpu_on          = <0x84000003>;
+       };
+
         soc {
                 #address-cells = <1>;
                 #size-cells = <1>;

4) excluded platsmp.c from build

diff --git a/arch/arm/mach-keystone/Makefile 
b/arch/arm/mach-keystone/Makefile
index 25d9239..5cc5ca4 100644
--- a/arch/arm/mach-keystone/Makefile
+++ b/arch/arm/mach-keystone/Makefile
@@ -3,7 +3,9 @@ obj-y                                   := keystone.o smc.o
  plus_sec := $(call as-instr,.arch_extension sec,+sec)
  AFLAGS_smc.o                           :=-Wa,-march=armv7-a$(plus_sec)

+ifneq ($(CONFIG_ARM_PSCI),y)
  obj-$(CONFIG_SMP)                      += platsmp.o
+endif

  # PM domain driver for Keystone SOCs
  obj-$(CONFIG_ARCH_KEYSTONE)            += pm_domain.o
diff --git a/arch/arm/mach-keystone/keystone.c 
b/arch/arm/mach-keystone/keystone.c
index e2880105..9cc489c 100644
--- a/arch/arm/mach-keystone/keystone.c
+++ b/arch/arm/mach-keystone/keystone.c
@@ -105,7 +105,9 @@ DT_MACHINE_START(KEYSTONE, "Keystone")
  #if defined(CONFIG_ZONE_DMA) && defined(CONFIG_ARM_LPAE)
         .dma_zone_size  = SZ_2G,
  #endif
+#ifndef CONFIG_ARM_PSCI
         .smp            = smp_ops(keystone_smp_ops),
+#endif
         .init_machine   = keystone_init,
         .dt_compat      = keystone_match,
         .pv_fixup       = keystone_pv_fixup,

5) enabled CONFIG_ARM_PSCI and CONFIG_HOTPLUG_CPU

The K2HK EVM booted fine with all 4 CPUs on.
The HOTPLUG also works fine. I was able to get CPUs off and on multiple 
times.

Thanks,
Vitaly