* [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
@ 2013-06-18 15:26 Tomasz Figa
2013-06-18 17:45 ` Tomasz Figa
0 siblings, 1 reply; 16+ messages in thread
From: Tomasz Figa @ 2013-06-18 15:26 UTC (permalink / raw)
To: linux-samsung-soc
Cc: linux-arm-kernel, Kukjin Kim, Russell King - ARM Linux,
Jingoo Han, Jonghwan Choi, Abhilash Kesavan, linux-kernel,
Tomasz Figa, stable, Kyungmin Park
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-18 15:26 [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers Tomasz Figa
@ 2013-06-18 17:45 ` Tomasz Figa
2013-06-18 17:59 ` Kukjin Kim
0 siblings, 1 reply; 16+ messages in thread
From: Tomasz Figa @ 2013-06-18 17:45 UTC (permalink / raw)
To: Tomasz Figa
Cc: linux-samsung-soc, linux-arm-kernel, Kukjin Kim,
Russell King - ARM Linux, Jingoo Han, Jonghwan Choi,
Abhilash Kesavan, linux-kernel, stable, Kyungmin Park, arnd,
olof
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;
> }
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-18 17:45 ` Tomasz Figa
@ 2013-06-18 17:59 ` Kukjin Kim
2013-06-19 12:09 ` Chander Kashyap
0 siblings, 1 reply; 16+ messages in thread
From: Kukjin Kim @ 2013-06-18 17:59 UTC (permalink / raw)
To: Tomasz Figa
Cc: Tomasz Figa, linux-samsung-soc, linux-arm-kernel, Kukjin Kim,
Russell King - ARM Linux, Jingoo Han, Jonghwan Choi,
Abhilash Kesavan, linux-kernel, stable, Kyungmin Park, arnd,
olof
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;
>> }
> --
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-18 17:59 ` Kukjin Kim
@ 2013-06-19 12:09 ` Chander Kashyap
2013-06-19 12:50 ` Tomasz Figa
0 siblings, 1 reply; 16+ messages in thread
From: Chander Kashyap @ 2013-06-19 12:09 UTC (permalink / raw)
To: Kukjin Kim
Cc: Tomasz Figa, Tomasz Figa, linux-samsung-soc, linux-arm-kernel,
Russell King - ARM Linux, Jingoo Han, Jonghwan Choi,
Abhilash Kesavan, linux-kernel, stable, Kyungmin Park,
Arnd Bergmann, Olof Johansson
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 12:09 ` Chander Kashyap
@ 2013-06-19 12:50 ` Tomasz Figa
2013-06-19 13:24 ` Chander Kashyap
2013-06-19 13:24 ` Lorenzo Pieralisi
0 siblings, 2 replies; 16+ messages in thread
From: Tomasz Figa @ 2013-06-19 12:50 UTC (permalink / raw)
To: Chander Kashyap
Cc: Kukjin Kim, Tomasz Figa, linux-samsung-soc, linux-arm-kernel,
Russell King - ARM Linux, Jingoo Han, Jonghwan Choi,
Abhilash Kesavan, linux-kernel, stable, Kyungmin Park,
Arnd Bergmann, Olof Johansson, Nicolas Pitre, Will Deacon,
Lorenzo Pieralisi, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 12:50 ` Tomasz Figa
@ 2013-06-19 13:24 ` Chander Kashyap
2013-06-19 13:24 ` Lorenzo Pieralisi
1 sibling, 0 replies; 16+ messages in thread
From: Chander Kashyap @ 2013-06-19 13:24 UTC (permalink / raw)
To: Tomasz Figa
Cc: Kukjin Kim, Tomasz Figa, linux-samsung-soc, linux-arm-kernel,
Russell King - ARM Linux, Jingoo Han, Jonghwan Choi,
Abhilash Kesavan, linux-kernel, stable, Kyungmin Park,
Arnd Bergmann, Olof Johansson, Nicolas Pitre, Will Deacon,
Lorenzo Pieralisi, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 12:50 ` Tomasz Figa
2013-06-19 13:24 ` Chander Kashyap
@ 2013-06-19 13:24 ` Lorenzo Pieralisi
2013-06-19 13:31 ` Chander Kashyap
2013-06-19 13:49 ` Tomasz Figa
1 sibling, 2 replies; 16+ messages in thread
From: Lorenzo Pieralisi @ 2013-06-19 13:24 UTC (permalink / raw)
To: Tomasz Figa
Cc: Chander Kashyap, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 13:24 ` Lorenzo Pieralisi
@ 2013-06-19 13:31 ` Chander Kashyap
2013-06-19 13:49 ` Tomasz Figa
1 sibling, 0 replies; 16+ messages in thread
From: Chander Kashyap @ 2013-06-19 13:31 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Tomasz Figa, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 13:24 ` Lorenzo Pieralisi
2013-06-19 13:31 ` Chander Kashyap
@ 2013-06-19 13:49 ` Tomasz Figa
2013-06-19 13:55 ` Chander Kashyap
1 sibling, 1 reply; 16+ messages in thread
From: Tomasz Figa @ 2013-06-19 13:49 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Chander Kashyap, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 13:49 ` Tomasz Figa
@ 2013-06-19 13:55 ` Chander Kashyap
2013-06-19 14:28 ` Tomasz Figa
0 siblings, 1 reply; 16+ messages in thread
From: Chander Kashyap @ 2013-06-19 13:55 UTC (permalink / raw)
To: Tomasz Figa
Cc: Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 13:55 ` Chander Kashyap
@ 2013-06-19 14:28 ` Tomasz Figa
2013-06-19 14:56 ` Chander Kashyap
0 siblings, 1 reply; 16+ messages in thread
From: Tomasz Figa @ 2013-06-19 14:28 UTC (permalink / raw)
To: Chander Kashyap
Cc: Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 14:28 ` Tomasz Figa
@ 2013-06-19 14:56 ` Chander Kashyap
2013-06-19 15:01 ` Tomasz Figa
0 siblings, 1 reply; 16+ messages in thread
From: Chander Kashyap @ 2013-06-19 14:56 UTC (permalink / raw)
To: Tomasz Figa
Cc: Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 14:56 ` Chander Kashyap
@ 2013-06-19 15:01 ` Tomasz Figa
2013-06-19 15:07 ` Chander Kashyap
2013-06-19 15:19 ` Nicolas Pitre
0 siblings, 2 replies; 16+ messages in thread
From: Tomasz Figa @ 2013-06-19 15:01 UTC (permalink / raw)
To: Chander Kashyap
Cc: Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 15:01 ` Tomasz Figa
@ 2013-06-19 15:07 ` Chander Kashyap
2013-06-19 15:19 ` Nicolas Pitre
1 sibling, 0 replies; 16+ messages in thread
From: Chander Kashyap @ 2013-06-19 15:07 UTC (permalink / raw)
To: Tomasz Figa
Cc: Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa, linux-samsung-soc,
linux-arm-kernel, Russell King - ARM Linux, Jingoo Han,
Jonghwan Choi, Abhilash Kesavan, linux-kernel, stable,
Kyungmin Park, Arnd Bergmann, Olof Johansson, Nicolas Pitre,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 15:01 ` Tomasz Figa
2013-06-19 15:07 ` Chander Kashyap
@ 2013-06-19 15:19 ` Nicolas Pitre
2013-06-20 10:16 ` Tomasz Figa
1 sibling, 1 reply; 16+ messages in thread
From: Nicolas Pitre @ 2013-06-19 15:19 UTC (permalink / raw)
To: Tomasz Figa
Cc: Chander Kashyap, Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa,
linux-samsung-soc, linux-arm-kernel, Russell King - ARM Linux,
Jingoo Han, Jonghwan Choi, Abhilash Kesavan, linux-kernel,
stable, Kyungmin Park, Arnd Bergmann, Olof Johansson,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers
2013-06-19 15:19 ` Nicolas Pitre
@ 2013-06-20 10:16 ` Tomasz Figa
0 siblings, 0 replies; 16+ messages in thread
From: Tomasz Figa @ 2013-06-20 10:16 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Chander Kashyap, Lorenzo Pieralisi, Kukjin Kim, Tomasz Figa,
linux-samsung-soc, linux-arm-kernel, Russell King - ARM Linux,
Jingoo Han, Jonghwan Choi, Abhilash Kesavan, linux-kernel,
stable, Kyungmin Park, Arnd Bergmann, Olof Johansson,
Will Deacon, Stephen Boyd
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
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-06-20 10:17 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-18 15:26 [PATCH] ARM: EXYNOS: Fix incorrect usage of S5P_ARM_CORE1_* registers Tomasz Figa
2013-06-18 17:45 ` Tomasz Figa
2013-06-18 17:59 ` Kukjin Kim
2013-06-19 12:09 ` Chander Kashyap
2013-06-19 12:50 ` Tomasz Figa
2013-06-19 13:24 ` Chander Kashyap
2013-06-19 13:24 ` Lorenzo Pieralisi
2013-06-19 13:31 ` Chander Kashyap
2013-06-19 13:49 ` Tomasz Figa
2013-06-19 13:55 ` Chander Kashyap
2013-06-19 14:28 ` Tomasz Figa
2013-06-19 14:56 ` Chander Kashyap
2013-06-19 15:01 ` Tomasz Figa
2013-06-19 15:07 ` Chander Kashyap
2013-06-19 15:19 ` Nicolas Pitre
2013-06-20 10:16 ` Tomasz Figa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).