[PATCH] keystone: psci: adds cpu_die implementation

[PATCH] keystone: psci: adds cpu_die implementation

[Vitaly Andrianov]

This commit add cpu_die implementation using psci api

Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
---
 arch/arm/mach-keystone/platsmp.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
index 5f46a7c..2c40cc0 100644
--- a/arch/arm/mach-keystone/platsmp.c
+++ b/arch/arm/mach-keystone/platsmp.c
@@ -20,6 +20,7 @@
 #include <asm/prom.h>
 #include <asm/tlbflush.h>
 #include <asm/pgtable.h>
+#include <asm/psci.h>
 
 #include "keystone.h"
 
@@ -51,7 +52,38 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
 {}
 #endif
 
+
+#ifdef CONFIG_HOTPLUG_CPU
+static void keystone_cpu_die(unsigned int cpu)
+{
+#ifdef CONFIG_ARM_PSCI
+	struct psci_power_state pwr_state = {0, 0, 0};
+
+	pr_info("keystone_cpu_die(%d) from %d using PSCI\n", cpu,
+	       smp_processor_id());
+
+	if (psci_ops.cpu_off)
+		psci_ops.cpu_off(pwr_state);
+#else
+	/*
+	 * We may want to add here a direct smc call to monitor
+	 * if the kernel doesn't support PSCI API
+	 */
+#endif
+
+	/*
+	 * 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: psci: adds cpu_die implementation

[Mark Rutland]

Hi,

On Thu, Jun 25, 2015 at 03:02:50PM +0100, Vitaly Andrianov wrote:
> This commit add cpu_die implementation using psci api

I don't understand. If you have a PSCI implementation, it should be
sufficient to have a PSCI node (and enable-method) in your DT, and the
generic code will be used. Nothing should be required in your board
code.

You should also use CPU_ON to bring secondaries online rather than
mixing up PSCI and platform-specific mechanisms.

> 
> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> ---
>  arch/arm/mach-keystone/platsmp.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
> index 5f46a7c..2c40cc0 100644
> --- a/arch/arm/mach-keystone/platsmp.c
> +++ b/arch/arm/mach-keystone/platsmp.c
> @@ -20,6 +20,7 @@
>  #include <asm/prom.h>
>  #include <asm/tlbflush.h>
>  #include <asm/pgtable.h>
> +#include <asm/psci.h>
>  
>  #include "keystone.h"
>  
> @@ -51,7 +52,38 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
>  {}
>  #endif
>  
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +static void keystone_cpu_die(unsigned int cpu)
> +{
> +#ifdef CONFIG_ARM_PSCI
> +	struct psci_power_state pwr_state = {0, 0, 0};
> +
> +	pr_info("keystone_cpu_die(%d) from %d using PSCI\n", cpu,
> +	       smp_processor_id());
> +
> +	if (psci_ops.cpu_off)
> +		psci_ops.cpu_off(pwr_state);
> +#else
> +	/*
> +	 * We may want to add here a direct smc call to monitor
> +	 * if the kernel doesn't support PSCI API
> +	 */
> +#endif

You should determine this from your DT. Your FW/bootloader can patch in
the relevant nodes and properties when support is present, so the
presence of such nodes should guarantee that PSCI is available.

> +
> +	/*
> +	 * we shouldn't come here. But in case something went
> +	 * wrong the code below prevents kernel from crush
> +	 */
> +	while (1)
> +		cpu_do_idle();

This will make kexec appear to work, yet it won't. 

Thanks,
Mark.

> +}
> +#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
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH] keystone: psci: adds cpu_die implementation

[santosh shilimkar]

On 6/25/2015 7:45 AM, Mark Rutland wrote:
> Hi,
>
> On Thu, Jun 25, 2015 at 03:02:50PM +0100, Vitaly Andrianov wrote:
>> This commit add cpu_die implementation using psci api
>
> I don't understand. If you have a PSCI implementation, it should be
> sufficient to have a PSCI node (and enable-method) in your DT, and the
> generic code will be used. Nothing should be required in your board
> code.
>
> You should also use CPU_ON to bring secondaries online rather than
> mixing up PSCI and platform-specific mechanisms.
>
Good point about CPU_ON. We need that as well.

