[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
unconditionally resetting CPU1 because of a kexec boot issue I was seeing
earlier on omap4 when doing kexec boot between two different kernel
versions.

This caused issues on some systems. We should only reset CPU1 as a last
resort option, and try to avoid it where possible. Doing an unconditional
CPU1 reset causes issues for example when booting a bootloader configured
secure OS running on CPU1 as reported by Andrew F. Davis <afd@ti.com>.

We can't completely remove the reset of CPU1 as it would break
kexec booting from older kernels. Also it looks like omap4 suspend/resume
cycle occasionally fails to bring up CPU1 without the reset of CPU1.

The ideal fix is to park CPU1 to the bootrom loop during CPU hot-unplug.
This way we can be assured that the CPU1 is properly configured by
bootloader or the previous kernel. However, we can't do that for the
earlier kernels out there capable of kexec booting new kernels.

So let's fix the issue reported by Andrew by making CPU1 reset conditional
and only do it if CPU1 is configured to boot at an address that is within
the booting kernel. In these cases we know for sure that we just overwrote
the configured boot_secondary() during booting and that it's invalid.

And while at it, let's replace legacy cpu_is usage with soc_is to avoid
confusion with mixed use of both checks.

Additionally we also need to fix the hot-unplug code to properly park CPU1
to the bootrom loop so it's not affected by SDRAM changes done by kexec
booting kernel.

Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
Reported-by: Andrew F. Davis <afd@ti.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/common.h              |  1 +
 arch/arm/mach-omap2/omap-mpuss-lowpower.c | 30 +++++++++----
 arch/arm/mach-omap2/omap-smp.c            | 70 +++++++++++++++++++++++++------
 3 files changed, 80 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
 extern int omap4_mpuss_init(void);
 extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
 extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
+extern u32 omap4_get_cpu1_ns_pa_addr(void);
 #else
 static inline int omap4_enter_lowpower(unsigned int cpu,
 					unsigned int power_state)
diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -64,6 +64,7 @@
 #include "prm-regbits-44xx.h"
 
 static void __iomem *sar_base;
+static u32 old_cpu1_ns_pa_addr;
 
 #if defined(CONFIG_PM) && defined(CONFIG_SMP)
 
@@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
 {}
 #endif
 
+u32 omap4_get_cpu1_ns_pa_addr(void)
+{
+	return old_cpu1_ns_pa_addr;
+}
+
 /**
  * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
  * The purpose of this function is to manage low power programming
@@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x0);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x1);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
 		save_l2x0_context();
 	}
 
-	if (cpu_is_omap44xx()) {
+	if (soc_is_omap44xx()) {
 		omap_pm_ops.finish_suspend = omap4_finish_suspend;
 		omap_pm_ops.resume = omap4_cpu_resume;
 		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
@@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
 		enable_mercury_retention_mode();
 	}
 
-	if (cpu_is_omap446x())
+	if (soc_is_omap446x())
 		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
 
 	return 0;
@@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
 void __init omap4_mpuss_early_init(void)
 {
 	unsigned long startup_pa;
+	void __iomem *ns_pa_addr;
 
-	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
+	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
 		return;
 
 	sar_base = omap4_get_sar_ram_base();
 
-	if (cpu_is_omap443x())
+	/* Save old NS_PA_ADDR for validity checks later on */
+	if (soc_is_omap44xx())
+		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	else
+		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
+
+	if (soc_is_omap443x())
 		startup_pa = __pa_symbol(omap4_secondary_startup);
-	else if (cpu_is_omap446x())
+	else if (soc_is_omap446x())
 		startup_pa = __pa_symbol(omap4460_secondary_startup);
 	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
 		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
 	else
 		startup_pa = __pa_symbol(omap5_secondary_startup);
 
-	if (cpu_is_omap44xx())
+	if (soc_is_omap44xx())
 		writel_relaxed(startup_pa, sar_base +
 			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
 	else
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/irqchip/arm-gic.h>
 
+#include <asm/sections.h>
 #include <asm/smp_scu.h>
 #include <asm/virt.h>
 
@@ -44,6 +45,7 @@ struct omap_smp_config {
 	unsigned long cpu1_rstctrl_pa;
 	void __iomem *cpu1_rstctrl_va;
 	void __iomem *scu_base;
+	void __iomem *wakeupgen_base;
 	void *startup_addr;
 };
 
@@ -140,7 +142,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	static struct clockdomain *cpu1_clkdm;
 	static bool booted;
 	static struct powerdomain *cpu1_pwrdm;
-	void __iomem *base = omap_get_wakeupgen_base();
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -157,7 +158,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	if (omap_secure_apis_support())
 		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
 	else
-		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
+		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
 
 	if (!cpu1_clkdm && !cpu1_pwrdm) {
 		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
@@ -261,9 +262,59 @@ static void __init omap4_smp_init_cpus(void)
 		set_cpu_possible(i, true);
 }
 
+/*
+ * For now, just make sure the start-up address is not within the booting
+ * kernel space as that means we just overwrote whatever secondary_startup()
+ * code there was.
+ */
+static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
+{
+	if ((addr >= __pa(PAGE_OFFSET)) && (addr <= __pa(__bss_start)))
+		return false;
+
+	return true;
+}
+
+/*
+ * We may need to reset CPU1 before configuring, otherwise kexec boot can end
+ * up trying to use old kernel startup address or suspend-resume will
+ * occasionally fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper
+ * idle states.
+ */
+static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
+{
+	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
+	bool needs_reset = false;
+
+	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
+					OMAP_AUX_CORE_BOOT_1);
+	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
+
+	/* Did the configured secondary_startup() get overwritten? */
+	if (!omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
+		needs_reset = true;
+
+	/*
+	 * If omap4 or 5 has NS_PA_ADDR configured, CPU1 may be in a
+	 * deeper idle state in WFI and wake to an invalid address.
+	 */
+	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
+	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr))
+		needs_reset = true;
+
+	if (!needs_reset || !c->cpu1_rstctrl_va)
+		return;
+
+	pr_info("smp: Already configured CPU1, needs reset (0x%lx 0x%lx)\n",
+		cpu1_startup_pa, cpu1_ns_pa_addr);
+
+	writel_relaxed(1, c->cpu1_rstctrl_va);
+	readl_relaxed(c->cpu1_rstctrl_va);
+	writel_relaxed(0, c->cpu1_rstctrl_va);
+}
+
 static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 {
-	void __iomem *base = omap_get_wakeupgen_base();
 	const struct omap_smp_config *c = NULL;
 
 	if (soc_is_omap443x())
@@ -281,6 +332,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	/* Must preserve cfg.scu_base set earlier */
 	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
 	cfg.startup_addr = c->startup_addr;
+	cfg.wakeupgen_base = omap_get_wakeupgen_base();
 
 	if (soc_is_dra74x() || soc_is_omap54xx()) {
 		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
@@ -299,15 +351,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	if (cfg.scu_base)
 		scu_enable(cfg.scu_base);
 
-	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
-	 */
-	if (cfg.cpu1_rstctrl_va) {
-		writel_relaxed(1, cfg.cpu1_rstctrl_va);
-		readl_relaxed(cfg.cpu1_rstctrl_va);
-		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-	}
+	omap4_smp_maybe_reset_cpu1(&cfg);
 
 	/*
 	 * Write the address of secondary startup routine into the
@@ -319,7 +363,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
 	else
 		writel_relaxed(__pa_symbol(cfg.startup_addr),
-			       base + OMAP_AUX_CORE_BOOT_1);
+			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
 }
 
 const struct smp_operations omap4_smp_ops __initconst = {
-- 
2.11.1

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
unconditionally resetting CPU1 because of a kexec boot issue I was seeing
earlier on omap4 when doing kexec boot between two different kernel
versions.

This caused issues on some systems. We should only reset CPU1 as a last
resort option, and try to avoid it where possible. Doing an unconditional
CPU1 reset causes issues for example when booting a bootloader configured
secure OS running on CPU1 as reported by Andrew F. Davis <afd@ti.com>.

We can't completely remove the reset of CPU1 as it would break
kexec booting from older kernels. Also it looks like omap4 suspend/resume
cycle occasionally fails to bring up CPU1 without the reset of CPU1.

The ideal fix is to park CPU1 to the bootrom loop during CPU hot-unplug.
This way we can be assured that the CPU1 is properly configured by
bootloader or the previous kernel. However, we can't do that for the
earlier kernels out there capable of kexec booting new kernels.

So let's fix the issue reported by Andrew by making CPU1 reset conditional
and only do it if CPU1 is configured to boot at an address that is within
the booting kernel. In these cases we know for sure that we just overwrote
the configured boot_secondary() during booting and that it's invalid.

And while at it, let's replace legacy cpu_is usage with soc_is to avoid
confusion with mixed use of both checks.

Additionally we also need to fix the hot-unplug code to properly park CPU1
to the bootrom loop so it's not affected by SDRAM changes done by kexec
booting kernel.

Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
Reported-by: Andrew F. Davis <afd@ti.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/common.h              |  1 +
 arch/arm/mach-omap2/omap-mpuss-lowpower.c | 30 +++++++++----
 arch/arm/mach-omap2/omap-smp.c            | 70 +++++++++++++++++++++++++------
 3 files changed, 80 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
 extern int omap4_mpuss_init(void);
 extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
 extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
+extern u32 omap4_get_cpu1_ns_pa_addr(void);
 #else
 static inline int omap4_enter_lowpower(unsigned int cpu,
 					unsigned int power_state)
diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -64,6 +64,7 @@
 #include "prm-regbits-44xx.h"
 
 static void __iomem *sar_base;
+static u32 old_cpu1_ns_pa_addr;
 
 #if defined(CONFIG_PM) && defined(CONFIG_SMP)
 
@@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
 {}
 #endif
 
+u32 omap4_get_cpu1_ns_pa_addr(void)
+{
+	return old_cpu1_ns_pa_addr;
+}
+
 /**
  * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
  * The purpose of this function is to manage low power programming
@@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x0);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x1);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
 		save_l2x0_context();
 	}
 
-	if (cpu_is_omap44xx()) {
+	if (soc_is_omap44xx()) {
 		omap_pm_ops.finish_suspend = omap4_finish_suspend;
 		omap_pm_ops.resume = omap4_cpu_resume;
 		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
@@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
 		enable_mercury_retention_mode();
 	}
 
-	if (cpu_is_omap446x())
+	if (soc_is_omap446x())
 		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
 
 	return 0;
@@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
 void __init omap4_mpuss_early_init(void)
 {
 	unsigned long startup_pa;
+	void __iomem *ns_pa_addr;
 
-	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
+	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
 		return;
 
 	sar_base = omap4_get_sar_ram_base();
 
-	if (cpu_is_omap443x())
+	/* Save old NS_PA_ADDR for validity checks later on */
+	if (soc_is_omap44xx())
+		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	else
+		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
+
+	if (soc_is_omap443x())
 		startup_pa = __pa_symbol(omap4_secondary_startup);
-	else if (cpu_is_omap446x())
+	else if (soc_is_omap446x())
 		startup_pa = __pa_symbol(omap4460_secondary_startup);
 	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
 		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
 	else
 		startup_pa = __pa_symbol(omap5_secondary_startup);
 
-	if (cpu_is_omap44xx())
+	if (soc_is_omap44xx())
 		writel_relaxed(startup_pa, sar_base +
 			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
 	else
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -21,6 +21,7 @@
 #include <linux/io.h>
 #include <linux/irqchip/arm-gic.h>
 
+#include <asm/sections.h>
 #include <asm/smp_scu.h>
 #include <asm/virt.h>
 
@@ -44,6 +45,7 @@ struct omap_smp_config {
 	unsigned long cpu1_rstctrl_pa;
 	void __iomem *cpu1_rstctrl_va;
 	void __iomem *scu_base;
+	void __iomem *wakeupgen_base;
 	void *startup_addr;
 };
 
@@ -140,7 +142,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	static struct clockdomain *cpu1_clkdm;
 	static bool booted;
 	static struct powerdomain *cpu1_pwrdm;
-	void __iomem *base = omap_get_wakeupgen_base();
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -157,7 +158,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	if (omap_secure_apis_support())
 		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
 	else
-		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
+		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
 
 	if (!cpu1_clkdm && !cpu1_pwrdm) {
 		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
@@ -261,9 +262,59 @@ static void __init omap4_smp_init_cpus(void)
 		set_cpu_possible(i, true);
 }
 
+/*
+ * For now, just make sure the start-up address is not within the booting
+ * kernel space as that means we just overwrote whatever secondary_startup()
+ * code there was.
+ */
+static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
+{
+	if ((addr >= __pa(PAGE_OFFSET)) && (addr <= __pa(__bss_start)))
+		return false;
+
+	return true;
+}
+
+/*
+ * We may need to reset CPU1 before configuring, otherwise kexec boot can end
+ * up trying to use old kernel startup address or suspend-resume will
+ * occasionally fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper
+ * idle states.
+ */
+static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
+{
+	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
+	bool needs_reset = false;
+
+	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
+					OMAP_AUX_CORE_BOOT_1);
+	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
+
+	/* Did the configured secondary_startup() get overwritten? */
+	if (!omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
+		needs_reset = true;
+
+	/*
+	 * If omap4 or 5 has NS_PA_ADDR configured, CPU1 may be in a
+	 * deeper idle state in WFI and wake to an invalid address.
+	 */
+	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
+	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr))
+		needs_reset = true;
+
+	if (!needs_reset || !c->cpu1_rstctrl_va)
+		return;
+
+	pr_info("smp: Already configured CPU1, needs reset (0x%lx 0x%lx)\n",
+		cpu1_startup_pa, cpu1_ns_pa_addr);
+
+	writel_relaxed(1, c->cpu1_rstctrl_va);
+	readl_relaxed(c->cpu1_rstctrl_va);
+	writel_relaxed(0, c->cpu1_rstctrl_va);
+}
+
 static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 {
-	void __iomem *base = omap_get_wakeupgen_base();
 	const struct omap_smp_config *c = NULL;
 
 	if (soc_is_omap443x())
@@ -281,6 +332,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	/* Must preserve cfg.scu_base set earlier */
 	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
 	cfg.startup_addr = c->startup_addr;
+	cfg.wakeupgen_base = omap_get_wakeupgen_base();
 
 	if (soc_is_dra74x() || soc_is_omap54xx()) {
 		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
@@ -299,15 +351,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	if (cfg.scu_base)
 		scu_enable(cfg.scu_base);
 
-	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
-	 */
-	if (cfg.cpu1_rstctrl_va) {
-		writel_relaxed(1, cfg.cpu1_rstctrl_va);
-		readl_relaxed(cfg.cpu1_rstctrl_va);
-		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-	}
+	omap4_smp_maybe_reset_cpu1(&cfg);
 
 	/*
 	 * Write the address of secondary startup routine into the
@@ -319,7 +363,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
 	else
 		writel_relaxed(__pa_symbol(cfg.startup_addr),
-			       base + OMAP_AUX_CORE_BOOT_1);
+			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
 }
 
 const struct smp_operations omap4_smp_ops __initconst = {
-- 
2.11.1

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/13/2017 03:52 PM, Tony Lindgren wrote:
> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> unconditionally resetting CPU1 because of a kexec boot issue I was seeing
> earlier on omap4 when doing kexec boot between two different kernel
> versions.
> 
> This caused issues on some systems. We should only reset CPU1 as a last
> resort option, and try to avoid it where possible. Doing an unconditional
> CPU1 reset causes issues for example when booting a bootloader configured
> secure OS running on CPU1 as reported by Andrew F. Davis <afd@ti.com>.
> 
> We can't completely remove the reset of CPU1 as it would break
> kexec booting from older kernels. Also it looks like omap4 suspend/resume
> cycle occasionally fails to bring up CPU1 without the reset of CPU1.
> 
> The ideal fix is to park CPU1 to the bootrom loop during CPU hot-unplug.
> This way we can be assured that the CPU1 is properly configured by
> bootloader or the previous kernel. However, we can't do that for the
> earlier kernels out there capable of kexec booting new kernels.
> 
> So let's fix the issue reported by Andrew by making CPU1 reset conditional
> and only do it if CPU1 is configured to boot at an address that is within
> the booting kernel. In these cases we know for sure that we just overwrote
> the configured boot_secondary() during booting and that it's invalid.
> 

One thing that worries me is the false negatives, what happens if the
boot address register (WAKEUP_NS_PA_ADDR) is pointing outside the new
kernel (which will most likely be the case as it probably will be
pointing to the old kernel), but the core is not correctly parked (lets
say it is in WFI idle loop in old kernel), we will fail to reboot it
when in this case we should.

I think the best way to handle this is to set WAKEUP_NS_PA_ADDR to zero
when we park it, for all other cases we can assume it is not parked. As
long as we remember in the bootloader and kernel to zero that register
when parking it, then we shouldn't have any false positives or any
failures to reset when we should have.

Andrew

> And while at it, let's replace legacy cpu_is usage with soc_is to avoid
> confusion with mixed use of both checks.
> 
> Additionally we also need to fix the hot-unplug code to properly park CPU1
> to the bootrom loop so it's not affected by SDRAM changes done by kexec
> booting kernel.
> 
> Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
> Reported-by: Andrew F. Davis <afd@ti.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/common.h              |  1 +
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c | 30 +++++++++----
>  arch/arm/mach-omap2/omap-smp.c            | 70 +++++++++++++++++++++++++------
>  3 files changed, 80 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
>  extern int omap4_mpuss_init(void);
>  extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
>  extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
> +extern u32 omap4_get_cpu1_ns_pa_addr(void);
>  #else
>  static inline int omap4_enter_lowpower(unsigned int cpu,
>  					unsigned int power_state)
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -64,6 +64,7 @@
>  #include "prm-regbits-44xx.h"
>  
>  static void __iomem *sar_base;
> +static u32 old_cpu1_ns_pa_addr;
>  
>  #if defined(CONFIG_PM) && defined(CONFIG_SMP)
>  
> @@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
>  {}
>  #endif
>  
> +u32 omap4_get_cpu1_ns_pa_addr(void)
> +{
> +	return old_cpu1_ns_pa_addr;
> +}
> +
>  /**
>   * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
>   * The purpose of this function is to manage low power programming
> @@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x0);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x1);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
>  		save_l2x0_context();
>  	}
>  
> -	if (cpu_is_omap44xx()) {
> +	if (soc_is_omap44xx()) {
>  		omap_pm_ops.finish_suspend = omap4_finish_suspend;
>  		omap_pm_ops.resume = omap4_cpu_resume;
>  		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
> @@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
>  		enable_mercury_retention_mode();
>  	}
>  
> -	if (cpu_is_omap446x())
> +	if (soc_is_omap446x())
>  		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
>  
>  	return 0;
> @@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
>  void __init omap4_mpuss_early_init(void)
>  {
>  	unsigned long startup_pa;
> +	void __iomem *ns_pa_addr;
>  
> -	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
> +	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
>  		return;
>  
>  	sar_base = omap4_get_sar_ram_base();
>  
> -	if (cpu_is_omap443x())
> +	/* Save old NS_PA_ADDR for validity checks later on */
> +	if (soc_is_omap44xx())
> +		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	else
> +		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
> +
> +	if (soc_is_omap443x())
>  		startup_pa = __pa_symbol(omap4_secondary_startup);
> -	else if (cpu_is_omap446x())
> +	else if (soc_is_omap446x())
>  		startup_pa = __pa_symbol(omap4460_secondary_startup);
>  	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
>  		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
>  	else
>  		startup_pa = __pa_symbol(omap5_secondary_startup);
>  
> -	if (cpu_is_omap44xx())
> +	if (soc_is_omap44xx())
>  		writel_relaxed(startup_pa, sar_base +
>  			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
>  	else
> diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> --- a/arch/arm/mach-omap2/omap-smp.c
> +++ b/arch/arm/mach-omap2/omap-smp.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/irqchip/arm-gic.h>
>  
> +#include <asm/sections.h>
>  #include <asm/smp_scu.h>
>  #include <asm/virt.h>
>  
> @@ -44,6 +45,7 @@ struct omap_smp_config {
>  	unsigned long cpu1_rstctrl_pa;
>  	void __iomem *cpu1_rstctrl_va;
>  	void __iomem *scu_base;
> +	void __iomem *wakeupgen_base;
>  	void *startup_addr;
>  };
>  
> @@ -140,7 +142,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	static struct clockdomain *cpu1_clkdm;
>  	static bool booted;
>  	static struct powerdomain *cpu1_pwrdm;
> -	void __iomem *base = omap_get_wakeupgen_base();
>  
>  	/*
>  	 * Set synchronisation state between this boot processor
> @@ -157,7 +158,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	if (omap_secure_apis_support())
>  		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
>  	else
> -		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
> +		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
>  
>  	if (!cpu1_clkdm && !cpu1_pwrdm) {
>  		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
> @@ -261,9 +262,59 @@ static void __init omap4_smp_init_cpus(void)
>  		set_cpu_possible(i, true);
>  }
>  
> +/*
> + * For now, just make sure the start-up address is not within the booting
> + * kernel space as that means we just overwrote whatever secondary_startup()
> + * code there was.
> + */
> +static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
> +{
> +	if ((addr >= __pa(PAGE_OFFSET)) && (addr <= __pa(__bss_start)))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * We may need to reset CPU1 before configuring, otherwise kexec boot can end
> + * up trying to use old kernel startup address or suspend-resume will
> + * occasionally fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper
> + * idle states.
> + */
> +static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
> +{
> +	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
> +	bool needs_reset = false;
> +
> +	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
> +					OMAP_AUX_CORE_BOOT_1);
> +	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
> +
> +	/* Did the configured secondary_startup() get overwritten? */
> +	if (!omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
> +		needs_reset = true;
> +
> +	/*
> +	 * If omap4 or 5 has NS_PA_ADDR configured, CPU1 may be in a
> +	 * deeper idle state in WFI and wake to an invalid address.
> +	 */
> +	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
> +	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr))
> +		needs_reset = true;
> +
> +	if (!needs_reset || !c->cpu1_rstctrl_va)
> +		return;
> +
> +	pr_info("smp: Already configured CPU1, needs reset (0x%lx 0x%lx)\n",
> +		cpu1_startup_pa, cpu1_ns_pa_addr);
> +
> +	writel_relaxed(1, c->cpu1_rstctrl_va);
> +	readl_relaxed(c->cpu1_rstctrl_va);
> +	writel_relaxed(0, c->cpu1_rstctrl_va);
> +}
> +
>  static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  {
> -	void __iomem *base = omap_get_wakeupgen_base();
>  	const struct omap_smp_config *c = NULL;
>  
>  	if (soc_is_omap443x())
> @@ -281,6 +332,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	/* Must preserve cfg.scu_base set earlier */
>  	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
>  	cfg.startup_addr = c->startup_addr;
> +	cfg.wakeupgen_base = omap_get_wakeupgen_base();
>  
>  	if (soc_is_dra74x() || soc_is_omap54xx()) {
>  		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
> @@ -299,15 +351,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	if (cfg.scu_base)
>  		scu_enable(cfg.scu_base);
>  
> -	/*
> -	 * Reset CPU1 before configuring, otherwise kexec will
> -	 * end up trying to use old kernel startup address.
> -	 */
> -	if (cfg.cpu1_rstctrl_va) {
> -		writel_relaxed(1, cfg.cpu1_rstctrl_va);
> -		readl_relaxed(cfg.cpu1_rstctrl_va);
> -		writel_relaxed(0, cfg.cpu1_rstctrl_va);
> -	}
> +	omap4_smp_maybe_reset_cpu1(&cfg);
>  
>  	/*
>  	 * Write the address of secondary startup routine into the
> @@ -319,7 +363,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
>  	else
>  		writel_relaxed(__pa_symbol(cfg.startup_addr),
> -			       base + OMAP_AUX_CORE_BOOT_1);
> +			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
>  }
>  
>  const struct smp_operations omap4_smp_ops __initconst = {
> 

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/13/2017 03:52 PM, Tony Lindgren wrote:
> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> unconditionally resetting CPU1 because of a kexec boot issue I was seeing
> earlier on omap4 when doing kexec boot between two different kernel
> versions.
> 
> This caused issues on some systems. We should only reset CPU1 as a last
> resort option, and try to avoid it where possible. Doing an unconditional
> CPU1 reset causes issues for example when booting a bootloader configured
> secure OS running on CPU1 as reported by Andrew F. Davis <afd@ti.com>.
> 
> We can't completely remove the reset of CPU1 as it would break
> kexec booting from older kernels. Also it looks like omap4 suspend/resume
> cycle occasionally fails to bring up CPU1 without the reset of CPU1.
> 
> The ideal fix is to park CPU1 to the bootrom loop during CPU hot-unplug.
> This way we can be assured that the CPU1 is properly configured by
> bootloader or the previous kernel. However, we can't do that for the
> earlier kernels out there capable of kexec booting new kernels.
> 
> So let's fix the issue reported by Andrew by making CPU1 reset conditional
> and only do it if CPU1 is configured to boot at an address that is within
> the booting kernel. In these cases we know for sure that we just overwrote
> the configured boot_secondary() during booting and that it's invalid.
> 

One thing that worries me is the false negatives, what happens if the
boot address register (WAKEUP_NS_PA_ADDR) is pointing outside the new
kernel (which will most likely be the case as it probably will be
pointing to the old kernel), but the core is not correctly parked (lets
say it is in WFI idle loop in old kernel), we will fail to reboot it
when in this case we should.

I think the best way to handle this is to set WAKEUP_NS_PA_ADDR to zero
when we park it, for all other cases we can assume it is not parked. As
long as we remember in the bootloader and kernel to zero that register
when parking it, then we shouldn't have any false positives or any
failures to reset when we should have.

Andrew

> And while at it, let's replace legacy cpu_is usage with soc_is to avoid
> confusion with mixed use of both checks.
> 
> Additionally we also need to fix the hot-unplug code to properly park CPU1
> to the bootrom loop so it's not affected by SDRAM changes done by kexec
> booting kernel.
> 
> Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
> Reported-by: Andrew F. Davis <afd@ti.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/common.h              |  1 +
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c | 30 +++++++++----
>  arch/arm/mach-omap2/omap-smp.c            | 70 +++++++++++++++++++++++++------
>  3 files changed, 80 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
>  extern int omap4_mpuss_init(void);
>  extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
>  extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
> +extern u32 omap4_get_cpu1_ns_pa_addr(void);
>  #else
>  static inline int omap4_enter_lowpower(unsigned int cpu,
>  					unsigned int power_state)
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -64,6 +64,7 @@
>  #include "prm-regbits-44xx.h"
>  
>  static void __iomem *sar_base;
> +static u32 old_cpu1_ns_pa_addr;
>  
>  #if defined(CONFIG_PM) && defined(CONFIG_SMP)
>  
> @@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
>  {}
>  #endif
>  
> +u32 omap4_get_cpu1_ns_pa_addr(void)
> +{
> +	return old_cpu1_ns_pa_addr;
> +}
> +
>  /**
>   * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
>   * The purpose of this function is to manage low power programming
> @@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x0);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x1);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
>  		save_l2x0_context();
>  	}
>  
> -	if (cpu_is_omap44xx()) {
> +	if (soc_is_omap44xx()) {
>  		omap_pm_ops.finish_suspend = omap4_finish_suspend;
>  		omap_pm_ops.resume = omap4_cpu_resume;
>  		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
> @@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
>  		enable_mercury_retention_mode();
>  	}
>  
> -	if (cpu_is_omap446x())
> +	if (soc_is_omap446x())
>  		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
>  
>  	return 0;
> @@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
>  void __init omap4_mpuss_early_init(void)
>  {
>  	unsigned long startup_pa;
> +	void __iomem *ns_pa_addr;
>  
> -	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
> +	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
>  		return;
>  
>  	sar_base = omap4_get_sar_ram_base();
>  
> -	if (cpu_is_omap443x())
> +	/* Save old NS_PA_ADDR for validity checks later on */
> +	if (soc_is_omap44xx())
> +		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	else
> +		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
> +
> +	if (soc_is_omap443x())
>  		startup_pa = __pa_symbol(omap4_secondary_startup);
> -	else if (cpu_is_omap446x())
> +	else if (soc_is_omap446x())
>  		startup_pa = __pa_symbol(omap4460_secondary_startup);
>  	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
>  		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
>  	else
>  		startup_pa = __pa_symbol(omap5_secondary_startup);
>  
> -	if (cpu_is_omap44xx())
> +	if (soc_is_omap44xx())
>  		writel_relaxed(startup_pa, sar_base +
>  			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
>  	else
> diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> --- a/arch/arm/mach-omap2/omap-smp.c
> +++ b/arch/arm/mach-omap2/omap-smp.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/irqchip/arm-gic.h>
>  
> +#include <asm/sections.h>
>  #include <asm/smp_scu.h>
>  #include <asm/virt.h>
>  
> @@ -44,6 +45,7 @@ struct omap_smp_config {
>  	unsigned long cpu1_rstctrl_pa;
>  	void __iomem *cpu1_rstctrl_va;
>  	void __iomem *scu_base;
> +	void __iomem *wakeupgen_base;
>  	void *startup_addr;
>  };
>  
> @@ -140,7 +142,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	static struct clockdomain *cpu1_clkdm;
>  	static bool booted;
>  	static struct powerdomain *cpu1_pwrdm;
> -	void __iomem *base = omap_get_wakeupgen_base();
>  
>  	/*
>  	 * Set synchronisation state between this boot processor
> @@ -157,7 +158,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	if (omap_secure_apis_support())
>  		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
>  	else
> -		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
> +		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
>  
>  	if (!cpu1_clkdm && !cpu1_pwrdm) {
>  		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
> @@ -261,9 +262,59 @@ static void __init omap4_smp_init_cpus(void)
>  		set_cpu_possible(i, true);
>  }
>  
> +/*
> + * For now, just make sure the start-up address is not within the booting
> + * kernel space as that means we just overwrote whatever secondary_startup()
> + * code there was.
> + */
> +static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
> +{
> +	if ((addr >= __pa(PAGE_OFFSET)) && (addr <= __pa(__bss_start)))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * We may need to reset CPU1 before configuring, otherwise kexec boot can end
> + * up trying to use old kernel startup address or suspend-resume will
> + * occasionally fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper
> + * idle states.
> + */
> +static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
> +{
> +	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
> +	bool needs_reset = false;
> +
> +	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
> +					OMAP_AUX_CORE_BOOT_1);
> +	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
> +
> +	/* Did the configured secondary_startup() get overwritten? */
> +	if (!omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
> +		needs_reset = true;
> +
> +	/*
> +	 * If omap4 or 5 has NS_PA_ADDR configured, CPU1 may be in a
> +	 * deeper idle state in WFI and wake to an invalid address.
> +	 */
> +	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
> +	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr))
> +		needs_reset = true;
> +
> +	if (!needs_reset || !c->cpu1_rstctrl_va)
> +		return;
> +
> +	pr_info("smp: Already configured CPU1, needs reset (0x%lx 0x%lx)\n",
> +		cpu1_startup_pa, cpu1_ns_pa_addr);
> +
> +	writel_relaxed(1, c->cpu1_rstctrl_va);
> +	readl_relaxed(c->cpu1_rstctrl_va);
> +	writel_relaxed(0, c->cpu1_rstctrl_va);
> +}
> +
>  static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  {
> -	void __iomem *base = omap_get_wakeupgen_base();
>  	const struct omap_smp_config *c = NULL;
>  
>  	if (soc_is_omap443x())
> @@ -281,6 +332,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	/* Must preserve cfg.scu_base set earlier */
>  	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
>  	cfg.startup_addr = c->startup_addr;
> +	cfg.wakeupgen_base = omap_get_wakeupgen_base();
>  
>  	if (soc_is_dra74x() || soc_is_omap54xx()) {
>  		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
> @@ -299,15 +351,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	if (cfg.scu_base)
>  		scu_enable(cfg.scu_base);
>  
> -	/*
> -	 * Reset CPU1 before configuring, otherwise kexec will
> -	 * end up trying to use old kernel startup address.
> -	 */
> -	if (cfg.cpu1_rstctrl_va) {
> -		writel_relaxed(1, cfg.cpu1_rstctrl_va);
> -		readl_relaxed(cfg.cpu1_rstctrl_va);
> -		writel_relaxed(0, cfg.cpu1_rstctrl_va);
> -	}
> +	omap4_smp_maybe_reset_cpu1(&cfg);
>  
>  	/*
>  	 * Write the address of secondary startup routine into the
> @@ -319,7 +363,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
>  	else
>  		writel_relaxed(__pa_symbol(cfg.startup_addr),
> -			       base + OMAP_AUX_CORE_BOOT_1);
> +			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
>  }
>  
>  const struct smp_operations omap4_smp_ops __initconst = {
> 

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170313 14:30]:
> On 03/13/2017 03:52 PM, Tony Lindgren wrote:
> > So let's fix the issue reported by Andrew by making CPU1 reset conditional
> > and only do it if CPU1 is configured to boot at an address that is within
> > the booting kernel. In these cases we know for sure that we just overwrote
> > the configured boot_secondary() during booting and that it's invalid.
> 
> One thing that worries me is the false negatives, what happens if the
> boot address register (WAKEUP_NS_PA_ADDR) is pointing outside the new
> kernel (which will most likely be the case as it probably will be
> pointing to the old kernel), but the core is not correctly parked (lets
> say it is in WFI idle loop in old kernel), we will fail to reboot it
> when in this case we should.

Yup this won't handle all the cases for sure. The case you're describing
always happens with u-boot, I think it parks CPU1 to the end of DDR.
And then we just blindly assume CPU1 is properly configured.

For kexec booting, the old parked address will be almost certainly
overlapping with the new kernel and the code gets overwritten. I don't
think the kexec case of WAKEUP_NS_PA_ADDR being outside the booted
kernel can happen without patched kernel. But it probably can with
crashkernel configurations, and properly parking the kernel is the
real fix there.

Or do you have some specific check in mind we could add?

> I think the best way to handle this is to set WAKEUP_NS_PA_ADDR to zero
> when we park it, for all other cases we can assume it is not parked. As
> long as we remember in the bootloader and kernel to zero that register
> when parking it, then we shouldn't have any false positives or any
> failures to reset when we should have.

Yeah makes sense. So for CPU1 hot-unplug, we also need to bring up
CPU1 from idle and park it to bootrom to also prevent CPU1 waking
from idle to a wrong address.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170313 14:30]:
> On 03/13/2017 03:52 PM, Tony Lindgren wrote:
> > So let's fix the issue reported by Andrew by making CPU1 reset conditional
> > and only do it if CPU1 is configured to boot at an address that is within
> > the booting kernel. In these cases we know for sure that we just overwrote
> > the configured boot_secondary() during booting and that it's invalid.
> 
> One thing that worries me is the false negatives, what happens if the
> boot address register (WAKEUP_NS_PA_ADDR) is pointing outside the new
> kernel (which will most likely be the case as it probably will be
> pointing to the old kernel), but the core is not correctly parked (lets
> say it is in WFI idle loop in old kernel), we will fail to reboot it
> when in this case we should.

Yup this won't handle all the cases for sure. The case you're describing
always happens with u-boot, I think it parks CPU1 to the end of DDR.
And then we just blindly assume CPU1 is properly configured.

For kexec booting, the old parked address will be almost certainly
overlapping with the new kernel and the code gets overwritten. I don't
think the kexec case of WAKEUP_NS_PA_ADDR being outside the booted
kernel can happen without patched kernel. But it probably can with
crashkernel configurations, and properly parking the kernel is the
real fix there.

Or do you have some specific check in mind we could add?

> I think the best way to handle this is to set WAKEUP_NS_PA_ADDR to zero
> when we park it, for all other cases we can assume it is not parked. As
> long as we remember in the bootloader and kernel to zero that register
> when parking it, then we shouldn't have any false positives or any
> failures to reset when we should have.

Yeah makes sense. So for CPU1 hot-unplug, we also need to bring up
CPU1 from idle and park it to bootrom to also prevent CPU1 waking
from idle to a wrong address.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tero Kristo]

On 13/03/17 22:52, Tony Lindgren wrote:
> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> unconditionally resetting CPU1 because of a kexec boot issue I was seeing
> earlier on omap4 when doing kexec boot between two different kernel
> versions.
>
> This caused issues on some systems. We should only reset CPU1 as a last
> resort option, and try to avoid it where possible. Doing an unconditional
> CPU1 reset causes issues for example when booting a bootloader configured
> secure OS running on CPU1 as reported by Andrew F. Davis <afd@ti.com>.
>
> We can't completely remove the reset of CPU1 as it would break
> kexec booting from older kernels. Also it looks like omap4 suspend/resume
> cycle occasionally fails to bring up CPU1 without the reset of CPU1.
>
> The ideal fix is to park CPU1 to the bootrom loop during CPU hot-unplug.
> This way we can be assured that the CPU1 is properly configured by
> bootloader or the previous kernel. However, we can't do that for the
> earlier kernels out there capable of kexec booting new kernels.
>
> So let's fix the issue reported by Andrew by making CPU1 reset conditional
> and only do it if CPU1 is configured to boot at an address that is within
> the booting kernel. In these cases we know for sure that we just overwrote
> the configured boot_secondary() during booting and that it's invalid.
>
> And while at it, let's replace legacy cpu_is usage with soc_is to avoid
> confusion with mixed use of both checks.
>
> Additionally we also need to fix the hot-unplug code to properly park CPU1
> to the bootrom loop so it's not affected by SDRAM changes done by kexec
> booting kernel.

Imo, we are doing too much bandaid hackery for this issue now. How much 
do we care if the older kernels don't work properly with kexec? I know I 
don't care a bit myself. It means you just need to do 1 cold-boot for 
the system to fix it.

If we just drop all the cpu1 reset code and park cpu1 properly at end 
when executing kexec we should be just fine, and the implementation will 
be much cleaner.

Does the wakeup issue for cpu1 ever happen with suspend, so that you 
actually must reset the cpu so that it comes up? If that is the case, it 
is going to be a tricky situation overall and we should understand the 
root cause for it before trying to fix it.

-Tero

>
> Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
> Reported-by: Andrew F. Davis <afd@ti.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/common.h              |  1 +
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c | 30 +++++++++----
>  arch/arm/mach-omap2/omap-smp.c            | 70 +++++++++++++++++++++++++------
>  3 files changed, 80 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
>  extern int omap4_mpuss_init(void);
>  extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
>  extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
> +extern u32 omap4_get_cpu1_ns_pa_addr(void);
>  #else
>  static inline int omap4_enter_lowpower(unsigned int cpu,
>  					unsigned int power_state)
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -64,6 +64,7 @@
>  #include "prm-regbits-44xx.h"
>
>  static void __iomem *sar_base;
> +static u32 old_cpu1_ns_pa_addr;
>
>  #if defined(CONFIG_PM) && defined(CONFIG_SMP)
>
> @@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
>  {}
>  #endif
>
> +u32 omap4_get_cpu1_ns_pa_addr(void)
> +{
> +	return old_cpu1_ns_pa_addr;
> +}
> +
>  /**
>   * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
>   * The purpose of this function is to manage low power programming
> @@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x0);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x1);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
>  		save_l2x0_context();
>  	}
>
> -	if (cpu_is_omap44xx()) {
> +	if (soc_is_omap44xx()) {
>  		omap_pm_ops.finish_suspend = omap4_finish_suspend;
>  		omap_pm_ops.resume = omap4_cpu_resume;
>  		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
> @@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
>  		enable_mercury_retention_mode();
>  	}
>
> -	if (cpu_is_omap446x())
> +	if (soc_is_omap446x())
>  		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
>
>  	return 0;
> @@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
>  void __init omap4_mpuss_early_init(void)
>  {
>  	unsigned long startup_pa;
> +	void __iomem *ns_pa_addr;
>
> -	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
> +	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
>  		return;
>
>  	sar_base = omap4_get_sar_ram_base();
>
> -	if (cpu_is_omap443x())
> +	/* Save old NS_PA_ADDR for validity checks later on */
> +	if (soc_is_omap44xx())
> +		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	else
> +		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
> +
> +	if (soc_is_omap443x())
>  		startup_pa = __pa_symbol(omap4_secondary_startup);
> -	else if (cpu_is_omap446x())
> +	else if (soc_is_omap446x())
>  		startup_pa = __pa_symbol(omap4460_secondary_startup);
>  	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
>  		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
>  	else
>  		startup_pa = __pa_symbol(omap5_secondary_startup);
>
> -	if (cpu_is_omap44xx())
> +	if (soc_is_omap44xx())
>  		writel_relaxed(startup_pa, sar_base +
>  			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
>  	else
> diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> --- a/arch/arm/mach-omap2/omap-smp.c
> +++ b/arch/arm/mach-omap2/omap-smp.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/irqchip/arm-gic.h>
>
> +#include <asm/sections.h>
>  #include <asm/smp_scu.h>
>  #include <asm/virt.h>
>
> @@ -44,6 +45,7 @@ struct omap_smp_config {
>  	unsigned long cpu1_rstctrl_pa;
>  	void __iomem *cpu1_rstctrl_va;
>  	void __iomem *scu_base;
> +	void __iomem *wakeupgen_base;
>  	void *startup_addr;
>  };
>
> @@ -140,7 +142,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	static struct clockdomain *cpu1_clkdm;
>  	static bool booted;
>  	static struct powerdomain *cpu1_pwrdm;
> -	void __iomem *base = omap_get_wakeupgen_base();
>
>  	/*
>  	 * Set synchronisation state between this boot processor
> @@ -157,7 +158,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	if (omap_secure_apis_support())
>  		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
>  	else
> -		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
> +		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
>
>  	if (!cpu1_clkdm && !cpu1_pwrdm) {
>  		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
> @@ -261,9 +262,59 @@ static void __init omap4_smp_init_cpus(void)
>  		set_cpu_possible(i, true);
>  }
>
> +/*
> + * For now, just make sure the start-up address is not within the booting
> + * kernel space as that means we just overwrote whatever secondary_startup()
> + * code there was.
> + */
> +static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
> +{
> +	if ((addr >= __pa(PAGE_OFFSET)) && (addr <= __pa(__bss_start)))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * We may need to reset CPU1 before configuring, otherwise kexec boot can end
> + * up trying to use old kernel startup address or suspend-resume will
> + * occasionally fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper
> + * idle states.
> + */
> +static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
> +{
> +	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
> +	bool needs_reset = false;
> +
> +	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
> +					OMAP_AUX_CORE_BOOT_1);
> +	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
> +
> +	/* Did the configured secondary_startup() get overwritten? */
> +	if (!omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
> +		needs_reset = true;
> +
> +	/*
> +	 * If omap4 or 5 has NS_PA_ADDR configured, CPU1 may be in a
> +	 * deeper idle state in WFI and wake to an invalid address.
> +	 */
> +	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
> +	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr))
> +		needs_reset = true;
> +
> +	if (!needs_reset || !c->cpu1_rstctrl_va)
> +		return;
> +
> +	pr_info("smp: Already configured CPU1, needs reset (0x%lx 0x%lx)\n",
> +		cpu1_startup_pa, cpu1_ns_pa_addr);
> +
> +	writel_relaxed(1, c->cpu1_rstctrl_va);
> +	readl_relaxed(c->cpu1_rstctrl_va);
> +	writel_relaxed(0, c->cpu1_rstctrl_va);
> +}
> +
>  static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  {
> -	void __iomem *base = omap_get_wakeupgen_base();
>  	const struct omap_smp_config *c = NULL;
>
>  	if (soc_is_omap443x())
> @@ -281,6 +332,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	/* Must preserve cfg.scu_base set earlier */
>  	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
>  	cfg.startup_addr = c->startup_addr;
> +	cfg.wakeupgen_base = omap_get_wakeupgen_base();
>
>  	if (soc_is_dra74x() || soc_is_omap54xx()) {
>  		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
> @@ -299,15 +351,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	if (cfg.scu_base)
>  		scu_enable(cfg.scu_base);
>
> -	/*
> -	 * Reset CPU1 before configuring, otherwise kexec will
> -	 * end up trying to use old kernel startup address.
> -	 */
> -	if (cfg.cpu1_rstctrl_va) {
> -		writel_relaxed(1, cfg.cpu1_rstctrl_va);
> -		readl_relaxed(cfg.cpu1_rstctrl_va);
> -		writel_relaxed(0, cfg.cpu1_rstctrl_va);
> -	}
> +	omap4_smp_maybe_reset_cpu1(&cfg);
>
>  	/*
>  	 * Write the address of secondary startup routine into the
> @@ -319,7 +363,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
>  	else
>  		writel_relaxed(__pa_symbol(cfg.startup_addr),
> -			       base + OMAP_AUX_CORE_BOOT_1);
> +			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
>  }
>
>  const struct smp_operations omap4_smp_ops __initconst = {
>

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tero Kristo]

On 13/03/17 22:52, Tony Lindgren wrote:
> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> unconditionally resetting CPU1 because of a kexec boot issue I was seeing
> earlier on omap4 when doing kexec boot between two different kernel
> versions.
>
> This caused issues on some systems. We should only reset CPU1 as a last
> resort option, and try to avoid it where possible. Doing an unconditional
> CPU1 reset causes issues for example when booting a bootloader configured
> secure OS running on CPU1 as reported by Andrew F. Davis <afd@ti.com>.
>
> We can't completely remove the reset of CPU1 as it would break
> kexec booting from older kernels. Also it looks like omap4 suspend/resume
> cycle occasionally fails to bring up CPU1 without the reset of CPU1.
>
> The ideal fix is to park CPU1 to the bootrom loop during CPU hot-unplug.
> This way we can be assured that the CPU1 is properly configured by
> bootloader or the previous kernel. However, we can't do that for the
> earlier kernels out there capable of kexec booting new kernels.
>
> So let's fix the issue reported by Andrew by making CPU1 reset conditional
> and only do it if CPU1 is configured to boot at an address that is within
> the booting kernel. In these cases we know for sure that we just overwrote
> the configured boot_secondary() during booting and that it's invalid.
>
> And while at it, let's replace legacy cpu_is usage with soc_is to avoid
> confusion with mixed use of both checks.
>
> Additionally we also need to fix the hot-unplug code to properly park CPU1
> to the bootrom loop so it's not affected by SDRAM changes done by kexec
> booting kernel.

Imo, we are doing too much bandaid hackery for this issue now. How much 
do we care if the older kernels don't work properly with kexec? I know I 
don't care a bit myself. It means you just need to do 1 cold-boot for 
the system to fix it.

If we just drop all the cpu1 reset code and park cpu1 properly at end 
when executing kexec we should be just fine, and the implementation will 
be much cleaner.

Does the wakeup issue for cpu1 ever happen with suspend, so that you 
actually must reset the cpu so that it comes up? If that is the case, it 
is going to be a tricky situation overall and we should understand the 
root cause for it before trying to fix it.

-Tero

>
> Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
> Reported-by: Andrew F. Davis <afd@ti.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Russell King <rmk+kernel@armlinux.org.uk>
> Cc: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/common.h              |  1 +
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c | 30 +++++++++----
>  arch/arm/mach-omap2/omap-smp.c            | 70 +++++++++++++++++++++++++------
>  3 files changed, 80 insertions(+), 21 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
> --- a/arch/arm/mach-omap2/common.h
> +++ b/arch/arm/mach-omap2/common.h
> @@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
>  extern int omap4_mpuss_init(void);
>  extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
>  extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
> +extern u32 omap4_get_cpu1_ns_pa_addr(void);
>  #else
>  static inline int omap4_enter_lowpower(unsigned int cpu,
>  					unsigned int power_state)
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -64,6 +64,7 @@
>  #include "prm-regbits-44xx.h"
>
>  static void __iomem *sar_base;
> +static u32 old_cpu1_ns_pa_addr;
>
>  #if defined(CONFIG_PM) && defined(CONFIG_SMP)
>
> @@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
>  {}
>  #endif
>
> +u32 omap4_get_cpu1_ns_pa_addr(void)
> +{
> +	return old_cpu1_ns_pa_addr;
> +}
> +
>  /**
>   * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
>   * The purpose of this function is to manage low power programming
> @@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x0);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
>  	pm_info = &per_cpu(omap4_pm_info, 0x1);
>  	if (sar_base) {
>  		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
> -		if (cpu_is_omap44xx())
> +		if (soc_is_omap44xx())
>  			pm_info->wkup_sar_addr = sar_base +
>  				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
>  		else
> @@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
>  		save_l2x0_context();
>  	}
>
> -	if (cpu_is_omap44xx()) {
> +	if (soc_is_omap44xx()) {
>  		omap_pm_ops.finish_suspend = omap4_finish_suspend;
>  		omap_pm_ops.resume = omap4_cpu_resume;
>  		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
> @@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
>  		enable_mercury_retention_mode();
>  	}
>
> -	if (cpu_is_omap446x())
> +	if (soc_is_omap446x())
>  		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
>
>  	return 0;
> @@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
>  void __init omap4_mpuss_early_init(void)
>  {
>  	unsigned long startup_pa;
> +	void __iomem *ns_pa_addr;
>
> -	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
> +	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
>  		return;
>
>  	sar_base = omap4_get_sar_ram_base();
>
> -	if (cpu_is_omap443x())
> +	/* Save old NS_PA_ADDR for validity checks later on */
> +	if (soc_is_omap44xx())
> +		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	else
> +		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
> +	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
> +
> +	if (soc_is_omap443x())
>  		startup_pa = __pa_symbol(omap4_secondary_startup);
> -	else if (cpu_is_omap446x())
> +	else if (soc_is_omap446x())
>  		startup_pa = __pa_symbol(omap4460_secondary_startup);
>  	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
>  		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
>  	else
>  		startup_pa = __pa_symbol(omap5_secondary_startup);
>
> -	if (cpu_is_omap44xx())
> +	if (soc_is_omap44xx())
>  		writel_relaxed(startup_pa, sar_base +
>  			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
>  	else
> diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> --- a/arch/arm/mach-omap2/omap-smp.c
> +++ b/arch/arm/mach-omap2/omap-smp.c
> @@ -21,6 +21,7 @@
>  #include <linux/io.h>
>  #include <linux/irqchip/arm-gic.h>
>
> +#include <asm/sections.h>
>  #include <asm/smp_scu.h>
>  #include <asm/virt.h>
>
> @@ -44,6 +45,7 @@ struct omap_smp_config {
>  	unsigned long cpu1_rstctrl_pa;
>  	void __iomem *cpu1_rstctrl_va;
>  	void __iomem *scu_base;
> +	void __iomem *wakeupgen_base;
>  	void *startup_addr;
>  };
>
> @@ -140,7 +142,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	static struct clockdomain *cpu1_clkdm;
>  	static bool booted;
>  	static struct powerdomain *cpu1_pwrdm;
> -	void __iomem *base = omap_get_wakeupgen_base();
>
>  	/*
>  	 * Set synchronisation state between this boot processor
> @@ -157,7 +158,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  	if (omap_secure_apis_support())
>  		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
>  	else
> -		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
> +		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
>
>  	if (!cpu1_clkdm && !cpu1_pwrdm) {
>  		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
> @@ -261,9 +262,59 @@ static void __init omap4_smp_init_cpus(void)
>  		set_cpu_possible(i, true);
>  }
>
> +/*
> + * For now, just make sure the start-up address is not within the booting
> + * kernel space as that means we just overwrote whatever secondary_startup()
> + * code there was.
> + */
> +static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
> +{
> +	if ((addr >= __pa(PAGE_OFFSET)) && (addr <= __pa(__bss_start)))
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * We may need to reset CPU1 before configuring, otherwise kexec boot can end
> + * up trying to use old kernel startup address or suspend-resume will
> + * occasionally fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper
> + * idle states.
> + */
> +static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
> +{
> +	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
> +	bool needs_reset = false;
> +
> +	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
> +					OMAP_AUX_CORE_BOOT_1);
> +	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
> +
> +	/* Did the configured secondary_startup() get overwritten? */
> +	if (!omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
> +		needs_reset = true;
> +
> +	/*
> +	 * If omap4 or 5 has NS_PA_ADDR configured, CPU1 may be in a
> +	 * deeper idle state in WFI and wake to an invalid address.
> +	 */
> +	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
> +	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr))
> +		needs_reset = true;
> +
> +	if (!needs_reset || !c->cpu1_rstctrl_va)
> +		return;
> +
> +	pr_info("smp: Already configured CPU1, needs reset (0x%lx 0x%lx)\n",
> +		cpu1_startup_pa, cpu1_ns_pa_addr);
> +
> +	writel_relaxed(1, c->cpu1_rstctrl_va);
> +	readl_relaxed(c->cpu1_rstctrl_va);
> +	writel_relaxed(0, c->cpu1_rstctrl_va);
> +}
> +
>  static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  {
> -	void __iomem *base = omap_get_wakeupgen_base();
>  	const struct omap_smp_config *c = NULL;
>
>  	if (soc_is_omap443x())
> @@ -281,6 +332,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	/* Must preserve cfg.scu_base set earlier */
>  	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
>  	cfg.startup_addr = c->startup_addr;
> +	cfg.wakeupgen_base = omap_get_wakeupgen_base();
>
>  	if (soc_is_dra74x() || soc_is_omap54xx()) {
>  		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
> @@ -299,15 +351,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  	if (cfg.scu_base)
>  		scu_enable(cfg.scu_base);
>
> -	/*
> -	 * Reset CPU1 before configuring, otherwise kexec will
> -	 * end up trying to use old kernel startup address.
> -	 */
> -	if (cfg.cpu1_rstctrl_va) {
> -		writel_relaxed(1, cfg.cpu1_rstctrl_va);
> -		readl_relaxed(cfg.cpu1_rstctrl_va);
> -		writel_relaxed(0, cfg.cpu1_rstctrl_va);
> -	}
> +	omap4_smp_maybe_reset_cpu1(&cfg);
>
>  	/*
>  	 * Write the address of secondary startup routine into the
> @@ -319,7 +363,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
>  	else
>  		writel_relaxed(__pa_symbol(cfg.startup_addr),
> -			       base + OMAP_AUX_CORE_BOOT_1);
> +			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
>  }
>
>  const struct smp_operations omap4_smp_ops __initconst = {
>

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [170314 00:32]:
> On 13/03/17 22:52, Tony Lindgren wrote:
> > Additionally we also need to fix the hot-unplug code to properly park CPU1
> > to the bootrom loop so it's not affected by SDRAM changes done by kexec
> > booting kernel.
> 
> Imo, we are doing too much bandaid hackery for this issue now. How much do
> we care if the older kernels don't work properly with kexec? I know I don't
> care a bit myself. It means you just need to do 1 cold-boot for the system
> to fix it.

Well kexec is a standard Linux feature and it is currently working and
at least I care.

And if the CPU1 start-up address is programmed to be in the currently
booting kernel's area by something else, it's almost certainly totally
broken.

Note that this still does not remove the need to park CPU1 properly to
bootrom loop.

> If we just drop all the cpu1 reset code and park cpu1 properly at end when
> executing kexec we should be just fine, and the implementation will be much
> cleaner.
> 
> Does the wakeup issue for cpu1 ever happen with suspend, so that you
> actually must reset the cpu so that it comes up? If that is the case, it is
> going to be a tricky situation overall and we should understand the root
> cause for it before trying to fix it.

Yes omap4 CPU1 can fail occasional suspend/resume cycle. But let's assume
that's a separate issue or a side effect of not properly initializing
CPU1 during kexec before the suspend/resume cycle. I can do some more
tests on that.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [170314 00:32]:
> On 13/03/17 22:52, Tony Lindgren wrote:
> > Additionally we also need to fix the hot-unplug code to properly park CPU1
> > to the bootrom loop so it's not affected by SDRAM changes done by kexec
> > booting kernel.
> 
> Imo, we are doing too much bandaid hackery for this issue now. How much do
> we care if the older kernels don't work properly with kexec? I know I don't
> care a bit myself. It means you just need to do 1 cold-boot for the system
> to fix it.

Well kexec is a standard Linux feature and it is currently working and
at least I care.

And if the CPU1 start-up address is programmed to be in the currently
booting kernel's area by something else, it's almost certainly totally
broken.

Note that this still does not remove the need to park CPU1 properly to
bootrom loop.

> If we just drop all the cpu1 reset code and park cpu1 properly at end when
> executing kexec we should be just fine, and the implementation will be much
> cleaner.
> 
> Does the wakeup issue for cpu1 ever happen with suspend, so that you
> actually must reset the cpu so that it comes up? If that is the case, it is
> going to be a tricky situation overall and we should understand the root
> cause for it before trying to fix it.

Yes omap4 CPU1 can fail occasional suspend/resume cycle. But let's assume
that's a separate issue or a side effect of not properly initializing
CPU1 during kexec before the suspend/resume cycle. I can do some more
tests on that.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/14/2017 10:17 AM, Tony Lindgren wrote:
> * Tero Kristo <t-kristo@ti.com> [170314 00:32]:
>> On 13/03/17 22:52, Tony Lindgren wrote:
>>> Additionally we also need to fix the hot-unplug code to properly park CPU1
>>> to the bootrom loop so it's not affected by SDRAM changes done by kexec
>>> booting kernel.
>>
>> Imo, we are doing too much bandaid hackery for this issue now. How much do
>> we care if the older kernels don't work properly with kexec? I know I don't
>> care a bit myself. It means you just need to do 1 cold-boot for the system
>> to fix it.
> 
> Well kexec is a standard Linux feature and it is currently working and
> at least I care.
> 
> And if the CPU1 start-up address is programmed to be in the currently
> booting kernel's area by something else, it's almost certainly totally
> broken.
> 

I disagree, the core can be parked correctly and still have the boot-to
address point to anywhere. There are two registers in play here, one
sets the target jump-to address, the other lets it out of the parking
loop. To know we are not parked we need to check the parking release
register, the jump-to address is completely irrelevant as a check for
proper parking.

> Note that this still does not remove the need to park CPU1 properly to
> bootrom loop.
> 

I agree, and if we always park the the core correctly every time then
there is no reason to do all these checks and resets, we just boot as
normal.

If the core is not parked correctly then the system is messed up and
should be cold-booted, we have no way to know whether that core is off
running in space wreaking stuff.

I cannot think of any reliable test, for now the focus should be
figuring out why we have escaped cores in the first place.

In my testing, core1 always ends up looping in omap4_cpu_die(), the loop
in this function doesn't check for the boot-to address register, just
the core release register. I'm not sure how this works at all, if even
in the same kernel, when trying to bring this core back up it will
ignore our set boot-to address and return to the caller of this
function. Maybe the kernel is able to cope with that, but it does not
seem to be the intended wakeup path.

I'll do some testing and see if this will help in omap4_cpu_die():

if (boot_cpu == smp_processor_id()) {
	/*
	 * OK, proper wakeup, we're done
	 */
+
+	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
+	boot_address();
+
	break;
}

Andrew

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/14/2017 10:17 AM, Tony Lindgren wrote:
> * Tero Kristo <t-kristo@ti.com> [170314 00:32]:
>> On 13/03/17 22:52, Tony Lindgren wrote:
>>> Additionally we also need to fix the hot-unplug code to properly park CPU1
>>> to the bootrom loop so it's not affected by SDRAM changes done by kexec
>>> booting kernel.
>>
>> Imo, we are doing too much bandaid hackery for this issue now. How much do
>> we care if the older kernels don't work properly with kexec? I know I don't
>> care a bit myself. It means you just need to do 1 cold-boot for the system
>> to fix it.
> 
> Well kexec is a standard Linux feature and it is currently working and
> at least I care.
> 
> And if the CPU1 start-up address is programmed to be in the currently
> booting kernel's area by something else, it's almost certainly totally
> broken.
> 

I disagree, the core can be parked correctly and still have the boot-to
address point to anywhere. There are two registers in play here, one
sets the target jump-to address, the other lets it out of the parking
loop. To know we are not parked we need to check the parking release
register, the jump-to address is completely irrelevant as a check for
proper parking.

> Note that this still does not remove the need to park CPU1 properly to
> bootrom loop.
> 

I agree, and if we always park the the core correctly every time then
there is no reason to do all these checks and resets, we just boot as
normal.

If the core is not parked correctly then the system is messed up and
should be cold-booted, we have no way to know whether that core is off
running in space wreaking stuff.

I cannot think of any reliable test, for now the focus should be
figuring out why we have escaped cores in the first place.

In my testing, core1 always ends up looping in omap4_cpu_die(), the loop
in this function doesn't check for the boot-to address register, just
the core release register. I'm not sure how this works at all, if even
in the same kernel, when trying to bring this core back up it will
ignore our set boot-to address and return to the caller of this
function. Maybe the kernel is able to cope with that, but it does not
seem to be the intended wakeup path.

I'll do some testing and see if this will help in omap4_cpu_die():

if (boot_cpu == smp_processor_id()) {
	/*
	 * OK, proper wakeup, we're done
	 */
+
+	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
+	boot_address();
+
	break;
}

Andrew

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170314 09:04]:
> On 03/14/2017 10:17 AM, Tony Lindgren wrote:
> > * Tero Kristo <t-kristo@ti.com> [170314 00:32]:
> >> On 13/03/17 22:52, Tony Lindgren wrote:
> >>> Additionally we also need to fix the hot-unplug code to properly park CPU1
> >>> to the bootrom loop so it's not affected by SDRAM changes done by kexec
> >>> booting kernel.
> >>
> >> Imo, we are doing too much bandaid hackery for this issue now. How much do
> >> we care if the older kernels don't work properly with kexec? I know I don't
> >> care a bit myself. It means you just need to do 1 cold-boot for the system
> >> to fix it.
> > 
> > Well kexec is a standard Linux feature and it is currently working and
> > at least I care.
> > 
> > And if the CPU1 start-up address is programmed to be in the currently
> > booting kernel's area by something else, it's almost certainly totally
> > broken.
> > 
> 
> I disagree, the core can be parked correctly and still have the boot-to
> address point to anywhere. There are two registers in play here, one
> sets the target jump-to address, the other lets it out of the parking
> loop. To know we are not parked we need to check the parking release
> register, the jump-to address is completely irrelevant as a check for
> proper parking.

Well at this point we have CPU1 parked and configured for a wrong
start-up address. So the boot-to address is a valid check. I can
certainly add a check for CPU1 being parked too.

> > Note that this still does not remove the need to park CPU1 properly to
> > bootrom loop.
> > 
> 
> I agree, and if we always park the the core correctly every time then
> there is no reason to do all these checks and resets, we just boot as
> normal.

Correct. And these sanity checks are still needed to keep things
working and fix the regression you reported.

> If the core is not parked correctly then the system is messed up and
> should be cold-booted, we have no way to know whether that core is off
> running in space wreaking stuff.
> 
> I cannot think of any reliable test, for now the focus should be
> figuring out why we have escaped cores in the first place.

Well I think we got there as omap4 powers CPU1 off in idle. And now
we have dra7 using the same code and not doing that.

> In my testing, core1 always ends up looping in omap4_cpu_die(), the loop
> in this function doesn't check for the boot-to address register, just
> the core release register. I'm not sure how this works at all, if even
> in the same kernel, when trying to bring this core back up it will
> ignore our set boot-to address and return to the caller of this
> function. Maybe the kernel is able to cope with that, but it does not
> seem to be the intended wakeup path.

The kernel wakeup_secondary() loops won't work without reset naturally
as that area of memory will be overwritten with different code by the
new kernel booting depending on the kernel version. It only works
kexec booting the same exact kernel version by luck because the code
at that address stays the same. So not very usable :)

> I'll do some testing and see if this will help in omap4_cpu_die():
> 
> if (boot_cpu == smp_processor_id()) {
> 	/*
> 	 * OK, proper wakeup, we're done
> 	 */
> +
> +	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
> +	boot_address();
> +
> 	break;
> }

That's not going to help unless the boot_address is in the bootrom
so it won't get trashed by the next booting kernel in kexec case.
Or else we need to relocate the loop out of the way and that too
gets messy as it can still be overwritten by appended dtb or
initramfs. So it would really be best to have bootloader configure
CPU1, then park it back to bootrom loop both in u-boot and kernel.
Presumably the bootrom loop won't affect CPU1 state so this should
be doable.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170314 09:04]:
> On 03/14/2017 10:17 AM, Tony Lindgren wrote:
> > * Tero Kristo <t-kristo@ti.com> [170314 00:32]:
> >> On 13/03/17 22:52, Tony Lindgren wrote:
> >>> Additionally we also need to fix the hot-unplug code to properly park CPU1
> >>> to the bootrom loop so it's not affected by SDRAM changes done by kexec
> >>> booting kernel.
> >>
> >> Imo, we are doing too much bandaid hackery for this issue now. How much do
> >> we care if the older kernels don't work properly with kexec? I know I don't
> >> care a bit myself. It means you just need to do 1 cold-boot for the system
> >> to fix it.
> > 
> > Well kexec is a standard Linux feature and it is currently working and
> > at least I care.
> > 
> > And if the CPU1 start-up address is programmed to be in the currently
> > booting kernel's area by something else, it's almost certainly totally
> > broken.
> > 
> 
> I disagree, the core can be parked correctly and still have the boot-to
> address point to anywhere. There are two registers in play here, one
> sets the target jump-to address, the other lets it out of the parking
> loop. To know we are not parked we need to check the parking release
> register, the jump-to address is completely irrelevant as a check for
> proper parking.

Well at this point we have CPU1 parked and configured for a wrong
start-up address. So the boot-to address is a valid check. I can
certainly add a check for CPU1 being parked too.

> > Note that this still does not remove the need to park CPU1 properly to
> > bootrom loop.
> > 
> 
> I agree, and if we always park the the core correctly every time then
> there is no reason to do all these checks and resets, we just boot as
> normal.

Correct. And these sanity checks are still needed to keep things
working and fix the regression you reported.

> If the core is not parked correctly then the system is messed up and
> should be cold-booted, we have no way to know whether that core is off
> running in space wreaking stuff.
> 
> I cannot think of any reliable test, for now the focus should be
> figuring out why we have escaped cores in the first place.

Well I think we got there as omap4 powers CPU1 off in idle. And now
we have dra7 using the same code and not doing that.

> In my testing, core1 always ends up looping in omap4_cpu_die(), the loop
> in this function doesn't check for the boot-to address register, just
> the core release register. I'm not sure how this works at all, if even
> in the same kernel, when trying to bring this core back up it will
> ignore our set boot-to address and return to the caller of this
> function. Maybe the kernel is able to cope with that, but it does not
> seem to be the intended wakeup path.

The kernel wakeup_secondary() loops won't work without reset naturally
as that area of memory will be overwritten with different code by the
new kernel booting depending on the kernel version. It only works
kexec booting the same exact kernel version by luck because the code
at that address stays the same. So not very usable :)

> I'll do some testing and see if this will help in omap4_cpu_die():
> 
> if (boot_cpu == smp_processor_id()) {
> 	/*
> 	 * OK, proper wakeup, we're done
> 	 */
> +
> +	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
> +	boot_address();
> +
> 	break;
> }

That's not going to help unless the boot_address is in the bootrom
so it won't get trashed by the next booting kernel in kexec case.
Or else we need to relocate the loop out of the way and that too
gets messy as it can still be overwritten by appended dtb or
initramfs. So it would really be best to have bootloader configure
CPU1, then park it back to bootrom loop both in u-boot and kernel.
Presumably the bootrom loop won't affect CPU1 state so this should
be doable.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/14/2017 11:41 AM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [170314 09:04]:
>> On 03/14/2017 10:17 AM, Tony Lindgren wrote:
>>> * Tero Kristo <t-kristo@ti.com> [170314 00:32]:
>>>> On 13/03/17 22:52, Tony Lindgren wrote:
>>>>> Additionally we also need to fix the hot-unplug code to properly park CPU1
>>>>> to the bootrom loop so it's not affected by SDRAM changes done by kexec
>>>>> booting kernel.
>>>>
>>>> Imo, we are doing too much bandaid hackery for this issue now. How much do
>>>> we care if the older kernels don't work properly with kexec? I know I don't
>>>> care a bit myself. It means you just need to do 1 cold-boot for the system
>>>> to fix it.
>>>
>>> Well kexec is a standard Linux feature and it is currently working and
>>> at least I care.
>>>
>>> And if the CPU1 start-up address is programmed to be in the currently
>>> booting kernel's area by something else, it's almost certainly totally
>>> broken.
>>>
>>
>> I disagree, the core can be parked correctly and still have the boot-to
>> address point to anywhere. There are two registers in play here, one
>> sets the target jump-to address, the other lets it out of the parking
>> loop. To know we are not parked we need to check the parking release
>> register, the jump-to address is completely irrelevant as a check for
>> proper parking.
> 
> Well at this point we have CPU1 parked and configured for a wrong
> start-up address. So the boot-to address is a valid check. I can
> certainly add a check for CPU1 being parked too.
> 
>>> Note that this still does not remove the need to park CPU1 properly to
>>> bootrom loop.
>>>
>>
>> I agree, and if we always park the the core correctly every time then
>> there is no reason to do all these checks and resets, we just boot as
>> normal.
> 
> Correct. And these sanity checks are still needed to keep things
> working and fix the regression you reported.
> 
>> If the core is not parked correctly then the system is messed up and
>> should be cold-booted, we have no way to know whether that core is off
>> running in space wreaking stuff.
>>
>> I cannot think of any reliable test, for now the focus should be
>> figuring out why we have escaped cores in the first place.
> 
> Well I think we got there as omap4 powers CPU1 off in idle. And now
> we have dra7 using the same code and not doing that.
> 

I think this is the case, it may be useful to have them take different
paths.

>> In my testing, core1 always ends up looping in omap4_cpu_die(), the loop
>> in this function doesn't check for the boot-to address register, just
>> the core release register. I'm not sure how this works at all, if even
>> in the same kernel, when trying to bring this core back up it will
>> ignore our set boot-to address and return to the caller of this
>> function. Maybe the kernel is able to cope with that, but it does not
>> seem to be the intended wakeup path.
> 
> The kernel wakeup_secondary() loops won't work without reset naturally
> as that area of memory will be overwritten with different code by the
> new kernel booting depending on the kernel version. It only works
> kexec booting the same exact kernel version by luck because the code
> at that address stays the same. So not very usable :)
> 
>> I'll do some testing and see if this will help in omap4_cpu_die():
>>
>> if (boot_cpu == smp_processor_id()) {
>> 	/*
>> 	 * OK, proper wakeup, we're done
>> 	 */
>> +
>> +	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
>> +	boot_address();
>> +
>> 	break;
>> }
> 
> That's not going to help unless the boot_address is in the bootrom
> so it won't get trashed by the next booting kernel in kexec case.
> Or else we need to relocate the loop out of the way and that too
> gets messy as it can still be overwritten by appended dtb or
> initramfs. So it would really be best to have bootloader configure
> CPU1, then park it back to bootrom loop both in u-boot and kernel.
> Presumably the bootrom loop won't affect CPU1 state so this should
> be doable.
> 

We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
arch/arm/mach-omap2/omap5/sec_entry_cpu1.S

omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
makes an SMC call to setup the core, then re-parks itself back in bootROM.
Relevant code to run on cpu1:

> #define AUX_CORE_BOOT_0		0x48281800
> #define AUX_CORE_BOOT_1		0x48281804
> /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
>  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
>  * This address is same for J6 and J6 Eco.
>  */
> #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
>
> ldr	r4, =AUX_CORE_BOOT_0
> mov	r5, #0x0
> str	r5, [r4]
> ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> bx	r4	@ Jump back to ROM

We would have to do this in kernel. The issue I'm thinking we are going
to hit is that the parking function in bootROM expects the MMU to be
off. Although we don't really need to turn it off as long as we can
identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
we plan to exit the parking loop.

Andrew

> Regards,
> 
> Tony
> 

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/14/2017 11:41 AM, Tony Lindgren wrote:
> * Andrew F. Davis <afd@ti.com> [170314 09:04]:
>> On 03/14/2017 10:17 AM, Tony Lindgren wrote:
>>> * Tero Kristo <t-kristo@ti.com> [170314 00:32]:
>>>> On 13/03/17 22:52, Tony Lindgren wrote:
>>>>> Additionally we also need to fix the hot-unplug code to properly park CPU1
>>>>> to the bootrom loop so it's not affected by SDRAM changes done by kexec
>>>>> booting kernel.
>>>>
>>>> Imo, we are doing too much bandaid hackery for this issue now. How much do
>>>> we care if the older kernels don't work properly with kexec? I know I don't
>>>> care a bit myself. It means you just need to do 1 cold-boot for the system
>>>> to fix it.
>>>
>>> Well kexec is a standard Linux feature and it is currently working and
>>> at least I care.
>>>
>>> And if the CPU1 start-up address is programmed to be in the currently
>>> booting kernel's area by something else, it's almost certainly totally
>>> broken.
>>>
>>
>> I disagree, the core can be parked correctly and still have the boot-to
>> address point to anywhere. There are two registers in play here, one
>> sets the target jump-to address, the other lets it out of the parking
>> loop. To know we are not parked we need to check the parking release
>> register, the jump-to address is completely irrelevant as a check for
>> proper parking.
> 
> Well at this point we have CPU1 parked and configured for a wrong
> start-up address. So the boot-to address is a valid check. I can
> certainly add a check for CPU1 being parked too.
> 
>>> Note that this still does not remove the need to park CPU1 properly to
>>> bootrom loop.
>>>
>>
>> I agree, and if we always park the the core correctly every time then
>> there is no reason to do all these checks and resets, we just boot as
>> normal.
> 
> Correct. And these sanity checks are still needed to keep things
> working and fix the regression you reported.
> 
>> If the core is not parked correctly then the system is messed up and
>> should be cold-booted, we have no way to know whether that core is off
>> running in space wreaking stuff.
>>
>> I cannot think of any reliable test, for now the focus should be
>> figuring out why we have escaped cores in the first place.
> 
> Well I think we got there as omap4 powers CPU1 off in idle. And now
> we have dra7 using the same code and not doing that.
> 

I think this is the case, it may be useful to have them take different
paths.

>> In my testing, core1 always ends up looping in omap4_cpu_die(), the loop
>> in this function doesn't check for the boot-to address register, just
>> the core release register. I'm not sure how this works at all, if even
>> in the same kernel, when trying to bring this core back up it will
>> ignore our set boot-to address and return to the caller of this
>> function. Maybe the kernel is able to cope with that, but it does not
>> seem to be the intended wakeup path.
> 
> The kernel wakeup_secondary() loops won't work without reset naturally
> as that area of memory will be overwritten with different code by the
> new kernel booting depending on the kernel version. It only works
> kexec booting the same exact kernel version by luck because the code
> at that address stays the same. So not very usable :)
> 
>> I'll do some testing and see if this will help in omap4_cpu_die():
>>
>> if (boot_cpu == smp_processor_id()) {
>> 	/*
>> 	 * OK, proper wakeup, we're done
>> 	 */
>> +
>> +	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
>> +	boot_address();
>> +
>> 	break;
>> }
> 
> That's not going to help unless the boot_address is in the bootrom
> so it won't get trashed by the next booting kernel in kexec case.
> Or else we need to relocate the loop out of the way and that too
> gets messy as it can still be overwritten by appended dtb or
> initramfs. So it would really be best to have bootloader configure
> CPU1, then park it back to bootrom loop both in u-boot and kernel.
> Presumably the bootrom loop won't affect CPU1 state so this should
> be doable.
> 

We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
arch/arm/mach-omap2/omap5/sec_entry_cpu1.S

omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
makes an SMC call to setup the core, then re-parks itself back in bootROM.
Relevant code to run on cpu1:

> #define AUX_CORE_BOOT_0		0x48281800
> #define AUX_CORE_BOOT_1		0x48281804
> /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
>  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
>  * This address is same for J6 and J6 Eco.
>  */
> #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
>
> ldr	r4, =AUX_CORE_BOOT_0
> mov	r5, #0x0
> str	r5, [r4]
> ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> bx	r4	@ Jump back to ROM

We would have to do this in kernel. The issue I'm thinking we are going
to hit is that the parking function in bootROM expects the MMU to be
off. Although we don't really need to turn it off as long as we can
identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
we plan to exit the parking loop.

Andrew

> Regards,
> 
> Tony
> 

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170314 10:59]:
> On 03/14/2017 11:41 AM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [170314 09:04]:
> >> On 03/14/2017 10:17 AM, Tony Lindgren wrote:
> >>> And if the CPU1 start-up address is programmed to be in the currently
> >>> booting kernel's area by something else, it's almost certainly totally
> >>> broken.
> >>>
> >>
> >> I disagree, the core can be parked correctly and still have the boot-to
> >> address point to anywhere. There are two registers in play here, one
> >> sets the target jump-to address, the other lets it out of the parking
> >> loop. To know we are not parked we need to check the parking release
> >> register, the jump-to address is completely irrelevant as a check for
> >> proper parking.
> > 
> > Well at this point we have CPU1 parked and configured for a wrong
> > start-up address. So the boot-to address is a valid check. I can
> > certainly add a check for CPU1 being parked too.

OK I sent out v3 version of the patch doing that. Can you please
review and test that one?

<snip>

Then onto the other related patches..

> We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> arch/arm/mach-omap2/omap5/sec_entry_cpu1.S

OK good to hear.

> omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> makes an SMC call to setup the core, then re-parks itself back in bootROM.
> Relevant code to run on cpu1:
> 
> > #define AUX_CORE_BOOT_0		0x48281800
> > #define AUX_CORE_BOOT_1		0x48281804
> > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> >  * This address is same for J6 and J6 Eco.
> >  */
> > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> >
> > ldr	r4, =AUX_CORE_BOOT_0
> > mov	r5, #0x0
> > str	r5, [r4]
> > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > bx	r4	@ Jump back to ROM
> 
> We would have to do this in kernel. The issue I'm thinking we are going
> to hit is that the parking function in bootROM expects the MMU to be
> off. Although we don't really need to turn it off as long as we can
> identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> we plan to exit the parking loop.

Would be good to hear what Russell thinks we should do here to park
CPU1.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170314 10:59]:
> On 03/14/2017 11:41 AM, Tony Lindgren wrote:
> > * Andrew F. Davis <afd@ti.com> [170314 09:04]:
> >> On 03/14/2017 10:17 AM, Tony Lindgren wrote:
> >>> And if the CPU1 start-up address is programmed to be in the currently
> >>> booting kernel's area by something else, it's almost certainly totally
> >>> broken.
> >>>
> >>
> >> I disagree, the core can be parked correctly and still have the boot-to
> >> address point to anywhere. There are two registers in play here, one
> >> sets the target jump-to address, the other lets it out of the parking
> >> loop. To know we are not parked we need to check the parking release
> >> register, the jump-to address is completely irrelevant as a check for
> >> proper parking.
> > 
> > Well at this point we have CPU1 parked and configured for a wrong
> > start-up address. So the boot-to address is a valid check. I can
> > certainly add a check for CPU1 being parked too.

OK I sent out v3 version of the patch doing that. Can you please
review and test that one?

<snip>

Then onto the other related patches..

> We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> arch/arm/mach-omap2/omap5/sec_entry_cpu1.S

OK good to hear.

> omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> makes an SMC call to setup the core, then re-parks itself back in bootROM.
> Relevant code to run on cpu1:
> 
> > #define AUX_CORE_BOOT_0		0x48281800
> > #define AUX_CORE_BOOT_1		0x48281804
> > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> >  * This address is same for J6 and J6 Eco.
> >  */
> > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> >
> > ldr	r4, =AUX_CORE_BOOT_0
> > mov	r5, #0x0
> > str	r5, [r4]
> > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > bx	r4	@ Jump back to ROM
> 
> We would have to do this in kernel. The issue I'm thinking we are going
> to hit is that the parking function in bootROM expects the MMU to be
> off. Although we don't really need to turn it off as long as we can
> identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> we plan to exit the parking loop.

Would be good to hear what Russell thinks we should do here to park
CPU1.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170314 11:16]:
> * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> Then onto the other related patches..
> 
> > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> 
> OK good to hear.
> 
> > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > Relevant code to run on cpu1:
> > 
> > > #define AUX_CORE_BOOT_0		0x48281800
> > > #define AUX_CORE_BOOT_1		0x48281804
> > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > >  * This address is same for J6 and J6 Eco.
> > >  */
> > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > >
> > > ldr	r4, =AUX_CORE_BOOT_0
> > > mov	r5, #0x0
> > > str	r5, [r4]
> > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > bx	r4	@ Jump back to ROM
> > 
> > We would have to do this in kernel. The issue I'm thinking we are going
> > to hit is that the parking function in bootROM expects the MMU to be
> > off. Although we don't really need to turn it off as long as we can
> > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > we plan to exit the parking loop.
> 
> Would be good to hear what Russell thinks we should do here to park
> CPU1.

How about we introduce smp_park_secondary()?

Then soft_restart() can call it between setup_mm_for_reboot()
and cpu_proc_fin()?

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170314 11:16]:
> * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> Then onto the other related patches..
> 
> > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> 
> OK good to hear.
> 
> > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > Relevant code to run on cpu1:
> > 
> > > #define AUX_CORE_BOOT_0		0x48281800
> > > #define AUX_CORE_BOOT_1		0x48281804
> > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > >  * This address is same for J6 and J6 Eco.
> > >  */
> > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > >
> > > ldr	r4, =AUX_CORE_BOOT_0
> > > mov	r5, #0x0
> > > str	r5, [r4]
> > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > bx	r4	@ Jump back to ROM
> > 
> > We would have to do this in kernel. The issue I'm thinking we are going
> > to hit is that the parking function in bootROM expects the MMU to be
> > off. Although we don't really need to turn it off as long as we can
> > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > we plan to exit the parking loop.
> 
> Would be good to hear what Russell thinks we should do here to park
> CPU1.

