[2/3] ARM: exynos: Simplify code in Exynos3250 CPU core restart path

exynos_core_restart() is called by secondary CPU boot procedure, used by
CPU hotplug. Replace of_machine_is_compatible() call with a simple SoC
revision check.

of_machine_is_compatible() function performs a dozen of string comparisons
during the full device tree walk, while soc_is_exynos3250() is a simple
integer check on SoC revision variable.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

---
 arch/arm/mach-exynos/platsmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.17.1

On Fri, 22 Mar 2019 at 12:48, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>

> exynos_core_restart() is called by secondary CPU boot procedure, used by

> CPU hotplug. Replace of_machine_is_compatible() call with a simple SoC

> revision check.

>

> of_machine_is_compatible() function performs a dozen of string comparisons

> during the full device tree walk, while soc_is_exynos3250() is a simple

> integer check on SoC revision variable.


Yes but it is against the effort of getting rid of all soc_is_() (see
https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=43565&state=*
). It also makes this code sticky to mach - we cannot move it to
drivers. See also Arnd's opinion:
https://marc.info/?l=devicetree&m=139291569126848&w=2

Did you measure the practical performance impact of this change?

Best regards,
Krzysztof

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---

>  arch/arm/mach-exynos/platsmp.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

>

> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c

> index 65da13923b8f..d5d48fbdab17 100644

> --- a/arch/arm/mach-exynos/platsmp.c

> +++ b/arch/arm/mach-exynos/platsmp.c

> @@ -216,7 +216,7 @@ void exynos_core_restart(u32 core_id)

