| Message ID | 1478230764-13748-4-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pankaj, On 11/04/2016 09:09 AM, Pankaj Dubey wrote: > Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC based boards. > Instead use mapping from device tree node of SCU. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/mach-exynos/exynos.c | 22 ---------------------- > arch/arm/mach-exynos/include/mach/map.h | 2 -- > arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- > arch/arm/mach-exynos/pm.c | 14 +++++++++++--- > arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- > arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- > 6 files changed, 33 insertions(+), 42 deletions(-) > > diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c > index 757fc11..fa08ef9 100644 > --- a/arch/arm/mach-exynos/exynos.c > +++ b/arch/arm/mach-exynos/exynos.c > @@ -28,15 +28,6 @@ > > #include "common.h" > > -static struct map_desc exynos4_iodesc[] __initdata = { > - { > - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, > - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), > - .length = SZ_8K, > - .type = MT_DEVICE, > - }, > -}; > - > static struct platform_device exynos_cpuidle = { > .name = "exynos_cpuidle", > #ifdef CONFIG_ARM_EXYNOS_CPUIDLE > @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname, > return 1; > } > > -/* > - * exynos_map_io > - * > - * register the standard cpu IO areas > - */ > -static void __init exynos_map_io(void) > -{ > - if (soc_is_exynos4()) > - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); > -} > - > static void __init exynos_init_io(void) > { > debug_ll_io_init(); > @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) > > /* detect cpu id and rev. */ > s5p_init_cpu(S5P_VA_CHIPID); > - > - exynos_map_io(); > } > > /* > diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h > index 5fb0040..0eef407 100644 > --- a/arch/arm/mach-exynos/include/mach/map.h > +++ b/arch/arm/mach-exynos/include/mach/map.h > @@ -18,6 +18,4 @@ > > #define EXYNOS_PA_CHIPID 0x10000000 > > -#define EXYNOS4_PA_COREPERI 0x10500000 > - > #endif /* __ASM_ARCH_MAP_H */ > diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c > index a5d6841..553d0d9 100644 > --- a/arch/arm/mach-exynos/platsmp.c > +++ b/arch/arm/mach-exynos/platsmp.c > @@ -224,11 +224,6 @@ static void write_pen_release(int val) > sync_cache_w(&pen_release); > } > > -static void __iomem *scu_base_addr(void) > -{ > - return (void __iomem *)(S5P_VA_SCU); > -} > - > static DEFINE_SPINLOCK(boot_lock); > > static void exynos_secondary_init(unsigned int cpu) > @@ -387,14 +382,23 @@ fail: > > static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) > { > + struct device_node *np; > + void __iomem *scu_base; > int i; > > exynos_sysram_init(); > > exynos_set_delayed_reset_assertion(true); > > - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) > - scu_enable(scu_base_addr()); > + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { > + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); what if of_find_compatible_node() fails? May be add a error check for the same? > + scu_base = of_iomap(np, 0); > + if (scu_base) { > + scu_enable(scu_base); > + iounmap(scu_base); > + } > + of_node_put(np); > + } > > /* > * Write the address of secondary startup into the > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c > index 487295f..60e6827 100644 > --- a/arch/arm/mach-exynos/pm.c > +++ b/arch/arm/mach-exynos/pm.c > @@ -18,6 +18,7 @@ > #include <linux/cpu_pm.h> > #include <linux/io.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/soc/samsung/exynos-regs-pmu.h> > #include <linux/soc/samsung/exynos-pmu.h> > > @@ -26,8 +27,6 @@ > #include <asm/suspend.h> > #include <asm/cacheflush.h> > > -#include <mach/map.h> > - > #include "common.h" > > static inline void __iomem *exynos_boot_vector_addr(void) > @@ -158,6 +157,8 @@ static int exynos_aftr_finisher(unsigned long flags) > > void exynos_enter_aftr(void) > { > + struct device_node *np; > + void __iomem *scu_base; > unsigned int cpuid = smp_processor_id(); > > cpu_pm_enter(); > @@ -177,7 +178,14 @@ void exynos_enter_aftr(void) > cpu_suspend(0, exynos_aftr_finisher); > > if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { > - scu_enable(S5P_VA_SCU); > + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); same as above > + scu_base = of_iomap(np, 0); > + if (scu_base) { > + scu_enable(scu_base); > + iounmap(scu_base); > + } > + of_node_put(np); > + > if (call_firmware_op(resume) == -ENOSYS) > exynos_cpu_restore_register(); > } > diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c > index 06332f6..7ab7e67 100644 > --- a/arch/arm/mach-exynos/suspend.c > +++ b/arch/arm/mach-exynos/suspend.c > @@ -34,8 +34,6 @@ > #include <asm/smp_scu.h> > #include <asm/suspend.h> > > -#include <mach/map.h> > - > #include <plat/pm-common.h> > > #include "common.h" > @@ -453,6 +451,8 @@ static void exynos_pm_release_retention(void) > > static void exynos_pm_resume(void) > { > + struct device_node *np; > + void __iomem *scu_base; > u32 cpuid = read_cpuid_part(); > > if (exynos_pm_central_resume()) > @@ -461,8 +461,15 @@ static void exynos_pm_resume(void) > /* For release retention */ > exynos_pm_release_retention(); > > - if (cpuid == ARM_CPU_PART_CORTEX_A9) > - scu_enable(S5P_VA_SCU); > + if (cpuid == ARM_CPU_PART_CORTEX_A9) { > + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); and here otherwise patch looks good. > + scu_base = of_iomap(np, 0); > + if (scu_base) { > + scu_enable(scu_base); > + iounmap(scu_base); > + } > + of_node_put(np); > + } > > if (call_firmware_op(resume) == -ENOSYS > && cpuid == ARM_CPU_PART_CORTEX_A9) > diff --git a/arch/arm/plat-samsung/include/plat/map-s5p.h b/arch/arm/plat-samsung/include/plat/map-s5p.h > index 0fe2828..512ed1f 100644 > --- a/arch/arm/plat-samsung/include/plat/map-s5p.h > +++ b/arch/arm/plat-samsung/include/plat/map-s5p.h > @@ -15,10 +15,6 @@ > > #define S5P_VA_CHIPID S3C_ADDR(0x02000000) > > -#define S5P_VA_COREPERI_BASE S3C_ADDR(0x02800000) > -#define S5P_VA_COREPERI(x) (S5P_VA_COREPERI_BASE + (x)) > -#define S5P_VA_SCU S5P_VA_COREPERI(0x0) > - > #define VA_VIC(x) (S3C_VA_IRQ + ((x) * 0x10000)) > #define VA_VIC0 VA_VIC(0) > #define VA_VIC1 VA_VIC(1) >
Hi Alim, On Friday 04 November 2016 06:56 PM, Alim Akhtar wrote: > Hi Pankaj, > > On 11/04/2016 09:09 AM, Pankaj Dubey wrote: >> Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC >> based boards. >> Instead use mapping from device tree node of SCU. >> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> arch/arm/mach-exynos/exynos.c | 22 >> ---------------------- >> arch/arm/mach-exynos/include/mach/map.h | 2 -- >> arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- >> arch/arm/mach-exynos/pm.c | 14 +++++++++++--- >> arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- >> arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- >> 6 files changed, 33 insertions(+), 42 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/exynos.c >> b/arch/arm/mach-exynos/exynos.c >> index 757fc11..fa08ef9 100644 >> --- a/arch/arm/mach-exynos/exynos.c >> +++ b/arch/arm/mach-exynos/exynos.c >> @@ -28,15 +28,6 @@ >> >> #include "common.h" >> >> -static struct map_desc exynos4_iodesc[] __initdata = { >> - { >> - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, >> - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), >> - .length = SZ_8K, >> - .type = MT_DEVICE, >> - }, >> -}; >> - >> static struct platform_device exynos_cpuidle = { >> .name = "exynos_cpuidle", >> #ifdef CONFIG_ARM_EXYNOS_CPUIDLE >> @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned >> long node, const char *uname, >> return 1; >> } >> >> -/* >> - * exynos_map_io >> - * >> - * register the standard cpu IO areas >> - */ >> -static void __init exynos_map_io(void) >> -{ >> - if (soc_is_exynos4()) >> - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); >> -} >> - >> static void __init exynos_init_io(void) >> { >> debug_ll_io_init(); >> @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) >> >> /* detect cpu id and rev. */ >> s5p_init_cpu(S5P_VA_CHIPID); >> - >> - exynos_map_io(); >> } >> >> /* >> diff --git a/arch/arm/mach-exynos/include/mach/map.h >> b/arch/arm/mach-exynos/include/mach/map.h >> index 5fb0040..0eef407 100644 >> --- a/arch/arm/mach-exynos/include/mach/map.h >> +++ b/arch/arm/mach-exynos/include/mach/map.h >> @@ -18,6 +18,4 @@ >> >> #define EXYNOS_PA_CHIPID 0x10000000 >> >> -#define EXYNOS4_PA_COREPERI 0x10500000 >> - >> #endif /* __ASM_ARCH_MAP_H */ >> diff --git a/arch/arm/mach-exynos/platsmp.c >> b/arch/arm/mach-exynos/platsmp.c >> index a5d6841..553d0d9 100644 >> --- a/arch/arm/mach-exynos/platsmp.c >> +++ b/arch/arm/mach-exynos/platsmp.c >> @@ -224,11 +224,6 @@ static void write_pen_release(int val) >> sync_cache_w(&pen_release); >> } >> >> -static void __iomem *scu_base_addr(void) >> -{ >> - return (void __iomem *)(S5P_VA_SCU); >> -} >> - >> static DEFINE_SPINLOCK(boot_lock); >> >> static void exynos_secondary_init(unsigned int cpu) >> @@ -387,14 +382,23 @@ fail: >> >> static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) >> { >> + struct device_node *np; >> + void __iomem *scu_base; >> int i; >> >> exynos_sysram_init(); >> >> exynos_set_delayed_reset_assertion(true); >> >> - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) >> - scu_enable(scu_base_addr()); >> + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { >> + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); > > what if of_find_compatible_node() fails? May be add a error check for > the same? Thanks for review. You are right of_find_compatible_node() is bound to fail, but only in case supplied compatible is missing in DT. In our case this piece of code will execute only for Cortex-A9 based SoC (which in case of Exynos SoC is applicable only for Exynos4 series) and we will for sure providing "arm,cortex-a9-scu" in DT, so there is no chance of failure. So I feel extra check on "np" for NULL will add no benefit here. Thanks, Pankaj Dubey
Hi Pankaj, On 11/07/2016 08:05 AM, pankaj.dubey wrote: > Hi Alim, > > On Friday 04 November 2016 06:56 PM, Alim Akhtar wrote: >> Hi Pankaj, >> >> On 11/04/2016 09:09 AM, Pankaj Dubey wrote: >>> Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC >>> based boards. >>> Instead use mapping from device tree node of SCU. >>> >>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >>> --- >>> arch/arm/mach-exynos/exynos.c | 22 >>> ---------------------- >>> arch/arm/mach-exynos/include/mach/map.h | 2 -- >>> arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- >>> arch/arm/mach-exynos/pm.c | 14 +++++++++++--- >>> arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- >>> arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- >>> 6 files changed, 33 insertions(+), 42 deletions(-) >>> >>> diff --git a/arch/arm/mach-exynos/exynos.c >>> b/arch/arm/mach-exynos/exynos.c >>> index 757fc11..fa08ef9 100644 >>> --- a/arch/arm/mach-exynos/exynos.c >>> +++ b/arch/arm/mach-exynos/exynos.c >>> @@ -28,15 +28,6 @@ >>> >>> #include "common.h" >>> >>> -static struct map_desc exynos4_iodesc[] __initdata = { >>> - { >>> - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, >>> - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), >>> - .length = SZ_8K, >>> - .type = MT_DEVICE, >>> - }, >>> -}; >>> - >>> static struct platform_device exynos_cpuidle = { >>> .name = "exynos_cpuidle", >>> #ifdef CONFIG_ARM_EXYNOS_CPUIDLE >>> @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned >>> long node, const char *uname, >>> return 1; >>> } >>> >>> -/* >>> - * exynos_map_io >>> - * >>> - * register the standard cpu IO areas >>> - */ >>> -static void __init exynos_map_io(void) >>> -{ >>> - if (soc_is_exynos4()) >>> - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); >>> -} >>> - >>> static void __init exynos_init_io(void) >>> { >>> debug_ll_io_init(); >>> @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) >>> >>> /* detect cpu id and rev. */ >>> s5p_init_cpu(S5P_VA_CHIPID); >>> - >>> - exynos_map_io(); >>> } >>> >>> /* >>> diff --git a/arch/arm/mach-exynos/include/mach/map.h >>> b/arch/arm/mach-exynos/include/mach/map.h >>> index 5fb0040..0eef407 100644 >>> --- a/arch/arm/mach-exynos/include/mach/map.h >>> +++ b/arch/arm/mach-exynos/include/mach/map.h >>> @@ -18,6 +18,4 @@ >>> >>> #define EXYNOS_PA_CHIPID 0x10000000 >>> >>> -#define EXYNOS4_PA_COREPERI 0x10500000 >>> - >>> #endif /* __ASM_ARCH_MAP_H */ >>> diff --git a/arch/arm/mach-exynos/platsmp.c >>> b/arch/arm/mach-exynos/platsmp.c >>> index a5d6841..553d0d9 100644 >>> --- a/arch/arm/mach-exynos/platsmp.c >>> +++ b/arch/arm/mach-exynos/platsmp.c >>> @@ -224,11 +224,6 @@ static void write_pen_release(int val) >>> sync_cache_w(&pen_release); >>> } >>> >>> -static void __iomem *scu_base_addr(void) >>> -{ >>> - return (void __iomem *)(S5P_VA_SCU); >>> -} >>> - >>> static DEFINE_SPINLOCK(boot_lock); >>> >>> static void exynos_secondary_init(unsigned int cpu) >>> @@ -387,14 +382,23 @@ fail: >>> >>> static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) >>> { >>> + struct device_node *np; >>> + void __iomem *scu_base; >>> int i; >>> >>> exynos_sysram_init(); >>> >>> exynos_set_delayed_reset_assertion(true); >>> >>> - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) >>> - scu_enable(scu_base_addr()); >>> + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { >>> + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); >> >> what if of_find_compatible_node() fails? May be add a error check for >> the same? > > Thanks for review. > > You are right of_find_compatible_node() is bound to fail, but only in > case supplied compatible is missing in DT. In our case this piece of > code will execute only for Cortex-A9 based SoC (which in case of Exynos > SoC is applicable only for Exynos4 series) and we will for sure > providing "arm,cortex-a9-scu" in DT, so there is no chance of failure. > So I feel extra check on "np" for NULL will add no benefit here. > Well I am not entirely convenience here, I still feel it better to have those check, lets not assume anything about future, but when I see of_find_compatible_node() uses elsewhere in kernel, both kind of uses are there (with/without error check). So, I leave it to you and maintainer to take a call on this, otherwise this patch looks good. Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com> > > Thanks, > Pankaj Dubey > >
Hi Alim, On 7 November 2016 at 10:19, Alim Akhtar <alim.akhtar@samsung.com> wrote: > > Hi Pankaj, > > > On 11/07/2016 08:05 AM, pankaj.dubey wrote: >> >> Hi Alim, >> >> On Friday 04 November 2016 06:56 PM, Alim Akhtar wrote: >>> >>> Hi Pankaj, >>> >>> On 11/04/2016 09:09 AM, Pankaj Dubey wrote: >>>> >>>> Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC >>>> based boards. >>>> Instead use mapping from device tree node of SCU. >>>> >>>> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >>>> --- >>>> arch/arm/mach-exynos/exynos.c | 22 >>>> ---------------------- >>>> arch/arm/mach-exynos/include/mach/map.h | 2 -- >>>> arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- >>>> arch/arm/mach-exynos/pm.c | 14 +++++++++++--- >>>> arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- >>>> arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- >>>> 6 files changed, 33 insertions(+), 42 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-exynos/exynos.c >>>> b/arch/arm/mach-exynos/exynos.c >>>> index 757fc11..fa08ef9 100644 >>>> --- a/arch/arm/mach-exynos/exynos.c >>>> +++ b/arch/arm/mach-exynos/exynos.c >>>> @@ -28,15 +28,6 @@ >>>> >>>> #include "common.h" >>>> >>>> -static struct map_desc exynos4_iodesc[] __initdata = { >>>> - { >>>> - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, >>>> - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), >>>> - .length = SZ_8K, >>>> - .type = MT_DEVICE, >>>> - }, >>>> -}; >>>> - >>>> static struct platform_device exynos_cpuidle = { >>>> .name = "exynos_cpuidle", >>>> #ifdef CONFIG_ARM_EXYNOS_CPUIDLE >>>> @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned >>>> long node, const char *uname, >>>> return 1; >>>> } >>>> >>>> -/* >>>> - * exynos_map_io >>>> - * >>>> - * register the standard cpu IO areas >>>> - */ >>>> -static void __init exynos_map_io(void) >>>> -{ >>>> - if (soc_is_exynos4()) >>>> - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); >>>> -} >>>> - >>>> static void __init exynos_init_io(void) >>>> { >>>> debug_ll_io_init(); >>>> @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) >>>> >>>> /* detect cpu id and rev. */ >>>> s5p_init_cpu(S5P_VA_CHIPID); >>>> - >>>> - exynos_map_io(); >>>> } >>>> >>>> /* >>>> diff --git a/arch/arm/mach-exynos/include/mach/map.h >>>> b/arch/arm/mach-exynos/include/mach/map.h >>>> index 5fb0040..0eef407 100644 >>>> --- a/arch/arm/mach-exynos/include/mach/map.h >>>> +++ b/arch/arm/mach-exynos/include/mach/map.h >>>> @@ -18,6 +18,4 @@ >>>> >>>> #define EXYNOS_PA_CHIPID 0x10000000 >>>> >>>> -#define EXYNOS4_PA_COREPERI 0x10500000 >>>> - >>>> #endif /* __ASM_ARCH_MAP_H */ >>>> diff --git a/arch/arm/mach-exynos/platsmp.c >>>> b/arch/arm/mach-exynos/platsmp.c >>>> index a5d6841..553d0d9 100644 >>>> --- a/arch/arm/mach-exynos/platsmp.c >>>> +++ b/arch/arm/mach-exynos/platsmp.c >>>> @@ -224,11 +224,6 @@ static void write_pen_release(int val) >>>> sync_cache_w(&pen_release); >>>> } >>>> >>>> -static void __iomem *scu_base_addr(void) >>>> -{ >>>> - return (void __iomem *)(S5P_VA_SCU); >>>> -} >>>> - >>>> static DEFINE_SPINLOCK(boot_lock); >>>> >>>> static void exynos_secondary_init(unsigned int cpu) >>>> @@ -387,14 +382,23 @@ fail: >>>> >>>> static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) >>>> { >>>> + struct device_node *np; >>>> + void __iomem *scu_base; >>>> int i; >>>> >>>> exynos_sysram_init(); >>>> >>>> exynos_set_delayed_reset_assertion(true); >>>> >>>> - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) >>>> - scu_enable(scu_base_addr()); >>>> + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { >>>> + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); >>> >>> >>> what if of_find_compatible_node() fails? May be add a error check for >>> the same? >> >> >> Thanks for review. >> >> You are right of_find_compatible_node() is bound to fail, but only in >> case supplied compatible is missing in DT. In our case this piece of >> code will execute only for Cortex-A9 based SoC (which in case of Exynos >> SoC is applicable only for Exynos4 series) and we will for sure >> providing "arm,cortex-a9-scu" in DT, so there is no chance of failure. >> So I feel extra check on "np" for NULL will add no benefit here. >> > Well I am not entirely convenience here, I still feel it better to have those check, lets not assume anything about future, but when I see of_find_compatible_node() uses elsewhere in kernel, both kind of uses are there (with/without error check). > So, I leave it to you and maintainer to take a call on this, otherwise this patch looks good. > > Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com> > Well, I further checked details of of_find_compatible_node() and of_iomap and below is function call chain of_find_compatible_node //Returns vaild pointer for match, and NULL for no match --> __of_device_is_compatible () //Returns 0 for no match, and a positive integer on match So in our case lets say for any reason "arm,cortex-a9-scu" compatible is not present, "np" will be set as NULL. Next we have following line without check on "np" for NULL, scu_base = of_iomap(np, 0); If we see function call sequence of of_iomap we have, of_iomap(..) //Returns NULL is of_address_to_resource returns non-zero --> of_address_to_resource(...) // Returns -EINVAL if "addrp" is NULL --> of_get_address(...) //Returns NULL if "parent" is NULL --> of_get_parent(...) //Returns np with refcount incremented if np is NOT NULL else returns NULL So in our case we will get scu_base as NULL if we pass "np" as NULL, and on very next line we have check on scu_base, which will prevent us from de-referencing a NULL pointer. So there won't be any CRASH or abnormal behavior, due to not checking "np" for NULL. I think this is the reason in many places in kernel there is no check for NULL for of_find_compatible_node, surely some places there are checks, but as per current code flow if someone is calling of_iomap with np as NULL there won't be any crash and of_iomap will gracefully returns NULL in that case. Also of_node_put and of_node_get has check for NULL received device_node and handles it gracefully. But one thing I missed is that in case scu_base is NULL we are not suppose to move ahead in function exynos_smp_prepare_cpus(..), This part I will update and post next version with appropriate change. Thanks, Pankaj Dubey >> >> >> Thanks, >> Pankaj Dubey >> >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 04, 2016 at 09:09:23AM +0530, Pankaj Dubey wrote: > Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC based boards. > Instead use mapping from device tree node of SCU. > > Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> > --- > arch/arm/mach-exynos/exynos.c | 22 ---------------------- > arch/arm/mach-exynos/include/mach/map.h | 2 -- > arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- > arch/arm/mach-exynos/pm.c | 14 +++++++++++--- > arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- > arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- > 6 files changed, 33 insertions(+), 42 deletions(-) > > diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c > index 757fc11..fa08ef9 100644 > --- a/arch/arm/mach-exynos/exynos.c > +++ b/arch/arm/mach-exynos/exynos.c > @@ -28,15 +28,6 @@ > > #include "common.h" > > -static struct map_desc exynos4_iodesc[] __initdata = { > - { > - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, > - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), > - .length = SZ_8K, > - .type = MT_DEVICE, > - }, > -}; > - > static struct platform_device exynos_cpuidle = { > .name = "exynos_cpuidle", > #ifdef CONFIG_ARM_EXYNOS_CPUIDLE > @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname, > return 1; > } > > -/* > - * exynos_map_io > - * > - * register the standard cpu IO areas > - */ > -static void __init exynos_map_io(void) > -{ > - if (soc_is_exynos4()) > - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); > -} > - > static void __init exynos_init_io(void) > { > debug_ll_io_init(); > @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) > > /* detect cpu id and rev. */ > s5p_init_cpu(S5P_VA_CHIPID); > - > - exynos_map_io(); > } > > /* > diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h > index 5fb0040..0eef407 100644 > --- a/arch/arm/mach-exynos/include/mach/map.h > +++ b/arch/arm/mach-exynos/include/mach/map.h > @@ -18,6 +18,4 @@ > > #define EXYNOS_PA_CHIPID 0x10000000 > > -#define EXYNOS4_PA_COREPERI 0x10500000 > - > #endif /* __ASM_ARCH_MAP_H */ > diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c > index a5d6841..553d0d9 100644 > --- a/arch/arm/mach-exynos/platsmp.c > +++ b/arch/arm/mach-exynos/platsmp.c > @@ -224,11 +224,6 @@ static void write_pen_release(int val) > sync_cache_w(&pen_release); > } > > -static void __iomem *scu_base_addr(void) > -{ > - return (void __iomem *)(S5P_VA_SCU); > -} > - > static DEFINE_SPINLOCK(boot_lock); > > static void exynos_secondary_init(unsigned int cpu) > @@ -387,14 +382,23 @@ fail: > > static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) > { > + struct device_node *np; > + void __iomem *scu_base; > int i; > > exynos_sysram_init(); > > exynos_set_delayed_reset_assertion(true); > > - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) > - scu_enable(scu_base_addr()); > + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { > + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); > + scu_base = of_iomap(np, 0); > + if (scu_base) { > + scu_enable(scu_base); > + iounmap(scu_base); > + } > + of_node_put(np); I do not like the duplication - this code appears in three places and they are the same. Could you split it into separate function? As you mentioned to Alim, in case of lack of node, it would be nice to notify the user (pr_err() etc). Best regards, Krzysztof
Hi Krzysztof, On Monday 07 November 2016 11:23 PM, Krzysztof Kozlowski wrote: > On Fri, Nov 04, 2016 at 09:09:23AM +0530, Pankaj Dubey wrote: >> Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC based boards. >> Instead use mapping from device tree node of SCU. >> >> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> >> --- >> arch/arm/mach-exynos/exynos.c | 22 ---------------------- >> arch/arm/mach-exynos/include/mach/map.h | 2 -- >> arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- >> arch/arm/mach-exynos/pm.c | 14 +++++++++++--- >> arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- >> arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- >> 6 files changed, 33 insertions(+), 42 deletions(-) >> >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >> index 757fc11..fa08ef9 100644 >> --- a/arch/arm/mach-exynos/exynos.c >> +++ b/arch/arm/mach-exynos/exynos.c >> @@ -28,15 +28,6 @@ >> >> #include "common.h" >> >> -static struct map_desc exynos4_iodesc[] __initdata = { >> - { >> - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, >> - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), >> - .length = SZ_8K, >> - .type = MT_DEVICE, >> - }, >> -}; >> - >> static struct platform_device exynos_cpuidle = { >> .name = "exynos_cpuidle", >> #ifdef CONFIG_ARM_EXYNOS_CPUIDLE >> @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname, >> return 1; >> } >> >> -/* >> - * exynos_map_io >> - * >> - * register the standard cpu IO areas >> - */ >> -static void __init exynos_map_io(void) >> -{ >> - if (soc_is_exynos4()) >> - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); >> -} >> - >> static void __init exynos_init_io(void) >> { >> debug_ll_io_init(); >> @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) >> >> /* detect cpu id and rev. */ >> s5p_init_cpu(S5P_VA_CHIPID); >> - >> - exynos_map_io(); >> } >> >> /* >> diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h >> index 5fb0040..0eef407 100644 >> --- a/arch/arm/mach-exynos/include/mach/map.h >> +++ b/arch/arm/mach-exynos/include/mach/map.h >> @@ -18,6 +18,4 @@ >> >> #define EXYNOS_PA_CHIPID 0x10000000 >> >> -#define EXYNOS4_PA_COREPERI 0x10500000 >> - >> #endif /* __ASM_ARCH_MAP_H */ >> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c >> index a5d6841..553d0d9 100644 >> --- a/arch/arm/mach-exynos/platsmp.c >> +++ b/arch/arm/mach-exynos/platsmp.c >> @@ -224,11 +224,6 @@ static void write_pen_release(int val) >> sync_cache_w(&pen_release); >> } >> >> -static void __iomem *scu_base_addr(void) >> -{ >> - return (void __iomem *)(S5P_VA_SCU); >> -} >> - >> static DEFINE_SPINLOCK(boot_lock); >> >> static void exynos_secondary_init(unsigned int cpu) >> @@ -387,14 +382,23 @@ fail: >> >> static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) >> { >> + struct device_node *np; >> + void __iomem *scu_base; >> int i; >> >> exynos_sysram_init(); >> >> exynos_set_delayed_reset_assertion(true); >> >> - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) >> - scu_enable(scu_base_addr()); >> + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { >> + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); >> + scu_base = of_iomap(np, 0); >> + if (scu_base) { >> + scu_enable(scu_base); >> + iounmap(scu_base); >> + } >> + of_node_put(np); > > I do not like the duplication - this code appears in three places and > they are the same. Could you split it into separate function? > OK, will do these changes in v2. Only pm.c and suspend.c file's scu_enable can be moved to single place without introducing more dependencies between these mach files. > As you mentioned to Alim, in case of lack of node, it would be nice to notify the > user (pr_err() etc). > OK, will do these changes in v2. Thanks, Pankaj Dubey > Best regards, > Krzysztof > > > >
diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c index 757fc11..fa08ef9 100644 --- a/arch/arm/mach-exynos/exynos.c +++ b/arch/arm/mach-exynos/exynos.c @@ -28,15 +28,6 @@ #include "common.h" -static struct map_desc exynos4_iodesc[] __initdata = { - { - .virtual = (unsigned long)S5P_VA_COREPERI_BASE, - .pfn = __phys_to_pfn(EXYNOS4_PA_COREPERI), - .length = SZ_8K, - .type = MT_DEVICE, - }, -}; - static struct platform_device exynos_cpuidle = { .name = "exynos_cpuidle", #ifdef CONFIG_ARM_EXYNOS_CPUIDLE @@ -99,17 +90,6 @@ static int __init exynos_fdt_map_chipid(unsigned long node, const char *uname, return 1; } -/* - * exynos_map_io - * - * register the standard cpu IO areas - */ -static void __init exynos_map_io(void) -{ - if (soc_is_exynos4()) - iotable_init(exynos4_iodesc, ARRAY_SIZE(exynos4_iodesc)); -} - static void __init exynos_init_io(void) { debug_ll_io_init(); @@ -118,8 +98,6 @@ static void __init exynos_init_io(void) /* detect cpu id and rev. */ s5p_init_cpu(S5P_VA_CHIPID); - - exynos_map_io(); } /* diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h index 5fb0040..0eef407 100644 --- a/arch/arm/mach-exynos/include/mach/map.h +++ b/arch/arm/mach-exynos/include/mach/map.h @@ -18,6 +18,4 @@ #define EXYNOS_PA_CHIPID 0x10000000 -#define EXYNOS4_PA_COREPERI 0x10500000 - #endif /* __ASM_ARCH_MAP_H */ diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c index a5d6841..553d0d9 100644 --- a/arch/arm/mach-exynos/platsmp.c +++ b/arch/arm/mach-exynos/platsmp.c @@ -224,11 +224,6 @@ static void write_pen_release(int val) sync_cache_w(&pen_release); } -static void __iomem *scu_base_addr(void) -{ - return (void __iomem *)(S5P_VA_SCU); -} - static DEFINE_SPINLOCK(boot_lock); static void exynos_secondary_init(unsigned int cpu) @@ -387,14 +382,23 @@ fail: static void __init exynos_smp_prepare_cpus(unsigned int max_cpus) { + struct device_node *np; + void __iomem *scu_base; int i; exynos_sysram_init(); exynos_set_delayed_reset_assertion(true); - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) - scu_enable(scu_base_addr()); + if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); + scu_base = of_iomap(np, 0); + if (scu_base) { + scu_enable(scu_base); + iounmap(scu_base); + } + of_node_put(np); + } /* * Write the address of secondary startup into the diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c index 487295f..60e6827 100644 --- a/arch/arm/mach-exynos/pm.c +++ b/arch/arm/mach-exynos/pm.c @@ -18,6 +18,7 @@ #include <linux/cpu_pm.h> #include <linux/io.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/soc/samsung/exynos-regs-pmu.h> #include <linux/soc/samsung/exynos-pmu.h> @@ -26,8 +27,6 @@ #include <asm/suspend.h> #include <asm/cacheflush.h> -#include <mach/map.h> - #include "common.h" static inline void __iomem *exynos_boot_vector_addr(void) @@ -158,6 +157,8 @@ static int exynos_aftr_finisher(unsigned long flags) void exynos_enter_aftr(void) { + struct device_node *np; + void __iomem *scu_base; unsigned int cpuid = smp_processor_id(); cpu_pm_enter(); @@ -177,7 +178,14 @@ void exynos_enter_aftr(void) cpu_suspend(0, exynos_aftr_finisher); if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { - scu_enable(S5P_VA_SCU); + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); + scu_base = of_iomap(np, 0); + if (scu_base) { + scu_enable(scu_base); + iounmap(scu_base); + } + of_node_put(np); + if (call_firmware_op(resume) == -ENOSYS) exynos_cpu_restore_register(); } diff --git a/arch/arm/mach-exynos/suspend.c b/arch/arm/mach-exynos/suspend.c index 06332f6..7ab7e67 100644 --- a/arch/arm/mach-exynos/suspend.c +++ b/arch/arm/mach-exynos/suspend.c @@ -34,8 +34,6 @@ #include <asm/smp_scu.h> #include <asm/suspend.h> -#include <mach/map.h> - #include <plat/pm-common.h> #include "common.h" @@ -453,6 +451,8 @@ static void exynos_pm_release_retention(void) static void exynos_pm_resume(void) { + struct device_node *np; + void __iomem *scu_base; u32 cpuid = read_cpuid_part(); if (exynos_pm_central_resume()) @@ -461,8 +461,15 @@ static void exynos_pm_resume(void) /* For release retention */ exynos_pm_release_retention(); - if (cpuid == ARM_CPU_PART_CORTEX_A9) - scu_enable(S5P_VA_SCU); + if (cpuid == ARM_CPU_PART_CORTEX_A9) { + np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu"); + scu_base = of_iomap(np, 0); + if (scu_base) { + scu_enable(scu_base); + iounmap(scu_base); + } + of_node_put(np); + } if (call_firmware_op(resume) == -ENOSYS && cpuid == ARM_CPU_PART_CORTEX_A9) diff --git a/arch/arm/plat-samsung/include/plat/map-s5p.h b/arch/arm/plat-samsung/include/plat/map-s5p.h index 0fe2828..512ed1f 100644 --- a/arch/arm/plat-samsung/include/plat/map-s5p.h +++ b/arch/arm/plat-samsung/include/plat/map-s5p.h @@ -15,10 +15,6 @@ #define S5P_VA_CHIPID S3C_ADDR(0x02000000) -#define S5P_VA_COREPERI_BASE S3C_ADDR(0x02800000) -#define S5P_VA_COREPERI(x) (S5P_VA_COREPERI_BASE + (x)) -#define S5P_VA_SCU S5P_VA_COREPERI(0x0) - #define VA_VIC(x) (S3C_VA_IRQ + ((x) * 0x10000)) #define VA_VIC0 VA_VIC(0) #define VA_VIC1 VA_VIC(1)
Lets remove static mapping of SCU SFR mainly used in CORTEX-A9 SoC based boards. Instead use mapping from device tree node of SCU. Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> --- arch/arm/mach-exynos/exynos.c | 22 ---------------------- arch/arm/mach-exynos/include/mach/map.h | 2 -- arch/arm/mach-exynos/platsmp.c | 18 +++++++++++------- arch/arm/mach-exynos/pm.c | 14 +++++++++++--- arch/arm/mach-exynos/suspend.c | 15 +++++++++++---- arch/arm/plat-samsung/include/plat/map-s5p.h | 4 ---- 6 files changed, 33 insertions(+), 42 deletions(-)