How about we introduce smp_park_secondary()?

Then soft_restart() can call it between setup_mm_for_reboot()
and cpu_proc_fin()?

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170315 10:24]:
> * Tony Lindgren <tony@atomide.com> [170314 11:16]:
> > * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> > Then onto the other related patches..
> > 
> > > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> > 
> > OK good to hear.
> > 
> > > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > > Relevant code to run on cpu1:
> > > 
> > > > #define AUX_CORE_BOOT_0		0x48281800
> > > > #define AUX_CORE_BOOT_1		0x48281804
> > > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > > >  * This address is same for J6 and J6 Eco.
> > > >  */
> > > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > > >
> > > > ldr	r4, =AUX_CORE_BOOT_0
> > > > mov	r5, #0x0
> > > > str	r5, [r4]
> > > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > > bx	r4	@ Jump back to ROM
> > > 
> > > We would have to do this in kernel. The issue I'm thinking we are going
> > > to hit is that the parking function in bootROM expects the MMU to be
> > > off. Although we don't really need to turn it off as long as we can
> > > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > > we plan to exit the parking loop.
> > 
> > Would be good to hear what Russell thinks we should do here to park
> > CPU1.
> 
> How about we introduce smp_park_secondary()?
> 
> Then soft_restart() can call it between setup_mm_for_reboot()
> and cpu_proc_fin()?