>>
>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>> ---
>>   arch/arm/mach-keystone/platsmp.c | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>
>> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
>> index 5f46a7c..2c40cc0 100644
>> --- a/arch/arm/mach-keystone/platsmp.c
>> +++ b/arch/arm/mach-keystone/platsmp.c
>> @@ -20,6 +20,7 @@
>>   #include <asm/prom.h>
>>   #include <asm/tlbflush.h>
>>   #include <asm/pgtable.h>
>> +#include <asm/psci.h>
>>
>>   #include "keystone.h"
>>
>> @@ -51,7 +52,38 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
>>   {}
>>   #endif
>>
>> +
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +static void keystone_cpu_die(unsigned int cpu)
>> +{
>> +#ifdef CONFIG_ARM_PSCI
>> +	struct psci_power_state pwr_state = {0, 0, 0};
>> +
>> +	pr_info("keystone_cpu_die(%d) from %d using PSCI\n", cpu,
>> +	       smp_processor_id());
>> +
>> +	if (psci_ops.cpu_off)
>> +		psci_ops.cpu_off(pwr_state);
>> +#else
>> +	/*
>> +	 * We may want to add here a direct smc call to monitor
>> +	 * if the kernel doesn't support PSCI API
>> +	 */
>> +#endif
>
> You should determine this from your DT. Your FW/bootloader can patch in
> the relevant nodes and properties when support is present, so the
> presence of such nodes should guarantee that PSCI is available.
>
That will be a nice trick. FW already does some tweaks of dts for
LPAE/non_LPAE tweaks.

Regards,
Santosh

[PATCH] keystone: psci: adds cpu_die implementation

[Vitaly Andrianov]

On 06/25/2015 10:59 AM, santosh shilimkar wrote:
> On 6/25/2015 7:45 AM, Mark Rutland wrote:
>> Hi,
>>
>> On Thu, Jun 25, 2015 at 03:02:50PM +0100, Vitaly Andrianov wrote:
>>> This commit add cpu_die implementation using psci api
>>
>> I don't understand. If you have a PSCI implementation, it should be
>> sufficient to have a PSCI node (and enable-method) in your DT, and the
>> generic code will be used. Nothing should be required in your board
>> code.
>>
>> You should also use CPU_ON to bring secondaries online rather than
>> mixing up PSCI and platform-specific mechanisms.
>>
> Good point about CPU_ON. We need that as well.
>
Does it mean that keystone_defconfig must always have CONFIG_HOTPLUG_CPU 
and CONFIG_ARM_PSCI enabled?

What is if someone doesn't want to have HOTPLUG_CPU?
How he can boot secondary CPU w/o platform-specific mechanizm?


>>>
>>> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
>>> ---
>>>   arch/arm/mach-keystone/platsmp.c | 32 ++++++++++++++++++++++++++++++++
>>>   1 file changed, 32 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-keystone/platsmp.c
>>> b/arch/arm/mach-keystone/platsmp.c
>>> index 5f46a7c..2c40cc0 100644
>>> --- a/arch/arm/mach-keystone/platsmp.c
>>> +++ b/arch/arm/mach-keystone/platsmp.c
>>> @@ -20,6 +20,7 @@
>>>   #include <asm/prom.h>
>>>   #include <asm/tlbflush.h>
>>>   #include <asm/pgtable.h>
>>> +#include <asm/psci.h>
>>>
>>>   #include "keystone.h"
>>>
>>> @@ -51,7 +52,38 @@ static inline void __cpuinit
>>> keystone_smp_secondary_initmem(unsigned int cpu)
>>>   {}
>>>   #endif
>>>
>>> +
>>> +#ifdef CONFIG_HOTPLUG_CPU
>>> +static void keystone_cpu_die(unsigned int cpu)
>>> +{
>>> +#ifdef CONFIG_ARM_PSCI
>>> +    struct psci_power_state pwr_state = {0, 0, 0};
>>> +
>>> +    pr_info("keystone_cpu_die(%d) from %d using PSCI\n", cpu,
>>> +           smp_processor_id());
>>> +
>>> +    if (psci_ops.cpu_off)
>>> +        psci_ops.cpu_off(pwr_state);
>>> +#else
>>> +    /*
>>> +     * We may want to add here a direct smc call to monitor
>>> +     * if the kernel doesn't support PSCI API
>>> +     */
>>> +#endif
>>
>> You should determine this from your DT. Your FW/bootloader can patch in
>> the relevant nodes and properties when support is present, so the
>> presence of such nodes should guarantee that PSCI is available.
>>
> That will be a nice trick. FW already does some tweaks of dts for
> LPAE/non_LPAE tweaks.
>
> Regards,
> Santosh

Thanks,
-Vitaly

[PATCH] keystone: psci: adds cpu_die implementation

[Mark Rutland]

On Thu, Jun 25, 2015 at 05:01:36PM +0100, Vitaly Andrianov wrote:
> 
> 
> On 06/25/2015 10:59 AM, santosh shilimkar wrote:
> > On 6/25/2015 7:45 AM, Mark Rutland wrote:
> >> Hi,
> >>
> >> On Thu, Jun 25, 2015 at 03:02:50PM +0100, Vitaly Andrianov wrote:
> >>> This commit add cpu_die implementation using psci api
> >>
> >> I don't understand. If you have a PSCI implementation, it should be
> >> sufficient to have a PSCI node (and enable-method) in your DT, and the
> >> generic code will be used. Nothing should be required in your board
> >> code.
> >>
> >> You should also use CPU_ON to bring secondaries online rather than
> >> mixing up PSCI and platform-specific mechanisms.
> >>
> > Good point about CPU_ON. We need that as well.
> >
> Does it mean that keystone_defconfig must always have CONFIG_HOTPLUG_CPU 
> and CONFIG_ARM_PSCI enabled?

No.

You need CONFIG_ARM_PSCI for PSCI CPU_ON. Without CONFIG_HOTPLUG_CPU,
you'll get PSCI CPU_ON but not PSCI CPU_OFF.

> What is if someone doesn't want to have HOTPLUG_CPU?

Then you don't get PSCI CPU_OFF, but can still have PSCI CPU_ON.

Only the portions of the PSCI code required for turning CPUs off are
dependent on HOTPLUG_CPU.

> How he can boot secondary CPU w/o platform-specific mechanizm?

By using PSCI CPU_ON.

Thanks,
Mark

[PATCH] keystone: psci: adds cpu_die implementation

[Vitaly Andrianov]

On 06/25/2015 12:13 PM, Mark Rutland wrote:
> On Thu, Jun 25, 2015 at 05:01:36PM +0100, Vitaly Andrianov wrote:
>>
>>
>> On 06/25/2015 10:59 AM, santosh shilimkar wrote:
>>> On 6/25/2015 7:45 AM, Mark Rutland wrote:
>>>> Hi,
>>>>
>>>> On Thu, Jun 25, 2015 at 03:02:50PM +0100, Vitaly Andrianov wrote:
>>>>> This commit add cpu_die implementation using psci api
>>>>
>>>> I don't understand. If you have a PSCI implementation, it should be
>>>> sufficient to have a PSCI node (and enable-method) in your DT, and the
>>>> generic code will be used. Nothing should be required in your board
>>>> code.
>>>>
>>>> You should also use CPU_ON to bring secondaries online rather than
>>>> mixing up PSCI and platform-specific mechanisms.
>>>>
>>> Good point about CPU_ON. We need that as well.
>>>
>> Does it mean that keystone_defconfig must always have CONFIG_HOTPLUG_CPU
>> and CONFIG_ARM_PSCI enabled?
>
> No.
>
> You need CONFIG_ARM_PSCI for PSCI CPU_ON. Without CONFIG_HOTPLUG_CPU,
> you'll get PSCI CPU_ON but not PSCI CPU_OFF.
>
>> What is if someone doesn't want to have HOTPLUG_CPU?
>
> Then you don't get PSCI CPU_OFF, but can still have PSCI CPU_ON.
>
> Only the portions of the PSCI code required for turning CPUs off are
> dependent on HOTPLUG_CPU.
>
>> How he can boot secondary CPU w/o platform-specific mechanizm?
>
> By using PSCI CPU_ON.
>
> Thanks,
> Mark
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
Thanks Mark,

I need rework and re-test the patch.
One more question. Shall I post the dts related commit, which add PSCI 
command together with this commit? Or it may be posted later independently?

Thanks,
Vitaly

[PATCH] keystone: psci: adds cpu_die implementation

[Mark Rutland]

On Thu, Jun 25, 2015 at 05:55:40PM +0100, Vitaly Andrianov wrote:
> 
> 
> On 06/25/2015 12:13 PM, Mark Rutland wrote:
> > On Thu, Jun 25, 2015 at 05:01:36PM +0100, Vitaly Andrianov wrote:
> >>
> >>
> >> On 06/25/2015 10:59 AM, santosh shilimkar wrote:
> >>> On 6/25/2015 7:45 AM, Mark Rutland wrote:
> >>>> Hi,
> >>>>
> >>>> On Thu, Jun 25, 2015 at 03:02:50PM +0100, Vitaly Andrianov wrote:
> >>>>> This commit add cpu_die implementation using psci api
> >>>>
> >>>> I don't understand. If you have a PSCI implementation, it should be
> >>>> sufficient to have a PSCI node (and enable-method) in your DT, and the
> >>>> generic code will be used. Nothing should be required in your board
> >>>> code.
> >>>>
> >>>> You should also use CPU_ON to bring secondaries online rather than
> >>>> mixing up PSCI and platform-specific mechanisms.
> >>>>
> >>> Good point about CPU_ON. We need that as well.
> >>>
> >> Does it mean that keystone_defconfig must always have CONFIG_HOTPLUG_CPU
> >> and CONFIG_ARM_PSCI enabled?
> >
> > No.
> >
> > You need CONFIG_ARM_PSCI for PSCI CPU_ON. Without CONFIG_HOTPLUG_CPU,
> > you'll get PSCI CPU_ON but not PSCI CPU_OFF.
> >
> >> What is if someone doesn't want to have HOTPLUG_CPU?
> >
> > Then you don't get PSCI CPU_OFF, but can still have PSCI CPU_ON.
> >
> > Only the portions of the PSCI code required for turning CPUs off are
> > dependent on HOTPLUG_CPU.
> >
> >> How he can boot secondary CPU w/o platform-specific mechanizm?
> >
> > By using PSCI CPU_ON.
> >
> > Thanks,
> > Mark
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> Thanks Mark,
> 
> I need rework and re-test the patch.
> One more question. Shall I post the dts related commit, which add PSCI 
> command together with this commit? Or it may be posted later independently?

The DTS and Kconfig changes can be seaprate patches, but they'll need to
go through at the same time.

You shouldn't need to add any platform code, but you may need to add a
some guards to support existing (non-PSCI) systems and PSCI systems with
the same kernel if your board code unconditionally pokes hardware that
the PSCI implementation will be in charge of (with PSCI the kernel
should no nothing about said hardware).

Thanks,
Mark.

[PATCH] keystone: psci: adds cpu_die implementation

[Mark Rutland]

> > I need rework and re-test the patch.
> > One more question. Shall I post the dts related commit, which add PSCI 
> > command together with this commit? Or it may be posted later independently?
> 
> The DTS and Kconfig changes can be seaprate patches, but they'll need to
> go through at the same time.

If your bootloader patches the DTB then you don't even need a dts
update. That should make things less confusing for existing users...

Thanks,
Mark.

[PATCH] keystone: psci: adds cpu_die implementation

[santosh shilimkar]

On 6/25/2015 10:20 AM, Mark Rutland wrote:
>>> I need rework and re-test the patch.
>>> One more question. Shall I post the dts related commit, which add PSCI
>>> command together with this commit? Or it may be posted later independently?
>>
>> The DTS and Kconfig changes can be seaprate patches, but they'll need to
>> go through at the same time.
>
> If your bootloader patches the DTB then you don't even need a dts
> update. That should make things less confusing for existing users...
>
More than confusing we need to keep existing DTB binding work with
updated kernel at least for as basic as booting all the CPUs.

Regards,
Santosh

[PATCH] keystone: psci: adds cpu_die implementation

[Vitaly Andrianov]

On 06/25/2015 02:42 PM, santosh shilimkar wrote:
> On 6/25/2015 10:20 AM, Mark Rutland wrote:
>>>> I need rework and re-test the patch.
>>>> One more question. Shall I post the dts related commit, which add PSCI
>>>> command together with this commit? Or it may be posted later
>>>> independently?
>>>
>>> The DTS and Kconfig changes can be seaprate patches, but they'll need to
>>> go through at the same time.
>>
>> If your bootloader patches the DTB then you don't even need a dts
>> update. That should make things less confusing for existing users...
>>
> More than confusing we need to keep existing DTB binding work with
> updated kernel at least for as basic as booting all the CPUs.
>
> Regards,
> Santosh
>
>
OK. Now I'm confused :) We may have several different configurations here:

1) CONFIG_HOTPLUG_CPU and CONFIG_ARM_PSCI are not set.
    In this case keystone arch needs to have
    keystone_smp_boot_secondary();

2) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI is not set.
    keystone_smp_boot_secondary() is required and non PSCI
    implementation of keystone_cpu_die() is also required.

