[RFC] ARM: EXYNOS: reset KFC cores when cpu is up

Commit Message

Chanho Park July 15, 2015, 1:36 a.m. UTC

The cpu booting of exynos5422 has been still broken since we discussed
it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
hardkernel, it could help to boot 8 cores well. This patch need to have
more test like STR and other SoC especially exynos5800 which is variant
of exynos5422. If this patch is broken on exynos5800, I'll find another
way to check exynos5422.

This patch is top of my previous exynos5422 cpu ordering patch[2] and
need to enable CONFIG_EXYNOS5420_MCPM=y

[    0.047509] CPU0: update cpu_capacity 448
[    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
[    0.047874] Setting up static identity map for 0x400082c0 -
0x40008318
[    0.048340] ARM CCI driver probed
[    0.048597] Exynos MCPM support installed
[    0.065676] CPU1: update cpu_capacity 448
[    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
[    0.070672] CPU2: update cpu_capacity 448
[    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
[    0.075644] CPU3: update cpu_capacity 448
[    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
[    0.080590] CPU4: update cpu_capacity 1535
[    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.085591] CPU5: update cpu_capacity 1535
[    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
[    0.090590] CPU6: update cpu_capacity 1535
[    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
[    0.095585] CPU7: update cpu_capacity 1535
[    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
[    0.095720] Brought up 8 CPUs

[1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
[2]:https://patchwork.kernel.org/patch/6782891/

Cc: Joonyoung Shim <[e-mail]>
Cc: Chanwoo Choi <[e-mail]>
Cc: Kevin Hilman <[e-mail]>
Cc: Heesub Shin <[e-mail]>
Cc: Mauro Ribeiro <[e-mail]>
Cc: Abhilash Kesavan <[e-mail]>
Cc: Przemyslaw Marczak <[e-mail]>
Cc: Marek Szyprowski <[e-mail]>
Cc: Krzysztof Kozlowski <[e-mail]>
Signed-off-by: Chanho Park <[e-mail]>
Signed-off-by: Chanho Park <[e-mail]>
---
 arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
 arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

Comments

Krzysztof Kozlowski July 15, 2015, 1:46 a.m. UTC | #1

On 15.07.2015 10:36, Chanho Park wrote:
> The cpu booting of exynos5422 has been still broken since we discussed
> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
> hardkernel, it could help to boot 8 cores well. This patch need to have
> more test like STR and other SoC especially exynos5800 which is variant
> of exynos5422. If this patch is broken on exynos5800, I'll find another
> way to check exynos5422.
> 
> This patch is top of my previous exynos5422 cpu ordering patch[2] and
> need to enable CONFIG_EXYNOS5420_MCPM=y

1. Why the ordering patch is required? It seems unrelated...
2. I tried this already (as in vendor sources from
opensource.samsung.com) and it did not help on my Odroid. Are you sure
that this patch solely fixes the problem?

Best regards,
Krzysztof

> 
> [    0.047509] CPU0: update cpu_capacity 448
> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
> [    0.047874] Setting up static identity map for 0x400082c0 -
> 0x40008318
> [    0.048340] ARM CCI driver probed
> [    0.048597] Exynos MCPM support installed
> [    0.065676] CPU1: update cpu_capacity 448
> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
> [    0.070672] CPU2: update cpu_capacity 448
> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
> [    0.075644] CPU3: update cpu_capacity 448
> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
> [    0.080590] CPU4: update cpu_capacity 1535
> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.085591] CPU5: update cpu_capacity 1535
> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
> [    0.090590] CPU6: update cpu_capacity 1535
> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
> [    0.095585] CPU7: update cpu_capacity 1535
> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
> [    0.095720] Brought up 8 CPUs
> 
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
> [2]:https://patchwork.kernel.org/patch/6782891/
> 
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Chanwoo Choi <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Heesub Shin <[e-mail]>
> Cc: Mauro Ribeiro <[e-mail]>
> Cc: Abhilash Kesavan <[e-mail]>
> Cc: Przemyslaw Marczak <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 9bdf547..a076dde 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>  		cluster >= EXYNOS5420_NR_CLUSTERS)
>  		return -EINVAL;
> 
> -	exynos_cpu_power_up(cpunr);
> +	if (!exynos_cpu_power_state(cpunr)) {
> +		exynos_cpu_power_up(cpunr);
> +
> +		if (soc_is_exynos5800() && cluster) {
> +			while (!pmu_raw_readl(S5P_PMU_SPARE2))
> +				udelay(10);
> +
> +			pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
> +					EXYNOS_SWRESET);
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index b761433..fba9068 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -513,6 +513,12 @@ static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
>  #define SPREAD_ENABLE						0xF
>  #define SPREAD_USE_STANDWFI					0xF
>  
> +#define EXYNOS5420_KFC_CORE_RESET0				BIT(8)
> +#define EXYNOS5420_KFC_ETM_RESET0				BIT(20)
> +
> +#define EXYNOS5420_KFC_CORE_RESET(_nr)				\
> +	((EXYNOS5420_KFC_CORE_RESET0 | EXYNOS5420_KFC_ETM_RESET0) << (_nr))
> +
>  #define EXYNOS5420_BB_CON1					0x0784
>  #define EXYNOS5420_BB_SEL_EN					BIT(31)
>  #define EXYNOS5420_BB_PMOS_EN					BIT(7)
>

Chanho Park July 15, 2015, 2:31 a.m. UTC | #2

Hi,

On Wed, Jul 15, 2015 at 10:46 AM, Krzysztof Kozlowski
<[e-mail]> wrote:
> On 15.07.2015 10:36, Chanho Park wrote:
>> The cpu booting of exynos5422 has been still broken since we discussed
>> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
>> hardkernel, it could help to boot 8 cores well. This patch need to have
>> more test like STR and other SoC especially exynos5800 which is variant
>> of exynos5422. If this patch is broken on exynos5800, I'll find another
>> way to check exynos5422.
>>
>> This patch is top of my previous exynos5422 cpu ordering patch[2] and
>> need to enable CONFIG_EXYNOS5420_MCPM=y
>
> 1. Why the ordering patch is required? It seems unrelated...

Because it related with my booting log :)

> 2. I tried this already (as in vendor sources from
> opensource.samsung.com) and it did not help on my Odroid. Are you sure
> that this patch solely fixes the problem?

If you turned on CONFIG_DEBUG_LL, you'll need Joonyoung's patch to
correct uart address of exynos5422.
http://www.spinics.net/lists/linux-samsung-soc/msg37318.html

Krzysztof Kozlowski July 15, 2015, 3:42 a.m. UTC | #3

On 15.07.2015 11:31, Chanho Park wrote:
> Hi,
> 
> On Wed, Jul 15, 2015 at 10:46 AM, Krzysztof Kozlowski
> <[e-mail]> wrote:
>> On 15.07.2015 10:36, Chanho Park wrote:
>>> The cpu booting of exynos5422 has been still broken since we discussed
>>> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
>>> hardkernel, it could help to boot 8 cores well. This patch need to have
>>> more test like STR and other SoC especially exynos5800 which is variant
>>> of exynos5422. If this patch is broken on exynos5800, I'll find another
>>> way to check exynos5422.
>>>
>>> This patch is top of my previous exynos5422 cpu ordering patch[2] and
>>> need to enable CONFIG_EXYNOS5420_MCPM=y
>>
>> 1. Why the ordering patch is required? It seems unrelated...
> 
> Because it related with my booting log :)
> 
>> 2. I tried this already (as in vendor sources from
>> opensource.samsung.com) and it did not help on my Odroid. Are you sure
>> that this patch solely fixes the problem?
> 
> If you turned on CONFIG_DEBUG_LL, you'll need Joonyoung's patch to
> correct uart address of exynos5422.
> http://www.spinics.net/lists/linux-samsung-soc/msg37318.html

I mean that you don't need anything else to fix the problem? I'll give
it a try later on my board but I was quite sure that I checked this
before :) .

Best regards,
Krzysztof

Anand Moon July 15, 2015, 5:20 a.m. UTC | #4

Hi Chanho,

On 15 July 2015 at 07:06, Chanho Park <[e-mail]> wrote:
> The cpu booting of exynos5422 has been still broken since we discussed
> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
> hardkernel, it could help to boot 8 cores well. This patch need to have
> more test like STR and other SoC especially exynos5800 which is variant
> of exynos5422. If this patch is broken on exynos5800, I'll find another
> way to check exynos5422.
>
> This patch is top of my previous exynos5422 cpu ordering patch[2] and
> need to enable CONFIG_EXYNOS5420_MCPM=y
>
> [    0.047509] CPU0: update cpu_capacity 448
> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
> [    0.047874] Setting up static identity map for 0x400082c0 -
> 0x40008318
> [    0.048340] ARM CCI driver probed
> [    0.048597] Exynos MCPM support installed
> [    0.065676] CPU1: update cpu_capacity 448
> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
> [    0.070672] CPU2: update cpu_capacity 448
> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
> [    0.075644] CPU3: update cpu_capacity 448
> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
> [    0.080590] CPU4: update cpu_capacity 1535
> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.085591] CPU5: update cpu_capacity 1535
> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
> [    0.090590] CPU6: update cpu_capacity 1535
> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
> [    0.095585] CPU7: update cpu_capacity 1535
> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
> [    0.095720] Brought up 8 CPUs
>
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
> [2]:https://patchwork.kernel.org/patch/6782891/
>
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Chanwoo Choi <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Heesub Shin <[e-mail]>
> Cc: Mauro Ribeiro <[e-mail]>
> Cc: Abhilash Kesavan <[e-mail]>
> Cc: Przemyslaw Marczak <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 9bdf547..a076dde 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>                 cluster >= EXYNOS5420_NR_CLUSTERS)
>                 return -EINVAL;
>
> -       exynos_cpu_power_up(cpunr);
> +       if (!exynos_cpu_power_state(cpunr)) {
> +               exynos_cpu_power_up(cpunr);
> +
> +               if (soc_is_exynos5800() && cluster) {
> +                       while (!pmu_raw_readl(S5P_PMU_SPARE2))
> +                               udelay(10);
> +
> +                       pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
> +                                       EXYNOS_SWRESET);
> +               }
> +       }
> +
>         return 0;
>  }
>
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index b761433..fba9068 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -513,6 +513,12 @@ static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
>  #define SPREAD_ENABLE                                          0xF
>  #define SPREAD_USE_STANDWFI                                    0xF
>
> +#define EXYNOS5420_KFC_CORE_RESET0                             BIT(8)
> +#define EXYNOS5420_KFC_ETM_RESET0                              BIT(20)
> +
> +#define EXYNOS5420_KFC_CORE_RESET(_nr)                         \
> +       ((EXYNOS5420_KFC_CORE_RESET0 | EXYNOS5420_KFC_ETM_RESET0) << (_nr))
> +
>  #define EXYNOS5420_BB_CON1                                     0x0784
>  #define EXYNOS5420_BB_SEL_EN                                   BIT(31)
>  #define EXYNOS5420_BB_PMOS_EN                                  BIT(7)
> --
> 2.1.0
>

