On 11/12/2014 04:13 PM, Bartlomiej Zolnierkiewicz wrote:
Hi Bartlomiej,
[ cut ]
- using arch_send_wakeup_ipi_mask() instead of dsb_sev() (this matches CPU hotplug code in arch/arm/mach-exynos/platsmp.c)
I am curious. You experienced very rare hangs after running the tests a few hours, right ? Is the SEV replaced by the IPI solving the issue ? If yes, how did you catch it ?
Rare hangs showed up after about 30-40 minutes of testing with the attached app and script (running of "./cpuidle_state1_test.sh script 2 500" has never completed on the umodified driver).
The problem turned out to be in the following loop waiting for CPU1 to get stuck in the BOOT ROM:
/* * Wait for cpu1 to get stuck in the boot rom */ while ((__raw_readl(BOOT_VECTOR) != 0) && !atomic_read(&cpu1_wakeup)) cpu_relax();[ Removal of the loop fixed the problem. ]
Just for my personal information, do you know why ?
Using the SEV instead of the IPI was not a issue but it was changed to match the existing Exynos platform code (exynos_boot_secondary() in arch/arm/mach-exynos/platsmp.c) and as preparation for Exynos4412 (quad core) support.
Ah, ok. Thanks for the info.
[ cut ]
+#ifdef CONFIG_ARM_EXYNOS_CPUIDLE
- if (of_machine_is_compatible("samsung,exynos4210"))
exynos_cpuidle.dev.platform_data = &cpuidle_coupled_exynos_data;+#endif
You should not add those #ifdef.
Without those #ifdef I get:
LD init/built-in.o arch/arm/mach-exynos/built-in.o: In function `exynos_dt_machine_init': /home/bzolnier/sam/linux-sprc/arch/arm/mach-exynos/exynos.c:334: undefined reference to `cpuidle_coupled_exynos_data' make: *** [vmlinux] Error 1
when CONFIG_EXYNOS_CPU_SUSPEND is disabled.
Here, we are introducing some dependencies I tried to drop in the different drivers.
I looked more closely at the code and especially the 'cpuidle_coupled_exynos_data'. I don't think it is worth to have it because it adds more complexity and you have to define this structure to be visible from the drivers/cpuidle files.
I suggest you create an simple function in "pm.c"
int exynos_coupled_aftr(int cpu) { pre_enter...
if (!cpu) cpu0_enter_aftr() else cpu1_powerdown()
post_enter... }
and in the cpuidle driver itself, you just use the already existing anonymous callback 'exynos_enter_aftr' (and mutate it to conform the parameters).
You won't have to share any structure between the arch code and the cpuidle driver.
if (of_machine_is_compatible("samsung,exynos4210") || of_machine_is_compatible("samsung,exynos4212") || (of_machine_is_compatible("samsung,exynos4412") &&diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
[ cut ]
- exynos_enter_aftr = (void *)(pdev->dev.platform_data);
- if (of_machine_is_compatible("samsung,exynos4210")) {
exynos_cpuidle_pdata = pdev->dev.platform_data;exynos_idle_driver.states[1].enter =exynos_enter_coupled_lowpower;exynos_idle_driver.states[1].exit_latency = 5000;exynos_idle_driver.states[1].target_residency = 10000;exynos_idle_driver.states[1].flags |= CPUIDLE_FLAG_COUPLED |CPUIDLE_FLAG_TIMER_STOP;I tried to remove those dynamic state allocation everywhere in the different drivers. I would prefer to have another cpuidle_driver to be registered with its states instead of overwriting the existing idle state.
struct cpuidle_driver exynos4210_idle_driver = { .name = "exynos4210_idle", .owner = THIS_MODULE, .states = { [0] = ARM_CPUIDLE_WFI_STATE, [1] = { .enter = exynos_enter_coupled_lowpower, .exit_latency = 5000, .target_residency = 10000, .flags = CPUIDLE_FLAG_TIME_VALID | CPUIDLE_FLAG_COUPLED | CPUIDLE_FLAG_TIMER_STOP, .name = "C1", .desc = "ARM power down", }, } };
and then:
if (of_machine_is_compatible("samsung,exynos4210")) { ... ret = cpuidle_register(&exynos4210_idle_driver, cpu_online_mask); ... } ...
OK, I will fix it but (if you are OK with it) I will make the code use "exynos_coupled" naming instead of "exynos4210" one to not have to change it later.
If we can reuse this mechanism, which I believe it is possible to, for 4420 and 5250. Then we will be able to refactor this out again.
Ok, sounds good.
I plan to add support for Exynos3250 next as it should be the simplest (it is also dual core) and I need it for other reasons anyway. Exynos4412 (quad core) support requires more work but should also be doable.
When it comes to Exynos5250 I was thinking about disabling normal AFTR mode support for it as according to my testing (on Arndale board) it has never worked (at least in upstream kernels, I don't know about Linaro or internal ones).
The AFTR state worked on my 5250 very well. It is a Arndale board.
Thanks for resurrecting the patch and providing the multi core idle support. I am too busy to refocus on that right now.
-- Daniel