3) CONFIG_HOTPLUG_CPU is not set and CONFIG_ARM_PSCI=y
4) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI=y

    How do I boot secondary CPUs in cases of 3 and 4?
    Do I need to implement PSCI version of the
    keystone_smp_boot_secondary() of adding PSCI commands to DTB is
    enough?

    Do I need to implement keystone_cpu_die() if PSCI commands are
    added to DTB?

Thanks,
Vitaly 	

[PATCH] keystone: psci: adds cpu_die implementation

[Mark Rutland]

> OK. Now I'm confused :) We may have several different configurations here:
> 
> 1) CONFIG_HOTPLUG_CPU and CONFIG_ARM_PSCI are not set.
>     In this case keystone arch needs to have
>     keystone_smp_boot_secondary();
> 
> 2) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI is not set.
>     keystone_smp_boot_secondary() is required and non PSCI
>     implementation of keystone_cpu_die() is also required.
> 
> 3) CONFIG_HOTPLUG_CPU is not set and CONFIG_ARM_PSCI=y
> 4) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI=y
> 
>     How do I boot secondary CPUs in cases of 3 and 4?
>     Do I need to implement PSCI version of the
>     keystone_smp_boot_secondary() of adding PSCI commands to DTB is
>     enough?

The DTB additions alone _should_ be sufficient.

