[PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

S5P_ARM_CORE1_* registers affect only core 1. To control further cores
properly another registers must be used.

This patch replaces S5P_ARM_CORE1_* register definitions with
S5P_ARM_CORE_*(x) macro which return addresses of registers for specified
core.

This fixes CPU hotplug on quad core Exynos SoCs on which currently
offlining CPUs 2 or 3 caused CPU 1 to be turned off. In addition,
bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
secondary cores by default.

Cc: stable@vger.kernel.org
Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 arch/arm/mach-exynos/hotplug.c               |  9 +++++----
 arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
 arch/arm/mach-exynos/platsmp.c               |  9 +++++----
 3 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index af90cfa..c089943 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
 	for (;;) {
+		void __iomem *reg_base;
+		unsigned int phys_cpu = cpu_logical_map(cpu);
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
+		__raw_writel(0, reg_base);
 
 		/*
 		 * here's the WFI
@@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 		    :
 		    : "memory", "cc");
 
-		if (pen_release == cpu_logical_map(cpu)) {
+		if (pen_release == phys_cpu) {
 			/*
 			 * OK, proper wakeup, we're done
 			 */
diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
index 57344b7..cf40b86 100644
--- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
+++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
@@ -125,10 +125,14 @@
 #define S5P_GPS_ALIVE_LOWPWR			S5P_PMUREG(0x13A0)
 
 #define S5P_ARM_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
 #define S5P_ARM_CORE0_OPTION			S5P_PMUREG(0x2008)
-#define S5P_ARM_CORE1_CONFIGURATION		S5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS			S5P_PMUREG(0x2084)
-#define S5P_ARM_CORE1_OPTION			S5P_PMUREG(0x2088)
+#define S5P_ARM_CORE_CONFIGURATION(_nr)		\
+		(S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
+#define S5P_ARM_CORE_STATUS(_nr)		\
+		(S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
+#define S5P_ARM_CORE_OPTION(_nr)		\
+		(S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
 
 #define S5P_ARM_COMMON_OPTION			S5P_PMUREG(0x2408)
 #define S5P_TOP_PWR_OPTION			S5P_PMUREG(0x2C48)
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index d9c6d0a..2cbabc8 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -109,14 +109,15 @@ static int __cpuinit exynos_boot_secondary(unsigned int cpu, struct task_struct
 	 */
 	write_pen_release(phys_cpu);
 
-	if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) {
+	if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
+	    & S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(phys_cpu));
 
 		timeout = 10;
 
 		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned int cpu, struct task_struct
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			printk(KERN_ERR "cpu%u power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
-- 
1.8.2.1

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

Ccing Arnd and Olof, because I forgot to add them to git send-email...

Sorry for the noise.

On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> S5P_ARM_CORE1_* registers affect only core 1. To control further cores
> properly another registers must be used.
> 
> This patch replaces S5P_ARM_CORE1_* register definitions with
> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> specified core.
> 
> This fixes CPU hotplug on quad core Exynos SoCs on which currently
> offlining CPUs 2 or 3 caused CPU 1 to be turned off.

Obviously this doesn't happen currently because of the if (cpu == 1), but 
if logical cpu1 turned out not to be physical cpu1, then it would crash.

Best regards,
Tomasz

> In addition,
> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
> secondary cores by default.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>  arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>  arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>  3 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/hotplug.c
> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>  static inline void platform_do_lowpower(unsigned int cpu, int
> *spurious) {
>  	for (;;) {
> +		void __iomem *reg_base;
> +		unsigned int phys_cpu = cpu_logical_map(cpu);
> 
> -		/* make cpu1 to be turned off at next WFI command */
> -		if (cpu == 1)
> -			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> +		reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> +		__raw_writel(0, reg_base);
> 
>  		/*
>  		 * here's the WFI
> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
> cpu, int *spurious)
>  		    : "memory", "cc");
> 
> -		if (pen_release == cpu_logical_map(cpu)) {
> +		if (pen_release == phys_cpu) {
>  			/*
>  			 * OK, proper wakeup, we're done
>  			 */
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
> 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -125,10 +125,14 @@
>  #define S5P_GPS_ALIVE_LOWPWR			S5P_PMUREG(0x13A0)
> 
>  #define S5P_ARM_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
>  #define S5P_ARM_CORE0_OPTION			S5P_PMUREG(0x2008)
> -#define S5P_ARM_CORE1_CONFIGURATION		S5P_PMUREG(0x2080)
> -#define S5P_ARM_CORE1_STATUS			S5P_PMUREG(0x2084)
> -#define S5P_ARM_CORE1_OPTION			S5P_PMUREG(0x2088)
> +#define S5P_ARM_CORE_CONFIGURATION(_nr)		\
> +		(S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
> +#define S5P_ARM_CORE_STATUS(_nr)		\
> +		(S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
> +#define S5P_ARM_CORE_OPTION(_nr)		\
> +		(S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
> 
>  #define S5P_ARM_COMMON_OPTION			S5P_PMUREG(0x2408)
>  #define S5P_TOP_PWR_OPTION			S5P_PMUREG(0x2C48)
> diff --git a/arch/arm/mach-exynos/platsmp.c
> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -109,14 +109,15 @@ static int __cpuinit
> exynos_boot_secondary(unsigned int cpu, struct task_struct */
>  	write_pen_release(phys_cpu);
> 
> -	if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) 
{
> +	if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
> +	    & S5P_CORE_LOCAL_PWR_EN)) {
>  		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -			     S5P_ARM_CORE1_CONFIGURATION);
> +			     S5P_ARM_CORE_CONFIGURATION(phys_cpu));
> 
>  		timeout = 10;
> 
>  		/* wait max 10 ms until cpu1 is on */
> -		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> +		while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>  			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) 
{
>  			if (timeout-- == 0)
>  				break;
> @@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
> int cpu, struct task_struct }
> 
>  		if (timeout == 0) {
> -			printk(KERN_ERR "cpu1 power enable failed");
> +			printk(KERN_ERR "cpu%u power enable failed", cpu);
>  			spin_unlock(&boot_lock);
>  			return -ETIMEDOUT;
>  		}

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Kukjin Kim]

On 06/19/13 02:45, Tomasz Figa wrote:
> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>
> Sorry for the noise.
>
> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> S5P_ARM_CORE1_* registers affect only core 1. To control further cores
>> properly another registers must be used.
>>
>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> specified core.
>>
>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>
> Obviously this doesn't happen currently because of the if (cpu == 1), but

Yes, not happened...and just note exynos5440 doesn't support hotplug :) 
so this is available on exynos4412 and added 5420.

> if logical cpu1 turned out not to be physical cpu1, then it would crash.
>
> Best regards,
> Tomasz
>
>> In addition,
>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
>> secondary cores by default.
>>
I need to test on board about above...

>> Cc: stable@vger.kernel.org
>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> ---
>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>>   3 files changed, 17 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>>   static inline void platform_do_lowpower(unsigned int cpu, int
>> *spurious) {
>>   	for (;;) {
>> +		void __iomem *reg_base;
>> +		unsigned int phys_cpu = cpu_logical_map(cpu);
>>
>> -		/* make cpu1 to be turned off at next WFI command */
>> -		if (cpu == 1)
>> -			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> +		reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> +		__raw_writel(0, reg_base);
>>
>>   		/*
>>   		 * here's the WFI
>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
>> cpu, int *spurious)
>>   		    : "memory", "cc");
>>
>> -		if (pen_release == cpu_logical_map(cpu)) {
>> +		if (pen_release == phys_cpu) {
>>   			/*
>>   			 * OK, proper wakeup, we're done
>>   			 */
>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
>> 100644
>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> @@ -125,10 +125,14 @@
>>   #define S5P_GPS_ALIVE_LOWPWR			S5P_PMUREG(0x13A0)
>>
>>   #define S5P_ARM_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
>> +#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
>>   #define S5P_ARM_CORE0_OPTION			S5P_PMUREG(0x2008)
>> -#define S5P_ARM_CORE1_CONFIGURATION		S5P_PMUREG(0x2080)
>> -#define S5P_ARM_CORE1_STATUS			S5P_PMUREG(0x2084)
>> -#define S5P_ARM_CORE1_OPTION			S5P_PMUREG(0x2088)
>> +#define S5P_ARM_CORE_CONFIGURATION(_nr)		\
>> +		(S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
>> +#define S5P_ARM_CORE_STATUS(_nr)		\
>> +		(S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
>> +#define S5P_ARM_CORE_OPTION(_nr)		\
>> +		(S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
>>
>>   #define S5P_ARM_COMMON_OPTION			S5P_PMUREG(0x2408)
>>   #define S5P_TOP_PWR_OPTION			S5P_PMUREG(0x2C48)
>> diff --git a/arch/arm/mach-exynos/platsmp.c
>> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
>> --- a/arch/arm/mach-exynos/platsmp.c
>> +++ b/arch/arm/mach-exynos/platsmp.c
>> @@ -109,14 +109,15 @@ static int __cpuinit
>> exynos_boot_secondary(unsigned int cpu, struct task_struct */
>>   	write_pen_release(phys_cpu);
>>
>> -	if (!(__raw_readl(S5P_ARM_CORE1_STATUS)&  S5P_CORE_LOCAL_PWR_EN))
> {
>> +	if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>> +	&  S5P_CORE_LOCAL_PWR_EN)) {
>>   		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> -			     S5P_ARM_CORE1_CONFIGURATION);
>> +			     S5P_ARM_CORE_CONFIGURATION(phys_cpu));
>>
>>   		timeout = 10;
>>
>>   		/* wait max 10 ms until cpu1 is on */
>> -		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> +		while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>>   			&  S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
> {
>>   			if (timeout-- == 0)
>>   				break;
>> @@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
>> int cpu, struct task_struct }
>>
>>   		if (timeout == 0) {
>> -			printk(KERN_ERR "cpu1 power enable failed");
>> +			printk(KERN_ERR "cpu%u power enable failed", cpu);
>>   			spin_unlock(&boot_lock);
>>   			return -ETIMEDOUT;
>>   		}
> --

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Chander Kashyap]

On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
> On 06/19/13 02:45, Tomasz Figa wrote:
>>
>> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>>
>> Sorry for the noise.
>>
>> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>>>
>>> S5P_ARM_CORE1_* registers affect only core 1. To control further cores
>>> properly another registers must be used.
>>>
>>> This patch replaces S5P_ARM_CORE1_* register definitions with
>>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>>> specified core.
>>>
>>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>>
>>
>> Obviously this doesn't happen currently because of the if (cpu == 1), but
>
>
> Yes, not happened...and just note exynos5440 doesn't support hotplug :) so
> this is available on exynos4412 and added 5420.
>
>
>> if logical cpu1 turned out not to be physical cpu1, then it would crash.
>>
>> Best regards,
>> Tomasz
>>
>>> In addition,
>>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers off
>>> secondary cores by default.
>>>
> I need to test on board about above...
>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>> ---
>>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>>>   3 files changed, 17 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>>> --- a/arch/arm/mach-exynos/hotplug.c
>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>>>   static inline void platform_do_lowpower(unsigned int cpu, int
>>> *spurious) {
>>>         for (;;) {
>>> +               void __iomem *reg_base;
>>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
>>>
>>> -               /* make cpu1 to be turned off at next WFI command */
>>> -               if (cpu == 1)
>>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
Tomasz,
This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then for
cpu0 phys_cpu value will be 0x100,
and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
((0x101) * 0x80)), which is wrong.
>>> +               __raw_writel(0, reg_base);
>>>
>>>                 /*
>>>                  * here's the WFI
>>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned int
>>> cpu, int *spurious)
>>>                     : "memory", "cc");
>>>
>>> -               if (pen_release == cpu_logical_map(cpu)) {
>>> +               if (pen_release == phys_cpu) {
>>>                         /*
>>>                          * OK, proper wakeup, we're done
>>>                          */
>>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
>>> 100644
>>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
>>> @@ -125,10 +125,14 @@
>>>   #define S5P_GPS_ALIVE_LOWPWR                  S5P_PMUREG(0x13A0)
>>>
>>>   #define S5P_ARM_CORE0_CONFIGURATION           S5P_PMUREG(0x2000)
>>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>>>   #define S5P_ARM_CORE0_OPTION                  S5P_PMUREG(0x2008)
>>> -#define S5P_ARM_CORE1_CONFIGURATION            S5P_PMUREG(0x2080)
>>> -#define S5P_ARM_CORE1_STATUS                   S5P_PMUREG(0x2084)
>>> -#define S5P_ARM_CORE1_OPTION                   S5P_PMUREG(0x2088)
>>> +#define S5P_ARM_CORE_CONFIGURATION(_nr)                \
>>> +               (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
>>> +#define S5P_ARM_CORE_STATUS(_nr)               \
>>> +               (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
>>> +#define S5P_ARM_CORE_OPTION(_nr)               \
>>> +               (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
>>>
>>>   #define S5P_ARM_COMMON_OPTION                 S5P_PMUREG(0x2408)
>>>   #define S5P_TOP_PWR_OPTION                    S5P_PMUREG(0x2C48)
>>> diff --git a/arch/arm/mach-exynos/platsmp.c
>>> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
>>> --- a/arch/arm/mach-exynos/platsmp.c
>>> +++ b/arch/arm/mach-exynos/platsmp.c
>>> @@ -109,14 +109,15 @@ static int __cpuinit
>>> exynos_boot_secondary(unsigned int cpu, struct task_struct */
>>>         write_pen_release(phys_cpu);
>>>
>>> -       if (!(__raw_readl(S5P_ARM_CORE1_STATUS)&  S5P_CORE_LOCAL_PWR_EN))
>>
>> {
>>>
>>> +       if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>>> +       &  S5P_CORE_LOCAL_PWR_EN)) {
>>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>>> -                            S5P_ARM_CORE1_CONFIGURATION);
>>> +                            S5P_ARM_CORE_CONFIGURATION(phys_cpu));
>>>
>>>                 timeout = 10;
>>>
>>>                 /* wait max 10 ms until cpu1 is on */
>>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
Ditto
>>>                         &  S5P_CORE_LOCAL_PWR_EN) !=
>>> S5P_CORE_LOCAL_PWR_EN)
>>
>> {
>>>
>>>                         if (timeout-- == 0)
>>>                                 break;
>>> @@ -125,7 +126,7 @@ static int __cpuinit exynos_boot_secondary(unsigned
>>> int cpu, struct task_struct }
>>>
>>>                 if (timeout == 0) {
>>> -                       printk(KERN_ERR "cpu1 power enable failed");
>>> +                       printk(KERN_ERR "cpu%u power enable failed",
>>> cpu);
>>>                         spin_unlock(&boot_lock);
>>>                         return -ETIMEDOUT;
>>>                 }
>>
>> --
>
> --
> 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



--
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > On 06/19/13 02:45, Tomasz Figa wrote:
> >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
> >> 
> >> Sorry for the noise.
> >> 
> >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
> >>> cores
> >>> properly another registers must be used.
> >>> 
> >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> >>> specified core.
> >>> 
> >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
> >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> >> 
> >> Obviously this doesn't happen currently because of the if (cpu == 1),
> >> but> 
> > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
> > so this is available on exynos4412 and added 5420.
> > 
> >> if logical cpu1 turned out not to be physical cpu1, then it would
> >> crash.
> >> 
> >> Best regards,
> >> Tomasz
> >> 
> >>> In addition,
> >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
> >>> off
> >>> secondary cores by default.
> > 
> > I need to test on board about above...
> > 
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >>> ---
> >>> 
> >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
> >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
> >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
> >>>   3 files changed, 17 insertions(+), 11 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> >>> --- a/arch/arm/mach-exynos/hotplug.c
> >>> +++ b/arch/arm/mach-exynos/hotplug.c
> >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> >>> 
> >>>   static inline void platform_do_lowpower(unsigned int cpu, int
> >>> 
> >>> *spurious) {
> >>> 
> >>>         for (;;) {
> >>> 
> >>> +               void __iomem *reg_base;
> >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
> >>> 
> >>> -               /* make cpu1 to be turned off at next WFI command */
> >>> -               if (cpu == 1)
> >>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> >>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> 
> Tomasz,
> This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then for
> cpu0 phys_cpu value will be 0x100,
> and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
> ((0x101) * 0x80)), which is wrong.

Hmm, according to the code initializing __cpu_logical_map[] array this is 
not true.

Here's the code:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468

and for used macros and bitmasks:

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45

Now the structure of the MPIDR register:

http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html

As you can see, the value read from the register in 
smp_setup_processor_id() is only the physical CPU ID, so I don't see any 
problem here.

If I'm wrong, feel free to correct me.

Cc'ing people who should have more knowledge on this.

Best regards,
Tomasz

> >>> +               __raw_writel(0, reg_base);
> >>> 
> >>>                 /*
> >>>                 
> >>>                  * here's the WFI
> >>> 
> >>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned
> >>> int
> >>> cpu, int *spurious)
> >>> 
> >>>                     : "memory", "cc");
> >>> 
> >>> -               if (pen_release == cpu_logical_map(cpu)) {
> >>> +               if (pen_release == phys_cpu) {
> >>> 
> >>>                         /*
> >>>                         
> >>>                          * OK, proper wakeup, we're done
> >>>                          */
> >>> 
> >>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> >>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
> >>> 100644
> >>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> >>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> >>> @@ -125,10 +125,14 @@
> >>> 
> >>>   #define S5P_GPS_ALIVE_LOWPWR                  S5P_PMUREG(0x13A0)
> >>>   
> >>>   #define S5P_ARM_CORE0_CONFIGURATION           S5P_PMUREG(0x2000)
> >>> 
> >>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
> >>> 
> >>>   #define S5P_ARM_CORE0_OPTION                  S5P_PMUREG(0x2008)
> >>> 
> >>> -#define S5P_ARM_CORE1_CONFIGURATION            S5P_PMUREG(0x2080)
> >>> -#define S5P_ARM_CORE1_STATUS                   S5P_PMUREG(0x2084)
> >>> -#define S5P_ARM_CORE1_OPTION                   S5P_PMUREG(0x2088)
> >>> +#define S5P_ARM_CORE_CONFIGURATION(_nr)                \
> >>> +               (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
> >>> +#define S5P_ARM_CORE_STATUS(_nr)               \
> >>> +               (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
> >>> +#define S5P_ARM_CORE_OPTION(_nr)               \
> >>> +               (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
> >>> 
> >>>   #define S5P_ARM_COMMON_OPTION                 S5P_PMUREG(0x2408)
> >>>   #define S5P_TOP_PWR_OPTION                    S5P_PMUREG(0x2C48)
> >>> 
> >>> diff --git a/arch/arm/mach-exynos/platsmp.c
> >>> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
> >>> --- a/arch/arm/mach-exynos/platsmp.c
> >>> +++ b/arch/arm/mach-exynos/platsmp.c
> >>> @@ -109,14 +109,15 @@ static int __cpuinit
> >>> exynos_boot_secondary(unsigned int cpu, struct task_struct */
> >>> 
> >>>         write_pen_release(phys_cpu);
> >>> 
> >>> -       if (!(__raw_readl(S5P_ARM_CORE1_STATUS)& 
> >>> S5P_CORE_LOCAL_PWR_EN))>> 
> >> {
> >> 
> >>> +       if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
> >>> +       &  S5P_CORE_LOCAL_PWR_EN)) {
> >>> 
> >>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
> >>> 
> >>> -                            S5P_ARM_CORE1_CONFIGURATION);
> >>> +                            S5P_ARM_CORE_CONFIGURATION(phys_cpu));
> >>> 
> >>>                 timeout = 10;
> >>>                 
> >>>                 /* wait max 10 ms until cpu1 is on */
> >>> 
> >>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> >>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
> 
> Ditto
> 
> >>>                         &  S5P_CORE_LOCAL_PWR_EN) !=
> >>> 
> >>> S5P_CORE_LOCAL_PWR_EN)
> >> 
> >> {
> >> 
> >>>                         if (timeout-- == 0)
> >>>                         
> >>>                                 break;
> >>> 
> >>> @@ -125,7 +126,7 @@ static int __cpuinit
> >>> exynos_boot_secondary(unsigned
> >>> int cpu, struct task_struct }
> >>> 
> >>>                 if (timeout == 0) {
> >>> 
> >>> -                       printk(KERN_ERR "cpu1 power enable failed");
> >>> +                       printk(KERN_ERR "cpu%u power enable failed",
> >>> cpu);
> >>> 
> >>>                         spin_unlock(&boot_lock);
> >>>                         return -ETIMEDOUT;
> >>>                 
> >>>                 }
> >> 
> >> --
> > 
> > --
> > 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
> 
> --
> with warm regards,
> Chander Kashyap
> --
> 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

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Chander Kashyap]

On 19 June 2013 18:20, Tomasz Figa <t.figa@samsung.com> wrote:
> On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > On 06/19/13 02:45, Tomasz Figa wrote:
>> >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>> >>
>> >> Sorry for the noise.
>> >>
>> >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
>> >>> cores
>> >>> properly another registers must be used.
>> >>>
>> >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> >>> specified core.
>> >>>
>> >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>> >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> >>
>> >> Obviously this doesn't happen currently because of the if (cpu == 1),
>> >> but>
>> > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
>> > so this is available on exynos4412 and added 5420.
>> >
>> >> if logical cpu1 turned out not to be physical cpu1, then it would
>> >> crash.
>> >>
>> >> Best regards,
>> >> Tomasz
>> >>
>> >>> In addition,
>> >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
>> >>> off
>> >>> secondary cores by default.
>> >
>> > I need to test on board about above...
>> >
>> >>> Cc: stable@vger.kernel.org
>> >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>> >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> >>> ---
>> >>>
>> >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>> >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>> >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>> >>>   3 files changed, 17 insertions(+), 11 deletions(-)
>> >>>
>> >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> >>> --- a/arch/arm/mach-exynos/hotplug.c
>> >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>> >>>
>> >>>   static inline void platform_do_lowpower(unsigned int cpu, int
>> >>>
>> >>> *spurious) {
>> >>>
>> >>>         for (;;) {
>> >>>
>> >>> +               void __iomem *reg_base;
>> >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
>> >>>
>> >>> -               /* make cpu1 to be turned off at next WFI command */
>> >>> -               if (cpu == 1)
>> >>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> >>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>>
>> Tomasz,
>> This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then for
>> cpu0 phys_cpu value will be 0x100,
>> and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
>> ((0x101) * 0x80)), which is wrong.
>
> Hmm, according to the code initializing __cpu_logical_map[] array this is
> not true.
>
> Here's the code:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>
> and for used macros and bitmasks:
>
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>
> Now the structure of the MPIDR register:
>
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
>
> As you can see, the value read from the register in
> smp_setup_processor_id() is only the physical CPU ID, so I don't see any
> problem here.
Kindly look at implementation of "arm_dt_init_cpu_maps"  in
"arch/arm/kernel/devtree.c". When you pass the cpu nodes thru DT, and
cluster id is non zero, the phys_cpu will include clusterID. Say e.g
exynos4210 has CLusterID=9, hence phys_cpu value for cpu0 will be
0x901.
>
> If I'm wrong, feel free to correct me.
>
> Cc'ing people who should have more knowledge on this.
>
> Best regards,
> Tomasz
>
>> >>> +               __raw_writel(0, reg_base);
>> >>>
>> >>>                 /*
>> >>>
>> >>>                  * here's the WFI
>> >>>
>> >>> @@ -106,7 +107,7 @@ static inline void platform_do_lowpower(unsigned
>> >>> int
>> >>> cpu, int *spurious)
>> >>>
>> >>>                     : "memory", "cc");
>> >>>
>> >>> -               if (pen_release == cpu_logical_map(cpu)) {
>> >>> +               if (pen_release == phys_cpu) {
>> >>>
>> >>>                         /*
>> >>>
>> >>>                          * OK, proper wakeup, we're done
>> >>>                          */
>> >>>
>> >>> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> >>> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index 57344b7..cf40b86
>> >>> 100644
>> >>> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> >>> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
>> >>> @@ -125,10 +125,14 @@
>> >>>
>> >>>   #define S5P_GPS_ALIVE_LOWPWR                  S5P_PMUREG(0x13A0)
>> >>>
>> >>>   #define S5P_ARM_CORE0_CONFIGURATION           S5P_PMUREG(0x2000)
>> >>>
>> >>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>> >>>
>> >>>   #define S5P_ARM_CORE0_OPTION                  S5P_PMUREG(0x2008)
>> >>>
>> >>> -#define S5P_ARM_CORE1_CONFIGURATION            S5P_PMUREG(0x2080)
>> >>> -#define S5P_ARM_CORE1_STATUS                   S5P_PMUREG(0x2084)
>> >>> -#define S5P_ARM_CORE1_OPTION                   S5P_PMUREG(0x2088)
>> >>> +#define S5P_ARM_CORE_CONFIGURATION(_nr)                \
>> >>> +               (S5P_ARM_CORE0_CONFIGURATION + ((_nr) * 0x80))
>> >>> +#define S5P_ARM_CORE_STATUS(_nr)               \
>> >>> +               (S5P_ARM_CORE0_STATUS + ((_nr) * 0x80))
>> >>> +#define S5P_ARM_CORE_OPTION(_nr)               \
>> >>> +               (S5P_ARM_CORE0_OPTION + ((_nr) * 0x80))
>> >>>
>> >>>   #define S5P_ARM_COMMON_OPTION                 S5P_PMUREG(0x2408)
>> >>>   #define S5P_TOP_PWR_OPTION                    S5P_PMUREG(0x2C48)
>> >>>
>> >>> diff --git a/arch/arm/mach-exynos/platsmp.c
>> >>> b/arch/arm/mach-exynos/platsmp.c index d9c6d0a..2cbabc8 100644
>> >>> --- a/arch/arm/mach-exynos/platsmp.c
>> >>> +++ b/arch/arm/mach-exynos/platsmp.c
>> >>> @@ -109,14 +109,15 @@ static int __cpuinit
>> >>> exynos_boot_secondary(unsigned int cpu, struct task_struct */
>> >>>
>> >>>         write_pen_release(phys_cpu);
>> >>>
>> >>> -       if (!(__raw_readl(S5P_ARM_CORE1_STATUS)&
>> >>> S5P_CORE_LOCAL_PWR_EN))>>
>> >> {
>> >>
>> >>> +       if (!(__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>> >>> +       &  S5P_CORE_LOCAL_PWR_EN)) {
>> >>>
>> >>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> >>>
>> >>> -                            S5P_ARM_CORE1_CONFIGURATION);
>> >>> +                            S5P_ARM_CORE_CONFIGURATION(phys_cpu));
>> >>>
>> >>>                 timeout = 10;
>> >>>
>> >>>                 /* wait max 10 ms until cpu1 is on */
>> >>>
>> >>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> >>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(phys_cpu))
>>
>> Ditto
>>
>> >>>                         &  S5P_CORE_LOCAL_PWR_EN) !=
>> >>>
>> >>> S5P_CORE_LOCAL_PWR_EN)
>> >>
>> >> {
>> >>
>> >>>                         if (timeout-- == 0)
>> >>>
>> >>>                                 break;
>> >>>
>> >>> @@ -125,7 +126,7 @@ static int __cpuinit
>> >>> exynos_boot_secondary(unsigned
>> >>> int cpu, struct task_struct }
>> >>>
>> >>>                 if (timeout == 0) {
>> >>>
>> >>> -                       printk(KERN_ERR "cpu1 power enable failed");
>> >>> +                       printk(KERN_ERR "cpu%u power enable failed",
>> >>> cpu);
>> >>>
>> >>>                         spin_unlock(&boot_lock);
>> >>>                         return -ETIMEDOUT;
>> >>>
>> >>>                 }
>> >>
>> >> --
>> >
>> > --
>> > 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
>>
>> --
>> with warm regards,
>> Chander Kashyap
>> --
>> 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



--
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Lorenzo Pieralisi]

On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > > On 06/19/13 02:45, Tomasz Figa wrote:
> > >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
> > >> 
> > >> Sorry for the noise.
> > >> 
> > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
> > >>> cores
> > >>> properly another registers must be used.
> > >>> 
> > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> > >>> specified core.
> > >>> 
> > >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
> > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> > >> 
> > >> Obviously this doesn't happen currently because of the if (cpu == 1),
> > >> but> 
> > > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
> > > so this is available on exynos4412 and added 5420.
> > > 
> > >> if logical cpu1 turned out not to be physical cpu1, then it would
> > >> crash.
> > >> 
> > >> Best regards,
> > >> Tomasz
> > >> 
> > >>> In addition,
> > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
> > >>> off
> > >>> secondary cores by default.
> > > 
> > > I need to test on board about above...
> > > 
> > >>> Cc: stable@vger.kernel.org
> > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
> > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > >>> ---
> > >>> 
> > >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
> > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
> > >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
> > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
> > >>> 
> > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> > >>> --- a/arch/arm/mach-exynos/hotplug.c
> > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> > >>> 
> > >>>   static inline void platform_do_lowpower(unsigned int cpu, int
> > >>> 
> > >>> *spurious) {
> > >>> 
> > >>>         for (;;) {
> > >>> 
> > >>> +               void __iomem *reg_base;
> > >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
> > >>> 
> > >>> -               /* make cpu1 to be turned off at next WFI command */
> > >>> -               if (cpu == 1)
> > >>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> > >>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > 
> > Tomasz,
> > This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then for
> > cpu0 phys_cpu value will be 0x100,
> > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
> > ((0x101) * 0x80)), which is wrong.

Honestly, I did not understand the reasoning above, please clarify.

> 
> Hmm, according to the code initializing __cpu_logical_map[] array this is 
> not true.
> 
> Here's the code:
> 
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> 
> and for used macros and bitmasks:
> 
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> 
> Now the structure of the MPIDR register:
> 
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
> 
> As you can see, the value read from the register in 
> smp_setup_processor_id() is only the physical CPU ID, so I don't see any 
> problem here.

Define "physical CPU ID" :-)

There is a problem here: the MPIDR is not an index, and the cpu_logical_map is
populated in arm_dt_init_cpu_maps in:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6

with all affinity levels.

You need to perform a mapping between logical cpus and registers offset,
you can't use the cpu_logical_map directly for that.

Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED), where
cpu_logical_map is used erroneously as an index.

Lorenzo

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Chander Kashyap]

On 19 June 2013 18:54, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > > On 06/19/13 02:45, Tomasz Figa wrote:
>> > >> Ccing Arnd and Olof, because I forgot to add them to git send-email...
>> > >>
>> > >> Sorry for the noise.
>> > >>
>> > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
>> > >>> cores
>> > >>> properly another registers must be used.
>> > >>>
>> > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> > >>> specified core.
>> > >>>
>> > >>> This fixes CPU hotplug on quad core Exynos SoCs on which currently
>> > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> > >>
>> > >> Obviously this doesn't happen currently because of the if (cpu == 1),
>> > >> but>
>> > > Yes, not happened...and just note exynos5440 doesn't support hotplug :)
>> > > so this is available on exynos4412 and added 5420.
>> > >
>> > >> if logical cpu1 turned out not to be physical cpu1, then it would
>> > >> crash.
>> > >>
>> > >> Best regards,
>> > >> Tomasz
>> > >>
>> > >>> In addition,
>> > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader powers
>> > >>> off
>> > >>> secondary cores by default.
>> > >
>> > > I need to test on board about above...
>> > >
>> > >>> Cc: stable@vger.kernel.org
>> > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>> > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> > >>> ---
>> > >>>
>> > >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>> > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>> > >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>> > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
>> > >>>
>> > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>> > >>>
>> > >>>   static inline void platform_do_lowpower(unsigned int cpu, int
>> > >>>
>> > >>> *spurious) {
>> > >>>
>> > >>>         for (;;) {
>> > >>>
>> > >>> +               void __iomem *reg_base;
>> > >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
>> > >>>
>> > >>> -               /* make cpu1 to be turned off at next WFI command */
>> > >>> -               if (cpu == 1)
>> > >>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> > >>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >
>> > Tomasz,
>> > This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then for
>> > cpu0 phys_cpu value will be 0x100,
>> > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
>> > ((0x101) * 0x80)), which is wrong.
>
> Honestly, I did not understand the reasoning above, please clarify.
Sorry not non-zero MPIDR but non-zero ClusterID.
All the power register addresses are at offset 0x80.
>
>>
>> Hmm, according to the code initializing __cpu_logical_map[] array this is
>> not true.
>>
>> Here's the code:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>>
>> and for used macros and bitmasks:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/arch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>>
>> Now the structure of the MPIDR register:
>>
>> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CIHEBGFG.html
>>
>> As you can see, the value read from the register in
>> smp_setup_processor_id() is only the physical CPU ID, so I don't see any
>> problem here.
>
> Define "physical CPU ID" :-)
>
> There is a problem here: the MPIDR is not an index, and the cpu_logical_map is
> populated in arm_dt_init_cpu_maps in:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>
> with all affinity levels.
>
> You need to perform a mapping between logical cpus and registers offset,
> you can't use the cpu_logical_map directly for that.
>
> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED), where
> cpu_logical_map is used erroneously as an index.
>
> Lorenzo
>



--
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> > > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > > > On 06/19/13 02:45, Tomasz Figa wrote:
> > > >> Ccing Arnd and Olof, because I forgot to add them to git
> > > >> send-email...
> > > >> 
> > > >> Sorry for the noise.
> > > >> 
> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
> > > >>> cores
> > > >>> properly another registers must be used.
> > > >>> 
> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
> > > >>> specified core.
> > > >>> 
> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
> > > >>> currently
> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> > > >> 
> > > >> Obviously this doesn't happen currently because of the if (cpu ==
> > > >> 1),
> > > >> but>
> > > > 
> > > > Yes, not happened...and just note exynos5440 doesn't support
> > > > hotplug :)
> > > > so this is available on exynos4412 and added 5420.
> > > > 
> > > >> if logical cpu1 turned out not to be physical cpu1, then it would
> > > >> crash.
> > > >> 
> > > >> Best regards,
> > > >> Tomasz
> > > >> 
> > > >>> In addition,
> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
> > > >>> powers
> > > >>> off
> > > >>> secondary cores by default.
> > > > 
> > > > I need to test on board about above...
> > > > 
> > > >>> Cc: stable@vger.kernel.org
> > > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
> > > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> > > >>> ---
> > > >>> 
> > > >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
> > > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
> > > >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
> > > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
> > > >>> 
> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> > > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
> > > >>> 
> > > >>>   static inline void platform_do_lowpower(unsigned int cpu, int
> > > >>> 
> > > >>> *spurious) {
> > > >>> 
> > > >>>         for (;;) {
> > > >>> 
> > > >>> +               void __iomem *reg_base;
> > > >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
> > > >>> 
> > > >>> -               /* make cpu1 to be turned off at next WFI command
> > > >>> */
> > > >>> -               if (cpu == 1)
> > > >>> -                       __raw_writel(0,
> > > >>> S5P_ARM_CORE1_CONFIGURATION);
> > > >>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > > 
> > > Tomasz,
> > > This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then
> > > for
> > > cpu0 phys_cpu value will be 0x100,
> > > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
> > > ((0x101) * 0x80)), which is wrong.
> 
> Honestly, I did not understand the reasoning above, please clarify.
> 
> > Hmm, according to the code initializing __cpu_logical_map[] array this
> > is not true.
> > 
> > Here's the code:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> > 
> > and for used macros and bitmasks:
> > 
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> > 
> > Now the structure of the MPIDR register:
> > 
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CI
> > HEBGFG.html
> > 
> > As you can see, the value read from the register in
> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
> > any
> > problem here.
> 
> Define "physical CPU ID" :-)
> 
> There is a problem here: the MPIDR is not an index, and the
> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch
> /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
> 
> with all affinity levels.

OK. This is what I was missing. Thanks.

> 
> You need to perform a mapping between logical cpus and registers offset,
> you can't use the cpu_logical_map directly for that.

Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value and 
use them appropriately to calculate register offsets?

Best regards,
Tomasz

> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
> where cpu_logical_map is used erroneously as an index.
> 
> Lorenzo

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Chander Kashyap]

On 19 June 2013 19:19, Tomasz Figa <t.figa@samsung.com> wrote:
> On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
>> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> > > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > > > On 06/19/13 02:45, Tomasz Figa wrote:
>> > > >> Ccing Arnd and Olof, because I forgot to add them to git
>> > > >> send-email...
>> > > >>
>> > > >> Sorry for the noise.
>> > > >>
>> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control further
>> > > >>> cores
>> > > >>> properly another registers must be used.
>> > > >>>
>> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers for
>> > > >>> specified core.
>> > > >>>
>> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
>> > > >>> currently
>> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> > > >>
>> > > >> Obviously this doesn't happen currently because of the if (cpu ==
>> > > >> 1),
>> > > >> but>
>> > > >
>> > > > Yes, not happened...and just note exynos5440 doesn't support
>> > > > hotplug :)
>> > > > so this is available on exynos4412 and added 5420.
>> > > >
>> > > >> if logical cpu1 turned out not to be physical cpu1, then it would
>> > > >> crash.
>> > > >>
>> > > >> Best regards,
>> > > >> Tomasz
>> > > >>
>> > > >>> In addition,
>> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
>> > > >>> powers
>> > > >>> off
>> > > >>> secondary cores by default.
>> > > >
>> > > > I need to test on board about above...
>> > > >
>> > > >>> Cc: stable@vger.kernel.org
>> > > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>> > > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> > > >>> ---
>> > > >>>
>> > > >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>> > > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>> > > >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>> > > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
>> > > >>>
>> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> > > >>> @@ -93,10 +93,11 @@ static inline void cpu_leave_lowpower(void)
>> > > >>>
>> > > >>>   static inline void platform_do_lowpower(unsigned int cpu, int
>> > > >>>
>> > > >>> *spurious) {
>> > > >>>
>> > > >>>         for (;;) {
>> > > >>>
>> > > >>> +               void __iomem *reg_base;
>> > > >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
>> > > >>>
>> > > >>> -               /* make cpu1 to be turned off at next WFI command
>> > > >>> */
>> > > >>> -               if (cpu == 1)
>> > > >>> -                       __raw_writel(0,
>> > > >>> S5P_ARM_CORE1_CONFIGURATION);
>> > > >>> +               reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> > >
>> > > Tomasz,
>> > > This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then
>> > > for
>> > > cpu0 phys_cpu value will be 0x100,
>> > > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
>> > > ((0x101) * 0x80)), which is wrong.
>>
>> Honestly, I did not understand the reasoning above, please clarify.
>>
>> > Hmm, according to the code initializing __cpu_logical_map[] array this
>> > is not true.
>> >
>> > Here's the code:
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
>> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>> >
>> > and for used macros and bitmasks:
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/a
>> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>> >
>> > Now the structure of the MPIDR register:
>> >
>> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e/CI
>> > HEBGFG.html
>> >
>> > As you can see, the value read from the register in
>> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
>> > any
>> > problem here.
>>
>> Define "physical CPU ID" :-)
>>
>> There is a problem here: the MPIDR is not an index, and the
>> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch
>> /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>>
>> with all affinity levels.
>
> OK. This is what I was missing. Thanks.
>
>>
>> You need to perform a mapping between logical cpus and registers offset,
>> you can't use the cpu_logical_map directly for that.
>
> Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value and
> use them appropriately to calculate register offsets?
That will create problem for multi-cluster systems. Say we have two
clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
0x1/0x101 ans so on.

>
> Best regards,
> Tomasz
>
>> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
>> where cpu_logical_map is used erroneously as an index.
>>
>> Lorenzo



--
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
> On 19 June 2013 19:19, Tomasz Figa <t.figa@samsung.com> wrote:
> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> >> > > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
> >> > > >> send-email...
> >> > > >> 
> >> > > >> Sorry for the noise.
> >> > > >> 
> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
> >> > > >>> further
> >> > > >>> cores
> >> > > >>> properly another registers must be used.
> >> > > >>> 
> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
> >> > > >>> for
> >> > > >>> specified core.
> >> > > >>> 
> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
> >> > > >>> currently
> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> >> > > >> 
> >> > > >> Obviously this doesn't happen currently because of the if (cpu
> >> > > >> ==
> >> > > >> 1),
> >> > > >> but>
> >> > > > 
> >> > > > Yes, not happened...and just note exynos5440 doesn't support
> >> > > > hotplug :)
> >> > > > so this is available on exynos4412 and added 5420.
> >> > > > 
> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
> >> > > >> would
> >> > > >> crash.
> >> > > >> 
> >> > > >> Best regards,
> >> > > >> Tomasz
> >> > > >> 
> >> > > >>> In addition,
> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
> >> > > >>> powers
> >> > > >>> off
> >> > > >>> secondary cores by default.
> >> > > > 
> >> > > > I need to test on board about above...
> >> > > > 
> >> > > >>> Cc: stable@vger.kernel.org
> >> > > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
> >> > > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> > > >>> ---
> >> > > >>> 
> >> > > >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
> >> > > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
> >> > > >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
> >> > > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
> >> > > >>> 
> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> >> > > >>> @@ -93,10 +93,11 @@ static inline void
> >> > > >>> cpu_leave_lowpower(void)
> >> > > >>> 
> >> > > >>>   static inline void platform_do_lowpower(unsigned int cpu,
> >> > > >>>   int
> >> > > >>> 
> >> > > >>> *spurious) {
> >> > > >>> 
> >> > > >>>         for (;;) {
> >> > > >>> 
> >> > > >>> +               void __iomem *reg_base;
> >> > > >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
> >> > > >>> 
> >> > > >>> -               /* make cpu1 to be turned off at next WFI
> >> > > >>> command
> >> > > >>> */
> >> > > >>> -               if (cpu == 1)
> >> > > >>> -                       __raw_writel(0,
> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
> >> > > >>> +               reg_base =
> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> >> > > 
> >> > > Tomasz,
> >> > > This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then
> >> > > for
> >> > > cpu0 phys_cpu value will be 0x100,
> >> > > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
> >> > > ((0x101) * 0x80)), which is wrong.
> >> 
> >> Honestly, I did not understand the reasoning above, please clarify.
> >> 
> >> > Hmm, according to the code initializing __cpu_logical_map[] array
> >> > this
> >> > is not true.
> >> > 
> >> > Here's the code:
> >> > 
> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
> >> > e/a
> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> >> > 
> >> > and for used macros and bitmasks:
> >> > 
> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
> >> > e/a
> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> >> > 
> >> > Now the structure of the MPIDR register:
> >> > 
> >> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e
> >> > /CI
> >> > HEBGFG.html
> >> > 
> >> > As you can see, the value read from the register in
> >> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
> >> > any
> >> > problem here.
> >> 
> >> Define "physical CPU ID" :-)
> >> 
> >> There is a problem here: the MPIDR is not an index, and the
> >> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
> >> 
> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/a
> >> rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
> >> 
> >> with all affinity levels.
> > 
> > OK. This is what I was missing. Thanks.
> > 
> >> You need to perform a mapping between logical cpus and registers
> >> offset,
> >> you can't use the cpu_logical_map directly for that.
> > 
> > Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
> > and
> > use them appropriately to calculate register offsets?
> 
> That will create problem for multi-cluster systems. Say we have two
> clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
> 0x1/0x101 ans so on.

I mean, calculate register offset based on two parameters - cluster ID and 
CPU ID, like:

	...

	u32 mpidr = cpu_logical_map(cpu);
	u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);

	if (soc_is_exynosXXXX()) {
		u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);

		phys_cpu += EXYNOSXXXX_CPUS_PER_CLUSTER * cluster;
	}

	reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
	__raw_writel(0, reg_base);

	...

Best regards,
Tomasz

> > Best regards,
> > Tomasz
> > 
> >> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
> >> where cpu_logical_map is used erroneously as an index.
> >> 
> >> Lorenzo
> 
> --
> with warm regards,
> Chander Kashyap
> --
> 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

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Chander Kashyap]