Hmm that probably won't help as we'd have to still have a 1:1
mapping in cpu_die() to put CPU1 into WFI and release it in
smp_park_secondary() to the bootrom.

Anyways, I think in your use case it's the suspend/resume that needs
the state preserved for CPU1. Do you need kexec working in your use
case?

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170315 10:24]:
> * Tony Lindgren <tony@atomide.com> [170314 11:16]:
> > * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> > Then onto the other related patches..
> > 
> > > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> > 
> > OK good to hear.
> > 
> > > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > > Relevant code to run on cpu1:
> > > 
> > > > #define AUX_CORE_BOOT_0		0x48281800
> > > > #define AUX_CORE_BOOT_1		0x48281804
> > > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > > >  * This address is same for J6 and J6 Eco.
> > > >  */
> > > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > > >
> > > > ldr	r4, =AUX_CORE_BOOT_0
> > > > mov	r5, #0x0
> > > > str	r5, [r4]
> > > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > > bx	r4	@ Jump back to ROM
> > > 
> > > We would have to do this in kernel. The issue I'm thinking we are going
> > > to hit is that the parking function in bootROM expects the MMU to be
> > > off. Although we don't really need to turn it off as long as we can
> > > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > > we plan to exit the parking loop.
> > 
> > Would be good to hear what Russell thinks we should do here to park
> > CPU1.
> 
> How about we introduce smp_park_secondary()?
> 
> Then soft_restart() can call it between setup_mm_for_reboot()
> and cpu_proc_fin()?