>     Do I need to implement keystone_cpu_die() if PSCI commands are
>     added to DTB?

You _should not_ need to add any platform code to use PSCI.

However, if you have existing platform code which pokes the hardware
logically owned by your FW PSCI implementation, you need to ensure that
this code doesn't poke that hardware when PSCI is in use.

Thanks,
Mark.

[PATCH] keystone: psci: adds cpu_die implementation

[Grygorii Strashko]

Hi,

On 06/26/2015 07:57 PM, Vitaly Andrianov wrote:
> On 06/25/2015 02:42 PM, santosh shilimkar wrote:
>> On 6/25/2015 10:20 AM, Mark Rutland wrote:
>>>>> I need rework and re-test the patch.
>>>>> One more question. Shall I post the dts related commit, which add PSCI
>>>>> command together with this commit? Or it may be posted later
>>>>> independently?
>>>>
>>>> The DTS and Kconfig changes can be seaprate patches, but they'll 
>>>> need to
>>>> go through at the same time.
>>>
>>> If your bootloader patches the DTB then you don't even need a dts
>>> update. That should make things less confusing for existing users...
>>>
>> More than confusing we need to keep existing DTB binding work with
>> updated kernel at least for as basic as booting all the CPUs.
>>
>> Regards,
>> Santosh
>>
>>
> OK. Now I'm confused :) We may have several different configurations here:
> 
> 1) CONFIG_HOTPLUG_CPU and CONFIG_ARM_PSCI are not set.
>     In this case keystone arch needs to have
>     keystone_smp_boot_secondary();
> 
> 2) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI is not set.
>     keystone_smp_boot_secondary() is required and non PSCI
>     implementation of keystone_cpu_die() is also required.
> 
> 3) CONFIG_HOTPLUG_CPU is not set and CONFIG_ARM_PSCI=y
> 4) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI=y
> 
>     How do I boot secondary CPUs in cases of 3 and 4?
>     Do I need to implement PSCI version of the
>     keystone_smp_boot_secondary() of adding PSCI commands to DTB is
>     enough?
> 
>     Do I need to implement keystone_cpu_die() if PSCI commands are
>     added to DTB?

