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.
Best regards,
Tomasz
--
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