Hmm that probably won't help as we'd have to still have a 1:1
mapping in cpu_die() to put CPU1 into WFI and release it in
smp_park_secondary() to the bootrom.

Anyways, I think in your use case it's the suspend/resume that needs
the state preserved for CPU1. Do you need kexec working in your use
case?

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 08:29:25AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170315 10:24]:
> > * Tony Lindgren <tony@atomide.com> [170314 11:16]:
> > > * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> > > Then onto the other related patches..
> > > 
> > > > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > > > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> > > 
> > > OK good to hear.
> > > 
> > > > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > > > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > > > Relevant code to run on cpu1:
> > > > 
> > > > > #define AUX_CORE_BOOT_0		0x48281800
> > > > > #define AUX_CORE_BOOT_1		0x48281804
> > > > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > > > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > > > >  * This address is same for J6 and J6 Eco.
> > > > >  */
> > > > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > > > >
> > > > > ldr	r4, =AUX_CORE_BOOT_0
> > > > > mov	r5, #0x0
> > > > > str	r5, [r4]
> > > > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > > > bx	r4	@ Jump back to ROM
> > > > 
> > > > We would have to do this in kernel. The issue I'm thinking we are going
> > > > to hit is that the parking function in bootROM expects the MMU to be
> > > > off. Although we don't really need to turn it off as long as we can
> > > > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > > > we plan to exit the parking loop.
> > > 
> > > Would be good to hear what Russell thinks we should do here to park
> > > CPU1.
> > 
> > How about we introduce smp_park_secondary()?
> > 
> > Then soft_restart() can call it between setup_mm_for_reboot()
> > and cpu_proc_fin()?
> 
> Hmm that probably won't help as we'd have to still have a 1:1
> mapping in cpu_die() to put CPU1 into WFI and release it in
> smp_park_secondary() to the bootrom.
> 
> Anyways, I think in your use case it's the suspend/resume that needs
> the state preserved for CPU1. Do you need kexec working in your use
> case?

I think you're trying way too hard and making this too complicated.  I
put forward my comments on it via email on Monday - is there a reason
why you can't follow my recommendations (which are exactly what Linux
expects) of the to-be-unplugged CPU?

Why do you need the CPU to spin in the kernel when you hot-unplug it?

Why can't you reset the CPU when it's died, possibly releasing the
reset again to get it into the boot rom (which is where it'll be at
boot time.)

To detect older kernels, you could write a magic identifier into the
boot address registers from a new kernel when taking the CPU offline.
(eg, the value it has at normal system boot.)  When you attempt to
bring CPUs online, check the value, if it isn't set correctly, assume
that the CPU has gone awol, and reset it as you're doing now.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Russell King - ARM Linux]

On Thu, Mar 16, 2017 at 08:29:25AM -0700, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170315 10:24]:
> > * Tony Lindgren <tony@atomide.com> [170314 11:16]:
> > > * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> > > Then onto the other related patches..
> > > 
> > > > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > > > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> > > 
> > > OK good to hear.
> > > 
> > > > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > > > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > > > Relevant code to run on cpu1:
> > > > 
> > > > > #define AUX_CORE_BOOT_0		0x48281800
> > > > > #define AUX_CORE_BOOT_1		0x48281804
> > > > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > > > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > > > >  * This address is same for J6 and J6 Eco.
> > > > >  */
> > > > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > > > >
> > > > > ldr	r4, =AUX_CORE_BOOT_0
> > > > > mov	r5, #0x0
> > > > > str	r5, [r4]
> > > > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > > > bx	r4	@ Jump back to ROM
> > > > 
> > > > We would have to do this in kernel. The issue I'm thinking we are going
> > > > to hit is that the parking function in bootROM expects the MMU to be
> > > > off. Although we don't really need to turn it off as long as we can
> > > > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > > > we plan to exit the parking loop.
> > > 
> > > Would be good to hear what Russell thinks we should do here to park
> > > CPU1.
> > 
> > How about we introduce smp_park_secondary()?
> > 
> > Then soft_restart() can call it between setup_mm_for_reboot()
> > and cpu_proc_fin()?
> 
> Hmm that probably won't help as we'd have to still have a 1:1
> mapping in cpu_die() to put CPU1 into WFI and release it in
> smp_park_secondary() to the bootrom.
> 
> Anyways, I think in your use case it's the suspend/resume that needs
> the state preserved for CPU1. Do you need kexec working in your use
> case?

I think you're trying way too hard and making this too complicated.  I
put forward my comments on it via email on Monday - is there a reason
why you can't follow my recommendations (which are exactly what Linux
expects) of the to-be-unplugged CPU?

Why do you need the CPU to spin in the kernel when you hot-unplug it?

Why can't you reset the CPU when it's died, possibly releasing the
reset again to get it into the boot rom (which is where it'll be at
boot time.)

To detect older kernels, you could write a magic identifier into the
boot address registers from a new kernel when taking the CPU offline.
(eg, the value it has at normal system boot.)  When you attempt to
bring CPUs online, check the value, if it isn't set correctly, assume
that the CPU has gone awol, and reset it as you're doing now.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Russell King - ARM Linux <linux@armlinux.org.uk> [170317 02:27]:
> On Thu, Mar 16, 2017 at 08:29:25AM -0700, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170315 10:24]:
> > > * Tony Lindgren <tony@atomide.com> [170314 11:16]:
> > > > * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> > > > Then onto the other related patches..
> > > > 
> > > > > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > > > > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> > > > 
> > > > OK good to hear.
> > > > 
> > > > > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > > > > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > > > > Relevant code to run on cpu1:
> > > > > 
> > > > > > #define AUX_CORE_BOOT_0		0x48281800
> > > > > > #define AUX_CORE_BOOT_1		0x48281804
> > > > > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > > > > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > > > > >  * This address is same for J6 and J6 Eco.
> > > > > >  */
> > > > > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > > > > >
> > > > > > ldr	r4, =AUX_CORE_BOOT_0
> > > > > > mov	r5, #0x0
> > > > > > str	r5, [r4]
> > > > > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > > > > bx	r4	@ Jump back to ROM
> > > > > 
> > > > > We would have to do this in kernel. The issue I'm thinking we are going
> > > > > to hit is that the parking function in bootROM expects the MMU to be
> > > > > off. Although we don't really need to turn it off as long as we can
> > > > > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > > > > we plan to exit the parking loop.
> > > > 
> > > > Would be good to hear what Russell thinks we should do here to park
> > > > CPU1.
> > > 
> > > How about we introduce smp_park_secondary()?
> > > 
> > > Then soft_restart() can call it between setup_mm_for_reboot()
> > > and cpu_proc_fin()?
> > 
> > Hmm that probably won't help as we'd have to still have a 1:1
> > mapping in cpu_die() to put CPU1 into WFI and release it in
> > smp_park_secondary() to the bootrom.
> > 
> > Anyways, I think in your use case it's the suspend/resume that needs
> > the state preserved for CPU1. Do you need kexec working in your use
> > case?
> 
> I think you're trying way too hard and making this too complicated.  I
> put forward my comments on it via email on Monday - is there a reason
> why you can't follow my recommendations (which are exactly what Linux
> expects) of the to-be-unplugged CPU?

Well I don't have a problem with that. But I think Andrew has a use case
where he needs CPU1 configuration from bootloader preserved for HS
dra7. Maybe we can preserve and restore it in suspend/resume case?

> Why do you need the CPU to spin in the kernel when you hot-unplug it?

I don't need it spinning in my use cases. So let's wait to hear what
exactly Andrew needs.

> Why can't you reset the CPU when it's died, possibly releasing the
> reset again to get it into the boot rom (which is where it'll be at
> boot time.)

Yes that would make most sense to me. Andrew, does that break your use
case again for CPU1 for suspend/restore?

> To detect older kernels, you could write a magic identifier into the
> boot address registers from a new kernel when taking the CPU offline.
> (eg, the value it has at normal system boot.)  When you attempt to
> bring CPUs online, check the value, if it isn't set correctly, assume
> that the CPU has gone awol, and reset it as you're doing now.

Yeah OK. I think I got the kexec boot reset handling check part sorted
out in v2 of this patch.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Russell King - ARM Linux <linux@armlinux.org.uk> [170317 02:27]:
> On Thu, Mar 16, 2017 at 08:29:25AM -0700, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170315 10:24]:
> > > * Tony Lindgren <tony@atomide.com> [170314 11:16]:
> > > > * Andrew F. Davis <afd@ti.com> [170314 10:59]:
> > > > Then onto the other related patches..
> > > > 
> > > > > We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
> > > > > arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
> > > > 
> > > > OK good to hear.
> > > > 
> > > > > omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
> > > > > makes an SMC call to setup the core, then re-parks itself back in bootROM.
> > > > > Relevant code to run on cpu1:
> > > > > 
> > > > > > #define AUX_CORE_BOOT_0		0x48281800
> > > > > > #define AUX_CORE_BOOT_1		0x48281804
> > > > > > /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
> > > > > >  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
> > > > > >  * This address is same for J6 and J6 Eco.
> > > > > >  */
> > > > > > #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
> > > > > >
> > > > > > ldr	r4, =AUX_CORE_BOOT_0
> > > > > > mov	r5, #0x0
> > > > > > str	r5, [r4]
> > > > > > ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
> > > > > > bx	r4	@ Jump back to ROM
> > > > > 
> > > > > We would have to do this in kernel. The issue I'm thinking we are going
> > > > > to hit is that the parking function in bootROM expects the MMU to be
> > > > > off. Although we don't really need to turn it off as long as we can
> > > > > identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
> > > > > we plan to exit the parking loop.
> > > > 
> > > > Would be good to hear what Russell thinks we should do here to park
> > > > CPU1.
> > > 
> > > How about we introduce smp_park_secondary()?
> > > 
> > > Then soft_restart() can call it between setup_mm_for_reboot()
> > > and cpu_proc_fin()?
> > 
> > Hmm that probably won't help as we'd have to still have a 1:1
> > mapping in cpu_die() to put CPU1 into WFI and release it in
> > smp_park_secondary() to the bootrom.
> > 
> > Anyways, I think in your use case it's the suspend/resume that needs
> > the state preserved for CPU1. Do you need kexec working in your use
> > case?
> 
> I think you're trying way too hard and making this too complicated.  I
> put forward my comments on it via email on Monday - is there a reason
> why you can't follow my recommendations (which are exactly what Linux
> expects) of the to-be-unplugged CPU?

Well I don't have a problem with that. But I think Andrew has a use case
where he needs CPU1 configuration from bootloader preserved for HS
dra7. Maybe we can preserve and restore it in suspend/resume case?

> Why do you need the CPU to spin in the kernel when you hot-unplug it?

I don't need it spinning in my use cases. So let's wait to hear what
exactly Andrew needs.

> Why can't you reset the CPU when it's died, possibly releasing the
> reset again to get it into the boot rom (which is where it'll be at
> boot time.)

Yes that would make most sense to me. Andrew, does that break your use
case again for CPU1 for suspend/restore?

> To detect older kernels, you could write a magic identifier into the
> boot address registers from a new kernel when taking the CPU offline.
> (eg, the value it has at normal system boot.)  When you attempt to
> bring CPUs online, check the value, if it isn't set correctly, assume
> that the CPU has gone awol, and reset it as you're doing now.

Yeah OK. I think I got the kexec boot reset handling check part sorted
out in v2 of this patch.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/17/2017 08:57 AM, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@armlinux.org.uk> [170317 02:27]:
>> On Thu, Mar 16, 2017 at 08:29:25AM -0700, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [170315 10:24]:
>>>> * Tony Lindgren <tony@atomide.com> [170314 11:16]:
>>>>> * Andrew F. Davis <afd@ti.com> [170314 10:59]:
>>>>> Then onto the other related patches..
>>>>>
>>>>>> We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
>>>>>> arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
>>>>>
>>>>> OK good to hear.
>>>>>
>>>>>> omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
>>>>>> makes an SMC call to setup the core, then re-parks itself back in bootROM.
>>>>>> Relevant code to run on cpu1:
>>>>>>
>>>>>>> #define AUX_CORE_BOOT_0		0x48281800
>>>>>>> #define AUX_CORE_BOOT_1		0x48281804
>>>>>>> /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
>>>>>>>  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
>>>>>>>  * This address is same for J6 and J6 Eco.
>>>>>>>  */
>>>>>>> #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
>>>>>>>
>>>>>>> ldr	r4, =AUX_CORE_BOOT_0
>>>>>>> mov	r5, #0x0
>>>>>>> str	r5, [r4]
>>>>>>> ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
>>>>>>> bx	r4	@ Jump back to ROM
>>>>>>
>>>>>> We would have to do this in kernel. The issue I'm thinking we are going
>>>>>> to hit is that the parking function in bootROM expects the MMU to be
>>>>>> off. Although we don't really need to turn it off as long as we can
>>>>>> identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
>>>>>> we plan to exit the parking loop.
>>>>>
>>>>> Would be good to hear what Russell thinks we should do here to park
>>>>> CPU1.
>>>>
>>>> How about we introduce smp_park_secondary()?
>>>>
>>>> Then soft_restart() can call it between setup_mm_for_reboot()
>>>> and cpu_proc_fin()?
>>>
>>> Hmm that probably won't help as we'd have to still have a 1:1
>>> mapping in cpu_die() to put CPU1 into WFI and release it in
>>> smp_park_secondary() to the bootrom.
>>>
>>> Anyways, I think in your use case it's the suspend/resume that needs
>>> the state preserved for CPU1. Do you need kexec working in your use
>>> case?
>>
>> I think you're trying way too hard and making this too complicated.  I
>> put forward my comments on it via email on Monday - is there a reason
>> why you can't follow my recommendations (which are exactly what Linux
>> expects) of the to-be-unplugged CPU?
> 
> Well I don't have a problem with that. But I think Andrew has a use case
> where he needs CPU1 configuration from bootloader preserved for HS
> dra7. Maybe we can preserve and restore it in suspend/resume case?
> 

I think this would add even more complexity. The secure context cannot
be modified by the non-secure, I'm not sure how we could go about
preserving and restoring it without calls into the secure monitor.

>> Why do you need the CPU to spin in the kernel when you hot-unplug it?
> 
> I don't need it spinning in my use cases. So let's wait to hear what
> exactly Andrew needs.
> 

You do need it spinning somewhere, we have no real CPU-off state,
reseting it simply moves the spinning back up into ROM. It's doesn't
matter to me any where the parking loop happens, as long as you don't
use a hard-reset to get there.

>> Why can't you reset the CPU when it's died, possibly releasing the
>> reset again to get it into the boot rom (which is where it'll be at
>> boot time.)
> 
> Yes that would make most sense to me. Andrew, does that break your use
> case again for CPU1 for suspend/restore?
> 

We cannot reset the CPU, that is the only constraint I have. If you let
a core go awol then it is lost for good, you should try to avoid that.
The secure side on that core may still be running just fine, and may be
performing very important tasks, resetting the whole core because the
non-secure side lost control of its side is not okay.

If you find that the core is lost (using whatever method), *and* that it
is an unlocked developer board, you can reset it as a recovery method
and probably be safe as nothing is running on the secure side.
This kind of recovery is the exception and should not be a used as the
standard method for parking the core for any reason as it will not be
allowed on production devices (firewalled reset controls to prevent this
sort of thing).

>> To detect older kernels, you could write a magic identifier into the
>> boot address registers from a new kernel when taking the CPU offline.
>> (eg, the value it has at normal system boot.)  When you attempt to
>> bring CPUs online, check the value, if it isn't set correctly, assume
>> that the CPU has gone awol, and reset it as you're doing now.
> 
> Yeah OK. I think I got the kexec boot reset handling check part sorted
> out in v2 of this patch.
> 
> Regards,
> 
> Tony
> 

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Andrew F. Davis]

On 03/17/2017 08:57 AM, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@armlinux.org.uk> [170317 02:27]:
>> On Thu, Mar 16, 2017 at 08:29:25AM -0700, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [170315 10:24]:
>>>> * Tony Lindgren <tony@atomide.com> [170314 11:16]:
>>>>> * Andrew F. Davis <afd@ti.com> [170314 10:59]:
>>>>> Then onto the other related patches..
>>>>>
>>>>>> We re-park cpu1 in bootROM in U-Boot already, in the U-Boot tree:
>>>>>> arch/arm/mach-omap2/omap5/sec_entry_cpu1.S
>>>>>
>>>>> OK good to hear.
>>>>>
>>>>>> omap_smc_sec_cpu1() wakes up cpu1 into the function cpu1_entry(), this
>>>>>> makes an SMC call to setup the core, then re-parks itself back in bootROM.
>>>>>> Relevant code to run on cpu1:
>>>>>>
>>>>>>> #define AUX_CORE_BOOT_0		0x48281800
>>>>>>> #define AUX_CORE_BOOT_1		0x48281804
>>>>>>> /* DRA7xx ROM code function "startup_BootSlave". This function is where CPU1
>>>>>>>  * waits on WFE, polling on AUX_CORE_BOOT_x registers.
>>>>>>>  * This address is same for J6 and J6 Eco.
>>>>>>>  */
>>>>>>> #define ROM_FXN_STARTUP_BOOTSLAVE     0x00038a64
>>>>>>>
>>>>>>> ldr	r4, =AUX_CORE_BOOT_0
>>>>>>> mov	r5, #0x0
>>>>>>> str	r5, [r4]
>>>>>>> ldr	r4, =ROM_FXN_STARTUP_BOOTSLAVE
>>>>>>> bx	r4	@ Jump back to ROM
>>>>>>
>>>>>> We would have to do this in kernel. The issue I'm thinking we are going
>>>>>> to hit is that the parking function in bootROM expects the MMU to be
>>>>>> off. Although we don't really need to turn it off as long as we can
>>>>>> identity map the bootROM area, the AUX_CORE_BOOT_0/1 space, and wherever
>>>>>> we plan to exit the parking loop.
>>>>>
>>>>> Would be good to hear what Russell thinks we should do here to park
>>>>> CPU1.
>>>>
>>>> How about we introduce smp_park_secondary()?
>>>>
>>>> Then soft_restart() can call it between setup_mm_for_reboot()
>>>> and cpu_proc_fin()?
>>>
>>> Hmm that probably won't help as we'd have to still have a 1:1
>>> mapping in cpu_die() to put CPU1 into WFI and release it in
>>> smp_park_secondary() to the bootrom.
>>>
>>> Anyways, I think in your use case it's the suspend/resume that needs
>>> the state preserved for CPU1. Do you need kexec working in your use
>>> case?
>>
>> I think you're trying way too hard and making this too complicated.  I
>> put forward my comments on it via email on Monday - is there a reason
>> why you can't follow my recommendations (which are exactly what Linux
>> expects) of the to-be-unplugged CPU?
> 
> Well I don't have a problem with that. But I think Andrew has a use case
> where he needs CPU1 configuration from bootloader preserved for HS
> dra7. Maybe we can preserve and restore it in suspend/resume case?
> 

I think this would add even more complexity. The secure context cannot
be modified by the non-secure, I'm not sure how we could go about
preserving and restoring it without calls into the secure monitor.

>> Why do you need the CPU to spin in the kernel when you hot-unplug it?
> 
> I don't need it spinning in my use cases. So let's wait to hear what
> exactly Andrew needs.
> 

You do need it spinning somewhere, we have no real CPU-off state,
reseting it simply moves the spinning back up into ROM. It's doesn't
matter to me any where the parking loop happens, as long as you don't
use a hard-reset to get there.

>> Why can't you reset the CPU when it's died, possibly releasing the
>> reset again to get it into the boot rom (which is where it'll be at
>> boot time.)
> 
> Yes that would make most sense to me. Andrew, does that break your use
> case again for CPU1 for suspend/restore?
> 

We cannot reset the CPU, that is the only constraint I have. If you let
a core go awol then it is lost for good, you should try to avoid that.
The secure side on that core may still be running just fine, and may be
performing very important tasks, resetting the whole core because the
non-secure side lost control of its side is not okay.

If you find that the core is lost (using whatever method), *and* that it
is an unlocked developer board, you can reset it as a recovery method
and probably be safe as nothing is running on the secure side.
This kind of recovery is the exception and should not be a used as the
standard method for parking the core for any reason as it will not be
allowed on production devices (firewalled reset controls to prevent this
sort of thing).

>> To detect older kernels, you could write a magic identifier into the
>> boot address registers from a new kernel when taking the CPU offline.
>> (eg, the value it has at normal system boot.)  When you attempt to
>> bring CPUs online, check the value, if it isn't set correctly, assume
>> that the CPU has gone awol, and reset it as you're doing now.
> 
> Yeah OK. I think I got the kexec boot reset handling check part sorted
> out in v2 of this patch.
> 
> Regards,
> 
> Tony
> 

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170317 09:28]:
> On 03/17/2017 08:57 AM, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@armlinux.org.uk> [170317 02:27]:
> >> Why can't you reset the CPU when it's died, possibly releasing the
> >> reset again to get it into the boot rom (which is where it'll be at
> >> boot time.)
> > 
> > Yes that would make most sense to me. Andrew, does that break your use
> > case again for CPU1 for suspend/restore?
> > 
> 
> We cannot reset the CPU, that is the only constraint I have. If you let
> a core go awol then it is lost for good, you should try to avoid that.
> The secure side on that core may still be running just fine, and may be
> performing very important tasks, resetting the whole core because the
> non-secure side lost control of its side is not okay.

For suspend/resume, I think the right solution is to put dra7
CPU1 into retention instead trying to put it into off which will
fail. This should fix the "smp_ops.cpu_die() returned, trying to
resuscitate" issue. And then there is no need to repark CPU1
anywhere for suspend/resume as it's not spinning. And it's context
won't get lost and no reset is needed. And some power is saved.

For kexec, things are trickier. If CPU1 is in some deeper power
state, it will wake to the configured address and there's no way
to reconfigure that AFAIK. So reset CPU1 reset is needed from that
state. Note that HS omap4 implementations do seem to survive CPU1
off mode just fine.

Andrew, does your use case really need kexec supported? If not,
I think the sane thing for is to just disable kexec in the HS
kernel for dra7 during runtime.

And then if it turns out we really need to repark CPU1 for kexec
into boot ROM, at least the following changes are needed:

1. We need to differentiate between suspend/resume and kexec for
   CPU1 hot-unplug so we can put CPU1 into retention for
   suspend/resume and park CPU1 for kexec

2. When parking CPU1 for kexec, it can be done in two stages.
   We can first park it to a loop waiting for flat memory mapping,
   then jump to boot ROM loop. But flat memory mapping is not
   enough we also need to remap boot ROM and L4.. Probably waiting
   for MMU off won't work here

So depending on the use cases, I don't know if implementing these
two are really needed or make sense.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting CPU1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170317 09:28]:
> On 03/17/2017 08:57 AM, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@armlinux.org.uk> [170317 02:27]:
> >> Why can't you reset the CPU when it's died, possibly releasing the
> >> reset again to get it into the boot rom (which is where it'll be at
> >> boot time.)
> > 
> > Yes that would make most sense to me. Andrew, does that break your use
> > case again for CPU1 for suspend/restore?
> > 
> 
> We cannot reset the CPU, that is the only constraint I have. If you let
> a core go awol then it is lost for good, you should try to avoid that.
> The secure side on that core may still be running just fine, and may be
> performing very important tasks, resetting the whole core because the
> non-secure side lost control of its side is not okay.

For suspend/resume, I think the right solution is to put dra7
CPU1 into retention instead trying to put it into off which will
fail. This should fix the "smp_ops.cpu_die() returned, trying to
resuscitate" issue. And then there is no need to repark CPU1
anywhere for suspend/resume as it's not spinning. And it's context
won't get lost and no reset is needed. And some power is saved.