Things are more or less simple here :)
1) to support psci you need to have DT entry like below:
	psci {
		compatible	= "arm,psci";
		method		= "smc";
		cpu_off		= <0x84000002>;
		cpu_on		= <0x84000003>;
	};
and CONFIG_ARM_PSCI=y

in this case Kernel will ignore mach.smp = smp_ops(keystone_smp_ops),
and will use PSCU interface (see setup_arch()0

2) if don't have PSCI DT entry, but still have custom smp_operations - 
they will be used.

So, question here is not about   CONFIG_HOTPLUG_CPU, but rather 
will you support "PSCI only" or "PSCI and legacy boot".

For the last case You should keep mach specific code in mach-kestone/platsmp.c.
For the first case "PSCI only" - above code can be removed.

3) if you'd like CONFIG_HOTPLUG_CPU in legacy mode - platsmp.c can be updated as below,
without using psci:


+#define KEYSTONE_MON_CPU_DOWN_IDX              0x01
+#ifdef CONFIG_HOTPLUG_CPU
+static void __ref keystone_smp_cpu_die(unsigned int cpu)
+{
+       int error;
+
+       error = keystone_cpu_smc(KEYSTONE_MON_CPU_DOWN_IDX, cpu, 0);
+       if (error)
+               pr_err("CPU %u->%u down failed with %d\n",
+                      smp_processor_id(), cpu, error);
+
+       cpu_do_idle();
+}
+#endif

