linux-kernel - Re: [PATCH v3 1/4] ARM: EXYNOS: fix CPU1 hotplug for AFTR mode on Exynos3250

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 18 Mar 2015 14:10:31 +0100
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:	Kukjin Kim <kgene.kim@...sung.com>, Kukjin Kim <kgene@...nel.org>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	linux-samsung-soc@...r.kernel.org, linux-pm@...r.kernel.org,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Tomasz Figa <tomasz.figa@...il.com>,
	linux-kernel@...r.kernel.org, Chanwoo Choi <cw00.choi@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 1/4] ARM: EXYNOS: fix CPU1 hotplug for AFTR mode on Exynos3250

2015-03-18 13:51 GMT+01:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>:
> CPU1 hotplug may hang when AFTR is used.  Fix it by:
> - setting AUTOWAKEUP_EN bit in ARM_COREx_CONFIGURATION register in
>   exynos_cpu_power_up()
> - not clearing reserved bits of ARM_COREx_CONFIGURATION register in
>   exynos_cpu_power_down()
> - waiting while an undocumented register 0x0908 becomes non-zero in
>   exynos_core_restart()
> - using dsb_sev() instead of IPI in exynos_boot_secondary() on
>   Exynos3250
>
> Cc: Daniel Lezcano <daniel.lezcano@...aro.org>
> Signed-off-by: Chanwoo Choi <cw00.choi@...sung.com>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
> ---
>  arch/arm/mach-exynos/platsmp.c  | 23 ++++++++++++++++++++---
>  arch/arm/mach-exynos/regs-pmu.h |  2 ++
>  2 files changed, 22 insertions(+), 3 deletions(-)


Looks good (except one nit below) and this also fixes hotplug issues
during resume from S2R:
$ echo mem > /sys/power/state
[  156.517266] Disabling non-boot CPUs ...
[  156.517781] IRQ18 no longer affine to CPU1
[  156.518043] CPU1: shutdown
[  156.544718] Enabling non-boot CPUs ...
[  156.554925] CPU1: Software reset
[  158.552631] CPU1: failed to come online
[  158.552753] Error taking CPU1 up: -5

Reviewed and tested on Rinato (Gear 2/Exynos 3250) board:

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
Tested-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>

One comment below...

>
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index d2e9f12..ebd135b 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -126,6 +126,8 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   */
>  void exynos_cpu_power_down(int cpu)
>  {
> +       u32 core_conf;
> +
>         if (cpu == 0 && (soc_is_exynos5420() || soc_is_exynos5800())) {
>                 /*
>                  * Bypass power down for CPU0 during suspend. Check for
> @@ -137,7 +139,10 @@ void exynos_cpu_power_down(int cpu)
>                 if (!(val & S5P_CORE_LOCAL_PWR_EN))
>                         return;
>         }
> -       pmu_raw_writel(0, EXYNOS_ARM_CORE_CONFIGURATION(cpu));
> +
> +       core_conf = pmu_raw_readl(EXYNOS_ARM_CORE_CONFIGURATION(cpu));
> +       core_conf &= ~S5P_CORE_LOCAL_PWR_EN;
> +       pmu_raw_writel(core_conf, EXYNOS_ARM_CORE_CONFIGURATION(cpu));
>  }
>
>  /**
> @@ -148,7 +153,12 @@ void exynos_cpu_power_down(int cpu)
>   */
>  void exynos_cpu_power_up(int cpu)
>  {
> -       pmu_raw_writel(S5P_CORE_LOCAL_PWR_EN,
> +       u32 core_conf = S5P_CORE_LOCAL_PWR_EN;
> +
> +       if (soc_is_exynos3250())
> +               core_conf |= S5P_CORE_AUTOWAKEUP_EN;
> +
> +       pmu_raw_writel(core_conf,
>                         EXYNOS_ARM_CORE_CONFIGURATION(cpu));
>  }
>
> @@ -226,6 +236,10 @@ static void exynos_core_restart(u32 core_id)
>         if (!of_machine_is_compatible("samsung,exynos3250"))
>                 return;
>
> +       while (!pmu_raw_readl(S5P_PMU_SPARE2))
> +               udelay(10);
> +       udelay(10);

We really need to start documenting this. Please add short description
why this SPARE2 check is here and who uses it. Without documenting
this behavior future generations won't be able to debug this stuff.
Imagine replacing sboot with uboot by someone...

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/