For kexec, things are trickier. If CPU1 is in some deeper power
state, it will wake to the configured address and there's no way
to reconfigure that AFAIK. So reset CPU1 reset is needed from that
state. Note that HS omap4 implementations do seem to survive CPU1
off mode just fine.

Andrew, does your use case really need kexec supported? If not,
I think the sane thing for is to just disable kexec in the HS
kernel for dra7 during runtime.

And then if it turns out we really need to repark CPU1 for kexec
into boot ROM, at least the following changes are needed:

1. We need to differentiate between suspend/resume and kexec for
   CPU1 hot-unplug so we can put CPU1 into retention for
   suspend/resume and park CPU1 for kexec

2. When parking CPU1 for kexec, it can be done in two stages.
   We can first park it to a loop waiting for flat memory mapping,
   then jump to boot ROM loop. But flat memory mapping is not
   enough we also need to remap boot ROM and L4.. Probably waiting
   for MMU off won't work here

So depending on the use cases, I don't know if implementing these
two are really needed or make sense.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170217 12:28]:
> On 02/17/2017 09:55 AM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170216 11:08]:
> >> * Tony Lindgren <tony@atomide.com> [170216 08:55]:
> >>> For your use case, probably all we need is runtime checks for HS in
> >>> addition to parking cpu1 for kexec. If that's not enough, then maybe
> >>> a device specific DT property for never-reset-no-matter-what.
> >>
> >> Below is a first take on the last resort cpu1 reset done based on
> >> configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
> >> we can't merge it yet as it will break kexec boot for dra7. To fix that,
> >> we need to first do what you're suggesting and properly park cpu1 for
> >> kexec.
> > 
> > Something like the patch below might be doable for the first fix
> > before we have something to park cpu1 for kexec. I've added more checks
> > to attemp to detect cpu1 being in WFI. At least things keep on working
> > for kexec on omap4/5 and GP dra7.
> > 
> > Andrew, care to give it a try and see if can add some HS dra7 checks
> > there too to have kexec work on it? Basically we want to attempt to
> > detect if there's a chance cpu1 is in WFI, and if I think the only
> > optino is to reset it before bringing it up as otherwise outcome will
> > be unpredictable.
> > 
> 
> This patch seems stop the reset issue on my boards, so as long as it is
> just a temporary workaround until we can fix the real issue (a core that
> should be off going into omap_do_wfi() instead of parking), then this
> patch works for me.

OK

> > +
> > +	/* REVISIT: Anything to check for HS dra7? */
> > +	if (soc_is_dra74x() && omap_secure_apis_support())
> > +		return;
> 
> omap_secure_apis_support() only returns true when cpu_is_omap44xx() is
> true, so this will never be true. Someday we may add the secure APIs to
> secure DRA7xx devices, but I don't know of a good way to detect this
> from kernel right now.

OK so that can be removed.

Can you check what happens with suspend/resume cycle? Based on what
I've seen omap5 and dra7 currently always hit the "smp_ops.cpu_die()
returned, trying to resuscitate" on resume meaning it will get sent to
secondary_start_kernel on resume.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170217 12:28]:
> On 02/17/2017 09:55 AM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170216 11:08]:
> >> * Tony Lindgren <tony@atomide.com> [170216 08:55]:
> >>> For your use case, probably all we need is runtime checks for HS in
> >>> addition to parking cpu1 for kexec. If that's not enough, then maybe
> >>> a device specific DT property for never-reset-no-matter-what.
> >>
> >> Below is a first take on the last resort cpu1 reset done based on
> >> configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
> >> we can't merge it yet as it will break kexec boot for dra7. To fix that,
> >> we need to first do what you're suggesting and properly park cpu1 for
> >> kexec.
> > 
> > Something like the patch below might be doable for the first fix
> > before we have something to park cpu1 for kexec. I've added more checks
> > to attemp to detect cpu1 being in WFI. At least things keep on working
> > for kexec on omap4/5 and GP dra7.
> > 
> > Andrew, care to give it a try and see if can add some HS dra7 checks
> > there too to have kexec work on it? Basically we want to attempt to
> > detect if there's a chance cpu1 is in WFI, and if I think the only
> > optino is to reset it before bringing it up as otherwise outcome will
> > be unpredictable.
> > 
> 
> This patch seems stop the reset issue on my boards, so as long as it is
> just a temporary workaround until we can fix the real issue (a core that
> should be off going into omap_do_wfi() instead of parking), then this
> patch works for me.

OK

> > +
> > +	/* REVISIT: Anything to check for HS dra7? */
> > +	if (soc_is_dra74x() && omap_secure_apis_support())
> > +		return;
> 
> omap_secure_apis_support() only returns true when cpu_is_omap44xx() is
> true, so this will never be true. Someday we may add the secure APIs to
> secure DRA7xx devices, but I don't know of a good way to detect this
> from kernel right now.

OK so that can be removed.

Can you check what happens with suspend/resume cycle? Based on what
I've seen omap5 and dra7 currently always hit the "smp_ops.cpu_die()
returned, trying to resuscitate" on resume meaning it will get sent to
secondary_start_kernel on resume.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Andrew F. Davis]

On 02/17/2017 09:55 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170216 11:08]:
>> * Tony Lindgren <tony@atomide.com> [170216 08:55]:
>>> For your use case, probably all we need is runtime checks for HS in
>>> addition to parking cpu1 for kexec. If that's not enough, then maybe
>>> a device specific DT property for never-reset-no-matter-what.
>>
>> Below is a first take on the last resort cpu1 reset done based on
>> configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
>> we can't merge it yet as it will break kexec boot for dra7. To fix that,
>> we need to first do what you're suggesting and properly park cpu1 for
>> kexec.
> 
> Something like the patch below might be doable for the first fix
> before we have something to park cpu1 for kexec. I've added more checks
> to attemp to detect cpu1 being in WFI. At least things keep on working
> for kexec on omap4/5 and GP dra7.
> 
> Andrew, care to give it a try and see if can add some HS dra7 checks
> there too to have kexec work on it? Basically we want to attempt to
> detect if there's a chance cpu1 is in WFI, and if I think the only
> optino is to reset it before bringing it up as otherwise outcome will
> be unpredictable.
> 

This patch seems stop the reset issue on my boards, so as long as it is
just a temporary workaround until we can fix the real issue (a core that
should be off going into omap_do_wfi() instead of parking), then this
patch works for me.

[...]

> +
> +	/* REVISIT: Anything to check for HS dra7? */
> +	if (soc_is_dra74x() && omap_secure_apis_support())
> +		return;

omap_secure_apis_support() only returns true when cpu_is_omap44xx() is
true, so this will never be true. Someday we may add the secure APIs to
secure DRA7xx devices, but I don't know of a good way to detect this
from kernel right now.

Andrew

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Andrew F. Davis]

On 02/17/2017 09:55 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170216 11:08]:
>> * Tony Lindgren <tony@atomide.com> [170216 08:55]:
>>> For your use case, probably all we need is runtime checks for HS in
>>> addition to parking cpu1 for kexec. If that's not enough, then maybe
>>> a device specific DT property for never-reset-no-matter-what.
>>
>> Below is a first take on the last resort cpu1 reset done based on
>> configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
>> we can't merge it yet as it will break kexec boot for dra7. To fix that,
>> we need to first do what you're suggesting and properly park cpu1 for
>> kexec.
> 
> Something like the patch below might be doable for the first fix
> before we have something to park cpu1 for kexec. I've added more checks
> to attemp to detect cpu1 being in WFI. At least things keep on working
> for kexec on omap4/5 and GP dra7.
> 
> Andrew, care to give it a try and see if can add some HS dra7 checks
> there too to have kexec work on it? Basically we want to attempt to
> detect if there's a chance cpu1 is in WFI, and if I think the only
> optino is to reset it before bringing it up as otherwise outcome will
> be unpredictable.
> 

This patch seems stop the reset issue on my boards, so as long as it is
just a temporary workaround until we can fix the real issue (a core that
should be off going into omap_do_wfi() instead of parking), then this
patch works for me.

[...]

> +
> +	/* REVISIT: Anything to check for HS dra7? */
> +	if (soc_is_dra74x() && omap_secure_apis_support())
> +		return;

omap_secure_apis_support() only returns true when cpu_is_omap44xx() is
true, so this will never be true. Someday we may add the secure APIs to
secure DRA7xx devices, but I don't know of a good way to detect this
from kernel right now.

Andrew

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170216 11:08]:
> * Tony Lindgren <tony@atomide.com> [170216 08:55]:
> > For your use case, probably all we need is runtime checks for HS in
> > addition to parking cpu1 for kexec. If that's not enough, then maybe
> > a device specific DT property for never-reset-no-matter-what.
> 
> Below is a first take on the last resort cpu1 reset done based on
> configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
> we can't merge it yet as it will break kexec boot for dra7. To fix that,
> we need to first do what you're suggesting and properly park cpu1 for
> kexec.

Something like the patch below might be doable for the first fix
before we have something to park cpu1 for kexec. I've added more checks
to attemp to detect cpu1 being in WFI. At least things keep on working
for kexec on omap4/5 and GP dra7.

Andrew, care to give it a try and see if can add some HS dra7 checks
there too to have kexec work on it? Basically we want to attempt to
detect if there's a chance cpu1 is in WFI, and if I think the only
optino is to reset it before bringing it up as otherwise outcome will
be unpredictable.

Regards,

Tony

8< -----------------------
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
 extern int omap4_mpuss_init(void);
 extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
 extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
+extern u32 omap4_get_cpu1_ns_pa_addr(void);
 #else
 static inline int omap4_enter_lowpower(unsigned int cpu,
 					unsigned int power_state)
diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -64,6 +64,7 @@
 #include "prm-regbits-44xx.h"
 
 static void __iomem *sar_base;
+static u32 old_cpu1_ns_pa_addr;
 
 #if defined(CONFIG_PM) && defined(CONFIG_SMP)
 
@@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
 {}
 #endif
 
+u32 omap4_get_cpu1_ns_pa_addr(void)
+{
+	return old_cpu1_ns_pa_addr;
+}
+
 /**
  * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
  * The purpose of this function is to manage low power programming
@@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x0);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x1);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
 		save_l2x0_context();
 	}
 
-	if (cpu_is_omap44xx()) {
+	if (soc_is_omap44xx()) {
 		omap_pm_ops.finish_suspend = omap4_finish_suspend;
 		omap_pm_ops.resume = omap4_cpu_resume;
 		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
@@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
 		enable_mercury_retention_mode();
 	}
 
-	if (cpu_is_omap446x())
+	if (soc_is_omap446x())
 		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
 
 	return 0;
@@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
 void __init omap4_mpuss_early_init(void)
 {
 	unsigned long startup_pa;
+	void __iomem *ns_pa_addr;
 
-	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
+	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
 		return;
 
 	sar_base = omap4_get_sar_ram_base();
 
-	if (cpu_is_omap443x())
+	/* Restore old NS_PA_ADDR for validity checks later on */
+	if (soc_is_omap44xx())
+		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	else
+		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
+
+	if (soc_is_omap443x())
 		startup_pa = __pa_symbol(omap4_secondary_startup);
-	else if (cpu_is_omap446x())
+	else if (soc_is_omap446x())
 		startup_pa = __pa_symbol(omap4460_secondary_startup);
 	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
 		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
 	else
 		startup_pa = __pa_symbol(omap5_secondary_startup);
 