It brought all the 8 core up on my Odroid-XU3

Tested-by: Anand Moon <[e-mail]>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Javier Martinez Canillas July 15, 2015, 8:24 a.m. UTC | #5

Hello Chanho,

On 07/15/2015 03:36 AM, Chanho Park wrote:
> The cpu booting of exynos5422 has been still broken since we discussed
> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
> hardkernel, it could help to boot 8 cores well. This patch need to have
> more test like STR and other SoC especially exynos5800 which is variant
> of exynos5422. If this patch is broken on exynos5800, I'll find another
> way to check exynos5422.
> 
> This patch is top of my previous exynos5422 cpu ordering patch[2] and
> need to enable CONFIG_EXYNOS5420_MCPM=y
> 
> [    0.047509] CPU0: update cpu_capacity 448
> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
> [    0.047874] Setting up static identity map for 0x400082c0 -
> 0x40008318
> [    0.048340] ARM CCI driver probed
> [    0.048597] Exynos MCPM support installed
> [    0.065676] CPU1: update cpu_capacity 448
> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
> [    0.070672] CPU2: update cpu_capacity 448
> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
> [    0.075644] CPU3: update cpu_capacity 448
> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
> [    0.080590] CPU4: update cpu_capacity 1535
> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.085591] CPU5: update cpu_capacity 1535
> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
> [    0.090590] CPU6: update cpu_capacity 1535
> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
> [    0.095585] CPU7: update cpu_capacity 1535
> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
> [    0.095720] Brought up 8 CPUs
> 
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
> [2]:https://patchwork.kernel.org/patch/6782891/
> 
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Chanwoo Choi <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Heesub Shin <[e-mail]>
> Cc: Mauro Ribeiro <[e-mail]>
> Cc: Abhilash Kesavan <[e-mail]>
> Cc: Przemyslaw Marczak <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 9bdf547..a076dde 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>  		cluster >= EXYNOS5420_NR_CLUSTERS)
>  		return -EINVAL;
> 
> -	exynos_cpu_power_up(cpunr);
> +	if (!exynos_cpu_power_state(cpunr)) {
> +		exynos_cpu_power_up(cpunr);
> +
> +		if (soc_is_exynos5800() && cluster) {

I believe of_machine_is_compatible("samsung,exynos5800") is preferred
nowadays over soc_is_exynos5800().

Now, is correct to do this for all Exynos5422/5800 based boards? AFAIU
from the previous discussions, this behavior is specific to the Odroid
XU3 so maybe of_machine_is_compatible("hardkernel,odroid-xu3") instead?

I know this can affect other Exynos5800 boards that are shipped with a
similar binary BL1/BL2 blobs that also use SPARE2 as a sync mechanism
but maybe is better to add more exceptions later rather than do it for
all Exynos5800?

The Exynos5800 based Chromebook doesn't have this issue for example and
all the 8 cores are brought up correctly.

Having said that, I tested this patch on an Exynos5800 Peach Pi and all
the 8 cores are up and also after a S2R so seems to be a no-op when the
PMU_SPARE2 is not used for this.

Best regards,

Krzysztof Kozlowski July 15, 2015, 11:35 a.m. UTC | #6

2015-07-15 10:36 GMT+09:00 Chanho Park <[e-mail]>:
> The cpu booting of exynos5422 has been still broken since we discussed
> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
> hardkernel, it could help to boot 8 cores well. This patch need to have
> more test like STR and other SoC especially exynos5800 which is variant
> of exynos5422. If this patch is broken on exynos5800, I'll find another
> way to check exynos5422.
>
> This patch is top of my previous exynos5422 cpu ordering patch[2] and
> need to enable CONFIG_EXYNOS5420_MCPM=y
>
> [    0.047509] CPU0: update cpu_capacity 448
> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
> [    0.047874] Setting up static identity map for 0x400082c0 -
> 0x40008318
> [    0.048340] ARM CCI driver probed
> [    0.048597] Exynos MCPM support installed
> [    0.065676] CPU1: update cpu_capacity 448
> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
> [    0.070672] CPU2: update cpu_capacity 448
> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
> [    0.075644] CPU3: update cpu_capacity 448
> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
> [    0.080590] CPU4: update cpu_capacity 1535
> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.085591] CPU5: update cpu_capacity 1535
> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
> [    0.090590] CPU6: update cpu_capacity 1535
> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
> [    0.095585] CPU7: update cpu_capacity 1535
> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
> [    0.095720] Brought up 8 CPUs
>
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
> [2]:https://patchwork.kernel.org/patch/6782891/

Few questions/issues:
1. For the proper patch the commit message needs to be fixed a little.
The dmesg above is meaningless but instead you can put short dmesg (2
or 4 lines) with failed booting of CPUs.
2. Why only MCPM? Isn't this also needed without MCPM?

>
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Chanwoo Choi <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Heesub Shin <[e-mail]>
> Cc: Mauro Ribeiro <[e-mail]>
> Cc: Abhilash Kesavan <[e-mail]>
> Cc: Przemyslaw Marczak <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>

I think both Chanho Parks are the same person :) so you may leave only
one (the one matching "From").

> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
> index 9bdf547..a076dde 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>                 cluster >= EXYNOS5420_NR_CLUSTERS)
>                 return -EINVAL;
>
> -       exynos_cpu_power_up(cpunr);
> +       if (!exynos_cpu_power_state(cpunr)) {
> +               exynos_cpu_power_up(cpunr);

This second exynos_cpu_power_up() is needed? Do you know why?

> +
> +               if (soc_is_exynos5800() && cluster) {

If of_machine_is_compatible() can be used then please use it. Javier
also mentioned it and he pointed that this is not necessary on
Chromebooks.

> +                       while (!pmu_raw_readl(S5P_PMU_SPARE2))
> +                               udelay(10);
> +
> +                       pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
> +                                       EXYNOS_SWRESET);

This is quite similar to existing exynos_core_restart(). Can you
extend it for this purpose? Or at least put it as separate function
near exynos_core_restart() so this would be grouped close to each
other?

Documenting this behaviour would be really nice (you can combine some
of my findings on SPARE2 from previous discussion, if they are useful
of course).

> +               }
> +       }
> +
>         return 0;
>  }
>
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index b761433..fba9068 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -513,6 +513,12 @@ static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
>  #define SPREAD_ENABLE                                          0xF
>  #define SPREAD_USE_STANDWFI                                    0xF
>
> +#define EXYNOS5420_KFC_CORE_RESET0                             BIT(8)
> +#define EXYNOS5420_KFC_ETM_RESET0                              BIT(20)
> +
> +#define EXYNOS5420_KFC_CORE_RESET(_nr)                         \
> +       ((EXYNOS5420_KFC_CORE_RESET0 | EXYNOS5420_KFC_ETM_RESET0) << (_nr))
> +
>  #define EXYNOS5420_BB_CON1                                     0x0784
>  #define EXYNOS5420_BB_SEL_EN                                   BIT(31)
>  #define EXYNOS5420_BB_PMOS_EN                                  BIT(7)
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chanho Park July 16, 2015, 12:34 a.m. UTC | #7