On 19 June 2013 19:58, Tomasz Figa <t.figa@samsung.com> wrote:
> On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
>> On 19 June 2013 19:19, Tomasz Figa <t.figa@samsung.com> wrote:
>> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
>> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> >> > > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
>> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
>> >> > > >> send-email...
>> >> > > >>
>> >> > > >> Sorry for the noise.
>> >> > > >>
>> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
>> >> > > >>> further
>> >> > > >>> cores
>> >> > > >>> properly another registers must be used.
>> >> > > >>>
>> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions with
>> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
>> >> > > >>> for
>> >> > > >>> specified core.
>> >> > > >>>
>> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
>> >> > > >>> currently
>> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> >> > > >>
>> >> > > >> Obviously this doesn't happen currently because of the if (cpu
>> >> > > >> ==
>> >> > > >> 1),
>> >> > > >> but>
>> >> > > >
>> >> > > > Yes, not happened...and just note exynos5440 doesn't support
>> >> > > > hotplug :)
>> >> > > > so this is available on exynos4412 and added 5420.
>> >> > > >
>> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
>> >> > > >> would
>> >> > > >> crash.
>> >> > > >>
>> >> > > >> Best regards,
>> >> > > >> Tomasz
>> >> > > >>
>> >> > > >>> In addition,
>> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
>> >> > > >>> powers
>> >> > > >>> off
>> >> > > >>> secondary cores by default.
>> >> > > >
>> >> > > > I need to test on board about above...
>> >> > > >
>> >> > > >>> Cc: stable@vger.kernel.org
>> >> > > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>> >> > > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> >> > > >>> ---
>> >> > > >>>
>> >> > > >>>   arch/arm/mach-exynos/hotplug.c               |  9 +++++----
>> >> > > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10 +++++++---
>> >> > > >>>   arch/arm/mach-exynos/platsmp.c               |  9 +++++----
>> >> > > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
>> >> > > >>>
>> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943 100644
>> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> >> > > >>> @@ -93,10 +93,11 @@ static inline void
>> >> > > >>> cpu_leave_lowpower(void)
>> >> > > >>>
>> >> > > >>>   static inline void platform_do_lowpower(unsigned int cpu,
>> >> > > >>>   int
>> >> > > >>>
>> >> > > >>> *spurious) {
>> >> > > >>>
>> >> > > >>>         for (;;) {
>> >> > > >>>
>> >> > > >>> +               void __iomem *reg_base;
>> >> > > >>> +               unsigned int phys_cpu = cpu_logical_map(cpu);
>> >> > > >>>
>> >> > > >>> -               /* make cpu1 to be turned off at next WFI
>> >> > > >>> command
>> >> > > >>> */
>> >> > > >>> -               if (cpu == 1)
>> >> > > >>> -                       __raw_writel(0,
>> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
>> >> > > >>> +               reg_base =
>> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >> > >
>> >> > > Tomasz,
>> >> > > This will break for non-zero, MPIDR value.  Say if MPIDR is 1 then
>> >> > > for
>> >> > > cpu0 phys_cpu value will be 0x100,
>> >> > > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION +
>> >> > > ((0x101) * 0x80)), which is wrong.
>> >>
>> >> Honestly, I did not understand the reasoning above, please clarify.
>> >>
>> >> > Hmm, according to the code initializing __cpu_logical_map[] array
>> >> > this
>> >> > is not true.
>> >> >
>> >> > Here's the code:
>> >> >
>> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
>> >> > e/a
>> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>> >> >
>> >> > and for used macros and bitmasks:
>> >> >
>> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tre
>> >> > e/a
>> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>> >> >
>> >> > Now the structure of the MPIDR register:
>> >> >
>> >> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0388e
>> >> > /CI
>> >> > HEBGFG.html
>> >> >
>> >> > As you can see, the value read from the register in
>> >> > smp_setup_processor_id() is only the physical CPU ID, so I don't see
>> >> > any
>> >> > problem here.
>> >>
>> >> Define "physical CPU ID" :-)
>> >>
>> >> There is a problem here: the MPIDR is not an index, and the
>> >> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
>> >>
>> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/a
>> >> rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>> >>
>> >> with all affinity levels.
>> >
>> > OK. This is what I was missing. Thanks.
>> >
>> >> You need to perform a mapping between logical cpus and registers
>> >> offset,
>> >> you can't use the cpu_logical_map directly for that.
>> >
>> > Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
>> > and
>> > use them appropriately to calculate register offsets?
>>
>> That will create problem for multi-cluster systems. Say we have two
>> clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
>> 0x1/0x101 ans so on.
>
> I mean, calculate register offset based on two parameters - cluster ID and
> CPU ID, like:
>
>         ...
>
>         u32 mpidr = cpu_logical_map(cpu);
>         u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>
>         if (soc_is_exynosXXXX()) {
>                 u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>
>                 phys_cpu += EXYNOSXXXX_CPUS_PER_CLUSTER * cluster;
>         }
>
>         reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>         __raw_writel(0, reg_base);
>
This does not seems to viable solution, as eg. clusterID for
exynos4210 is 0x9 and exynos 4412 is 0xa. But if we wass the cpu nodes
thru DT, the we can comfortably rely on the logical cpu number. Also
EXYNOSXXXX_CPUS_PER_CLUSTER can vary from cluster to cluster.
>         ...
>
> Best regards,
> Tomasz
>
>> > Best regards,
>> > Tomasz
>> >
>> >> Next accident waiting to happen is GIC code (CONFIG_GIC_NON_BANKED),
>> >> where cpu_logical_map is used erroneously as an index.
>> >>
>> >> Lorenzo
>>
>> --
>> with warm regards,
>> Chander Kashyap
>> --
>> 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



--
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
> On 19 June 2013 19:58, Tomasz Figa <t.figa@samsung.com> wrote:
> > On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
> >> On 19 June 2013 19:19, Tomasz Figa <t.figa@samsung.com> wrote:
> >> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
> >> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
> >> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
> >> >> > > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com> 
wrote:
> >> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
> >> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
> >> >> > > >> send-email...
> >> >> > > >> 
> >> >> > > >> Sorry for the noise.
> >> >> > > >> 
> >> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
> >> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
> >> >> > > >>> further
> >> >> > > >>> cores
> >> >> > > >>> properly another registers must be used.
> >> >> > > >>> 
> >> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions
> >> >> > > >>> with
> >> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
> >> >> > > >>> for
> >> >> > > >>> specified core.
> >> >> > > >>> 
> >> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
> >> >> > > >>> currently
> >> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
> >> >> > > >> 
> >> >> > > >> Obviously this doesn't happen currently because of the if
> >> >> > > >> (cpu
> >> >> > > >> ==
> >> >> > > >> 1),
> >> >> > > >> but>
> >> >> > > > 
> >> >> > > > Yes, not happened...and just note exynos5440 doesn't support
> >> >> > > > hotplug :)
> >> >> > > > so this is available on exynos4412 and added 5420.
> >> >> > > > 
> >> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
> >> >> > > >> would
> >> >> > > >> crash.
> >> >> > > >> 
> >> >> > > >> Best regards,
> >> >> > > >> Tomasz
> >> >> > > >> 
> >> >> > > >>> In addition,
> >> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
> >> >> > > >>> powers
> >> >> > > >>> off
> >> >> > > >>> secondary cores by default.
> >> >> > > > 
> >> >> > > > I need to test on board about above...
> >> >> > > > 
> >> >> > > >>> Cc: stable@vger.kernel.org
> >> >> > > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
> >> >> > > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
> >> >> > > >>> ---
> >> >> > > >>> 
> >> >> > > >>>   arch/arm/mach-exynos/hotplug.c               |  9
> >> >> > > >>>   +++++----
> >> >> > > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10
> >> >> > > >>>   +++++++---
> >> >> > > >>>   arch/arm/mach-exynos/platsmp.c               |  9
> >> >> > > >>>   +++++----
> >> >> > > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
> >> >> > > >>> 
> >> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
> >> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943
> >> >> > > >>> 100644
> >> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
> >> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
> >> >> > > >>> @@ -93,10 +93,11 @@ static inline void
> >> >> > > >>> cpu_leave_lowpower(void)
> >> >> > > >>> 
> >> >> > > >>>   static inline void platform_do_lowpower(unsigned int cpu,
> >> >> > > >>>   int
> >> >> > > >>> 
> >> >> > > >>> *spurious) {
> >> >> > > >>> 
> >> >> > > >>>         for (;;) {
> >> >> > > >>> 
> >> >> > > >>> +               void __iomem *reg_base;
> >> >> > > >>> +               unsigned int phys_cpu =
> >> >> > > >>> cpu_logical_map(cpu);
> >> >> > > >>> 
> >> >> > > >>> -               /* make cpu1 to be turned off at next WFI
> >> >> > > >>> command
> >> >> > > >>> */
> >> >> > > >>> -               if (cpu == 1)
> >> >> > > >>> -                       __raw_writel(0,
> >> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
> >> >> > > >>> +               reg_base =
> >> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> >> >> > > 
> >> >> > > Tomasz,
> >> >> > > This will break for non-zero, MPIDR value.  Say if MPIDR is 1
> >> >> > > then
> >> >> > > for
> >> >> > > cpu0 phys_cpu value will be 0x100,
> >> >> > > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION
> >> >> > > +
> >> >> > > ((0x101) * 0x80)), which is wrong.
> >> >> 
> >> >> Honestly, I did not understand the reasoning above, please clarify.
> >> >> 
> >> >> > Hmm, according to the code initializing __cpu_logical_map[] array
> >> >> > this
> >> >> > is not true.
> >> >> > 
> >> >> > Here's the code:
> >> >> > 
> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
> >> >> > tre
> >> >> > e/a
> >> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
> >> >> > 
> >> >> > and for used macros and bitmasks:
> >> >> > 
> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
> >> >> > tre
> >> >> > e/a
> >> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
> >> >> > 
> >> >> > Now the structure of the MPIDR register:
> >> >> > 
> >> >> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi03
> >> >> > 88e
> >> >> > /CI
> >> >> > HEBGFG.html
> >> >> > 
> >> >> > As you can see, the value read from the register in
> >> >> > smp_setup_processor_id() is only the physical CPU ID, so I don't
> >> >> > see
> >> >> > any
> >> >> > problem here.
> >> >> 
> >> >> Define "physical CPU ID" :-)
> >> >> 
> >> >> There is a problem here: the MPIDR is not an index, and the
> >> >> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
> >> >> 
> >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tre
> >> >> e/a
> >> >> rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
> >> >> 
> >> >> with all affinity levels.
> >> > 
> >> > OK. This is what I was missing. Thanks.
> >> > 
> >> >> You need to perform a mapping between logical cpus and registers
> >> >> offset,
> >> >> you can't use the cpu_logical_map directly for that.
> >> > 
> >> > Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
> >> > and
> >> > use them appropriately to calculate register offsets?
> >> 
> >> That will create problem for multi-cluster systems. Say we have two
> >> clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
> >> 0x1/0x101 ans so on.
> > 
> > I mean, calculate register offset based on two parameters - cluster ID
> > and> 
> > CPU ID, like:
> >         ...
> >         
> >         u32 mpidr = cpu_logical_map(cpu);
> >         u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> >         
> >         if (soc_is_exynosXXXX()) {
> >         
> >                 u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> >                 
> >                 phys_cpu += EXYNOSXXXX_CPUS_PER_CLUSTER * cluster;
> >         
> >         }
> >         
> >         reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> >         __raw_writel(0, reg_base);
> 
> This does not seems to viable solution, as eg. clusterID for
> exynos4210 is 0x9 and exynos 4412 is 0xa.

We don't need to consider cluster ID for any SoC that has just one cluster. 
That's why there is the if (soc_is_exynosXXXX()) clause, where exynosXXXX 
is the SoC that we support and has more clusters.

> But if we wass the cpu nodes
> thru DT, the we can comfortably rely on the logical cpu number. Also
> EXYNOSXXXX_CPUS_PER_CLUSTER can vary from cluster to cluster.

There is nothing that prevents you from specifying the CPUs in DT in 
different order. Moreover, even if you specify them in correct order, there 
is nothing that prevents you from using any of the listed CPUs as boot CPU, 
which will get the logical ID of 0.

Best regards,
Tomasz

> >         ...
> > 
> > Best regards,
> > Tomasz
> > 
> >> > Best regards,
> >> > Tomasz
> >> > 
> >> >> Next accident waiting to happen is GIC code
> >> >> (CONFIG_GIC_NON_BANKED),
> >> >> where cpu_logical_map is used erroneously as an index.
> >> >> 
> >> >> Lorenzo
> >> 
> >> --
> >> with warm regards,
> >> Chander Kashyap
> >> --
> >> 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
> 
> --
> with warm regards,
> Chander Kashyap
> --
> 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

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Chander Kashyap]