-	if (cpu_is_omap44xx())
+	if (soc_is_omap44xx())
 		writel_relaxed(startup_pa, sar_base +
 			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
 	else
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -44,6 +44,7 @@ struct omap_smp_config {
 	unsigned long cpu1_rstctrl_pa;
 	void __iomem *cpu1_rstctrl_va;
 	void __iomem *scu_base;
+	void __iomem *wakeupgen_base;
 	void *startup_addr;
 };
 
@@ -140,7 +141,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	static struct clockdomain *cpu1_clkdm;
 	static bool booted;
 	static struct powerdomain *cpu1_pwrdm;
-	void __iomem *base = omap_get_wakeupgen_base();
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -157,7 +157,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	if (omap_secure_apis_support())
 		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
 	else
-		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
+		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
 
 	if (!cpu1_clkdm && !cpu1_pwrdm) {
 		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
@@ -261,9 +261,62 @@ static void __init omap4_smp_init_cpus(void)
 		set_cpu_possible(i, true);
 }
 
+/*
+ * For now, just make sure the start-up address is not within
+ * the first 1GB as that most likely means that CPU1 is configured
+ * by either the bootloader or previous kernel in kexec boot to
+ * something that will most likely fail without a reset.
+ */
+static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
+{
+	if ((addr >> 24) == 0x80)
+		return false;
+
+	return true;
+}
+
+/*
+ * We may need to reset CPU1 before configuring, otherwise kexec can end up
+ * trying to use old kernel startup address or suspend-resume will occasionally
+ * fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper idle states.
+ */
+static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
+{
+	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
+	bool needs_reset = false;
+
+	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
+					OMAP_AUX_CORE_BOOT_1);
+	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
+
+	/* REVISIT: Anything to check for HS dra7? */
+	if (soc_is_dra74x() && omap_secure_apis_support())
+		return;
+
+	/* If dra7 has AUX_CORE_BOOT_1 within first 1GB, CPU1 may be in WFI */
+	if (soc_is_dra74x() && !omap_secure_apis_support() &&
+	    !omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
+		needs_reset = true;
+
+	/* If omap4 or 5 has NS_PA_ADDR within first 1GB, CPU1 may be in WFI */
+	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
+	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr) &&
+	    c->cpu1_rstctrl_va)
+		needs_reset = true;
+
+	if (!needs_reset || !c->cpu1_rstctrl_va)
+		return;
+
+	pr_info("smp: Already configured omap cpu1, needs reset (0x%lx 0x%lx)\n",
+		cpu1_startup_pa, cpu1_ns_pa_addr);
+
+	writel_relaxed(1, c->cpu1_rstctrl_va);
+	readl_relaxed(c->cpu1_rstctrl_va);
+	writel_relaxed(0, c->cpu1_rstctrl_va);
+}
+
 static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 {
-	void __iomem *base = omap_get_wakeupgen_base();
 	const struct omap_smp_config *c = NULL;
 
 	if (soc_is_omap443x())
@@ -281,6 +334,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	/* Must preserve cfg.scu_base set earlier */
 	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
 	cfg.startup_addr = c->startup_addr;
+	cfg.wakeupgen_base = omap_get_wakeupgen_base();
 
 	if (soc_is_dra74x() || soc_is_omap54xx()) {
 		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
@@ -299,15 +353,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	if (cfg.scu_base)
 		scu_enable(cfg.scu_base);
 
-	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
-	 */
-	if (cfg.cpu1_rstctrl_va) {
-		writel_relaxed(1, cfg.cpu1_rstctrl_va);
-		readl_relaxed(cfg.cpu1_rstctrl_va);
-		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-	}
+	omap4_smp_maybe_reset_cpu1(&cfg);
 
 	/*
 	 * Write the address of secondary startup routine into the
@@ -319,7 +365,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
 	else
 		writel_relaxed(__pa_symbol(cfg.startup_addr),
-			       base + OMAP_AUX_CORE_BOOT_1);
+			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
 }
 
 const struct smp_operations omap4_smp_ops __initconst = {
-- 
2.11.1

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170216 11:08]:
> * Tony Lindgren <tony@atomide.com> [170216 08:55]:
> > For your use case, probably all we need is runtime checks for HS in
> > addition to parking cpu1 for kexec. If that's not enough, then maybe
> > a device specific DT property for never-reset-no-matter-what.
> 
> Below is a first take on the last resort cpu1 reset done based on
> configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
> we can't merge it yet as it will break kexec boot for dra7. To fix that,
> we need to first do what you're suggesting and properly park cpu1 for
> kexec.

Something like the patch below might be doable for the first fix
before we have something to park cpu1 for kexec. I've added more checks
to attemp to detect cpu1 being in WFI. At least things keep on working
for kexec on omap4/5 and GP dra7.

Andrew, care to give it a try and see if can add some HS dra7 checks
there too to have kexec work on it? Basically we want to attempt to
detect if there's a chance cpu1 is in WFI, and if I think the only
optino is to reset it before bringing it up as otherwise outcome will
be unpredictable.

Regards,

Tony

8< -----------------------
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
 extern int omap4_mpuss_init(void);
 extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
 extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
+extern u32 omap4_get_cpu1_ns_pa_addr(void);
 #else
 static inline int omap4_enter_lowpower(unsigned int cpu,
 					unsigned int power_state)
diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -64,6 +64,7 @@
 #include "prm-regbits-44xx.h"
 
 static void __iomem *sar_base;
+static u32 old_cpu1_ns_pa_addr;
 
 #if defined(CONFIG_PM) && defined(CONFIG_SMP)
 
@@ -212,6 +213,11 @@ static void __init save_l2x0_context(void)
 {}
 #endif
 
+u32 omap4_get_cpu1_ns_pa_addr(void)
+{
+	return old_cpu1_ns_pa_addr;
+}
+
 /**
  * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
  * The purpose of this function is to manage low power programming
@@ -371,7 +377,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x0);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -395,7 +401,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x1);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -432,7 +438,7 @@ int __init omap4_mpuss_init(void)
 		save_l2x0_context();
 	}
 
-	if (cpu_is_omap44xx()) {
+	if (soc_is_omap44xx()) {
 		omap_pm_ops.finish_suspend = omap4_finish_suspend;
 		omap_pm_ops.resume = omap4_cpu_resume;
 		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
@@ -443,7 +449,7 @@ int __init omap4_mpuss_init(void)
 		enable_mercury_retention_mode();
 	}
 
-	if (cpu_is_omap446x())
+	if (soc_is_omap446x())
 		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
 
 	return 0;
@@ -460,22 +466,30 @@ int __init omap4_mpuss_init(void)
 void __init omap4_mpuss_early_init(void)
 {
 	unsigned long startup_pa;
+	void __iomem *ns_pa_addr;
 
-	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
+	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
 		return;
 
 	sar_base = omap4_get_sar_ram_base();
 
-	if (cpu_is_omap443x())
+	/* Restore old NS_PA_ADDR for validity checks later on */
+	if (soc_is_omap44xx())
+		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	else
+		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
+
+	if (soc_is_omap443x())
 		startup_pa = __pa_symbol(omap4_secondary_startup);
-	else if (cpu_is_omap446x())
+	else if (soc_is_omap446x())
 		startup_pa = __pa_symbol(omap4460_secondary_startup);
 	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
 		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
 	else
 		startup_pa = __pa_symbol(omap5_secondary_startup);
 
-	if (cpu_is_omap44xx())
+	if (soc_is_omap44xx())
 		writel_relaxed(startup_pa, sar_base +
 			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
 	else
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -44,6 +44,7 @@ struct omap_smp_config {
 	unsigned long cpu1_rstctrl_pa;
 	void __iomem *cpu1_rstctrl_va;
 	void __iomem *scu_base;
+	void __iomem *wakeupgen_base;
 	void *startup_addr;
 };
 
@@ -140,7 +141,6 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	static struct clockdomain *cpu1_clkdm;
 	static bool booted;
 	static struct powerdomain *cpu1_pwrdm;
-	void __iomem *base = omap_get_wakeupgen_base();
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -157,7 +157,7 @@ static int omap4_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	if (omap_secure_apis_support())
 		omap_modify_auxcoreboot0(0x200, 0xfffffdff);
 	else
-		writel_relaxed(0x20, base + OMAP_AUX_CORE_BOOT_0);
+		writel_relaxed(0x20, cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_0);
 
 	if (!cpu1_clkdm && !cpu1_pwrdm) {
 		cpu1_clkdm = clkdm_lookup("mpu1_clkdm");
@@ -261,9 +261,62 @@ static void __init omap4_smp_init_cpus(void)
 		set_cpu_possible(i, true);
 }
 
+/*
+ * For now, just make sure the start-up address is not within
+ * the first 1GB as that most likely means that CPU1 is configured
+ * by either the bootloader or previous kernel in kexec boot to
+ * something that will most likely fail without a reset.
+ */
+static bool __init omap4_smp_cpu1_startup_valid(unsigned long addr)
+{
+	if ((addr >> 24) == 0x80)
+		return false;
+
+	return true;
+}
+
+/*
+ * We may need to reset CPU1 before configuring, otherwise kexec can end up
+ * trying to use old kernel startup address or suspend-resume will occasionally
+ * fail to bring up CPU1 on 4430 if CPU1 fails to enter deeper idle states.
+ */
+static void __init omap4_smp_maybe_reset_cpu1(struct omap_smp_config *c)
+{
+	unsigned long cpu1_startup_pa, cpu1_ns_pa_addr;
+	bool needs_reset = false;
+
+	cpu1_startup_pa = readl_relaxed(cfg.wakeupgen_base +
+					OMAP_AUX_CORE_BOOT_1);
+	cpu1_ns_pa_addr = omap4_get_cpu1_ns_pa_addr();
+
+	/* REVISIT: Anything to check for HS dra7? */
+	if (soc_is_dra74x() && omap_secure_apis_support())
+		return;
+
+	/* If dra7 has AUX_CORE_BOOT_1 within first 1GB, CPU1 may be in WFI */
+	if (soc_is_dra74x() && !omap_secure_apis_support() &&
+	    !omap4_smp_cpu1_startup_valid(cpu1_startup_pa))
+		needs_reset = true;
+
+	/* If omap4 or 5 has NS_PA_ADDR within first 1GB, CPU1 may be in WFI */
+	if ((soc_is_omap44xx() || soc_is_omap54xx()) &&
+	    !omap4_smp_cpu1_startup_valid(cpu1_ns_pa_addr) &&
+	    c->cpu1_rstctrl_va)
+		needs_reset = true;
+
+	if (!needs_reset || !c->cpu1_rstctrl_va)
+		return;
+
+	pr_info("smp: Already configured omap cpu1, needs reset (0x%lx 0x%lx)\n",
+		cpu1_startup_pa, cpu1_ns_pa_addr);
+
+	writel_relaxed(1, c->cpu1_rstctrl_va);
+	readl_relaxed(c->cpu1_rstctrl_va);
+	writel_relaxed(0, c->cpu1_rstctrl_va);
+}
+
 static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 {
-	void __iomem *base = omap_get_wakeupgen_base();
 	const struct omap_smp_config *c = NULL;
 
 	if (soc_is_omap443x())
@@ -281,6 +334,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	/* Must preserve cfg.scu_base set earlier */
 	cfg.cpu1_rstctrl_pa = c->cpu1_rstctrl_pa;
 	cfg.startup_addr = c->startup_addr;
+	cfg.wakeupgen_base = omap_get_wakeupgen_base();
 
 	if (soc_is_dra74x() || soc_is_omap54xx()) {
 		if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
@@ -299,15 +353,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 	if (cfg.scu_base)
 		scu_enable(cfg.scu_base);
 
-	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
-	 */
-	if (cfg.cpu1_rstctrl_va) {
-		writel_relaxed(1, cfg.cpu1_rstctrl_va);
-		readl_relaxed(cfg.cpu1_rstctrl_va);
-		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-	}
+	omap4_smp_maybe_reset_cpu1(&cfg);
 
 	/*
 	 * Write the address of secondary startup routine into the
@@ -319,7 +365,7 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		omap_auxcoreboot_addr(__pa_symbol(cfg.startup_addr));
 	else
 		writel_relaxed(__pa_symbol(cfg.startup_addr),
-			       base + OMAP_AUX_CORE_BOOT_1);
+			       cfg.wakeupgen_base + OMAP_AUX_CORE_BOOT_1);
 }
 
 const struct smp_operations omap4_smp_ops __initconst = {
-- 
2.11.1

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170216 08:55]:
> * Andrew F. Davis <afd@ti.com> [170216 08:30]:
> > On 02/16/2017 10:10 AM, Tony Lindgren wrote:
> > > * Tony Lindgren <tony@atomide.com> [170215 14:28]:
> > >> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> > >>> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > >>>> And also the same issue happens doing kexec on beagle-x15 naturally if
> > >>>> the cpu1 reset is removed.
> > >>>>
> > >>>
> > >>> When a core actually powers up it idles in ROM code waiting for
> > >>> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> > >>> powered off, we just let it spin in omap4_cpu_die() or
> > >>> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> > >>> it were still trapped in ROM after a reset.
> > > 
> > > OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> > > as we don't currently have omap5_secondary_startup() or any deeper
> > > idle mode support beyond retention for omap5 or dra7 in the mainline
> > > kernel.
> > > 
> > >>> The issue with this fake startup idle loop is that, unlike the ROM based
> > >>> startup idle loop, these do *not* jump to the address we stored in
> > >>> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> > >>> jump to the kernel startup function.
> > > 
> > > This does not seem to be the case here.
> > > 
> > 
> > Well this is what I am seeing every time, this code only works when it
> > is the same kernel we kexec, any changed addresses here will not work.
> 
> Hmm let's talk the mainline kernel here. Currently things do work in
> the mainline kernel because of the cpu1 reset. And without cpu1 reset
> things will currently go wrong in the mainline kernel both for kexec
> and suspend/resume.
> 
> > >>> So when we tell this core to boot, and it is not in the real ROM startup
> > >>> loop, it breaks stuff as it jumps to the old kernel's
> > >>> secondary_startup() even though we gave it the correct address in
> > >>> OMAP_AUX_CORE_BOOT_1.
> > > 
> > > And this is not happening. I think this is what I was seeing earlier,
> > > but it's not the omap5/dra7 issue.
> > > 
> > > What we have is cpu1 returning from previous kernel's omap_do_wfi()
> > > in the kexec booted kernel's code and that's when things go wrong.
> > > 
> > 
> > We are the ones sending it to omap_do_wfi(), in omap4_cpu_die() it gets
> > idled in a loop, it shouldn't be idled after it is shut off, it should
> > get parked, we should do this like we do in omap5_secondary_startup().
> 
> Yup agreed. We need to figure out if it's just normal cpuidle hot-unplug
> event vs shut down and park for kexec. Probably cpu_kill() is the place
> to park it, need to check.
> 
> > > So if cpu1 was configured for idle for any reason, it will never gets
> > > to omap5_secondary_startup without the reset currently.
> > > 
> > > The reason kexec and suspend/resume mostly works for omap4 without
> > > cpu1 reset is that we usually enter off mode for cpu1 and the context
> > > is lost and then we properly go through omap4_secondary_startup. Or
> > > that's my theory so far for the occasional flakeyness I've been seeing :)
> > > 
> > > Any ideas what we should try to check to see if cpu1 is in idle
> > > mode so we can do the reset if needed?
> > > 
> > 
> > You can never reset the core, resetting the core is not allowed on HS
> > devices and so it really doesn't matter what the core is doing. In no
> > case is reseting the core a valid work-around for not correctly parking
> > it. We need to fix the omap4_cpu_die() to not let the core go idle if
> > the return from idle path is the problem.
> 
> Yeah well from Linux point of view, what we're interested in is that
> cpu1 comes up reliably in all cases no matter what it takes. I agree
> doing a reset on it should be only done if nothing else helps. And I
> can see some HS implementations not allowing cpu1 reset. And I can see
> some product specific bootloaders idle cpu1 and that's where things
> break again.
> 
> For your use case, probably all we need is runtime checks for HS in
> addition to parking cpu1 for kexec. If that's not enough, then maybe
> a device specific DT property for never-reset-no-matter-what.

Below is a first take on the last resort cpu1 reset done based on
configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
we can't merge it yet as it will break kexec boot for dra7. To fix that,
we need to first do what you're suggesting and properly park cpu1 for
kexec.

Regards,

Tony

8< -----------------------------
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
 extern int omap4_mpuss_init(void);
 extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
 extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
+extern bool omap4_cpu1_may_need_reset(void);
 #else
 static inline int omap4_enter_lowpower(unsigned int cpu,
 					unsigned int power_state)
diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -64,6 +64,7 @@
 #include "prm-regbits-44xx.h"
 
 static void __iomem *sar_base;
+static u32 old_cpu1_ns_pa_addr;
 
 #if defined(CONFIG_PM) && defined(CONFIG_SMP)
 
@@ -213,6 +214,35 @@ static void __init save_l2x0_context(void)
 #endif
 
 /**
+ * omap4_cpu1_may_need_reset: Check if cpu1 needs to be reset on boot
+ *
+ * If cpu1 is configured for idle in the bootloader or in the previous
+ * kernel after kexec, it will wake-up from idle state to the configured
+ * restore address which is unsafe for booting kernel. In that case all
+ * we can do is reset cpu1 to get it to start_secondary. In at least
+ * omap5 case, the ROM code properly parks the bootloader at a higher
+ * kernel address, so for those cases no reset is needed. For anything
+ * configured for the first 1GiB at 0x80000000 let's assume we need a
+ * reset.
+ */
+bool omap4_cpu1_may_need_reset(void)
+{
+	bool unsafe_cpu1_ns_pa_addr;
+
+	if (!old_cpu1_ns_pa_addr)
+		return false;
+
+	unsafe_cpu1_ns_pa_addr = ((old_cpu1_ns_pa_addr >> 24) == 0x80);
+	if (!unsafe_cpu1_ns_pa_addr)
+		return false;
+
+	pr_info("smp: omap has configured ns_pa_addr: 0x%08x\n",
+		old_cpu1_ns_pa_addr);
+
+	return true;
+}
+
+/**
  * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
  * The purpose of this function is to manage low power programming
  * of OMAP4 MPUSS subsystem
@@ -371,7 +401,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x0);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -395,7 +425,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x1);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -432,7 +462,7 @@ int __init omap4_mpuss_init(void)
 		save_l2x0_context();
 	}
 
-	if (cpu_is_omap44xx()) {
+	if (soc_is_omap44xx()) {
 		omap_pm_ops.finish_suspend = omap4_finish_suspend;
 		omap_pm_ops.resume = omap4_cpu_resume;
 		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
@@ -443,7 +473,7 @@ int __init omap4_mpuss_init(void)
 		enable_mercury_retention_mode();
 	}
 
-	if (cpu_is_omap446x())
+	if (soc_is_omap446x())
 		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
 
 	return 0;
@@ -460,22 +490,30 @@ int __init omap4_mpuss_init(void)
 void __init omap4_mpuss_early_init(void)
 {
 	unsigned long startup_pa;
+	void __iomem *ns_pa_addr;
 
-	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
+	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
 		return;
 
 	sar_base = omap4_get_sar_ram_base();
 
-	if (cpu_is_omap443x())
+	/* Restore old NS_PA_ADDR for validity checks later on */
+	if (soc_is_omap44xx())
+		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	else
+		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
+
+	if (soc_is_omap443x())
 		startup_pa = __pa_symbol(omap4_secondary_startup);
-	else if (cpu_is_omap446x())
+	else if (soc_is_omap446x())
 		startup_pa = __pa_symbol(omap4460_secondary_startup);
 	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
 		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
 	else
 		startup_pa = __pa_symbol(omap5_secondary_startup);
 
-	if (cpu_is_omap44xx())
+	if (soc_is_omap44xx())
 		writel_relaxed(startup_pa, sar_base +
 			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
 	else
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -300,10 +300,13 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		scu_enable(cfg.scu_base);
 
 	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
+	 * Reset CPU1 before configuring, otherwise kexec can
+	 * end up trying to use old kernel startup address or
+	 * suspen/resume will fail bring up CPU1. Seen only on
+	 * 4430..
 	 */
-	if (cfg.cpu1_rstctrl_va) {
+	if (omap4_cpu1_may_need_reset() && cfg.cpu1_rstctrl_va) {
+		pr_info("smp: omap cpu1 idle configured, needs reset\n");
 		writel_relaxed(1, cfg.cpu1_rstctrl_va);
 		readl_relaxed(cfg.cpu1_rstctrl_va);
 		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-- 
2.11.1

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170216 08:55]:
> * Andrew F. Davis <afd@ti.com> [170216 08:30]:
> > On 02/16/2017 10:10 AM, Tony Lindgren wrote:
> > > * Tony Lindgren <tony@atomide.com> [170215 14:28]:
> > >> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> > >>> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > >>>> And also the same issue happens doing kexec on beagle-x15 naturally if
> > >>>> the cpu1 reset is removed.
> > >>>>
> > >>>
> > >>> When a core actually powers up it idles in ROM code waiting for
> > >>> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> > >>> powered off, we just let it spin in omap4_cpu_die() or
> > >>> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> > >>> it were still trapped in ROM after a reset.
> > > 
> > > OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> > > as we don't currently have omap5_secondary_startup() or any deeper
> > > idle mode support beyond retention for omap5 or dra7 in the mainline
> > > kernel.
> > > 
> > >>> The issue with this fake startup idle loop is that, unlike the ROM based
> > >>> startup idle loop, these do *not* jump to the address we stored in
> > >>> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> > >>> jump to the kernel startup function.
> > > 
> > > This does not seem to be the case here.
> > > 
> > 
> > Well this is what I am seeing every time, this code only works when it
> > is the same kernel we kexec, any changed addresses here will not work.
> 
> Hmm let's talk the mainline kernel here. Currently things do work in
> the mainline kernel because of the cpu1 reset. And without cpu1 reset
> things will currently go wrong in the mainline kernel both for kexec
> and suspend/resume.
> 
> > >>> So when we tell this core to boot, and it is not in the real ROM startup
> > >>> loop, it breaks stuff as it jumps to the old kernel's
> > >>> secondary_startup() even though we gave it the correct address in
> > >>> OMAP_AUX_CORE_BOOT_1.
> > > 
> > > And this is not happening. I think this is what I was seeing earlier,
> > > but it's not the omap5/dra7 issue.
> > > 
> > > What we have is cpu1 returning from previous kernel's omap_do_wfi()
> > > in the kexec booted kernel's code and that's when things go wrong.
> > > 
> > 
> > We are the ones sending it to omap_do_wfi(), in omap4_cpu_die() it gets
> > idled in a loop, it shouldn't be idled after it is shut off, it should
> > get parked, we should do this like we do in omap5_secondary_startup().
> 
> Yup agreed. We need to figure out if it's just normal cpuidle hot-unplug
> event vs shut down and park for kexec. Probably cpu_kill() is the place
> to park it, need to check.
> 
> > > So if cpu1 was configured for idle for any reason, it will never gets
> > > to omap5_secondary_startup without the reset currently.
> > > 
> > > The reason kexec and suspend/resume mostly works for omap4 without
> > > cpu1 reset is that we usually enter off mode for cpu1 and the context
> > > is lost and then we properly go through omap4_secondary_startup. Or
> > > that's my theory so far for the occasional flakeyness I've been seeing :)
> > > 
> > > Any ideas what we should try to check to see if cpu1 is in idle
> > > mode so we can do the reset if needed?
> > > 
> > 
> > You can never reset the core, resetting the core is not allowed on HS
> > devices and so it really doesn't matter what the core is doing. In no
> > case is reseting the core a valid work-around for not correctly parking
> > it. We need to fix the omap4_cpu_die() to not let the core go idle if
> > the return from idle path is the problem.
> 
> Yeah well from Linux point of view, what we're interested in is that
> cpu1 comes up reliably in all cases no matter what it takes. I agree
> doing a reset on it should be only done if nothing else helps. And I
> can see some HS implementations not allowing cpu1 reset. And I can see
> some product specific bootloaders idle cpu1 and that's where things
> break again.
> 
> For your use case, probably all we need is runtime checks for HS in
> addition to parking cpu1 for kexec. If that's not enough, then maybe
> a device specific DT property for never-reset-no-matter-what.

Below is a first take on the last resort cpu1 reset done based on
configured CPU1_WAKEUP_NS_PA_ADDR_OFFSET for omap4 and 5. Note that
we can't merge it yet as it will break kexec boot for dra7. To fix that,
we need to first do what you're suggesting and properly park cpu1 for
kexec.

Regards,

Tony

8< -----------------------------
diff --git a/arch/arm/mach-omap2/common.h b/arch/arm/mach-omap2/common.h
--- a/arch/arm/mach-omap2/common.h
+++ b/arch/arm/mach-omap2/common.h
@@ -270,6 +270,7 @@ extern const struct smp_operations omap4_smp_ops;
 extern int omap4_mpuss_init(void);
 extern int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state);
 extern int omap4_hotplug_cpu(unsigned int cpu, unsigned int power_state);
+extern bool omap4_cpu1_may_need_reset(void);
 #else
 static inline int omap4_enter_lowpower(unsigned int cpu,
 					unsigned int power_state)
diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -64,6 +64,7 @@
 #include "prm-regbits-44xx.h"
 
 static void __iomem *sar_base;
+static u32 old_cpu1_ns_pa_addr;
 
 #if defined(CONFIG_PM) && defined(CONFIG_SMP)
 
@@ -213,6 +214,35 @@ static void __init save_l2x0_context(void)
 #endif
 
 /**
+ * omap4_cpu1_may_need_reset: Check if cpu1 needs to be reset on boot
+ *
+ * If cpu1 is configured for idle in the bootloader or in the previous
+ * kernel after kexec, it will wake-up from idle state to the configured
+ * restore address which is unsafe for booting kernel. In that case all
+ * we can do is reset cpu1 to get it to start_secondary. In at least
+ * omap5 case, the ROM code properly parks the bootloader at a higher
+ * kernel address, so for those cases no reset is needed. For anything
+ * configured for the first 1GiB@0x80000000 let's assume we need a
+ * reset.
+ */
+bool omap4_cpu1_may_need_reset(void)
+{
+	bool unsafe_cpu1_ns_pa_addr;
+
+	if (!old_cpu1_ns_pa_addr)
+		return false;
+
+	unsafe_cpu1_ns_pa_addr = ((old_cpu1_ns_pa_addr >> 24) == 0x80);
+	if (!unsafe_cpu1_ns_pa_addr)
+		return false;
+
+	pr_info("smp: omap has configured ns_pa_addr: 0x%08x\n",
+		old_cpu1_ns_pa_addr);
+
+	return true;
+}
+
+/**
  * omap4_enter_lowpower: OMAP4 MPUSS Low Power Entry Function
  * The purpose of this function is to manage low power programming
  * of OMAP4 MPUSS subsystem
@@ -371,7 +401,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x0);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET0;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU0_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -395,7 +425,7 @@ int __init omap4_mpuss_init(void)
 	pm_info = &per_cpu(omap4_pm_info, 0x1);
 	if (sar_base) {
 		pm_info->scu_sar_addr = sar_base + SCU_OFFSET1;
-		if (cpu_is_omap44xx())
+		if (soc_is_omap44xx())
 			pm_info->wkup_sar_addr = sar_base +
 				CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
 		else
@@ -432,7 +462,7 @@ int __init omap4_mpuss_init(void)
 		save_l2x0_context();
 	}
 
-	if (cpu_is_omap44xx()) {
+	if (soc_is_omap44xx()) {
 		omap_pm_ops.finish_suspend = omap4_finish_suspend;
 		omap_pm_ops.resume = omap4_cpu_resume;
 		omap_pm_ops.scu_prepare = scu_pwrst_prepare;
@@ -443,7 +473,7 @@ int __init omap4_mpuss_init(void)
 		enable_mercury_retention_mode();
 	}
 
-	if (cpu_is_omap446x())
+	if (soc_is_omap446x())
 		omap_pm_ops.hotplug_restart = omap4460_secondary_startup;
 
 	return 0;
@@ -460,22 +490,30 @@ int __init omap4_mpuss_init(void)
 void __init omap4_mpuss_early_init(void)
 {
 	unsigned long startup_pa;
+	void __iomem *ns_pa_addr;
 
-	if (!(cpu_is_omap44xx() || soc_is_omap54xx()))
+	if (!(soc_is_omap44xx() || soc_is_omap54xx()))
 		return;
 
 	sar_base = omap4_get_sar_ram_base();
 
-	if (cpu_is_omap443x())
+	/* Restore old NS_PA_ADDR for validity checks later on */
+	if (soc_is_omap44xx())
+		ns_pa_addr = sar_base + CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	else
+		ns_pa_addr = sar_base + OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET;
+	old_cpu1_ns_pa_addr = readl_relaxed(ns_pa_addr);
+
+	if (soc_is_omap443x())
 		startup_pa = __pa_symbol(omap4_secondary_startup);
-	else if (cpu_is_omap446x())
+	else if (soc_is_omap446x())
 		startup_pa = __pa_symbol(omap4460_secondary_startup);
 	else if ((__boot_cpu_mode & MODE_MASK) == HYP_MODE)
 		startup_pa = __pa_symbol(omap5_secondary_hyp_startup);
 	else
 		startup_pa = __pa_symbol(omap5_secondary_startup);
 
-	if (cpu_is_omap44xx())
+	if (soc_is_omap44xx())
 		writel_relaxed(startup_pa, sar_base +
 			       CPU1_WAKEUP_NS_PA_ADDR_OFFSET);
 	else
diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -300,10 +300,13 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		scu_enable(cfg.scu_base);
 
 	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
+	 * Reset CPU1 before configuring, otherwise kexec can
+	 * end up trying to use old kernel startup address or
+	 * suspen/resume will fail bring up CPU1. Seen only on
+	 * 4430..
 	 */
-	if (cfg.cpu1_rstctrl_va) {
+	if (omap4_cpu1_may_need_reset() && cfg.cpu1_rstctrl_va) {
+		pr_info("smp: omap cpu1 idle configured, needs reset\n");
 		writel_relaxed(1, cfg.cpu1_rstctrl_va);
 		readl_relaxed(cfg.cpu1_rstctrl_va);
 		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-- 
2.11.1

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170216 08:30]:
> On 02/16/2017 10:10 AM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170215 14:28]:
> >> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> >>> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> >>>> And also the same issue happens doing kexec on beagle-x15 naturally if
> >>>> the cpu1 reset is removed.
> >>>>
> >>>
> >>> When a core actually powers up it idles in ROM code waiting for
> >>> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> >>> powered off, we just let it spin in omap4_cpu_die() or
> >>> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> >>> it were still trapped in ROM after a reset.
> > 
> > OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> > as we don't currently have omap5_secondary_startup() or any deeper
> > idle mode support beyond retention for omap5 or dra7 in the mainline
> > kernel.
> > 
> >>> The issue with this fake startup idle loop is that, unlike the ROM based
> >>> startup idle loop, these do *not* jump to the address we stored in
> >>> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> >>> jump to the kernel startup function.
> > 
> > This does not seem to be the case here.
> > 
> 
> Well this is what I am seeing every time, this code only works when it
> is the same kernel we kexec, any changed addresses here will not work.

Hmm let's talk the mainline kernel here. Currently things do work in
the mainline kernel because of the cpu1 reset. And without cpu1 reset
things will currently go wrong in the mainline kernel both for kexec
and suspend/resume.

> >>> So when we tell this core to boot, and it is not in the real ROM startup
> >>> loop, it breaks stuff as it jumps to the old kernel's
> >>> secondary_startup() even though we gave it the correct address in
> >>> OMAP_AUX_CORE_BOOT_1.
> > 
> > And this is not happening. I think this is what I was seeing earlier,
> > but it's not the omap5/dra7 issue.
> > 
> > What we have is cpu1 returning from previous kernel's omap_do_wfi()
> > in the kexec booted kernel's code and that's when things go wrong.
> > 
> 
> We are the ones sending it to omap_do_wfi(), in omap4_cpu_die() it gets
> idled in a loop, it shouldn't be idled after it is shut off, it should
> get parked, we should do this like we do in omap5_secondary_startup().

Yup agreed. We need to figure out if it's just normal cpuidle hot-unplug
event vs shut down and park for kexec. Probably cpu_kill() is the place
to park it, need to check.

> > So if cpu1 was configured for idle for any reason, it will never gets
> > to omap5_secondary_startup without the reset currently.
> > 
> > The reason kexec and suspend/resume mostly works for omap4 without
> > cpu1 reset is that we usually enter off mode for cpu1 and the context
> > is lost and then we properly go through omap4_secondary_startup. Or
> > that's my theory so far for the occasional flakeyness I've been seeing :)
> > 
> > Any ideas what we should try to check to see if cpu1 is in idle
> > mode so we can do the reset if needed?
> > 
> 
> You can never reset the core, resetting the core is not allowed on HS
> devices and so it really doesn't matter what the core is doing. In no
> case is reseting the core a valid work-around for not correctly parking
> it. We need to fix the omap4_cpu_die() to not let the core go idle if
> the return from idle path is the problem.

Yeah well from Linux point of view, what we're interested in is that
cpu1 comes up reliably in all cases no matter what it takes. I agree
doing a reset on it should be only done if nothing else helps. And I
can see some HS implementations not allowing cpu1 reset. And I can see
some product specific bootloaders idle cpu1 and that's where things
break again.

For your use case, probably all we need is runtime checks for HS in
addition to parking cpu1 for kexec. If that's not enough, then maybe
a device specific DT property for never-reset-no-matter-what.

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170216 08:30]:
> On 02/16/2017 10:10 AM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170215 14:28]:
> >> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> >>> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> >>>> And also the same issue happens doing kexec on beagle-x15 naturally if
> >>>> the cpu1 reset is removed.
> >>>>
> >>>
> >>> When a core actually powers up it idles in ROM code waiting for
> >>> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> >>> powered off, we just let it spin in omap4_cpu_die() or
> >>> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> >>> it were still trapped in ROM after a reset.
> > 
> > OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> > as we don't currently have omap5_secondary_startup() or any deeper
> > idle mode support beyond retention for omap5 or dra7 in the mainline
> > kernel.
> > 
> >>> The issue with this fake startup idle loop is that, unlike the ROM based
> >>> startup idle loop, these do *not* jump to the address we stored in
> >>> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> >>> jump to the kernel startup function.
> > 
> > This does not seem to be the case here.
> > 
> 
> Well this is what I am seeing every time, this code only works when it
> is the same kernel we kexec, any changed addresses here will not work.

Hmm let's talk the mainline kernel here. Currently things do work in
the mainline kernel because of the cpu1 reset. And without cpu1 reset
things will currently go wrong in the mainline kernel both for kexec
and suspend/resume.

> >>> So when we tell this core to boot, and it is not in the real ROM startup
> >>> loop, it breaks stuff as it jumps to the old kernel's
> >>> secondary_startup() even though we gave it the correct address in
> >>> OMAP_AUX_CORE_BOOT_1.
> > 
> > And this is not happening. I think this is what I was seeing earlier,
> > but it's not the omap5/dra7 issue.
> > 
> > What we have is cpu1 returning from previous kernel's omap_do_wfi()
> > in the kexec booted kernel's code and that's when things go wrong.
> > 
> 
> We are the ones sending it to omap_do_wfi(), in omap4_cpu_die() it gets
> idled in a loop, it shouldn't be idled after it is shut off, it should
> get parked, we should do this like we do in omap5_secondary_startup().

Yup agreed. We need to figure out if it's just normal cpuidle hot-unplug
event vs shut down and park for kexec. Probably cpu_kill() is the place
to park it, need to check.

> > So if cpu1 was configured for idle for any reason, it will never gets
> > to omap5_secondary_startup without the reset currently.
> > 
> > The reason kexec and suspend/resume mostly works for omap4 without
> > cpu1 reset is that we usually enter off mode for cpu1 and the context
> > is lost and then we properly go through omap4_secondary_startup. Or
> > that's my theory so far for the occasional flakeyness I've been seeing :)
> > 
> > Any ideas what we should try to check to see if cpu1 is in idle
> > mode so we can do the reset if needed?
> > 
> 
> You can never reset the core, resetting the core is not allowed on HS
> devices and so it really doesn't matter what the core is doing. In no
> case is reseting the core a valid work-around for not correctly parking
> it. We need to fix the omap4_cpu_die() to not let the core go idle if
> the return from idle path is the problem.

Yeah well from Linux point of view, what we're interested in is that
cpu1 comes up reliably in all cases no matter what it takes. I agree
doing a reset on it should be only done if nothing else helps. And I
can see some HS implementations not allowing cpu1 reset. And I can see
some product specific bootloaders idle cpu1 and that's where things
break again.

For your use case, probably all we need is runtime checks for HS in
addition to parking cpu1 for kexec. If that's not enough, then maybe
a device specific DT property for never-reset-no-matter-what.

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Andrew F. Davis]

On 02/16/2017 10:10 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170215 14:28]:
>> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
>>> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
>>>> And also the same issue happens doing kexec on beagle-x15 naturally if
>>>> the cpu1 reset is removed.
>>>>
>>>
>>> When a core actually powers up it idles in ROM code waiting for
>>> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
>>> powered off, we just let it spin in omap4_cpu_die() or
>>> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
>>> it were still trapped in ROM after a reset.
> 
> OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> as we don't currently have omap5_secondary_startup() or any deeper
> idle mode support beyond retention for omap5 or dra7 in the mainline
> kernel.
> 
>>> The issue with this fake startup idle loop is that, unlike the ROM based
>>> startup idle loop, these do *not* jump to the address we stored in
>>> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
>>> jump to the kernel startup function.
> 
> This does not seem to be the case here.
> 

Well this is what I am seeing every time, this code only works when it
is the same kernel we kexec, any changed addresses here will not work.

>>> So when we tell this core to boot, and it is not in the real ROM startup
>>> loop, it breaks stuff as it jumps to the old kernel's
>>> secondary_startup() even though we gave it the correct address in
>>> OMAP_AUX_CORE_BOOT_1.
> 
> And this is not happening. I think this is what I was seeing earlier,
> but it's not the omap5/dra7 issue.
> 
> What we have is cpu1 returning from previous kernel's omap_do_wfi()
> in the kexec booted kernel's code and that's when things go wrong.
> 

We are the ones sending it to omap_do_wfi(), in omap4_cpu_die() it gets
idled in a loop, it shouldn't be idled after it is shut off, it should
get parked, we should do this like we do in omap5_secondary_startup().

> So if cpu1 was configured for idle for any reason, it will never gets
> to omap5_secondary_startup without the reset currently.
> 
> The reason kexec and suspend/resume mostly works for omap4 without
> cpu1 reset is that we usually enter off mode for cpu1 and the context
> is lost and then we properly go through omap4_secondary_startup. Or
> that's my theory so far for the occasional flakeyness I've been seeing :)
> 
> Any ideas what we should try to check to see if cpu1 is in idle
> mode so we can do the reset if needed?
> 

You can never reset the core, resetting the core is not allowed on HS
devices and so it really doesn't matter what the core is doing. In no
case is reseting the core a valid work-around for not correctly parking
it. We need to fix the omap4_cpu_die() to not let the core go idle if
the return from idle path is the problem.

Andrew

> Regards,
> 
> Tony
> 

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Andrew F. Davis]

On 02/16/2017 10:10 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170215 14:28]:
>> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
>>> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
>>>> And also the same issue happens doing kexec on beagle-x15 naturally if
>>>> the cpu1 reset is removed.
>>>>
>>>
>>> When a core actually powers up it idles in ROM code waiting for
>>> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
>>> powered off, we just let it spin in omap4_cpu_die() or
>>> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
>>> it were still trapped in ROM after a reset.
> 
> OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> as we don't currently have omap5_secondary_startup() or any deeper
> idle mode support beyond retention for omap5 or dra7 in the mainline
> kernel.
> 
>>> The issue with this fake startup idle loop is that, unlike the ROM based
>>> startup idle loop, these do *not* jump to the address we stored in
>>> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
>>> jump to the kernel startup function.
> 
> This does not seem to be the case here.
> 

Well this is what I am seeing every time, this code only works when it
is the same kernel we kexec, any changed addresses here will not work.

>>> So when we tell this core to boot, and it is not in the real ROM startup
>>> loop, it breaks stuff as it jumps to the old kernel's
>>> secondary_startup() even though we gave it the correct address in
>>> OMAP_AUX_CORE_BOOT_1.
> 
> And this is not happening. I think this is what I was seeing earlier,
> but it's not the omap5/dra7 issue.
> 
> What we have is cpu1 returning from previous kernel's omap_do_wfi()
> in the kexec booted kernel's code and that's when things go wrong.
> 

We are the ones sending it to omap_do_wfi(), in omap4_cpu_die() it gets
idled in a loop, it shouldn't be idled after it is shut off, it should
get parked, we should do this like we do in omap5_secondary_startup().

> So if cpu1 was configured for idle for any reason, it will never gets
> to omap5_secondary_startup without the reset currently.
> 
> The reason kexec and suspend/resume mostly works for omap4 without
> cpu1 reset is that we usually enter off mode for cpu1 and the context
> is lost and then we properly go through omap4_secondary_startup. Or
> that's my theory so far for the occasional flakeyness I've been seeing :)
> 
> Any ideas what we should try to check to see if cpu1 is in idle
> mode so we can do the reset if needed?
> 

You can never reset the core, resetting the core is not allowed on HS
devices and so it really doesn't matter what the core is doing. In no
case is reseting the core a valid work-around for not correctly parking
it. We need to fix the omap4_cpu_die() to not let the core go idle if
the return from idle path is the problem.

Andrew

> Regards,
> 
> Tony
> 

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170216 08:11]:
> * Tony Lindgren <tony@atomide.com> [170215 14:28]:
> > * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> > > On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > > > And also the same issue happens doing kexec on beagle-x15 naturally if
> > > > the cpu1 reset is removed.
> > > > 
> > > 
> > > When a core actually powers up it idles in ROM code waiting for
> > > OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> > > powered off, we just let it spin in omap4_cpu_die() or
> > > omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> > > it were still trapped in ROM after a reset.
> 
> OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> as we don't currently have omap5_secondary_startup() or any deeper
> idle mode support beyond retention for omap5 or dra7 in the mainline
> kernel.
> 
> > > The issue with this fake startup idle loop is that, unlike the ROM based
> > > startup idle loop, these do *not* jump to the address we stored in
> > > OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> > > jump to the kernel startup function.
> 
> This does not seem to be the case here.
> 
> > > So when we tell this core to boot, and it is not in the real ROM startup
> > > loop, it breaks stuff as it jumps to the old kernel's
> > > secondary_startup() even though we gave it the correct address in
> > > OMAP_AUX_CORE_BOOT_1.
> 
> And this is not happening. I think this is what I was seeing earlier,
> but it's not the omap5/dra7 issue.
> 
> What we have is cpu1 returning from previous kernel's omap_do_wfi()
> in the kexec booted kernel's code and that's when things go wrong.
> 
> So if cpu1 was configured for idle for any reason, it will never gets
> to omap5_secondary_startup without the reset currently.
> 
> The reason kexec and suspend/resume mostly works for omap4 without
> cpu1 reset is that we usually enter off mode for cpu1 and the context
> is lost and then we properly go through omap4_secondary_startup. Or
> that's my theory so far for the occasional flakeyness I've been seeing :)
> 
> Any ideas what we should try to check to see if cpu1 is in idle
> mode so we can do the reset if needed?

