Re: [PATCH 1/7] ARM: EXYNOS: initial board support for exynos5260 SoC

From: Tomasz Figa <t.figa@samsung.com>
To: Rahul Sharma <rahul.sharma@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, mturquette@linaro.org,
	kgene.kim@samsung.com, thomas.ab@samsung.com,
	tomasz.figa@gmail.com, joshi@samsung.com,
	pankaj.dubey@samsung.com, yg1004.jang@samsung.com,
	arun.kk@samsung.com, r.sh.open@gmail.com
Subject: Re: [PATCH 1/7] ARM: EXYNOS: initial board support for exynos5260 SoC
Date: Tue, 10 Dec 2013 16:57:23 +0100	[thread overview]
Message-ID: <2452273.S9E2sU17uX@amdc1227> (raw)
In-Reply-To: <1386345391-23482-2-git-send-email-rahul.sharma@samsung.com>

Hi Pankaj, Rahul, Arun,

In addition to issues already pointed by Sachin, please also see my
comments inline.

On Friday 06 of December 2013 21:26:25 Rahul Sharma wrote:
> From: Pankaj Dubey <pankaj.dubey@samsung.com>
> 
> This patch add basic arch side support for exynos5260 SoC.
> 
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>  arch/arm/mach-exynos/Kconfig                 |    9 +++++++++
>  arch/arm/mach-exynos/common.c                |   19 ++++++++++++++++++-
>  arch/arm/mach-exynos/cpuidle.c               |    2 +-
>  arch/arm/mach-exynos/include/mach/map.h      |    1 +
>  arch/arm/mach-exynos/include/mach/regs-pmu.h |    4 ++++
>  arch/arm/mach-exynos/mach-exynos5-dt.c       |    1 +
>  arch/arm/plat-samsung/include/plat/cpu.h     |    8 ++++++++
>  arch/arm/plat-samsung/include/plat/map-s5p.h |    1 +
>  8 files changed, 43 insertions(+), 2 deletions(-)
[snip]
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index ddbfe87..405c11a 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -120,7 +120,7 @@ static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>  	cpu_suspend(0, idle_finisher);
>  
>  #ifdef CONFIG_SMP
> -	if (!soc_is_exynos5250())
> +	if (!soc_is_exynos5250() || soc_is_exynos5260())

The added OR condition doesn't affect the if condition in any way, because
when running on Exynos5260, soc_is_exynos5250() will return false and make
the whole condition evaluate to true.

Shouldn't it be

	if (!soc_is_exynos5250() && !soc_is_exynos5260())

if Exynos5260 doesn't need scu_enable(), or left as is if it needs?

>  		scu_enable(S5P_VA_SCU);
>  #endif
>  	cpu_pm_exit();
> diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h
> index 7b046b5..bd6fa02 100644
> --- a/arch/arm/mach-exynos/include/mach/map.h
> +++ b/arch/arm/mach-exynos/include/mach/map.h
> @@ -29,6 +29,7 @@
>  #define EXYNOS4210_PA_SYSRAM_NS		0x0203F000
>  #define EXYNOS4x12_PA_SYSRAM_NS		0x0204F000
>  #define EXYNOS5250_PA_SYSRAM_NS		0x0204F000
> +#define EXYNOS5260_PA_SYSRAM_NS		0x02073000
>  
>  #define EXYNOS_PA_CHIPID		0x10000000
>  
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> index 2cdb63e..09ae29a 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -234,6 +234,10 @@
>  
>  #define EXYNOS5_SYS_WDTRESET					(1 << 20)
>  
> +#define EXYNOS5260_A7_WDTRST					(1 << 24)
> +#define EXYNOS5260_A15_WDTRST					(1 << 23)
> +#define EXYNOS5260_SYS_WDTRESET	(EXYNOS5260_A7_WDTRST || EXYNOS5260_A15_WDTRST)
> +

Are these definitions needed? I don't see any user in this patch.

Best regards,
Tomasz