| Message ID | 1426683113-31209-2-git-send-email-b.zolnierkie@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2015-03-18 13:51 GMT+01:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.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@linaro.org> > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.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@samsung.com> Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.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
Hi, On Wednesday, March 18, 2015 02:10:31 PM Krzysztof Kozlowski wrote: > 2015-03-18 13:51 GMT+01:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.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@linaro.org> > > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Acked-by: Kyungmin Park <kyungmin.park@samsung.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@samsung.com> > Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Thank you! > 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... I've already planned to do this for this code and for coupled cpuidle use of SPARE2 as well. However I would really prefer to do it in an incremental patch if there are no other issues with this patchset. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
On ?ro, 2015-03-18 at 14:23 +0100, Bartlomiej Zolnierkiewicz wrote: > Hi, > > On Wednesday, March 18, 2015 02:10:31 PM Krzysztof Kozlowski wrote: > > 2015-03-18 13:51 GMT+01:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>: > > > > > > + 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... > > I've already planned to do this for this code and for coupled cpuidle > use of SPARE2 as well. However I would really prefer to do it in > an incremental patch if there are no other issues with this patchset. OK, please do so in incremental patch. Usage of various memory regions of sysram also should be documented. In patch 2 you add usage of 0x28 + 4*cpu. The various sysrams regions are spread over different files... Best regards, Krzysztof
On Wednesday, March 18, 2015 02:32:25 PM Krzysztof Kozlowski wrote: > On ?ro, 2015-03-18 at 14:23 +0100, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > > > On Wednesday, March 18, 2015 02:10:31 PM Krzysztof Kozlowski wrote: > > > 2015-03-18 13:51 GMT+01:00 Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>: > > > > > > > > + 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... > > > > I've already planned to do this for this code and for coupled cpuidle > > use of SPARE2 as well. However I would really prefer to do it in > > an incremental patch if there are no other issues with this patchset. > > OK, please do so in incremental patch. Usage of various memory regions OK. > of sysram also should be documented. In patch 2 you add usage of 0x28 + > 4*cpu. The various sysrams regions are spread over different files... I completely agree that there should be some Documentation file with the SYSRAM layout. I'll do it later unless someone beats me to it (please also add this to our internal TODO list of open issues, thanks!). Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
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); + val = pmu_raw_readl(EXYNOS_ARM_CORE_STATUS(core_id)); val |= S5P_CORE_WAKEUP_FROM_LOCAL_CFG; pmu_raw_writel(val, EXYNOS_ARM_CORE_STATUS(core_id)); @@ -346,7 +360,10 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle) call_firmware_op(cpu_boot, core_id); - arch_send_wakeup_ipi_mask(cpumask_of(cpu)); + if (soc_is_exynos3250()) + dsb_sev(); + else + arch_send_wakeup_ipi_mask(cpumask_of(cpu)); if (pen_release == -1) break; diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h index eb461e1..84ddce1 100644 --- a/arch/arm/mach-exynos/regs-pmu.h +++ b/arch/arm/mach-exynos/regs-pmu.h @@ -49,6 +49,7 @@ #define S5P_INFORM5 0x0814 #define S5P_INFORM6 0x0818 #define S5P_INFORM7 0x081C +#define S5P_PMU_SPARE2 0x0908 #define S5P_PMU_SPARE3 0x090C #define EXYNOS_IROM_DATA2 0x0988 @@ -182,6 +183,7 @@ #define S5P_CORE_LOCAL_PWR_EN 0x3 #define S5P_CORE_WAKEUP_FROM_LOCAL_CFG (0x3 << 8) +#define S5P_CORE_AUTOWAKEUP_EN (1 << 31) /* Only for EXYNOS4210 */ #define S5P_CMU_CLKSTOP_LCD1_LOWPWR 0x1154