Re: [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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/