Maybe we should do the reset if OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET
is not 0?

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170216 08:11]:
> * Tony Lindgren <tony@atomide.com> [170215 14:28]:
> > * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> > > On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > > > And also the same issue happens doing kexec on beagle-x15 naturally if
> > > > the cpu1 reset is removed.
> > > > 
> > > 
> > > When a core actually powers up it idles in ROM code waiting for
> > > OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> > > powered off, we just let it spin in omap4_cpu_die() or
> > > omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> > > it were still trapped in ROM after a reset.
> 
> OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
> as we don't currently have omap5_secondary_startup() or any deeper
> idle mode support beyond retention for omap5 or dra7 in the mainline
> kernel.
> 
> > > The issue with this fake startup idle loop is that, unlike the ROM based
> > > startup idle loop, these do *not* jump to the address we stored in
> > > OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> > > jump to the kernel startup function.
> 
> This does not seem to be the case here.
> 
> > > So when we tell this core to boot, and it is not in the real ROM startup
> > > loop, it breaks stuff as it jumps to the old kernel's
> > > secondary_startup() even though we gave it the correct address in
> > > OMAP_AUX_CORE_BOOT_1.
> 
> And this is not happening. I think this is what I was seeing earlier,
> but it's not the omap5/dra7 issue.
> 
> What we have is cpu1 returning from previous kernel's omap_do_wfi()
> in the kexec booted kernel's code and that's when things go wrong.
> 
> So if cpu1 was configured for idle for any reason, it will never gets
> to omap5_secondary_startup without the reset currently.
> 
> The reason kexec and suspend/resume mostly works for omap4 without
> cpu1 reset is that we usually enter off mode for cpu1 and the context
> is lost and then we properly go through omap4_secondary_startup. Or
> that's my theory so far for the occasional flakeyness I've been seeing :)
> 
> Any ideas what we should try to check to see if cpu1 is in idle
> mode so we can do the reset if needed?

Maybe we should do the reset if OMAP5_CPU1_WAKEUP_NS_PA_ADDR_OFFSET
is not 0?

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170215 14:28]:
> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> > On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > > And also the same issue happens doing kexec on beagle-x15 naturally if
> > > the cpu1 reset is removed.
> > > 
> > 
> > When a core actually powers up it idles in ROM code waiting for
> > OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> > powered off, we just let it spin in omap4_cpu_die() or
> > omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> > it were still trapped in ROM after a reset.

OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
as we don't currently have omap5_secondary_startup() or any deeper
idle mode support beyond retention for omap5 or dra7 in the mainline
kernel.

> > The issue with this fake startup idle loop is that, unlike the ROM based
> > startup idle loop, these do *not* jump to the address we stored in
> > OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> > jump to the kernel startup function.

This does not seem to be the case here.

> > So when we tell this core to boot, and it is not in the real ROM startup
> > loop, it breaks stuff as it jumps to the old kernel's
> > secondary_startup() even though we gave it the correct address in
> > OMAP_AUX_CORE_BOOT_1.

And this is not happening. I think this is what I was seeing earlier,
but it's not the omap5/dra7 issue.

What we have is cpu1 returning from previous kernel's omap_do_wfi()
in the kexec booted kernel's code and that's when things go wrong.

So if cpu1 was configured for idle for any reason, it will never gets
to omap5_secondary_startup without the reset currently.

The reason kexec and suspend/resume mostly works for omap4 without
cpu1 reset is that we usually enter off mode for cpu1 and the context
is lost and then we properly go through omap4_secondary_startup. Or
that's my theory so far for the occasional flakeyness I've been seeing :)

Any ideas what we should try to check to see if cpu1 is in idle
mode so we can do the reset if needed?

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170215 14:28]:
> * Andrew F. Davis <afd@ti.com> [170215 14:14]:
> > On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > > And also the same issue happens doing kexec on beagle-x15 naturally if
> > > the cpu1 reset is removed.
> > > 
> > 
> > When a core actually powers up it idles in ROM code waiting for
> > OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> > powered off, we just let it spin in omap4_cpu_die() or
> > omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> > it were still trapped in ROM after a reset.

OK so I debugged this a bit more. We have CPU1 in omap_do_wfi()
as we don't currently have omap5_secondary_startup() or any deeper
idle mode support beyond retention for omap5 or dra7 in the mainline
kernel.

> > The issue with this fake startup idle loop is that, unlike the ROM based
> > startup idle loop, these do *not* jump to the address we stored in
> > OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> > jump to the kernel startup function.

This does not seem to be the case here.

> > So when we tell this core to boot, and it is not in the real ROM startup
> > loop, it breaks stuff as it jumps to the old kernel's
> > secondary_startup() even though we gave it the correct address in
> > OMAP_AUX_CORE_BOOT_1.

And this is not happening. I think this is what I was seeing earlier,
but it's not the omap5/dra7 issue.

What we have is cpu1 returning from previous kernel's omap_do_wfi()
in the kexec booted kernel's code and that's when things go wrong.

So if cpu1 was configured for idle for any reason, it will never gets
to omap5_secondary_startup without the reset currently.

The reason kexec and suspend/resume mostly works for omap4 without
cpu1 reset is that we usually enter off mode for cpu1 and the context
is lost and then we properly go through omap4_secondary_startup. Or
that's my theory so far for the occasional flakeyness I've been seeing :)

Any ideas what we should try to check to see if cpu1 is in idle
mode so we can do the reset if needed?

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170215 14:14]:
> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170215 10:40]:
> >> * Tony Lindgren <tony@atomide.com> [170214 11:39]:
> >>> * Tony Lindgren <tony@atomide.com> [170213 13:51]:
> >>>> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> >>>> resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> >>>> on omap4 when doing kexec boot between two different kernel versions. The
> >>>> booted kernel ended up trying to use the old kernel start-up address unless
> >>>> cpu1 was reset before configuring the cpu1 start-up address.
> >>>>
> >>>> It seems the reset part was not correct but probably working around some
> >>>> other issue. I have not been able to reproduce this issue any longer despite
> >>>> testing with backported patches back to v4.6 kernel. So it is possible this
> >>>> issue was caused by other work in progress kexec patches I had applied. Or
> >>>> it is possible some other fixes have made the issue go way.
> >>>>
> >>>> The unconditional reset of cpu1 can cause issues booting some devices. For
> >>>> example, bootloader configured secure OS running on cpu1 will fail as the
> >>>> configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> >>>>
> >>>> Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> >>>> still need to reset cpu1 in some cases, we can add it back and do it
> >>>> conditionally.
> >>>
> >>> Actually with this I'm now seeing cpu1 not come up after a suspend/resume
> >>> cycle on duovero:
> >>>
> >>> [  118.257415] CPU1: shutdown
> >>> [  118.294616] Error taking CPU1 up: -2
> >>> [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
> >>> [  118.303802] PM: early resume of devices complete after 3.723 msecs
> >>>
> >>> So this issue needs to be investigated more.
> >>
> >> And then today the omap4 suspend/resume issue is no longer reproducable..
> >> Go figure.
> >>
> >> But then doing more testing I noticed that also omap5 needs the reset.
> >> Without it we get the following on omap5-uevm doing a kexec boot. So clearly
> >> the reset cannot be just removed at least for omap4 and omap5.
> > 
> > And also the same issue happens doing kexec on beagle-x15 naturally if
> > the cpu1 reset is removed.
> > 
> 
> When a core actually powers up it idles in ROM code waiting for
> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> powered off, we just let it spin in omap4_cpu_die() or
> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> it were still trapped in ROM after a reset.
> 
> The issue with this fake startup idle loop is that, unlike the ROM based
> startup idle loop, these do *not* jump to the address we stored in
> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> jump to the kernel startup function.
> 
> So when we tell this core to boot, and it is not in the real ROM startup
> loop, it breaks stuff as it jumps to the old kernel's
> secondary_startup() even though we gave it the correct address in
> OMAP_AUX_CORE_BOOT_1.

Yes this is probably what's going on here. Note that the error I pasted
was booting the same kernel where that address should be correct though.
So there might be something else to it also.

> Reseting the core to put it back in the real ROM idle loop is wrong, the
> two idle loop functions above should be fixed to respect the address in
> OMAP_AUX_CORE_BOOT_1 and not to make assumptions, this should take care
> of the kexec failure in a sane way.

OK care to try to patch it as now you also have a reproducable test
case for kexec too?

Regards,

Tony

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Andrew F. Davis <afd@ti.com> [170215 14:14]:
> On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> > * Tony Lindgren <tony@atomide.com> [170215 10:40]:
> >> * Tony Lindgren <tony@atomide.com> [170214 11:39]:
> >>> * Tony Lindgren <tony@atomide.com> [170213 13:51]:
> >>>> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> >>>> resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> >>>> on omap4 when doing kexec boot between two different kernel versions. The
> >>>> booted kernel ended up trying to use the old kernel start-up address unless
> >>>> cpu1 was reset before configuring the cpu1 start-up address.
> >>>>
> >>>> It seems the reset part was not correct but probably working around some
> >>>> other issue. I have not been able to reproduce this issue any longer despite
> >>>> testing with backported patches back to v4.6 kernel. So it is possible this
> >>>> issue was caused by other work in progress kexec patches I had applied. Or
> >>>> it is possible some other fixes have made the issue go way.
> >>>>
> >>>> The unconditional reset of cpu1 can cause issues booting some devices. For
> >>>> example, bootloader configured secure OS running on cpu1 will fail as the
> >>>> configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> >>>>
> >>>> Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> >>>> still need to reset cpu1 in some cases, we can add it back and do it
> >>>> conditionally.
> >>>
> >>> Actually with this I'm now seeing cpu1 not come up after a suspend/resume
> >>> cycle on duovero:
> >>>
> >>> [  118.257415] CPU1: shutdown
> >>> [  118.294616] Error taking CPU1 up: -2
> >>> [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
> >>> [  118.303802] PM: early resume of devices complete after 3.723 msecs
> >>>
> >>> So this issue needs to be investigated more.
> >>
> >> And then today the omap4 suspend/resume issue is no longer reproducable..
> >> Go figure.
> >>
> >> But then doing more testing I noticed that also omap5 needs the reset.
> >> Without it we get the following on omap5-uevm doing a kexec boot. So clearly
> >> the reset cannot be just removed at least for omap4 and omap5.
> > 
> > And also the same issue happens doing kexec on beagle-x15 naturally if
> > the cpu1 reset is removed.
> > 
> 
> When a core actually powers up it idles in ROM code waiting for
> OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
> powered off, we just let it spin in omap4_cpu_die() or
> omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
> it were still trapped in ROM after a reset.
> 
> The issue with this fake startup idle loop is that, unlike the ROM based
> startup idle loop, these do *not* jump to the address we stored in
> OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
> jump to the kernel startup function.
> 
> So when we tell this core to boot, and it is not in the real ROM startup
> loop, it breaks stuff as it jumps to the old kernel's
> secondary_startup() even though we gave it the correct address in
> OMAP_AUX_CORE_BOOT_1.

Yes this is probably what's going on here. Note that the error I pasted
was booting the same kernel where that address should be correct though.
So there might be something else to it also.

> Reseting the core to put it back in the real ROM idle loop is wrong, the
> two idle loop functions above should be fixed to respect the address in
> OMAP_AUX_CORE_BOOT_1 and not to make assumptions, this should take care
> of the kexec failure in a sane way.

OK care to try to patch it as now you also have a reproducable test
case for kexec too?

Regards,

Tony

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Andrew F. Davis]

On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170215 10:40]:
>> * Tony Lindgren <tony@atomide.com> [170214 11:39]:
>>> * Tony Lindgren <tony@atomide.com> [170213 13:51]:
>>>> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
>>>> resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
>>>> on omap4 when doing kexec boot between two different kernel versions. The
>>>> booted kernel ended up trying to use the old kernel start-up address unless
>>>> cpu1 was reset before configuring the cpu1 start-up address.
>>>>
>>>> It seems the reset part was not correct but probably working around some
>>>> other issue. I have not been able to reproduce this issue any longer despite
>>>> testing with backported patches back to v4.6 kernel. So it is possible this
>>>> issue was caused by other work in progress kexec patches I had applied. Or
>>>> it is possible some other fixes have made the issue go way.
>>>>
>>>> The unconditional reset of cpu1 can cause issues booting some devices. For
>>>> example, bootloader configured secure OS running on cpu1 will fail as the
>>>> configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
>>>>
>>>> Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
>>>> still need to reset cpu1 in some cases, we can add it back and do it
>>>> conditionally.
>>>
>>> Actually with this I'm now seeing cpu1 not come up after a suspend/resume
>>> cycle on duovero:
>>>
>>> [  118.257415] CPU1: shutdown
>>> [  118.294616] Error taking CPU1 up: -2
>>> [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
>>> [  118.303802] PM: early resume of devices complete after 3.723 msecs
>>>
>>> So this issue needs to be investigated more.
>>
>> And then today the omap4 suspend/resume issue is no longer reproducable..
>> Go figure.
>>
>> But then doing more testing I noticed that also omap5 needs the reset.
>> Without it we get the following on omap5-uevm doing a kexec boot. So clearly
>> the reset cannot be just removed at least for omap4 and omap5.
> 
> And also the same issue happens doing kexec on beagle-x15 naturally if
> the cpu1 reset is removed.
> 

When a core actually powers up it idles in ROM code waiting for
OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
powered off, we just let it spin in omap4_cpu_die() or
omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
it were still trapped in ROM after a reset.

The issue with this fake startup idle loop is that, unlike the ROM based
startup idle loop, these do *not* jump to the address we stored in
OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
jump to the kernel startup function.

So when we tell this core to boot, and it is not in the real ROM startup
loop, it breaks stuff as it jumps to the old kernel's
secondary_startup() even though we gave it the correct address in
OMAP_AUX_CORE_BOOT_1.

Reseting the core to put it back in the real ROM idle loop is wrong, the
two idle loop functions above should be fixed to respect the address in
OMAP_AUX_CORE_BOOT_1 and not to make assumptions, this should take care
of the kexec failure in a sane way.

Andrew

> Regards,
> 
> Tony
> 
>> 8< ---------------------
>> [    0.156796] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
>> [    0.163396] Setting up static identity map for 0x80100000 - 0x80100070
>> [    0.172246] smp: Bringing up secondary CPUs ...
>> [    0.178970] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> [    0.178974] pgd = c0004000
>> [    0.178977] [00000000] *pgd=00000000
>> [    0.178990] Internal error: Oops: 80000005 [#1] SMP ARM
>> [    0.178995] Modules linked in:
>> [    0.179005] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.10.0-rc8-next-20170215+ #120
>> [    0.179008] Hardware name: Generic OMAP5 (Flattened Device Tree)
>> [    0.179013] task: ee0c8ec0 task.stack: ee0ca000
>> [    0.179018] PC is at 0x0
>> [    0.179029] LR is at omap4_cpu_die+0x58/0x98
>> [    0.179034] pc : [<00000000>]    lr : [<c01243dc>]    psr: 60000093
>> [    0.179034] sp : ee0cbfb8  ip : 00000000  fp : 00000000
>> [    0.179038] r10: 00000000  r9 : c0d50569  r8 : 00000000
>> [    0.179042] r7 : c0c76448  r6 : c0d0792c  r5 : 00000001  r4 : c0b08054
>> [    0.179046] r3 : 00000001  r2 : f0880000  r1 : 00000003  r0 : 00000001
>> [    0.179051] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> [    0.179055] Control: 10c5387d  Table: 8000406a  DAC: 00000051
>> [    0.179059] Process swapper/1 (pid: 0, stack limit = 0xee0ca218)
>> [    0.179063] Stack: (0xee0cbfb8 to 0xee0cc000)
>> [    0.179068] bfa0:                                                       00000000 00000000
>> [    0.179075] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    0.179082] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 681b0041 cf3e4021
>> [    0.179092] [<c01243dc>] (omap4_cpu_die) from [<00000000>] (  (null))
>> [    0.179098] Code: bad PC value
>> [    0.179115] ---[ end trace e14406c260ce69db ]---
>> [    0.179121] Kernel panic - not syncing: Attempted to kill the idle task!
>> [    0.179135] CPU0: stopping
>> [    0.179141] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>> [    0.339715] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D         4.10.0-rc8-next-20170215+ #120
>> [    0.348927] Hardware name: Generic OMAP5 (Flattened Device Tree)
>> [    0.355112] [<c0110228>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14)
>> [    0.363083] [<c010c224>] (show_stack) from [<c04ca860>] (dump_stack+0xac/0xe0)
>> [    0.370513] [<c04ca860>] (dump_stack) from [<c010e72c>] (handle_IPI+0x358/0x3f8)
>> [    0.378120] [<c010e72c>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
>> [    0.385909] [<c01015a4>] (gic_handle_irq) from [<c083b270>] (__irq_svc+0x70/0x98)
>> [    0.393602] Exception stack(0xc0d01f38 to 0xc0d01f80)
>> [    0.398794] 1f20:                                                       c0108284 00000000
>> [    0.407205] 1f40: 00000000 00000000 c0d00000 c0d07994 c0d0792c c0c76448 c0d08560 c0d50569
>> [    0.415616] 1f60: 00000000 00000000 00000000 c0d01f88 c0108284 c0108288 60000013 ffffffff
>> [    0.424032] [<c083b270>] (__irq_svc) from [<c0108288>] (arch_cpu_idle+0x20/0x3c)
>> [    0.431643] [<c0108288>] (arch_cpu_idle) from [<c0190bc4>] (do_idle+0x164/0x218)
>> [    0.439251] [<c0190bc4>] (do_idle) from [<c0190ffc>] (cpu_startup_entry+0x18/0x1c)
>> [    0.447040] [<c0190ffc>] (cpu_startup_entry) from [<c0c00c40>] (start_kernel+0x35c/0x3d4)
>> [    0.455451] [<c0c00c40>] (start_kernel) from [<8000807c>] (0x8000807c)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Andrew F. Davis]

On 02/15/2017 01:12 PM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [170215 10:40]:
>> * Tony Lindgren <tony@atomide.com> [170214 11:39]:
>>> * Tony Lindgren <tony@atomide.com> [170213 13:51]:
>>>> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
>>>> resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
>>>> on omap4 when doing kexec boot between two different kernel versions. The
>>>> booted kernel ended up trying to use the old kernel start-up address unless
>>>> cpu1 was reset before configuring the cpu1 start-up address.
>>>>
>>>> It seems the reset part was not correct but probably working around some
>>>> other issue. I have not been able to reproduce this issue any longer despite
>>>> testing with backported patches back to v4.6 kernel. So it is possible this
>>>> issue was caused by other work in progress kexec patches I had applied. Or
>>>> it is possible some other fixes have made the issue go way.
>>>>
>>>> The unconditional reset of cpu1 can cause issues booting some devices. For
>>>> example, bootloader configured secure OS running on cpu1 will fail as the
>>>> configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
>>>>
>>>> Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
>>>> still need to reset cpu1 in some cases, we can add it back and do it
>>>> conditionally.
>>>
>>> Actually with this I'm now seeing cpu1 not come up after a suspend/resume
>>> cycle on duovero:
>>>
>>> [  118.257415] CPU1: shutdown
>>> [  118.294616] Error taking CPU1 up: -2
>>> [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
>>> [  118.303802] PM: early resume of devices complete after 3.723 msecs
>>>
>>> So this issue needs to be investigated more.
>>
>> And then today the omap4 suspend/resume issue is no longer reproducable..
>> Go figure.
>>
>> But then doing more testing I noticed that also omap5 needs the reset.
>> Without it we get the following on omap5-uevm doing a kexec boot. So clearly
>> the reset cannot be just removed at least for omap4 and omap5.
> 
> And also the same issue happens doing kexec on beagle-x15 naturally if
> the cpu1 reset is removed.
> 

When a core actually powers up it idles in ROM code waiting for
OMAP_AUX_CORE_BOOT_0 to be set. When we shutdown a core it is not really
powered off, we just let it spin in omap4_cpu_die() or
omap4_secondary_startup() waiting on OMAP_AUX_CORE_BOOT_0, just like if
it were still trapped in ROM after a reset.

The issue with this fake startup idle loop is that, unlike the ROM based
startup idle loop, these do *not* jump to the address we stored in
OMAP_AUX_CORE_BOOT_1, they just make the assumption that they can safely
jump to the kernel startup function.

So when we tell this core to boot, and it is not in the real ROM startup
loop, it breaks stuff as it jumps to the old kernel's
secondary_startup() even though we gave it the correct address in
OMAP_AUX_CORE_BOOT_1.

Reseting the core to put it back in the real ROM idle loop is wrong, the
two idle loop functions above should be fixed to respect the address in
OMAP_AUX_CORE_BOOT_1 and not to make assumptions, this should take care
of the kexec failure in a sane way.

Andrew

> Regards,
> 
> Tony
> 
>> 8< ---------------------
>> [    0.156796] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
>> [    0.163396] Setting up static identity map for 0x80100000 - 0x80100070
>> [    0.172246] smp: Bringing up secondary CPUs ...
>> [    0.178970] Unable to handle kernel NULL pointer dereference at virtual address 00000000
>> [    0.178974] pgd = c0004000
>> [    0.178977] [00000000] *pgd=00000000
>> [    0.178990] Internal error: Oops: 80000005 [#1] SMP ARM
>> [    0.178995] Modules linked in:
>> [    0.179005] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.10.0-rc8-next-20170215+ #120
>> [    0.179008] Hardware name: Generic OMAP5 (Flattened Device Tree)
>> [    0.179013] task: ee0c8ec0 task.stack: ee0ca000
>> [    0.179018] PC is at 0x0
>> [    0.179029] LR is at omap4_cpu_die+0x58/0x98
>> [    0.179034] pc : [<00000000>]    lr : [<c01243dc>]    psr: 60000093
>> [    0.179034] sp : ee0cbfb8  ip : 00000000  fp : 00000000
>> [    0.179038] r10: 00000000  r9 : c0d50569  r8 : 00000000
>> [    0.179042] r7 : c0c76448  r6 : c0d0792c  r5 : 00000001  r4 : c0b08054
>> [    0.179046] r3 : 00000001  r2 : f0880000  r1 : 00000003  r0 : 00000001
>> [    0.179051] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
>> [    0.179055] Control: 10c5387d  Table: 8000406a  DAC: 00000051
>> [    0.179059] Process swapper/1 (pid: 0, stack limit = 0xee0ca218)
>> [    0.179063] Stack: (0xee0cbfb8 to 0xee0cc000)
>> [    0.179068] bfa0:                                                       00000000 00000000
>> [    0.179075] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
>> [    0.179082] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 681b0041 cf3e4021
>> [    0.179092] [<c01243dc>] (omap4_cpu_die) from [<00000000>] (  (null))
>> [    0.179098] Code: bad PC value
>> [    0.179115] ---[ end trace e14406c260ce69db ]---
>> [    0.179121] Kernel panic - not syncing: Attempted to kill the idle task!
>> [    0.179135] CPU0: stopping
>> [    0.179141] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>> [    0.339715] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D         4.10.0-rc8-next-20170215+ #120
>> [    0.348927] Hardware name: Generic OMAP5 (Flattened Device Tree)
>> [    0.355112] [<c0110228>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14)
>> [    0.363083] [<c010c224>] (show_stack) from [<c04ca860>] (dump_stack+0xac/0xe0)
>> [    0.370513] [<c04ca860>] (dump_stack) from [<c010e72c>] (handle_IPI+0x358/0x3f8)
>> [    0.378120] [<c010e72c>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
>> [    0.385909] [<c01015a4>] (gic_handle_irq) from [<c083b270>] (__irq_svc+0x70/0x98)
>> [    0.393602] Exception stack(0xc0d01f38 to 0xc0d01f80)
>> [    0.398794] 1f20:                                                       c0108284 00000000
>> [    0.407205] 1f40: 00000000 00000000 c0d00000 c0d07994 c0d0792c c0c76448 c0d08560 c0d50569
>> [    0.415616] 1f60: 00000000 00000000 00000000 c0d01f88 c0108284 c0108288 60000013 ffffffff
>> [    0.424032] [<c083b270>] (__irq_svc) from [<c0108288>] (arch_cpu_idle+0x20/0x3c)
>> [    0.431643] [<c0108288>] (arch_cpu_idle) from [<c0190bc4>] (do_idle+0x164/0x218)
>> [    0.439251] [<c0190bc4>] (do_idle) from [<c0190ffc>] (cpu_startup_entry+0x18/0x1c)
>> [    0.447040] [<c0190ffc>] (cpu_startup_entry) from [<c0c00c40>] (start_kernel+0x35c/0x3d4)
>> [    0.455451] [<c0c00c40>] (start_kernel) from [<8000807c>] (0x8000807c)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170215 10:40]:
> * Tony Lindgren <tony@atomide.com> [170214 11:39]:
> > * Tony Lindgren <tony@atomide.com> [170213 13:51]:
> > > Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> > > resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> > > on omap4 when doing kexec boot between two different kernel versions. The
> > > booted kernel ended up trying to use the old kernel start-up address unless
> > > cpu1 was reset before configuring the cpu1 start-up address.
> > > 
> > > It seems the reset part was not correct but probably working around some
> > > other issue. I have not been able to reproduce this issue any longer despite
> > > testing with backported patches back to v4.6 kernel. So it is possible this
> > > issue was caused by other work in progress kexec patches I had applied. Or
> > > it is possible some other fixes have made the issue go way.
> > > 
> > > The unconditional reset of cpu1 can cause issues booting some devices. For
> > > example, bootloader configured secure OS running on cpu1 will fail as the
> > > configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> > > 
> > > Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> > > still need to reset cpu1 in some cases, we can add it back and do it
> > > conditionally.
> > 
> > Actually with this I'm now seeing cpu1 not come up after a suspend/resume
> > cycle on duovero:
> > 
> > [  118.257415] CPU1: shutdown
> > [  118.294616] Error taking CPU1 up: -2
> > [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
> > [  118.303802] PM: early resume of devices complete after 3.723 msecs
> > 
> > So this issue needs to be investigated more.
> 
> And then today the omap4 suspend/resume issue is no longer reproducable..
> Go figure.
> 
> But then doing more testing I noticed that also omap5 needs the reset.
> Without it we get the following on omap5-uevm doing a kexec boot. So clearly
> the reset cannot be just removed at least for omap4 and omap5.

And also the same issue happens doing kexec on beagle-x15 naturally if
the cpu1 reset is removed.

Regards,

Tony

> 8< ---------------------
> [    0.156796] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.163396] Setting up static identity map for 0x80100000 - 0x80100070
> [    0.172246] smp: Bringing up secondary CPUs ...
> [    0.178970] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [    0.178974] pgd = c0004000
> [    0.178977] [00000000] *pgd=00000000
> [    0.178990] Internal error: Oops: 80000005 [#1] SMP ARM
> [    0.178995] Modules linked in:
> [    0.179005] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.10.0-rc8-next-20170215+ #120
> [    0.179008] Hardware name: Generic OMAP5 (Flattened Device Tree)
> [    0.179013] task: ee0c8ec0 task.stack: ee0ca000
> [    0.179018] PC is at 0x0
> [    0.179029] LR is at omap4_cpu_die+0x58/0x98
> [    0.179034] pc : [<00000000>]    lr : [<c01243dc>]    psr: 60000093
> [    0.179034] sp : ee0cbfb8  ip : 00000000  fp : 00000000
> [    0.179038] r10: 00000000  r9 : c0d50569  r8 : 00000000
> [    0.179042] r7 : c0c76448  r6 : c0d0792c  r5 : 00000001  r4 : c0b08054
> [    0.179046] r3 : 00000001  r2 : f0880000  r1 : 00000003  r0 : 00000001
> [    0.179051] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [    0.179055] Control: 10c5387d  Table: 8000406a  DAC: 00000051
> [    0.179059] Process swapper/1 (pid: 0, stack limit = 0xee0ca218)
> [    0.179063] Stack: (0xee0cbfb8 to 0xee0cc000)
> [    0.179068] bfa0:                                                       00000000 00000000
> [    0.179075] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.179082] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 681b0041 cf3e4021
> [    0.179092] [<c01243dc>] (omap4_cpu_die) from [<00000000>] (  (null))
> [    0.179098] Code: bad PC value
> [    0.179115] ---[ end trace e14406c260ce69db ]---
> [    0.179121] Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.179135] CPU0: stopping
> [    0.179141] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.339715] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D         4.10.0-rc8-next-20170215+ #120
> [    0.348927] Hardware name: Generic OMAP5 (Flattened Device Tree)
> [    0.355112] [<c0110228>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14)
> [    0.363083] [<c010c224>] (show_stack) from [<c04ca860>] (dump_stack+0xac/0xe0)
> [    0.370513] [<c04ca860>] (dump_stack) from [<c010e72c>] (handle_IPI+0x358/0x3f8)
> [    0.378120] [<c010e72c>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
> [    0.385909] [<c01015a4>] (gic_handle_irq) from [<c083b270>] (__irq_svc+0x70/0x98)
> [    0.393602] Exception stack(0xc0d01f38 to 0xc0d01f80)
> [    0.398794] 1f20:                                                       c0108284 00000000
> [    0.407205] 1f40: 00000000 00000000 c0d00000 c0d07994 c0d0792c c0c76448 c0d08560 c0d50569
> [    0.415616] 1f60: 00000000 00000000 00000000 c0d01f88 c0108284 c0108288 60000013 ffffffff
> [    0.424032] [<c083b270>] (__irq_svc) from [<c0108288>] (arch_cpu_idle+0x20/0x3c)
> [    0.431643] [<c0108288>] (arch_cpu_idle) from [<c0190bc4>] (do_idle+0x164/0x218)
> [    0.439251] [<c0190bc4>] (do_idle) from [<c0190ffc>] (cpu_startup_entry+0x18/0x1c)
> [    0.447040] [<c0190ffc>] (cpu_startup_entry) from [<c0c00c40>] (start_kernel+0x35c/0x3d4)
> [    0.455451] [<c0c00c40>] (start_kernel) from [<8000807c>] (0x8000807c)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170215 10:40]:
> * Tony Lindgren <tony@atomide.com> [170214 11:39]:
> > * Tony Lindgren <tony@atomide.com> [170213 13:51]:
> > > Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> > > resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> > > on omap4 when doing kexec boot between two different kernel versions. The
> > > booted kernel ended up trying to use the old kernel start-up address unless
> > > cpu1 was reset before configuring the cpu1 start-up address.
> > > 
> > > It seems the reset part was not correct but probably working around some
> > > other issue. I have not been able to reproduce this issue any longer despite
> > > testing with backported patches back to v4.6 kernel. So it is possible this
> > > issue was caused by other work in progress kexec patches I had applied. Or
> > > it is possible some other fixes have made the issue go way.
> > > 
> > > The unconditional reset of cpu1 can cause issues booting some devices. For
> > > example, bootloader configured secure OS running on cpu1 will fail as the
> > > configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> > > 
> > > Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> > > still need to reset cpu1 in some cases, we can add it back and do it
> > > conditionally.
> > 
> > Actually with this I'm now seeing cpu1 not come up after a suspend/resume
> > cycle on duovero:
> > 
> > [  118.257415] CPU1: shutdown
> > [  118.294616] Error taking CPU1 up: -2
> > [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
> > [  118.303802] PM: early resume of devices complete after 3.723 msecs
> > 
> > So this issue needs to be investigated more.
> 
> And then today the omap4 suspend/resume issue is no longer reproducable..
> Go figure.
> 
> But then doing more testing I noticed that also omap5 needs the reset.
> Without it we get the following on omap5-uevm doing a kexec boot. So clearly
> the reset cannot be just removed at least for omap4 and omap5.

And also the same issue happens doing kexec on beagle-x15 naturally if
the cpu1 reset is removed.

Regards,

Tony

> 8< ---------------------
> [    0.156796] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
> [    0.163396] Setting up static identity map for 0x80100000 - 0x80100070
> [    0.172246] smp: Bringing up secondary CPUs ...
> [    0.178970] Unable to handle kernel NULL pointer dereference at virtual address 00000000
> [    0.178974] pgd = c0004000
> [    0.178977] [00000000] *pgd=00000000
> [    0.178990] Internal error: Oops: 80000005 [#1] SMP ARM
> [    0.178995] Modules linked in:
> [    0.179005] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.10.0-rc8-next-20170215+ #120
> [    0.179008] Hardware name: Generic OMAP5 (Flattened Device Tree)
> [    0.179013] task: ee0c8ec0 task.stack: ee0ca000
> [    0.179018] PC is at 0x0
> [    0.179029] LR is at omap4_cpu_die+0x58/0x98
> [    0.179034] pc : [<00000000>]    lr : [<c01243dc>]    psr: 60000093
> [    0.179034] sp : ee0cbfb8  ip : 00000000  fp : 00000000
> [    0.179038] r10: 00000000  r9 : c0d50569  r8 : 00000000
> [    0.179042] r7 : c0c76448  r6 : c0d0792c  r5 : 00000001  r4 : c0b08054
> [    0.179046] r3 : 00000001  r2 : f0880000  r1 : 00000003  r0 : 00000001
> [    0.179051] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
> [    0.179055] Control: 10c5387d  Table: 8000406a  DAC: 00000051
> [    0.179059] Process swapper/1 (pid: 0, stack limit = 0xee0ca218)
> [    0.179063] Stack: (0xee0cbfb8 to 0xee0cc000)
> [    0.179068] bfa0:                                                       00000000 00000000
> [    0.179075] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> [    0.179082] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 681b0041 cf3e4021
> [    0.179092] [<c01243dc>] (omap4_cpu_die) from [<00000000>] (  (null))
> [    0.179098] Code: bad PC value
> [    0.179115] ---[ end trace e14406c260ce69db ]---
> [    0.179121] Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.179135] CPU0: stopping
> [    0.179141] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.339715] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D         4.10.0-rc8-next-20170215+ #120
> [    0.348927] Hardware name: Generic OMAP5 (Flattened Device Tree)
> [    0.355112] [<c0110228>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14)
> [    0.363083] [<c010c224>] (show_stack) from [<c04ca860>] (dump_stack+0xac/0xe0)
> [    0.370513] [<c04ca860>] (dump_stack) from [<c010e72c>] (handle_IPI+0x358/0x3f8)
> [    0.378120] [<c010e72c>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
> [    0.385909] [<c01015a4>] (gic_handle_irq) from [<c083b270>] (__irq_svc+0x70/0x98)
> [    0.393602] Exception stack(0xc0d01f38 to 0xc0d01f80)
> [    0.398794] 1f20:                                                       c0108284 00000000
> [    0.407205] 1f40: 00000000 00000000 c0d00000 c0d07994 c0d0792c c0c76448 c0d08560 c0d50569
> [    0.415616] 1f60: 00000000 00000000 00000000 c0d01f88 c0108284 c0108288 60000013 ffffffff
> [    0.424032] [<c083b270>] (__irq_svc) from [<c0108288>] (arch_cpu_idle+0x20/0x3c)
> [    0.431643] [<c0108288>] (arch_cpu_idle) from [<c0190bc4>] (do_idle+0x164/0x218)
> [    0.439251] [<c0190bc4>] (do_idle) from [<c0190ffc>] (cpu_startup_entry+0x18/0x1c)
> [    0.447040] [<c0190ffc>] (cpu_startup_entry) from [<c0c00c40>] (start_kernel+0x35c/0x3d4)
> [    0.455451] [<c0c00c40>] (start_kernel) from [<8000807c>] (0x8000807c)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170214 11:39]:
> * Tony Lindgren <tony@atomide.com> [170213 13:51]:
> > Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> > resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> > on omap4 when doing kexec boot between two different kernel versions. The
> > booted kernel ended up trying to use the old kernel start-up address unless
> > cpu1 was reset before configuring the cpu1 start-up address.
> > 
> > It seems the reset part was not correct but probably working around some
> > other issue. I have not been able to reproduce this issue any longer despite
> > testing with backported patches back to v4.6 kernel. So it is possible this
> > issue was caused by other work in progress kexec patches I had applied. Or
> > it is possible some other fixes have made the issue go way.
> > 
> > The unconditional reset of cpu1 can cause issues booting some devices. For
> > example, bootloader configured secure OS running on cpu1 will fail as the
> > configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> > 
> > Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> > still need to reset cpu1 in some cases, we can add it back and do it
> > conditionally.
> 
> Actually with this I'm now seeing cpu1 not come up after a suspend/resume
> cycle on duovero:
> 
> [  118.257415] CPU1: shutdown
> [  118.294616] Error taking CPU1 up: -2
> [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
> [  118.303802] PM: early resume of devices complete after 3.723 msecs
> 
> So this issue needs to be investigated more.

And then today the omap4 suspend/resume issue is no longer reproducable..
Go figure.

But then doing more testing I noticed that also omap5 needs the reset.
Without it we get the following on omap5-uevm doing a kexec boot. So clearly
the reset cannot be just removed at least for omap4 and omap5.

Regards,

Tony

8< ---------------------
[    0.156796] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.163396] Setting up static identity map for 0x80100000 - 0x80100070
[    0.172246] smp: Bringing up secondary CPUs ...
[    0.178970] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[    0.178974] pgd = c0004000
[    0.178977] [00000000] *pgd=00000000
[    0.178990] Internal error: Oops: 80000005 [#1] SMP ARM
[    0.178995] Modules linked in:
[    0.179005] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.10.0-rc8-next-20170215+ #120
[    0.179008] Hardware name: Generic OMAP5 (Flattened Device Tree)
[    0.179013] task: ee0c8ec0 task.stack: ee0ca000
[    0.179018] PC is at 0x0
[    0.179029] LR is at omap4_cpu_die+0x58/0x98
[    0.179034] pc : [<00000000>]    lr : [<c01243dc>]    psr: 60000093
[    0.179034] sp : ee0cbfb8  ip : 00000000  fp : 00000000
[    0.179038] r10: 00000000  r9 : c0d50569  r8 : 00000000
[    0.179042] r7 : c0c76448  r6 : c0d0792c  r5 : 00000001  r4 : c0b08054
[    0.179046] r3 : 00000001  r2 : f0880000  r1 : 00000003  r0 : 00000001
[    0.179051] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    0.179055] Control: 10c5387d  Table: 8000406a  DAC: 00000051
[    0.179059] Process swapper/1 (pid: 0, stack limit = 0xee0ca218)
[    0.179063] Stack: (0xee0cbfb8 to 0xee0cc000)
[    0.179068] bfa0:                                                       00000000 00000000
[    0.179075] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    0.179082] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 681b0041 cf3e4021
[    0.179092] [<c01243dc>] (omap4_cpu_die) from [<00000000>] (  (null))
[    0.179098] Code: bad PC value
[    0.179115] ---[ end trace e14406c260ce69db ]---
[    0.179121] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.179135] CPU0: stopping
[    0.179141] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
[    0.339715] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D         4.10.0-rc8-next-20170215+ #120
[    0.348927] Hardware name: Generic OMAP5 (Flattened Device Tree)
[    0.355112] [<c0110228>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14)
[    0.363083] [<c010c224>] (show_stack) from [<c04ca860>] (dump_stack+0xac/0xe0)
[    0.370513] [<c04ca860>] (dump_stack) from [<c010e72c>] (handle_IPI+0x358/0x3f8)
[    0.378120] [<c010e72c>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
[    0.385909] [<c01015a4>] (gic_handle_irq) from [<c083b270>] (__irq_svc+0x70/0x98)
[    0.393602] Exception stack(0xc0d01f38 to 0xc0d01f80)
[    0.398794] 1f20:                                                       c0108284 00000000
[    0.407205] 1f40: 00000000 00000000 c0d00000 c0d07994 c0d0792c c0c76448 c0d08560 c0d50569
[    0.415616] 1f60: 00000000 00000000 00000000 c0d01f88 c0108284 c0108288 60000013 ffffffff
[    0.424032] [<c083b270>] (__irq_svc) from [<c0108288>] (arch_cpu_idle+0x20/0x3c)
[    0.431643] [<c0108288>] (arch_cpu_idle) from [<c0190bc4>] (do_idle+0x164/0x218)
[    0.439251] [<c0190bc4>] (do_idle) from [<c0190ffc>] (cpu_startup_entry+0x18/0x1c)
[    0.447040] [<c0190ffc>] (cpu_startup_entry) from [<c0c00c40>] (start_kernel+0x35c/0x3d4)
[    0.455451] [<c0c00c40>] (start_kernel) from [<8000807c>] (0x8000807c)

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170214 11:39]:
> * Tony Lindgren <tony@atomide.com> [170213 13:51]:
> > Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> > resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> > on omap4 when doing kexec boot between two different kernel versions. The
> > booted kernel ended up trying to use the old kernel start-up address unless
> > cpu1 was reset before configuring the cpu1 start-up address.
> > 
> > It seems the reset part was not correct but probably working around some
> > other issue. I have not been able to reproduce this issue any longer despite
> > testing with backported patches back to v4.6 kernel. So it is possible this
> > issue was caused by other work in progress kexec patches I had applied. Or
> > it is possible some other fixes have made the issue go way.
> > 
> > The unconditional reset of cpu1 can cause issues booting some devices. For
> > example, bootloader configured secure OS running on cpu1 will fail as the
> > configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> > 
> > Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> > still need to reset cpu1 in some cases, we can add it back and do it
> > conditionally.
> 
> Actually with this I'm now seeing cpu1 not come up after a suspend/resume
> cycle on duovero:
> 
> [  118.257415] CPU1: shutdown
> [  118.294616] Error taking CPU1 up: -2
> [  118.299072] PM: noirq resume of devices complete after 3.723 msecs
> [  118.303802] PM: early resume of devices complete after 3.723 msecs
> 
> So this issue needs to be investigated more.

