diff mbox

[3/4] ARM: EXYNOS: Remove static mapping of SCU SFR

Message ID 1478230764-13748-4-git-send-email-pankaj.dubey@samsung.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

Pankaj Dubey Nov. 4, 2016, 3:39 a.m. UTC
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(-)

Comments

Alim Akhtar Nov. 4, 2016, 1:26 p.m. UTC | #1
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)
>
--
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
Alim Akhtar Nov. 7, 2016, 4:49 a.m. UTC | #2
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
>
>
--
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
Pankaj Dubey Nov. 7, 2016, 4:59 p.m. UTC | #3
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
--
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
Krzysztof Kozlowski Nov. 7, 2016, 5:53 p.m. UTC | #4
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

--
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
Pankaj Dubey Nov. 8, 2016, 3:14 a.m. UTC | #5
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
> 
> 
> 
> 
--
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
diff mbox

Patch

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)