Hello Javier,

> I believe of_machine_is_compatible("samsung,exynos5800") is preferred
> nowadays over soc_is_exynos5800().
>
> Now, is correct to do this for all Exynos5422/5800 based boards? AFAIU
> from the previous discussions, this behavior is specific to the Odroid
> XU3 so maybe of_machine_is_compatible("hardkernel,odroid-xu3") instead?
>
> I know this can affect other Exynos5800 boards that are shipped with a
> similar binary BL1/BL2 blobs that also use SPARE2 as a sync mechanism
> but maybe is better to add more exceptions later rather than do it for
> all Exynos5800?
>
> The Exynos5800 based Chromebook doesn't have this issue for example and
> all the 8 cores are brought up correctly.
>
> Having said that, I tested this patch on an Exynos5800 Peach Pi and all
> the 8 cores are up and also after a S2R so seems to be a no-op when the
> PMU_SPARE2 is not used for this.

Thanks for the testing. I'll find more proper way it could be applied
only for exynos5422 boards.

Chanho Park July 16, 2015, 1:36 a.m. UTC | #8

Hi,

On Wed, Jul 15, 2015 at 8:35 PM, Krzysztof Kozlowski
<[e-mail]> wrote:
> 2015-07-15 10:36 GMT+09:00 Chanho Park <[e-mail]>:
>> The cpu booting of exynos5422 has been still broken since we discussed
>> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
>> hardkernel, it could help to boot 8 cores well. This patch need to have
>> more test like STR and other SoC especially exynos5800 which is variant
>> of exynos5422. If this patch is broken on exynos5800, I'll find another
>> way to check exynos5422.
>>
>> This patch is top of my previous exynos5422 cpu ordering patch[2] and
>> need to enable CONFIG_EXYNOS5420_MCPM=y
>>
>> [    0.047509] CPU0: update cpu_capacity 448
>> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
>> [    0.047874] Setting up static identity map for 0x400082c0 -
>> 0x40008318
>> [    0.048340] ARM CCI driver probed
>> [    0.048597] Exynos MCPM support installed
>> [    0.065676] CPU1: update cpu_capacity 448
>> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
>> [    0.070672] CPU2: update cpu_capacity 448
>> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
>> [    0.075644] CPU3: update cpu_capacity 448
>> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
>> [    0.080590] CPU4: update cpu_capacity 1535
>> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
>> [    0.085591] CPU5: update cpu_capacity 1535
>> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
>> [    0.090590] CPU6: update cpu_capacity 1535
>> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
>> [    0.095585] CPU7: update cpu_capacity 1535
>> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
>> [    0.095720] Brought up 8 CPUs
>>
>> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
>> [2]:https://patchwork.kernel.org/patch/6782891/
>
> Few questions/issues:
> 1. For the proper patch the commit message needs to be fixed a little.
> The dmesg above is meaningless but instead you can put short dmesg (2
> or 4 lines) with failed booting of CPUs.

They will be gone when I make a official patch.

> 2. Why only MCPM? Isn't this also needed without MCPM?

Current exynos platsmp is not aware multi clusters. MCPM covered
secondary cpu booting of exynos big.Little cores. Without MCPM, same
approach and codes will be required on platsmp thus I think it's
redundant.