Another question is how well current PSCI implementation supports keystone2/LPAE !?

- It seems, at least below hack should be applied :(

+++ b/arch/arm/kernel/psci_smp.c
@@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
        if (psci_ops.cpu_on)
                return psci_ops.cpu_on(cpu_logical_map(cpu),
-                                      __pa(secondary_startup));
+                                       virt_to_idmap(&secondary_startup));
        return -ENODEV;
 }

- and what to do with code in keystone_smp_secondary_initmem() ?:

static void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
{
	pgd_t *pgd0 = pgd_offset_k(0);
	cpu_set_ttbr(1, __pa(pgd0) + TTBR1_OFFSET);
	local_flush_tlb_all();
}


-- 
regards,
-grygorii

[PATCH] keystone: psci: adds cpu_die implementation

[Vitaly Andrianov]

On 06/26/2015 01:47 PM, Grygorii Strashko wrote:
> Hi,
>
> On 06/26/2015 07:57 PM, Vitaly Andrianov wrote:
>> On 06/25/2015 02:42 PM, santosh shilimkar wrote:
>>> On 6/25/2015 10:20 AM, Mark Rutland wrote:
>>>>>> I need rework and re-test the patch.
>>>>>> One more question. Shall I post the dts related commit, which add PSCI
>>>>>> command together with this commit? Or it may be posted later
>>>>>> independently?
>>>>>
>>>>> The DTS and Kconfig changes can be seaprate patches, but they'll
>>>>> need to
>>>>> go through at the same time.
>>>>
>>>> If your bootloader patches the DTB then you don't even need a dts
>>>> update. That should make things less confusing for existing users...
>>>>
>>> More than confusing we need to keep existing DTB binding work with
>>> updated kernel at least for as basic as booting all the CPUs.
>>>
>>> Regards,
>>> Santosh
>>>
>>>
>> OK. Now I'm confused :) We may have several different configurations here:
>>
>> 1) CONFIG_HOTPLUG_CPU and CONFIG_ARM_PSCI are not set.
>>      In this case keystone arch needs to have
>>      keystone_smp_boot_secondary();
>>
>> 2) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI is not set.
>>      keystone_smp_boot_secondary() is required and non PSCI
>>      implementation of keystone_cpu_die() is also required.
>>
>> 3) CONFIG_HOTPLUG_CPU is not set and CONFIG_ARM_PSCI=y
>> 4) CONFIG_HOTPLUG_CPU=y and CONFIG_ARM_PSCI=y
>>
>>      How do I boot secondary CPUs in cases of 3 and 4?
>>      Do I need to implement PSCI version of the
>>      keystone_smp_boot_secondary() of adding PSCI commands to DTB is
>>      enough?
>>
>>      Do I need to implement keystone_cpu_die() if PSCI commands are
>>      added to DTB?
>
> Things are more or less simple here :)
> 1) to support psci you need to have DT entry like below:
> 	psci {
> 		compatible	= "arm,psci";
> 		method		= "smc";
> 		cpu_off		= <0x84000002>;
> 		cpu_on		= <0x84000003>;
> 	};
> and CONFIG_ARM_PSCI=y
>
> in this case Kernel will ignore mach.smp = smp_ops(keystone_smp_ops),
> and will use PSCU interface (see setup_arch()0
>
> 2) if don't have PSCI DT entry, but still have custom smp_operations -
> they will be used.
>
> So, question here is not about   CONFIG_HOTPLUG_CPU, but rather
> will you support "PSCI only" or "PSCI and legacy boot".
>
> For the last case You should keep mach specific code in mach-kestone/platsmp.c.
> For the first case "PSCI only" - above code can be removed.
>
> 3) if you'd like CONFIG_HOTPLUG_CPU in legacy mode - platsmp.c can be updated as below,
> without using psci:
>
>
> +#define KEYSTONE_MON_CPU_DOWN_IDX              0x01
> +#ifdef CONFIG_HOTPLUG_CPU
> +static void __ref keystone_smp_cpu_die(unsigned int cpu)
> +{
> +       int error;
> +
> +       error = keystone_cpu_smc(KEYSTONE_MON_CPU_DOWN_IDX, cpu, 0);
> +       if (error)
> +               pr_err("CPU %u->%u down failed with %d\n",
> +                      smp_processor_id(), cpu, error);
> +
> +       cpu_do_idle();
> +}
> +#endif
>
> Another question is how well current PSCI implementation supports keystone2/LPAE !?
>
That is exactly what I'm working on now :)

