From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomasz Figa Subject: Re: [PATCH 1/7] ARM: EXYNOS: initial board support for exynos5260 SoC Date: Tue, 10 Dec 2013 16:57:23 +0100 Message-ID: <2452273.S9E2sU17uX@amdc1227> References: <1386345391-23482-1-git-send-email-rahul.sharma@samsung.com> <1386345391-23482-2-git-send-email-rahul.sharma@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7Bit Return-path: In-reply-to: <1386345391-23482-2-git-send-email-rahul.sharma@samsung.com> Sender: linux-samsung-soc-owner@vger.kernel.org To: Rahul Sharma 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 List-Id: devicetree@vger.kernel.org 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 > > This patch add basic arch side support for exynos5260 SoC. > > Signed-off-by: Pankaj Dubey > Signed-off-by: Arun Kumar K > --- > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: t.figa@samsung.com (Tomasz Figa) Date: Tue, 10 Dec 2013 16:57:23 +0100 Subject: [PATCH 1/7] ARM: EXYNOS: initial board support for exynos5260 SoC In-Reply-To: <1386345391-23482-2-git-send-email-rahul.sharma@samsung.com> References: <1386345391-23482-1-git-send-email-rahul.sharma@samsung.com> <1386345391-23482-2-git-send-email-rahul.sharma@samsung.com> Message-ID: <2452273.S9E2sU17uX@amdc1227> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 > > This patch add basic arch side support for exynos5260 SoC. > > Signed-off-by: Pankaj Dubey > Signed-off-by: Arun Kumar K > --- > 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