And then today the omap4 suspend/resume issue is no longer reproducable..
Go figure.

But then doing more testing I noticed that also omap5 needs the reset.
Without it we get the following on omap5-uevm doing a kexec boot. So clearly
the reset cannot be just removed at least for omap4 and omap5.

Regards,

Tony

8< ---------------------
[    0.156796] CPU0: thread -1, cpu 0, socket 0, mpidr 80000000
[    0.163396] Setting up static identity map for 0x80100000 - 0x80100070
[    0.172246] smp: Bringing up secondary CPUs ...
[    0.178970] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[    0.178974] pgd = c0004000
[    0.178977] [00000000] *pgd=00000000
[    0.178990] Internal error: Oops: 80000005 [#1] SMP ARM
[    0.178995] Modules linked in:
[    0.179005] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.10.0-rc8-next-20170215+ #120
[    0.179008] Hardware name: Generic OMAP5 (Flattened Device Tree)
[    0.179013] task: ee0c8ec0 task.stack: ee0ca000
[    0.179018] PC is at 0x0
[    0.179029] LR is@omap4_cpu_die+0x58/0x98
[    0.179034] pc : [<00000000>]    lr : [<c01243dc>]    psr: 60000093
[    0.179034] sp : ee0cbfb8  ip : 00000000  fp : 00000000
[    0.179038] r10: 00000000  r9 : c0d50569  r8 : 00000000
[    0.179042] r7 : c0c76448  r6 : c0d0792c  r5 : 00000001  r4 : c0b08054
[    0.179046] r3 : 00000001  r2 : f0880000  r1 : 00000003  r0 : 00000001
[    0.179051] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    0.179055] Control: 10c5387d  Table: 8000406a  DAC: 00000051
[    0.179059] Process swapper/1 (pid: 0, stack limit = 0xee0ca218)
[    0.179063] Stack: (0xee0cbfb8 to 0xee0cc000)
[    0.179068] bfa0:                                                       00000000 00000000
[    0.179075] bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    0.179082] bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 681b0041 cf3e4021
[    0.179092] [<c01243dc>] (omap4_cpu_die) from [<00000000>] (  (null))
[    0.179098] Code: bad PC value
[    0.179115] ---[ end trace e14406c260ce69db ]---
[    0.179121] Kernel panic - not syncing: Attempted to kill the idle task!
[    0.179135] CPU0: stopping
[    0.179141] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
[    0.339715] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G      D         4.10.0-rc8-next-20170215+ #120
[    0.348927] Hardware name: Generic OMAP5 (Flattened Device Tree)
[    0.355112] [<c0110228>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14)
[    0.363083] [<c010c224>] (show_stack) from [<c04ca860>] (dump_stack+0xac/0xe0)
[    0.370513] [<c04ca860>] (dump_stack) from [<c010e72c>] (handle_IPI+0x358/0x3f8)
[    0.378120] [<c010e72c>] (handle_IPI) from [<c01015a4>] (gic_handle_irq+0x9c/0xb8)
[    0.385909] [<c01015a4>] (gic_handle_irq) from [<c083b270>] (__irq_svc+0x70/0x98)
[    0.393602] Exception stack(0xc0d01f38 to 0xc0d01f80)
[    0.398794] 1f20:                                                       c0108284 00000000
[    0.407205] 1f40: 00000000 00000000 c0d00000 c0d07994 c0d0792c c0c76448 c0d08560 c0d50569
[    0.415616] 1f60: 00000000 00000000 00000000 c0d01f88 c0108284 c0108288 60000013 ffffffff
[    0.424032] [<c083b270>] (__irq_svc) from [<c0108288>] (arch_cpu_idle+0x20/0x3c)
[    0.431643] [<c0108288>] (arch_cpu_idle) from [<c0190bc4>] (do_idle+0x164/0x218)
[    0.439251] [<c0190bc4>] (do_idle) from [<c0190ffc>] (cpu_startup_entry+0x18/0x1c)
[    0.447040] [<c0190ffc>] (cpu_startup_entry) from [<c0c00c40>] (start_kernel+0x35c/0x3d4)
[    0.455451] [<c0c00c40>] (start_kernel) from [<8000807c>] (0x8000807c)

Re: [PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170213 13:51]:
> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> on omap4 when doing kexec boot between two different kernel versions. The
> booted kernel ended up trying to use the old kernel start-up address unless
> cpu1 was reset before configuring the cpu1 start-up address.
> 
> It seems the reset part was not correct but probably working around some
> other issue. I have not been able to reproduce this issue any longer despite
> testing with backported patches back to v4.6 kernel. So it is possible this
> issue was caused by other work in progress kexec patches I had applied. Or
> it is possible some other fixes have made the issue go way.
> 
> The unconditional reset of cpu1 can cause issues booting some devices. For
> example, bootloader configured secure OS running on cpu1 will fail as the
> configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> 
> Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> still need to reset cpu1 in some cases, we can add it back and do it
> conditionally.

Actually with this I'm now seeing cpu1 not come up after a suspend/resume
cycle on duovero:

[  118.257415] CPU1: shutdown
[  118.294616] Error taking CPU1 up: -2
[  118.299072] PM: noirq resume of devices complete after 3.723 msecs
[  118.303802] PM: early resume of devices complete after 3.723 msecs

So this issue needs to be investigated more.

Regards,

Tony

> Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Reported-by: Andrew F. Davis <afd@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/omap-smp.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> --- a/arch/arm/mach-omap2/omap-smp.c
> +++ b/arch/arm/mach-omap2/omap-smp.c
> @@ -300,16 +300,6 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  		scu_enable(cfg.scu_base);
>  
>  	/*
> -	 * Reset CPU1 before configuring, otherwise kexec will
> -	 * end up trying to use old kernel startup address.
> -	 */
> -	if (cfg.cpu1_rstctrl_va) {
> -		writel_relaxed(1, cfg.cpu1_rstctrl_va);
> -		readl_relaxed(cfg.cpu1_rstctrl_va);
> -		writel_relaxed(0, cfg.cpu1_rstctrl_va);
> -	}
> -
> -	/*
>  	 * Write the address of secondary startup routine into the
>  	 * AuxCoreBoot1 where ROM code will jump and start executing
>  	 * on secondary core once out of WFE
> -- 
> 2.11.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [170213 13:51]:
> Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
> resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
> on omap4 when doing kexec boot between two different kernel versions. The
> booted kernel ended up trying to use the old kernel start-up address unless
> cpu1 was reset before configuring the cpu1 start-up address.
> 
> It seems the reset part was not correct but probably working around some
> other issue. I have not been able to reproduce this issue any longer despite
> testing with backported patches back to v4.6 kernel. So it is possible this
> issue was caused by other work in progress kexec patches I had applied. Or
> it is possible some other fixes have made the issue go way.
> 
> The unconditional reset of cpu1 can cause issues booting some devices. For
> example, bootloader configured secure OS running on cpu1 will fail as the
> configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.
> 
> Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
> still need to reset cpu1 in some cases, we can add it back and do it
> conditionally.

Actually with this I'm now seeing cpu1 not come up after a suspend/resume
cycle on duovero:

[  118.257415] CPU1: shutdown
[  118.294616] Error taking CPU1 up: -2
[  118.299072] PM: noirq resume of devices complete after 3.723 msecs
[  118.303802] PM: early resume of devices complete after 3.723 msecs

So this issue needs to be investigated more.

Regards,

Tony

> Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> Reported-by: Andrew F. Davis <afd@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  arch/arm/mach-omap2/omap-smp.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
> --- a/arch/arm/mach-omap2/omap-smp.c
> +++ b/arch/arm/mach-omap2/omap-smp.c
> @@ -300,16 +300,6 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
>  		scu_enable(cfg.scu_base);
>  
>  	/*
> -	 * Reset CPU1 before configuring, otherwise kexec will
> -	 * end up trying to use old kernel startup address.
> -	 */
> -	if (cfg.cpu1_rstctrl_va) {
> -		writel_relaxed(1, cfg.cpu1_rstctrl_va);
> -		readl_relaxed(cfg.cpu1_rstctrl_va);
> -		writel_relaxed(0, cfg.cpu1_rstctrl_va);
> -	}
> -
> -	/*
>  	 * Write the address of secondary startup routine into the
>  	 * AuxCoreBoot1 where ROM code will jump and start executing
>  	 * on secondary core once out of WFE
> -- 
> 2.11.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
on omap4 when doing kexec boot between two different kernel versions. The
booted kernel ended up trying to use the old kernel start-up address unless
cpu1 was reset before configuring the cpu1 start-up address.

It seems the reset part was not correct but probably working around some
other issue. I have not been able to reproduce this issue any longer despite
testing with backported patches back to v4.6 kernel. So it is possible this
issue was caused by other work in progress kexec patches I had applied. Or
it is possible some other fixes have made the issue go way.

The unconditional reset of cpu1 can cause issues booting some devices. For
example, bootloader configured secure OS running on cpu1 will fail as the
configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.

Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
still need to reset cpu1 in some cases, we can add it back and do it
conditionally.

Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
Cc: Keerthy <j-keerthy@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Reported-by: Andrew F. Davis <afd@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/omap-smp.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -300,16 +300,6 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		scu_enable(cfg.scu_base);
 
 	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
-	 */
-	if (cfg.cpu1_rstctrl_va) {
-		writel_relaxed(1, cfg.cpu1_rstctrl_va);
-		readl_relaxed(cfg.cpu1_rstctrl_va);
-		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-	}
-
-	/*
 	 * Write the address of secondary startup routine into the
 	 * AuxCoreBoot1 where ROM code will jump and start executing
 	 * on secondary core once out of WFE
-- 
2.11.1

[PATCH] ARM: omap2+: Revert omap-smp.c changes resetting cpu1 during boot

[Tony Lindgren]

Commit 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec") started
resetting cpu1 because of a kexec boot issue I was seeing earlier in 2016
on omap4 when doing kexec boot between two different kernel versions. The
booted kernel ended up trying to use the old kernel start-up address unless
cpu1 was reset before configuring the cpu1 start-up address.

It seems the reset part was not correct but probably working around some
other issue. I have not been able to reproduce this issue any longer despite
testing with backported patches back to v4.6 kernel. So it is possible this
issue was caused by other work in progress kexec patches I had applied. Or
it is possible some other fixes have made the issue go way.

The unconditional reset of cpu1 can cause issues booting some devices. For
example, bootloader configured secure OS running on cpu1 will fail as the
configuration is not preserved as reported by Andrew F. Davis <afd@ti.com>.

Let's fix the issue by reverting the cpu1 reset parts. If it turns out we
still need to reset cpu1 in some cases, we can add it back and do it
conditionally.

Fixes: 3251885285e1 ("ARM: OMAP4+: Reset CPU1 properly for kexec")
Cc: Keerthy <j-keerthy@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
Reported-by: Andrew F. Davis <afd@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/omap-smp.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-smp.c b/arch/arm/mach-omap2/omap-smp.c
--- a/arch/arm/mach-omap2/omap-smp.c
+++ b/arch/arm/mach-omap2/omap-smp.c
@@ -300,16 +300,6 @@ static void __init omap4_smp_prepare_cpus(unsigned int max_cpus)
 		scu_enable(cfg.scu_base);
 
 	/*
-	 * Reset CPU1 before configuring, otherwise kexec will
-	 * end up trying to use old kernel startup address.
-	 */
-	if (cfg.cpu1_rstctrl_va) {
-		writel_relaxed(1, cfg.cpu1_rstctrl_va);
-		readl_relaxed(cfg.cpu1_rstctrl_va);
-		writel_relaxed(0, cfg.cpu1_rstctrl_va);
-	}
-
-	/*
 	 * Write the address of secondary startup routine into the
 	 * AuxCoreBoot1 where ROM code will jump and start executing
 	 * on secondary core once out of WFE
-- 
2.11.1