> - It seems, at least below hack should be applied :(
>
> +++ b/arch/arm/kernel/psci_smp.c
> @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>          if (psci_ops.cpu_on)
>                  return psci_ops.cpu_on(cpu_logical_map(cpu),
> -                                      __pa(secondary_startup));
> +                                       virt_to_idmap(&secondary_startup));
>          return -ENODEV;
>   }
>
> - and what to do with code in keystone_smp_secondary_initmem() ?:
>
> static void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
> {
> 	pgd_t *pgd0 = pgd_offset_k(0);
> 	cpu_set_ttbr(1, __pa(pgd0) + TTBR1_OFFSET);
> 	local_flush_tlb_all();
> }
>
>

Thanks,
Vitaly

[PATCH] keystone: psci: adds cpu_die implementation

[santosh shilimkar]

On 6/26/2015 10:47 AM, Grygorii Strashko wrote:
> Hi,
>
> On 06/26/2015 07:57 PM, Vitaly Andrianov wrote:
>> On 06/25/2015 02:42 PM, santosh shilimkar wrote:


> Another question is how well current PSCI implementation supports keystone2/LPAE !?
>
> - It seems, at least below hack should be applied :(
>
> +++ b/arch/arm/kernel/psci_smp.c
> @@ -51,7 +51,7 @@ static int psci_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>          if (psci_ops.cpu_on)
>                  return psci_ops.cpu_on(cpu_logical_map(cpu),
> -                                      __pa(secondary_startup));
> +                                       virt_to_idmap(&secondary_startup));

This is legitimate change should be there irrespectively.
I think you should post this as a proper patch.

Regards,
Santosh

[PATCH] keystone: psci: adds cpu_die implementation

[santosh shilimkar]

On 6/25/2015 7:02 AM, Vitaly Andrianov wrote:
> This commit add cpu_die implementation using psci api
>
> Signed-off-by: Vitaly Andrianov <vitalya@ti.com>
> ---
>   arch/arm/mach-keystone/platsmp.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm/mach-keystone/platsmp.c b/arch/arm/mach-keystone/platsmp.c
> index 5f46a7c..2c40cc0 100644
> --- a/arch/arm/mach-keystone/platsmp.c
> +++ b/arch/arm/mach-keystone/platsmp.c
> @@ -20,6 +20,7 @@
>   #include <asm/prom.h>
>   #include <asm/tlbflush.h>
>   #include <asm/pgtable.h>
> +#include <asm/psci.h>
>
>   #include "keystone.h"
>
> @@ -51,7 +52,38 @@ static inline void __cpuinit keystone_smp_secondary_initmem(unsigned int cpu)
>   {}
>   #endif
>
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +static void keystone_cpu_die(unsigned int cpu)
> +{
> +#ifdef CONFIG_ARM_PSCI
> +	struct psci_power_state pwr_state = {0, 0, 0};
> +
> +	pr_info("keystone_cpu_die(%d) from %d using PSCI\n", cpu,
> +	       smp_processor_id());
> +
> +	if (psci_ops.cpu_off)
> +		psci_ops.cpu_off(pwr_state);
> +#else
> +	/*
> +	 * We may want to add here a direct smc call to monitor
> +	 * if the kernel doesn't support PSCI API
> +	 */
> +#endif
I don't see much value adding the SMC. I mean CPU_HOTPLUG works with
PSCI o.w doesn't is good enough. If you still like to add it, just 
abstract above into two functions, one with PSCI and other with
SMC. That way you can avoid that ugly #defines in the middle of the
code.

Regards,
Santosh