>
>>
>> Cc: Joonyoung Shim <[e-mail]>
>> Cc: Chanwoo Choi <[e-mail]>
>> Cc: Kevin Hilman <[e-mail]>
>> Cc: Heesub Shin <[e-mail]>
>> Cc: Mauro Ribeiro <[e-mail]>
>> Cc: Abhilash Kesavan <[e-mail]>
>> Cc: Przemyslaw Marczak <[e-mail]>
>> Cc: Marek Szyprowski <[e-mail]>
>> Cc: Krzysztof Kozlowski <[e-mail]>
>> Signed-off-by: Chanho Park <[e-mail]>
>> Signed-off-by: Chanho Park <[e-mail]>
>
> I think both Chanho Parks are the same person :) so you may leave only
> one (the one matching "From").
>
>> ---
>>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
>> index 9bdf547..a076dde 100644
>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>>                 cluster >= EXYNOS5420_NR_CLUSTERS)
>>                 return -EINVAL;
>>
>> -       exynos_cpu_power_up(cpunr);
>> +       if (!exynos_cpu_power_state(cpunr)) {
>> +               exynos_cpu_power_up(cpunr);
>
> This second exynos_cpu_power_up() is needed? Do you know why?

It's redirected to exynos's platsmp and it turned on CPU through pmu registers.
 -> exynos_cpu_powerup: mcpm-exynos.c
 -> exynos_cpu_power_up: platsmp.c

Current mcpm exynos tried to run exynos_cpu_power_up twice during
secondary cpu is up. The first is from cpu_power_up of mcpm and the
second is from cpu_is_up. I want to avoid second power up the cpu if
it already turned on.

static void exynos_cpu_is_up(unsigned int cpu, unsigned int cluster)
{
        /* especially when resuming: make sure power control is set */

        exynos_cpu_powerup(cpu, cluster);
}


>
>> +
>> +               if (soc_is_exynos5800() && cluster) {
>
> If of_machine_is_compatible() can be used then please use it. Javier
> also mentioned it and he pointed that this is not necessary on
> Chromebooks.

Okay. I'll use of_machine_is_compatible() instead of soc_is_exynos5800
to avoid running it from exynos5800.

>
>> +                       while (!pmu_raw_readl(S5P_PMU_SPARE2))
>> +                               udelay(10);
>> +
>> +                       pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
>> +                                       EXYNOS_SWRESET);
>
> This is quite similar to existing exynos_core_restart(). Can you
> extend it for this purpose? Or at least put it as separate function
> near exynos_core_restart() so this would be grouped close to each
> other?

As I said above, the platsmp is not aware multi clusters. I feel like
it would be dirty if I try to merge them.

>
> Documenting this behaviour would be really nice (you can combine some
> of my findings on SPARE2 from previous discussion, if they are useful
> of course).

No one knows exactly why we should wait until SPARE2 is up. I'll
specify known issue of it with your findings.

Krzysztof Kozlowski July 16, 2015, 1:49 a.m. UTC | #9

On 16.07.2015 10:36, Chanho Park wrote:
> Hi,
> 
> On Wed, Jul 15, 2015 at 8:35 PM, Krzysztof Kozlowski
> <[e-mail]> wrote:
>> 2015-07-15 10:36 GMT+09:00 Chanho Park <[e-mail]>:
>>> The cpu booting of exynos5422 has been still broken since we discussed
>>> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
>>> hardkernel, it could help to boot 8 cores well. This patch need to have
>>> more test like STR and other SoC especially exynos5800 which is variant
>>> of exynos5422. If this patch is broken on exynos5800, I'll find another
>>> way to check exynos5422.
>>>
>>> This patch is top of my previous exynos5422 cpu ordering patch[2] and
>>> need to enable CONFIG_EXYNOS5420_MCPM=y
>>>
>>> [    0.047509] CPU0: update cpu_capacity 448
>>> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
>>> [    0.047874] Setting up static identity map for 0x400082c0 -
>>> 0x40008318
>>> [    0.048340] ARM CCI driver probed
>>> [    0.048597] Exynos MCPM support installed
>>> [    0.065676] CPU1: update cpu_capacity 448
>>> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
>>> [    0.070672] CPU2: update cpu_capacity 448
>>> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
>>> [    0.075644] CPU3: update cpu_capacity 448
>>> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
>>> [    0.080590] CPU4: update cpu_capacity 1535
>>> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
>>> [    0.085591] CPU5: update cpu_capacity 1535
>>> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
>>> [    0.090590] CPU6: update cpu_capacity 1535
>>> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
>>> [    0.095585] CPU7: update cpu_capacity 1535
>>> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
>>> [    0.095720] Brought up 8 CPUs
>>>
>>> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
>>> [2]:https://patchwork.kernel.org/patch/6782891/
>>
>> Few questions/issues:
>> 1. For the proper patch the commit message needs to be fixed a little.
>> The dmesg above is meaningless but instead you can put short dmesg (2
>> or 4 lines) with failed booting of CPUs.
> 
> They will be gone when I make a official patch.
> 
>> 2. Why only MCPM? Isn't this also needed without MCPM?
> 
> Current exynos platsmp is not aware multi clusters. MCPM covered
> secondary cpu booting of exynos big.Little cores. Without MCPM, same
> approach and codes will be required on platsmp thus I think it's
> redundant.

Okay.

> 
>>
>>>
>>> Cc: Joonyoung Shim <[e-mail]>
>>> Cc: Chanwoo Choi <[e-mail]>
>>> Cc: Kevin Hilman <[e-mail]>
>>> Cc: Heesub Shin <[e-mail]>
>>> Cc: Mauro Ribeiro <[e-mail]>
>>> Cc: Abhilash Kesavan <[e-mail]>
>>> Cc: Przemyslaw Marczak <[e-mail]>
>>> Cc: Marek Szyprowski <[e-mail]>
>>> Cc: Krzysztof Kozlowski <[e-mail]>
>>> Signed-off-by: Chanho Park <[e-mail]>
>>> Signed-off-by: Chanho Park <[e-mail]>
>>
>> I think both Chanho Parks are the same person :) so you may leave only
>> one (the one matching "From").
>>
>>> ---
>>>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>>>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
>>> index 9bdf547..a076dde 100644
>>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>>> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>>>                 cluster >= EXYNOS5420_NR_CLUSTERS)
>>>                 return -EINVAL;
>>>
>>> -       exynos_cpu_power_up(cpunr);
>>> +       if (!exynos_cpu_power_state(cpunr)) {
>>> +               exynos_cpu_power_up(cpunr);
>>
>> This second exynos_cpu_power_up() is needed? Do you know why?
> 
> It's redirected to exynos's platsmp and it turned on CPU through pmu registers.
>  -> exynos_cpu_powerup: mcpm-exynos.c
>  -> exynos_cpu_power_up: platsmp.c
> 
> Current mcpm exynos tried to run exynos_cpu_power_up twice during
> secondary cpu is up. The first is from cpu_power_up of mcpm and the
> second is from cpu_is_up. I want to avoid second power up the cpu if
> it already turned on.
> 
> static void exynos_cpu_is_up(unsigned int cpu, unsigned int cluster)
> {
>         /* especially when resuming: make sure power control is set */
> 
>         exynos_cpu_powerup(cpu, cluster);
> }

Okay, I get it. I misunderstood the patch. It's fine.

> 
> 
>>
>>> +
>>> +               if (soc_is_exynos5800() && cluster) {
>>
>> If of_machine_is_compatible() can be used then please use it. Javier
>> also mentioned it and he pointed that this is not necessary on
>> Chromebooks.
> 
> Okay. I'll use of_machine_is_compatible() instead of soc_is_exynos5800
> to avoid running it from exynos5800.
> 
>>
>>> +                       while (!pmu_raw_readl(S5P_PMU_SPARE2))
>>> +                               udelay(10);
>>> +
>>> +                       pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
>>> +                                       EXYNOS_SWRESET);
>>
>> This is quite similar to existing exynos_core_restart(). Can you
>> extend it for this purpose? Or at least put it as separate function
>> near exynos_core_restart() so this would be grouped close to each
>> other?
> 
> As I said above, the platsmp is not aware multi clusters. I feel like
> it would be dirty if I try to merge them.

But here you are not reseting the cluster but reseting a CPU. Very
similar function exists already - for waiting on SPARE2 and reseting
CPU. I don't see what multiple clusters are changing here?

>>
>> Documenting this behaviour would be really nice (you can combine some
>> of my findings on SPARE2 from previous discussion, if they are useful
>> of course).
> 
> No one knows exactly why we should wait until SPARE2 is up. I'll
> specify known issue of it with your findings.

Right, no one... at least publicly (probably the guys who have access to
sources of BL1/BL2 know). Still it is important to document the reason
why we think these steps are necessary. Some 3rd party developer won't
have a clue why you are spinning on some SPARE2 register...

Best regards,
Krzysztof

Sjoerd Simons July 17, 2015, 8:05 a.m. UTC | #10

On Wed, 2015-07-15 at 10:36 +0900, Chanho Park wrote:
> The cpu booting of exynos5422 has been still broken since we 
> discussed
> it in last year[1]. I found this resetting codes from odroid-xu3 
> kernel of
> hardkernel, it could help to boot 8 cores well. This patch need to 
> have
> more test like STR and other SoC especially exynos5800 which is 
> variant
> of exynos5422. If this patch is broken on exynos5800, I'll find 
> another
> way to check exynos5422.
> 
> This patch is top of my previous exynos5422 cpu ordering patch[2] and
> need to enable CONFIG_EXYNOS5420_MCPM=y
> 
> [    0.047509] CPU0: update cpu_capacity 448
> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
> [    0.047874] Setting up static identity map for 0x400082c0 -
> 0x40008318
> [    0.048340] ARM CCI driver probed
> [    0.048597] Exynos MCPM support installed
> [    0.065676] CPU1: update cpu_capacity 448
> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
> [    0.070672] CPU2: update cpu_capacity 448
> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
> [    0.075644] CPU3: update cpu_capacity 448
> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
> [    0.080590] CPU4: update cpu_capacity 1535
> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.085591] CPU5: update cpu_capacity 1535
> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
> [    0.090590] CPU6: update cpu_capacity 1535
> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
> [    0.095585] CPU7: update cpu_capacity 1535
> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
> [    0.095720] Brought up 8 CPUs
> 
> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015
> -June/350632.html
> [2]:https://patchwork.kernel.org/patch/6782891/
> 
> Cc: Joonyoung Shim <[e-mail]>
> Cc: Chanwoo Choi <[e-mail]>
> Cc: Kevin Hilman <[e-mail]>
> Cc: Heesub Shin <[e-mail]>
> Cc: Mauro Ribeiro <[e-mail]>
> Cc: Abhilash Kesavan <[e-mail]>
> Cc: Przemyslaw Marczak <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Krzysztof Kozlowski <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> Signed-off-by: Chanho Park <[e-mail]>
> ---
>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach
> -exynos/mcpm-exynos.c
> index 9bdf547..a076dde 100644
> --- a/arch/arm/mach-exynos/mcpm-exynos.c
> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, 
> unsigned int cluster)
>  		cluster >= EXYNOS5420_NR_CLUSTERS)
>  		return -EINVAL;
> 
> -	exynos_cpu_power_up(cpunr);
> +	if (!exynos_cpu_power_state(cpunr)) {
> +		exynos_cpu_power_up(cpunr);
> +
> +		if (soc_is_exynos5800() && cluster) {

This seems a tad subtle. You're assuming here cluster 1 is the little
cluster. Would be good to make this more specific/obvious.

From looking at other mcpm-exynos code the assumption seems valid
(otherwise more code would be broken), so it may make sense to simply
define LITTLE_CLUSTER and BIG_CLUSTER and check explicitely for that.

Krzysztof on IRC mention that the cluster layout is read from MPIDR so
in principle this assumption could be invalidated on some Exynos
versions, but then again, given the amount of fallout from that i don't
see it as a practical issue at the moment :) 

> +			while (!pmu_raw_readl(S5P_PMU_SPARE2))
> +				udelay(10);

Would love to see some documentation for this magical pmu register, but
i guess you can't have them all :)

Krzysztof Kozlowski July 17, 2015, 9:54 a.m. UTC | #11

2015-07-17 17:05 GMT+09:00 Sjoerd Simons <[e-mail]>:
> On Wed, 2015-07-15 at 10:36 +0900, Chanho Park wrote:
>> The cpu booting of exynos5422 has been still broken since we
>> discussed
>> it in last year[1]. I found this resetting codes from odroid-xu3
>> kernel of
>> hardkernel, it could help to boot 8 cores well. This patch need to
>> have
>> more test like STR and other SoC especially exynos5800 which is
>> variant
>> of exynos5422. If this patch is broken on exynos5800, I'll find
>> another
>> way to check exynos5422.
>>
>> This patch is top of my previous exynos5422 cpu ordering patch[2] and
>> need to enable CONFIG_EXYNOS5420_MCPM=y
>>
>> [    0.047509] CPU0: update cpu_capacity 448
>> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
>> [    0.047874] Setting up static identity map for 0x400082c0 -
>> 0x40008318
>> [    0.048340] ARM CCI driver probed
>> [    0.048597] Exynos MCPM support installed
>> [    0.065676] CPU1: update cpu_capacity 448
>> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
>> [    0.070672] CPU2: update cpu_capacity 448
>> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
>> [    0.075644] CPU3: update cpu_capacity 448
>> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
>> [    0.080590] CPU4: update cpu_capacity 1535
>> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
>> [    0.085591] CPU5: update cpu_capacity 1535
>> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
>> [    0.090590] CPU6: update cpu_capacity 1535
>> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
>> [    0.095585] CPU7: update cpu_capacity 1535
>> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
>> [    0.095720] Brought up 8 CPUs
>>
>> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015
>> -June/350632.html
>> [2]:https://patchwork.kernel.org/patch/6782891/
>>
>> Cc: Joonyoung Shim <[e-mail]>
>> Cc: Chanwoo Choi <[e-mail]>
>> Cc: Kevin Hilman <[e-mail]>
>> Cc: Heesub Shin <[e-mail]>
>> Cc: Mauro Ribeiro <[e-mail]>
>> Cc: Abhilash Kesavan <[e-mail]>
>> Cc: Przemyslaw Marczak <[e-mail]>
>> Cc: Marek Szyprowski <[e-mail]>
>> Cc: Krzysztof Kozlowski <[e-mail]>
>> Signed-off-by: Chanho Park <[e-mail]>
>> Signed-off-by: Chanho Park <[e-mail]>
>> ---
>>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach
>> -exynos/mcpm-exynos.c
>> index 9bdf547..a076dde 100644
>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu,
>> unsigned int cluster)
>>               cluster >= EXYNOS5420_NR_CLUSTERS)
>>               return -EINVAL;
>>
>> -     exynos_cpu_power_up(cpunr);
>> +     if (!exynos_cpu_power_state(cpunr)) {
>> +             exynos_cpu_power_up(cpunr);
>> +
>> +             if (soc_is_exynos5800() && cluster) {
>
> This seems a tad subtle. You're assuming here cluster 1 is the little
> cluster. Would be good to make this more specific/obvious.
>
> From looking at other mcpm-exynos code the assumption seems valid
> (otherwise more code would be broken), so it may make sense to simply
> define LITTLE_CLUSTER and BIG_CLUSTER and check explicitely for that.
>
> Krzysztof on IRC mention that the cluster layout is read from MPIDR so
> in principle this assumption could be invalidated on some Exynos
> versions, but then again, given the amount of fallout from that i don't
> see it as a practical issue at the moment :)

For these SoCs this cluster ID should be fixed. However you are right
that it is a little fuzzy so it would be better to find a more
reliable way.

>
>> +                     while (!pmu_raw_readl(S5P_PMU_SPARE2))
>> +                             udelay(10);
>
> Would love to see some documentation for this magical pmu register, but
> i guess you can't have them all :)

I started documenting more of such registers here:
Documentation/arm/Samsung/Bootloader-interface.txt
The SPARE2 register is 0x908. I would be good to extend the
documentation but unfortunately all I knowledge I got came from
observation or vendor's source code, not from bootloader source code.

I know that Przemyslaw Marczak is also looking at the issue so maybe
he could share some details?

Best regards,
Krzysztof

Krzysztof Kozlowski Aug. 29, 2015, 7:45 a.m. UTC | #12

2015-07-16 10:49 GMT+09:00 Krzysztof Kozlowski <[e-mail]>:
> On 16.07.2015 10:36, Chanho Park wrote:
>> Hi,
>>
>> On Wed, Jul 15, 2015 at 8:35 PM, Krzysztof Kozlowski
>> <[e-mail]> wrote:
>>> 2015-07-15 10:36 GMT+09:00 Chanho Park <[e-mail]>:
>>>> The cpu booting of exynos5422 has been still broken since we discussed
>>>> it in last year[1]. I found this resetting codes from odroid-xu3 kernel of
>>>> hardkernel, it could help to boot 8 cores well. This patch need to have
>>>> more test like STR and other SoC especially exynos5800 which is variant
>>>> of exynos5422. If this patch is broken on exynos5800, I'll find another
>>>> way to check exynos5422.
>>>>
>>>> This patch is top of my previous exynos5422 cpu ordering patch[2] and
>>>> need to enable CONFIG_EXYNOS5420_MCPM=y
>>>>
>>>> [    0.047509] CPU0: update cpu_capacity 448
>>>> [    0.047534] CPU0: thread -1, cpu 0, socket 1, mpidr 80000100
>>>> [    0.047874] Setting up static identity map for 0x400082c0 -
>>>> 0x40008318
>>>> [    0.048340] ARM CCI driver probed
>>>> [    0.048597] Exynos MCPM support installed
>>>> [    0.065676] CPU1: update cpu_capacity 448
>>>> [    0.065685] CPU1: thread -1, cpu 1, socket 1, mpidr 80000101
>>>> [    0.070672] CPU2: update cpu_capacity 448
>>>> [    0.070680] CPU2: thread -1, cpu 2, socket 1, mpidr 80000102
>>>> [    0.075644] CPU3: update cpu_capacity 448
>>>> [    0.075653] CPU3: thread -1, cpu 3, socket 1, mpidr 80000103
>>>> [    0.080590] CPU4: update cpu_capacity 1535
>>>> [    0.080600] CPU4: thread -1, cpu 0, socket 0, mpidr 80000000
>>>> [    0.085591] CPU5: update cpu_capacity 1535
>>>> [    0.085599] CPU5: thread -1, cpu 1, socket 0, mpidr 80000001
>>>> [    0.090590] CPU6: update cpu_capacity 1535
>>>> [    0.090598] CPU6: thread -1, cpu 2, socket 0, mpidr 80000002
>>>> [    0.095585] CPU7: update cpu_capacity 1535
>>>> [    0.095593] CPU7: thread -1, cpu 3, socket 0, mpidr 80000003
>>>> [    0.095720] Brought up 8 CPUs
>>>>
>>>> [1]:http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/350632.html
>>>> [2]:https://patchwork.kernel.org/patch/6782891/
>>>
>>> Few questions/issues:
>>> 1. For the proper patch the commit message needs to be fixed a little.
>>> The dmesg above is meaningless but instead you can put short dmesg (2
>>> or 4 lines) with failed booting of CPUs.
>>
>> They will be gone when I make a official patch.
>>
>>> 2. Why only MCPM? Isn't this also needed without MCPM?
>>
>> Current exynos platsmp is not aware multi clusters. MCPM covered
>> secondary cpu booting of exynos big.Little cores. Without MCPM, same
>> approach and codes will be required on platsmp thus I think it's
>> redundant.
>
> Okay.
>
>>
>>>
>>>>
>>>> Cc: Joonyoung Shim <[e-mail]>
>>>> Cc: Chanwoo Choi <[e-mail]>
>>>> Cc: Kevin Hilman <[e-mail]>
>>>> Cc: Heesub Shin <[e-mail]>
>>>> Cc: Mauro Ribeiro <[e-mail]>
>>>> Cc: Abhilash Kesavan <[e-mail]>
>>>> Cc: Przemyslaw Marczak <[e-mail]>
>>>> Cc: Marek Szyprowski <[e-mail]>
>>>> Cc: Krzysztof Kozlowski <[e-mail]>
>>>> Signed-off-by: Chanho Park <[e-mail]>
>>>> Signed-off-by: Chanho Park <[e-mail]>
>>>
>>> I think both Chanho Parks are the same person :) so you may leave only
>>> one (the one matching "From").
>>>
>>>> ---
>>>>  arch/arm/mach-exynos/mcpm-exynos.c | 13 ++++++++++++-
>>>>  arch/arm/mach-exynos/regs-pmu.h    |  6 ++++++
>>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
>>>> index 9bdf547..a076dde 100644
>>>> --- a/arch/arm/mach-exynos/mcpm-exynos.c
>>>> +++ b/arch/arm/mach-exynos/mcpm-exynos.c
>>>> @@ -70,7 +70,18 @@ static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
>>>>                 cluster >= EXYNOS5420_NR_CLUSTERS)
>>>>                 return -EINVAL;
>>>>
>>>> -       exynos_cpu_power_up(cpunr);
>>>> +       if (!exynos_cpu_power_state(cpunr)) {
>>>> +               exynos_cpu_power_up(cpunr);
>>>
>>> This second exynos_cpu_power_up() is needed? Do you know why?
>>
>> It's redirected to exynos's platsmp and it turned on CPU through pmu registers.
>>  -> exynos_cpu_powerup: mcpm-exynos.c
>>  -> exynos_cpu_power_up: platsmp.c
>>
>> Current mcpm exynos tried to run exynos_cpu_power_up twice during
>> secondary cpu is up. The first is from cpu_power_up of mcpm and the
>> second is from cpu_is_up. I want to avoid second power up the cpu if
>> it already turned on.
>>
>> static void exynos_cpu_is_up(unsigned int cpu, unsigned int cluster)
>> {
>>         /* especially when resuming: make sure power control is set */
>>
>>         exynos_cpu_powerup(cpu, cluster);
>> }
>
> Okay, I get it. I misunderstood the patch. It's fine.
>
>>
>>
>>>
>>>> +
>>>> +               if (soc_is_exynos5800() && cluster) {
>>>
>>> If of_machine_is_compatible() can be used then please use it. Javier
>>> also mentioned it and he pointed that this is not necessary on
>>> Chromebooks.
>>
>> Okay. I'll use of_machine_is_compatible() instead of soc_is_exynos5800
>> to avoid running it from exynos5800.
>>
>>>
>>>> +                       while (!pmu_raw_readl(S5P_PMU_SPARE2))
>>>> +                               udelay(10);
>>>> +
>>>> +                       pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
>>>> +                                       EXYNOS_SWRESET);
>>>
>>> This is quite similar to existing exynos_core_restart(). Can you
>>> extend it for this purpose? Or at least put it as separate function
>>> near exynos_core_restart() so this would be grouped close to each
>>> other?
>>
>> As I said above, the platsmp is not aware multi clusters. I feel like
>> it would be dirty if I try to merge them.
>
> But here you are not reseting the cluster but reseting a CPU. Very
> similar function exists already - for waiting on SPARE2 and reseting
> CPU. I don't see what multiple clusters are changing here?
>
>>>
>>> Documenting this behaviour would be really nice (you can combine some
>>> of my findings on SPARE2 from previous discussion, if they are useful
>>> of course).
>>
>> No one knows exactly why we should wait until SPARE2 is up. I'll
>> specify known issue of it with your findings.
>
> Right, no one... at least publicly (probably the guys who have access to
> sources of BL1/BL2 know). Still it is important to document the reason
> why we think these steps are necessary. Some 3rd party developer won't
> have a clue why you are spinning on some SPARE2 register...


Dear Chanho,

Do you plan to send next version of the patchset? I tested it on my
Odroid XU3L (5422) and it works good so the idea you had is correct.

Best regards,
Krzysztof

Patch

diff --git a/arch/arm/mach-exynos/mcpm-exynos.c b/arch/arm/mach-exynos/mcpm-exynos.c
index 9bdf547..a076dde 100644
--- a/arch/arm/mach-exynos/mcpm-exynos.c
+++ b/arch/arm/mach-exynos/mcpm-exynos.c
@@ -70,7 +70,18 @@  static int exynos_cpu_powerup(unsigned int cpu, unsigned int cluster)
 		cluster >= EXYNOS5420_NR_CLUSTERS)
 		return -EINVAL;

