Hi Tomasz,
On Tue, Jul 15, 2014 at 11:11 PM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
> Hi Chander,
>
> Please see my comments inline.
>
> On 01.07.2014 16:32, Chander Kashyap wrote:
>> Pre/post platform specific cpuidle operations are handled by pm_notifier.
>> But these operations are not same for all cpuidle states. Handle this by
>> moving cpuidle specific code from pm_notifier to cpuidle specific function.
>>
>> Signed-off-by: Chander Kashyap <k.chander@xxxxxxxxxxx>
>> ---
>> arch/arm/mach-exynos/common.h | 2 +-
>> arch/arm/mach-exynos/pm.c | 45 ++++++++++----------------------------
>> drivers/cpuidle/cpuidle-exynos.c | 7 ++++--
>> 3 files changed, 17 insertions(+), 37 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
>> index 1ee9176..7769f58 100644
>> --- a/arch/arm/mach-exynos/common.h
>> +++ b/arch/arm/mach-exynos/common.h
>> @@ -166,7 +166,7 @@ extern int exynos_cpu_power_state(int cpu);
>> extern void exynos_cluster_power_down(int cluster);
>> extern void exynos_cluster_power_up(int cluster);
>> extern int exynos_cluster_power_state(int cluster);
>> -extern void exynos_enter_aftr(void);
>> +extern void exynos_enter_aftr(int entering_idle);
>>
>> extern void s5p_init_cpu(void __iomem *cpuid_addr);
>> extern unsigned int samsung_rev(void);
>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> index a092cc3..328644f 100644
>> --- a/arch/arm/mach-exynos/pm.c
>> +++ b/arch/arm/mach-exynos/pm.c
>> @@ -188,14 +188,6 @@ static void exynos_cpu_set_boot_vector(long flags)
>> __raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG);
>> }
>>
>> -void exynos_enter_aftr(void)
>> -{
>> - exynos_set_wakeupmask(0x0000ff3e);
>> - exynos_cpu_set_boot_vector(S5P_CHECK_AFTR);
>> - /* Set value of power down register for aftr mode */
>> - exynos_sys_powerdown_conf(SYS_AFTR);
>> -}
>> -
>> static int exynos_cpu_suspend(unsigned long arg)
>> {
>> #ifdef CONFIG_CACHE_L2X0
>> @@ -386,40 +378,25 @@ static const struct platform_suspend_ops exynos_suspend_ops = {
>> .valid = suspend_valid_only_mem,
>> };
>>
>> -static int exynos_cpu_pm_notifier(struct notifier_block *self,
>> - unsigned long cmd, void *v)
>> +void exynos_enter_aftr(int entering_idle)
>> {
>> - int cpu = smp_processor_id();
>> -
>> - switch (cmd) {
>> - case CPU_PM_ENTER:
>> - if (cpu == 0)
>> - exynos_pm_central_suspend();
>> - break;
>> -
>> - case CPU_PM_EXIT:
>> - if (cpu == 0) {
>> - if (read_cpuid_part_number() ==
>> - ARM_CPU_PART_CORTEX_A9)
>> - scu_enable(S5P_VA_SCU);
>> - exynos_pm_central_resume();
>> - }
>> - break;
>> + if (entering_idle) {
>> + exynos_set_wakeupmask(0x0000ff3e);
>> + exynos_cpu_set_boot_vector(S5P_CHECK_AFTR);
>> + /* Set value of power down register for aftr mode */
>> + exynos_sys_powerdown_conf(SYS_AFTR);
>> + exynos_pm_central_suspend();
>> + } else {
>> + if (scu_a9_has_base())
>> + scu_enable(S5P_VA_SCU);
>> + exynos_pm_central_resume();
>
> Hmm. This is not very readable. Basically you have two functions that do
> completely different things packed into one function. I would suggest
> moving the calls to cpu_pm_enter/exit() and everything in between to
> this function then you wouldn't need anything like this and the whole
> low level logic would be in one place.
>
>> }
>> -
>> - return NOTIFY_OK;
>> }
>>
>> -static struct notifier_block exynos_cpu_pm_notifier_block = {
>> - .notifier_call = exynos_cpu_pm_notifier,
>> -};
>> -
>> void __init exynos_pm_init(void)
>> {
>> u32 tmp;
>>
>> - cpu_pm_register_notifier(&exynos_cpu_pm_notifier_block);
>> -
>> /* Platform-specific GIC callback */
>> gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
>>
>> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
>> index 7c01512..1196ca7 100644
>> --- a/drivers/cpuidle/cpuidle-exynos.c
>> +++ b/drivers/cpuidle/cpuidle-exynos.c
>> @@ -18,11 +18,10 @@
>> #include <asm/suspend.h>
>> #include <asm/cpuidle.h>
>>
>> -static void (*exynos_enter_aftr)(void);
>> +static void (*exynos_enter_aftr)(int);
>>
>> static int idle_finisher(unsigned long flags)
>> {
>> - exynos_enter_aftr();
>> cpu_do_idle();
>>
>> return 1;
>> @@ -32,8 +31,12 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
>> struct cpuidle_driver *drv,
>> int index)
>> {
>> + int entering_idle = true;
>> cpu_pm_enter();
>> + exynos_enter_aftr(entering_idle);
>> cpu_suspend(0, idle_finisher);
>> + entering_idle = false;
>> + exynos_enter_aftr(entering_idle);
>
> This doesn't look good. Couldn't you just have called it with constant
> arguments? E.g.
>
> exynos_enter_aftr(true);
> [...]
> exynos_enter_aftr(false);
>
> Well, sorry for late comments, I have missed this series, probably
> because I'm not on Cc list. Anyway, since this patch will need to be
> respun anyway, maybe it would be better to use the one I just posted
> today, which IMHO is a bit cleaner.
I am fine with this. In that case my patches can be ignored. Also take
the cleanup patch with yours series.
>
> Best regards,
> Tomasz
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