On 19 June 2013 20:31, Tomasz Figa <t.figa@samsung.com> wrote:
> On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
>> On 19 June 2013 19:58, Tomasz Figa <t.figa@samsung.com> wrote:
>> > On Wednesday 19 of June 2013 19:25:27 Chander Kashyap wrote:
>> >> On 19 June 2013 19:19, Tomasz Figa <t.figa@samsung.com> wrote:
>> >> > On Wednesday 19 of June 2013 14:24:17 Lorenzo Pieralisi wrote:
>> >> >> On Wed, Jun 19, 2013 at 01:50:57PM +0100, Tomasz Figa wrote:
>> >> >> > On Wednesday 19 of June 2013 17:39:21 Chander Kashyap wrote:
>> >> >> > > On 18 June 2013 23:29, Kukjin Kim <kgene.kim@samsung.com>
> wrote:
>> >> >> > > > On 06/19/13 02:45, Tomasz Figa wrote:
>> >> >> > > >> Ccing Arnd and Olof, because I forgot to add them to git
>> >> >> > > >> send-email...
>> >> >> > > >>
>> >> >> > > >> Sorry for the noise.
>> >> >> > > >>
>> >> >> > > >> On Tuesday 18 of June 2013 17:26:31 Tomasz Figa wrote:
>> >> >> > > >>> S5P_ARM_CORE1_* registers affect only core 1. To control
>> >> >> > > >>> further
>> >> >> > > >>> cores
>> >> >> > > >>> properly another registers must be used.
>> >> >> > > >>>
>> >> >> > > >>> This patch replaces S5P_ARM_CORE1_* register definitions
>> >> >> > > >>> with
>> >> >> > > >>> S5P_ARM_CORE_*(x) macro which return addresses of registers
>> >> >> > > >>> for
>> >> >> > > >>> specified core.
>> >> >> > > >>>
>> >> >> > > >>> This fixes CPU hotplug on quad core Exynos SoCs on which
>> >> >> > > >>> currently
>> >> >> > > >>> offlining CPUs 2 or 3 caused CPU 1 to be turned off.
>> >> >> > > >>
>> >> >> > > >> Obviously this doesn't happen currently because of the if
>> >> >> > > >> (cpu
>> >> >> > > >> ==
>> >> >> > > >> 1),
>> >> >> > > >> but>
>> >> >> > > >
>> >> >> > > > Yes, not happened...and just note exynos5440 doesn't support
>> >> >> > > > hotplug :)
>> >> >> > > > so this is available on exynos4412 and added 5420.
>> >> >> > > >
>> >> >> > > >> if logical cpu1 turned out not to be physical cpu1, then it
>> >> >> > > >> would
>> >> >> > > >> crash.
>> >> >> > > >>
>> >> >> > > >> Best regards,
>> >> >> > > >> Tomasz
>> >> >> > > >>
>> >> >> > > >>> In addition,
>> >> >> > > >>> bring-up of CPU 2 and 3 is fixed on boards where bootloader
>> >> >> > > >>> powers
>> >> >> > > >>> off
>> >> >> > > >>> secondary cores by default.
>> >> >> > > >
>> >> >> > > > I need to test on board about above...
>> >> >> > > >
>> >> >> > > >>> Cc: stable@vger.kernel.org
>> >> >> > > >>> Signed-off-by: Tomasz Figa<t.figa@samsung.com>
>> >> >> > > >>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>> >> >> > > >>> ---
>> >> >> > > >>>
>> >> >> > > >>>   arch/arm/mach-exynos/hotplug.c               |  9
>> >> >> > > >>>   +++++----
>> >> >> > > >>>   arch/arm/mach-exynos/include/mach/regs-pmu.h | 10
>> >> >> > > >>>   +++++++---
>> >> >> > > >>>   arch/arm/mach-exynos/platsmp.c               |  9
>> >> >> > > >>>   +++++----
>> >> >> > > >>>   3 files changed, 17 insertions(+), 11 deletions(-)
>> >> >> > > >>>
>> >> >> > > >>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> >> >> > > >>> b/arch/arm/mach-exynos/hotplug.c index af90cfa..c089943
>> >> >> > > >>> 100644
>> >> >> > > >>> --- a/arch/arm/mach-exynos/hotplug.c
>> >> >> > > >>> +++ b/arch/arm/mach-exynos/hotplug.c
>> >> >> > > >>> @@ -93,10 +93,11 @@ static inline void
>> >> >> > > >>> cpu_leave_lowpower(void)
>> >> >> > > >>>
>> >> >> > > >>>   static inline void platform_do_lowpower(unsigned int cpu,
>> >> >> > > >>>   int
>> >> >> > > >>>
>> >> >> > > >>> *spurious) {
>> >> >> > > >>>
>> >> >> > > >>>         for (;;) {
>> >> >> > > >>>
>> >> >> > > >>> +               void __iomem *reg_base;
>> >> >> > > >>> +               unsigned int phys_cpu =
>> >> >> > > >>> cpu_logical_map(cpu);
>> >> >> > > >>>
>> >> >> > > >>> -               /* make cpu1 to be turned off at next WFI
>> >> >> > > >>> command
>> >> >> > > >>> */
>> >> >> > > >>> -               if (cpu == 1)
>> >> >> > > >>> -                       __raw_writel(0,
>> >> >> > > >>> S5P_ARM_CORE1_CONFIGURATION);
>> >> >> > > >>> +               reg_base =
>> >> >> > > >>> S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >> >> > >
>> >> >> > > Tomasz,
>> >> >> > > This will break for non-zero, MPIDR value.  Say if MPIDR is 1
>> >> >> > > then
>> >> >> > > for
>> >> >> > > cpu0 phys_cpu value will be 0x100,
>> >> >> > > and address calculation will be   (S5P_ARM_CORE0_CONFIGURATION
>> >> >> > > +
>> >> >> > > ((0x101) * 0x80)), which is wrong.
>> >> >>
>> >> >> Honestly, I did not understand the reasoning above, please clarify.
>> >> >>
>> >> >> > Hmm, according to the code initializing __cpu_logical_map[] array
>> >> >> > this
>> >> >> > is not true.
>> >> >> >
>> >> >> > Here's the code:
>> >> >> >
>> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
>> >> >> > tre
>> >> >> > e/a
>> >> >> > rch/arm/kernel/setup.c?id=refs/tags/next-20130619#n468
>> >> >> >
>> >> >> > and for used macros and bitmasks:
>> >> >> >
>> >> >> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/
>> >> >> > tre
>> >> >> > e/a
>> >> >> > rch/arm/include/asm/cputype.h?id=refs/tags/next-20130619#n45
>> >> >> >
>> >> >> > Now the structure of the MPIDR register:
>> >> >> >
>> >> >> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi03
>> >> >> > 88e
>> >> >> > /CI
>> >> >> > HEBGFG.html
>> >> >> >
>> >> >> > As you can see, the value read from the register in
>> >> >> > smp_setup_processor_id() is only the physical CPU ID, so I don't
>> >> >> > see
>> >> >> > any
>> >> >> > problem here.
>> >> >>
>> >> >> Define "physical CPU ID" :-)
>> >> >>
>> >> >> There is a problem here: the MPIDR is not an index, and the
>> >> >> cpu_logical_map is populated in arm_dt_init_cpu_maps in:
>> >> >>
>> >> >> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tre
>> >> >> e/a
>> >> >> rch /arm/kernel/devtree.c?id=refs/tags/v3.10-rc6
>> >> >>
>> >> >> with all affinity levels.
>> >> >
>> >> > OK. This is what I was missing. Thanks.
>> >> >
>> >> >> You need to perform a mapping between logical cpus and registers
>> >> >> offset,
>> >> >> you can't use the cpu_logical_map directly for that.
>> >> >
>> >> > Hmm, can't I just extract cluster ID and CPU ID from the MPIDR value
>> >> > and
>> >> > use them appropriately to calculate register offsets?
>> >>
>> >> That will create problem for multi-cluster systems. Say we have two
>> >> clusters then with clusterID 0 and 1. So phys-cpu will be 0x0/0x100,
>> >> 0x1/0x101 ans so on.
>> >
>> > I mean, calculate register offset based on two parameters - cluster ID
>> > and>
>> > CPU ID, like:
>> >         ...
>> >
>> >         u32 mpidr = cpu_logical_map(cpu);
>> >         u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> >
>> >         if (soc_is_exynosXXXX()) {
>> >
>> >                 u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> >
>> >                 phys_cpu += EXYNOSXXXX_CPUS_PER_CLUSTER * cluster;
>> >
>> >         }
>> >
>> >         reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
>> >         __raw_writel(0, reg_base);
>>
>> This does not seems to viable solution, as eg. clusterID for
>> exynos4210 is 0x9 and exynos 4412 is 0xa.
>
> We don't need to consider cluster ID for any SoC that has just one cluster.
> That's why there is the if (soc_is_exynosXXXX()) clause, where exynosXXXX
> is the SoC that we support and has more clusters.
>
>> But if we wass the cpu nodes
>> thru DT, the we can comfortably rely on the logical cpu number. Also
>> EXYNOSXXXX_CPUS_PER_CLUSTER can vary from cluster to cluster.
>
> There is nothing that prevents you from specifying the CPUs in DT in
> different order. Moreover, even if you specify them in correct order, there
> is nothing that prevents you from using any of the listed CPUs as boot CPU,
> which will get the logical ID of 0.
Ah Sorry I missed this point.
Then either pass the power register address thru dt or do the way you described.
Thanks
>
> Best regards,
> Tomasz
>
>> >         ...
>> >
>> > Best regards,
>> > Tomasz
>> >
>> >> > Best regards,
>> >> > Tomasz
>> >> >
>> >> >> Next accident waiting to happen is GIC code
>> >> >> (CONFIG_GIC_NON_BANKED),
>> >> >> where cpu_logical_map is used erroneously as an index.
>> >> >>
>> >> >> Lorenzo
>> >>
>> >> --
>> >> with warm regards,
>> >> Chander Kashyap
>> >> --
>> >> 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
>>
>> --
>> with warm regards,
>> Chander Kashyap
>> --
>> 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



--
with warm regards,
Chander Kashyap

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Nicolas Pitre]

