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

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 <[email protected]>.

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 <[email protected]>

Cc Andrew F. Davis <[email protected]>

Cc Keerthy <[email protected]>

Cc Russell King <[email protected]>

Cc Tero Kristo <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 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(-)

arch/arm/mach-omap2/common.h +1 -0

@@ -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)

arch/arm/mach-omap2/omap-mpuss-lowpower.c +22 -8

@@ -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

arch/arm/mach-omap2/omap-smp.c +57 -13

@@ -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 = {

Quoted 4 lines · Tony Lindgren

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.

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 <[email protected]>.

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 <[email protected]>

Cc Andrew F. Davis <[email protected]>

Cc Keerthy <[email protected]>

Cc Russell King <[email protected]>

Cc Tero Kristo <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

---
 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(-)

arch/arm/mach-omap2/common.h +1 -0

@@ -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)

arch/arm/mach-omap2/omap-mpuss-lowpower.c +22 -8

@@ -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

arch/arm/mach-omap2/omap-smp.c +57 -13

@@ -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 = {

Quoted 3 lines · Tony Lindgren

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.

Quoted 9 lines · Tero Kristo

Quoted 3 lines · Tony Lindgren

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.

Quoted 11 lines · Tony Lindgren

Quoted 9 lines · Tero Kristo

Quoted 3 lines · Tony Lindgren

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.

Quoted 7 lines

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.

Quoted 3 lines

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
	 */

arch/arm/mach-omap2/omap-smp.c +4 -0

+
+	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
+	boot_address();
+

	break;
}

Quoted 20 lines · Andrew F. Davis

Quoted 11 lines · Tony Lindgren

Quoted 9 lines · Tero Kristo

Quoted 3 lines · Tony Lindgren

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.

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 7 lines

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 :)

Quoted 10 lines

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
	 */

arch/arm/mach-omap2/omap-smp.c +4 -0

+
+	boot_address = readl_relaxed(base + OMAP_AUX_CORE_BOOT_1);
+	boot_address();
+

Quoted 2 lines

	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.

#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

Regards,

Tony

Quoted 14 lines · Tony Lindgren

Quoted 12 lines · Andrew F. Davis

Quoted 4 lines · Tony Lindgren

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.

Quoted 3 lines

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.

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:

Quoted 8 lines

#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

Quoted 5 lines

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 F. Davis <[email protected]> [170314 10:59]:
Then onto the other related patches..

Quoted 2 lines

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.

Quoted 18 lines

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:

Quoted 13 lines

#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

Quoted 5 lines

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.

Quoted 32 lines · Tony Lindgren

* Andrew F. Davis <[email protected]> [170314 10:59]:
Then onto the other related patches..

Quoted 2 lines

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.

Quoted 23 lines

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:

Quoted 8 lines

#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

Quoted 5 lines

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.

Quoted 2 lines

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()?

Quoted 33 lines · Tony Lindgren

Quoted 32 lines · Tony Lindgren

* Andrew F. Davis <[email protected]> [170314 10:59]:
Then onto the other related patches..

Quoted 2 lines

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.

Quoted 23 lines

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:

Quoted 8 lines

#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

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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?

Quoted 46 lines · Tony Lindgren

Quoted 33 lines · Tony Lindgren

Quoted 32 lines · Tony Lindgren

* Andrew F. Davis <[email protected]> [170314 10:59]:
Then onto the other related patches..

Quoted 2 lines

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.

Quoted 23 lines

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:

Quoted 8 lines

#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

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 51 lines · Russell King - ARM Linux

Quoted 49 lines · Tony Lindgren

Quoted 33 lines · Tony Lindgren

Quoted 32 lines · Tony Lindgren

* Andrew F. Davis <[email protected]> [170314 10:59]:
Then onto the other related patches..

Quoted 2 lines

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.

Quoted 23 lines

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:

Quoted 8 lines

#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

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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?

Quoted 4 lines

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?

Quoted 1 line

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.

Quoted 3 lines

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?

Quoted 5 lines

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

Quoted 8 lines · Tony Lindgren

Quoted 3 lines · Russell King - ARM Linux

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.