>  {

>         u32 val;

>

> -       if (!of_machine_is_compatible("samsung,exynos3250"))

> +       if (!soc_is_exynos3250())

>                 return;

>

>         while (!pmu_raw_readl(S5P_PMU_SPARE2))

> --

> 2.17.1

>

Hi Krzysztof,

On 22.03.2019 14:10, Krzysztof Kozlowski wrote:
> On Fri, 22 Mar 2019 at 12:48, Marek Szyprowski <m.szyprowski@samsung.com> wrote:

>> exynos_core_restart() is called by secondary CPU boot procedure, used by

>> CPU hotplug. Replace of_machine_is_compatible() call with a simple SoC

>> revision check.

>>

>> of_machine_is_compatible() function performs a dozen of string comparisons

>> during the full device tree walk, while soc_is_exynos3250() is a simple

>> integer check on SoC revision variable.

> Yes but it is against the effort of getting rid of all soc_is_() (see

> https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=43565&state=*

> ). It also makes this code sticky to mach - we cannot move it to

> drivers. See also Arnd's opinion:

> https://marc.info/?l=devicetree&m=139291569126848&w=2

>

> Did you measure the practical performance impact of this change?


No, I didn't measure the performance, but recently I noticed the 
following warning:

=============================
WARNING: suspicious RCU usage
5.10.0-rc1-00001-g6f65599d1f4f-dirty #1800 Not tainted
-----------------------------
./include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!

other info that might help us debug this:


rcu_scheduler_active = 2, debug_locks = 1
RCU used illegally from extended quiescent state!
no locks held by swapper/0/0.

stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
5.10.0-rc1-00001-g6f65599d1f4f-dirty #1800
Hardware name: Samsung Exynos (Flattened Device Tree)
[<c0111514>] (unwind_backtrace) from [<c010ceb8>] (show_stack+0x10/0x14)
[<c010ceb8>] (show_stack) from [<c0b1d8dc>] (dump_stack+0xb4/0xd4)
[<c0b1d8dc>] (dump_stack) from [<c0194acc>] (lock_acquire+0x418/0x584)
[<c0194acc>] (lock_acquire) from [<c0b29e58>] 
(_raw_spin_lock_irqsave+0x4c/0x60)
[<c0b29e58>] (_raw_spin_lock_irqsave) from [<c0897af4>] 
(of_device_is_compatible+0x1c/0x4c)
[<c0897af4>] (of_device_is_compatible) from [<c01216d8>] 
(exynos_core_restart+0x14/0xb0)
[<c01216d8>] (exynos_core_restart) from [<c0120a78>] 
(exynos_cpu0_enter_aftr+0x1d0/0x1dc)
[<c0120a78>] (exynos_cpu0_enter_aftr) from [<c08575b0>] 
(exynos_enter_coupled_lowpower+0x44/0x74)
[<c08575b0>] (exynos_enter_coupled_lowpower) from [<c085477c>] 
(cpuidle_enter_state+0x178/0x660)
[<c085477c>] (cpuidle_enter_state) from [<c08572dc>] 
(cpuidle_enter_state_coupled+0x35c/0x378)
[<c08572dc>] (cpuidle_enter_state_coupled) from [<c0854cc8>] 
(cpuidle_enter+0x50/0x54)
[<c0854cc8>] (cpuidle_enter) from [<c0164854>] (do_idle+0x224/0x2a4)
[<c0164854>] (do_idle) from [<c0164c88>] (cpu_startup_entry+0x18/0x1c)
[<c0164c88>] (cpu_startup_entry) from [<c1100fa0>] 
(start_kernel+0x640/0x67c)
[<c1100fa0>] (start_kernel) from [<00000000>] (0x0)

I will add this to the commit message and resend. This looks like a good 
reason for this change.

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

>> ---

>>   arch/arm/mach-exynos/platsmp.c | 2 +-

>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c

>> index 65da13923b8f..d5d48fbdab17 100644

>> --- a/arch/arm/mach-exynos/platsmp.c

>> +++ b/arch/arm/mach-exynos/platsmp.c

>> @@ -216,7 +216,7 @@ void exynos_core_restart(u32 core_id)

>>   {

>>          u32 val;

>>

>> -       if (!of_machine_is_compatible("samsung,exynos3250"))

>> +       if (!soc_is_exynos3250())

>>                  return;

>>

>>          while (!pmu_raw_readl(S5P_PMU_SPARE2))

>> --

>> 2.17.1

>>

>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

On Mon, Oct 26, 2020 at 10:43:03PM +0100, Marek Szyprowski wrote:
> Hi Krzysztof,
> 
> On 22.03.2019 14:10, Krzysztof Kozlowski wrote:
> > On Fri, 22 Mar 2019 at 12:48, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> exynos_core_restart() is called by secondary CPU boot procedure, used by
> >> CPU hotplug. Replace of_machine_is_compatible() call with a simple SoC
> >> revision check.
> >>
> >> of_machine_is_compatible() function performs a dozen of string comparisons
> >> during the full device tree walk, while soc_is_exynos3250() is a simple
> >> integer check on SoC revision variable.
> > Yes but it is against the effort of getting rid of all soc_is_() (see
> > https://patchwork.kernel.org/project/linux-samsung-soc/list/?series=43565&state=*
> > ). It also makes this code sticky to mach - we cannot move it to
> > drivers. See also Arnd's opinion:
> > https://marc.info/?l=devicetree&m=139291569126848&w=2
> >
> > Did you measure the practical performance impact of this change?
> 
> No, I didn't measure the performance, but recently I noticed the 
> following warning:
> 
> =============================
> WARNING: suspicious RCU usage
> 5.10.0-rc1-00001-g6f65599d1f4f-dirty #1800 Not tainted
> -----------------------------
> ./include/trace/events/lock.h:37 suspicious rcu_dereference_check() usage!
> 
> other info that might help us debug this:
> 
> 
> rcu_scheduler_active = 2, debug_locks = 1
> RCU used illegally from extended quiescent state!
> no locks held by swapper/0/0.
> 
> stack backtrace:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 
> 5.10.0-rc1-00001-g6f65599d1f4f-dirty #1800
> Hardware name: Samsung Exynos (Flattened Device Tree)
> [<c0111514>] (unwind_backtrace) from [<c010ceb8>] (show_stack+0x10/0x14)
> [<c010ceb8>] (show_stack) from [<c0b1d8dc>] (dump_stack+0xb4/0xd4)
> [<c0b1d8dc>] (dump_stack) from [<c0194acc>] (lock_acquire+0x418/0x584)
> [<c0194acc>] (lock_acquire) from [<c0b29e58>] 
> (_raw_spin_lock_irqsave+0x4c/0x60)
> [<c0b29e58>] (_raw_spin_lock_irqsave) from [<c0897af4>] 
> (of_device_is_compatible+0x1c/0x4c)
> [<c0897af4>] (of_device_is_compatible) from [<c01216d8>] 
> (exynos_core_restart+0x14/0xb0)
> [<c01216d8>] (exynos_core_restart) from [<c0120a78>] 
> (exynos_cpu0_enter_aftr+0x1d0/0x1dc)
> [<c0120a78>] (exynos_cpu0_enter_aftr) from [<c08575b0>] 
> (exynos_enter_coupled_lowpower+0x44/0x74)
> [<c08575b0>] (exynos_enter_coupled_lowpower) from [<c085477c>] 
> (cpuidle_enter_state+0x178/0x660)
> [<c085477c>] (cpuidle_enter_state) from [<c08572dc>] 
> (cpuidle_enter_state_coupled+0x35c/0x378)
> [<c08572dc>] (cpuidle_enter_state_coupled) from [<c0854cc8>] 
> (cpuidle_enter+0x50/0x54)
> [<c0854cc8>] (cpuidle_enter) from [<c0164854>] (do_idle+0x224/0x2a4)
> [<c0164854>] (do_idle) from [<c0164c88>] (cpu_startup_entry+0x18/0x1c)
> [<c0164c88>] (cpu_startup_entry) from [<c1100fa0>] 
> (start_kernel+0x640/0x67c)
> [<c1100fa0>] (start_kernel) from [<00000000>] (0x0)
> 
> I will add this to the commit message and resend. This looks like a good 
> reason for this change.

Good point, thanks.

Best regards,
Krzysztof