On Wed, 19 Jun 2013, Tomasz Figa wrote:
> On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
> > On 19 June 2013 19:58, Tomasz Figa <t.figa@samsung.com> wrote:
> > > I mean, calculate register offset based on two parameters - cluster ID
> > > and> 
> > > CPU ID, like:
> > >         ...
> > >         
> > >         u32 mpidr = cpu_logical_map(cpu);
> > >         u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > >         
> > >         if (soc_is_exynosXXXX()) {
> > >         
> > >                 u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > >                 
> > >                 phys_cpu += EXYNOSXXXX_CPUS_PER_CLUSTER * cluster;
> > >         
> > >         }
> > >         
> > >         reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > >         __raw_writel(0, reg_base);
> > 
> > This does not seems to viable solution, as eg. clusterID for
> > exynos4210 is 0x9 and exynos 4412 is 0xa.
> 
> We don't need to consider cluster ID for any SoC that has just one cluster. 
> That's why there is the if (soc_is_exynosXXXX()) clause, where exynosXXXX 
> is the SoC that we support and has more clusters.
> 
> > But if we wass the cpu nodes
> > thru DT, the we can comfortably rely on the logical cpu number. Also
> > EXYNOSXXXX_CPUS_PER_CLUSTER can vary from cluster to cluster.
> 
> There is nothing that prevents you from specifying the CPUs in DT in 
> different order. Moreover, even if you specify them in correct order, there 
> is nothing that prevents you from using any of the listed CPUs as boot CPU, 
> which will get the logical ID of 0.