-	exynos_cpu_power_up(cpunr);
+	if (!exynos_cpu_power_state(cpunr)) {
+		exynos_cpu_power_up(cpunr);
+
+		if (soc_is_exynos5800() && cluster) {
+			while (!pmu_raw_readl(S5P_PMU_SPARE2))
+				udelay(10);
+
+			pmu_raw_writel(EXYNOS5420_KFC_CORE_RESET(cpu),
+					EXYNOS_SWRESET);
+		}
+	}
+
 	return 0;
 }
 
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index b761433..fba9068 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -513,6 +513,12 @@  static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
 #define SPREAD_ENABLE						0xF
 #define SPREAD_USE_STANDWFI					0xF
 
+#define EXYNOS5420_KFC_CORE_RESET0				BIT(8)
+#define EXYNOS5420_KFC_ETM_RESET0				BIT(20)
+
+#define EXYNOS5420_KFC_CORE_RESET(_nr)				\
+	((EXYNOS5420_KFC_CORE_RESET0 | EXYNOS5420_KFC_ETM_RESET0) << (_nr))
+
 #define EXYNOS5420_BB_CON1					0x0784
 #define EXYNOS5420_BB_SEL_EN					BIT(31)
 #define EXYNOS5420_BB_PMOS_EN					BIT(7)