Relying on the logical CPU number to index into hardware related 
register space is wrong, please don't do that.

If the MPIDR allocation isn't linear then this cannot be used either.

The best solution is probably to add this reg_base as a property of each 
CPU node in DT, and extract it at boot time to stash it into an array 
which can be indexed with the logical CPU number afterwards.


Nicolas

Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers

[Tomasz Figa]

On Wednesday 19 of June 2013 11:19:30 Nicolas Pitre wrote:
> On Wed, 19 Jun 2013, Tomasz Figa wrote:
> > On Wednesday 19 of June 2013 20:26:50 Chander Kashyap wrote:
> > > On 19 June 2013 19:58, Tomasz Figa <t.figa@samsung.com> wrote:
> > > > I mean, calculate register offset based on two parameters - cluster
> > > > ID
> > > > and>
> > > > 
> > > > CPU ID, like:
> > > >         ...
> > > >         
> > > >         u32 mpidr = cpu_logical_map(cpu);
> > > >         u32 phys_cpu = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > > >         
> > > >         if (soc_is_exynosXXXX()) {
> > > >         
> > > >                 u32 cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > > >                 
> > > >                 phys_cpu += EXYNOSXXXX_CPUS_PER_CLUSTER * cluster;
> > > >         
> > > >         }
> > > >         
> > > >         reg_base = S5P_ARM_CORE_CONFIGURATION(phys_cpu);
> > > >         __raw_writel(0, reg_base);
> > > 
> > > This does not seems to viable solution, as eg. clusterID for
> > > exynos4210 is 0x9 and exynos 4412 is 0xa.
> > 
> > We don't need to consider cluster ID for any SoC that has just one
> > cluster. That's why there is the if (soc_is_exynosXXXX()) clause,
> > where exynosXXXX is the SoC that we support and has more clusters.
> > 
> > > But if we wass the cpu nodes
> > > thru DT, the we can comfortably rely on the logical cpu number. Also
> > > EXYNOSXXXX_CPUS_PER_CLUSTER can vary from cluster to cluster.
> > 
> > There is nothing that prevents you from specifying the CPUs in DT in
> > different order. Moreover, even if you specify them in correct order,
> > there is nothing that prevents you from using any of the listed CPUs
> > as boot CPU, which will get the logical ID of 0.
> 
> Relying on the logical CPU number to index into hardware related
> register space is wrong, please don't do that.
> 
> If the MPIDR allocation isn't linear then this cannot be used either.
> 
> The best solution is probably to add this reg_base as a property of each
> CPU node in DT, and extract it at boot time to stash it into an array
> which can be indexed with the logical CPU number afterwards.

Currently we don't specify the cpu node in DT at all for Exynos 4210, 4212 
and 4412. It's sure that their device tree sources need to be modified to 
correct his, but since DT is considered an ABI, we should keep 
compatibility with old device tree blobs. 

This means that we can't strictly require this property to be present in 
device tree, because it will break existing boards with older device trees. 
So my suggestion is to user cluster ID and CPU ID to calculate the offset by 
default and add possibility to override it with a property in device tree 
if such need shows up.

What do you think?

Best regards,
Tomasz

> Nicolas
> --
> 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