ish: use magic number to verify persistent data

Move persistent data definitions to a structure and have linker script
define the address of the symbol into the AON ROM (persistent data
storage). Use the magic number "ISHd" to verify persistent data
storage and copy to static memory when valid. Commit changes from the
local copy during reset.

BUG=b:133779707,b:133647823,b:132059981
BRANCH=none
TEST=power-on is only reset flag under cold reset, panic data
persists, watchdog reset produces correct reset flags, UART always
printing system info on boot

Cq-Depend: chromium:1644188
Change-Id: I65a458cc2656f8fe26361ef2117ceb5439edff6c
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1644208
Commit-Queue: Jett Rink <jettrink@chromium.org>
Tested-by: Jett Rink <jettrink@chromium.org>

11 files changed, 149 insertions, 62 deletions

diff --git a/chip/ish/aontaskfw/ish_aon_share.h b/chip/ish/aontaskfw/ish_aon_share.h
index ee630a49bf..c8664165ea 100644
--- a/chip/ish/aontaskfw/ish_aon_share.h
+++ b/chip/ish/aontaskfw/ish_aon_share.h
@@ -6,6 +6,7 @@
 #ifndef __CROS_EC_ISH_AON_SHARE_H
 #define __CROS_EC_ISH_AON_SHARE_H
 
+#include "common.h"
 #include "ia_structs.h"
 
 /* magic ID for valid aontask image sanity check */
@@ -21,7 +22,6 @@
 struct ish_aon_share {
 	/* magic ID */
 	uint32_t magic_id;
-	/* last error */
 	/* error counter */
 	uint32_t error_count;
 	/* last error */
diff --git a/chip/ish/aontaskfw/ish_aontask.c b/chip/ish/aontaskfw/ish_aontask.c
index fc584857b1..5e16ee7d6a 100644
--- a/chip/ish/aontaskfw/ish_aontask.c
+++ b/chip/ish/aontaskfw/ish_aontask.c
@@ -192,7 +192,7 @@ static struct tss_entry aon_tss = {
 	.ebx = 0,
 	/* set stack top pointer at the end of usable aon memory */
 	.esp = CONFIG_AON_ROM_BASE - AON_SP_RESERVED,
-	.ebp = AON_SP_RESERVED,
+	.ebp = CONFIG_AON_ROM_BASE - AON_SP_RESERVED,
 	.esi = 0,
 	.edi = 0,
 	/* entry 1 in LDT for data segment */
diff --git a/chip/ish/build.mk b/chip/ish/build.mk
index b1481dfaf6..7ef31159eb 100644
--- a/chip/ish/build.mk
+++ b/chip/ish/build.mk
@@ -17,7 +17,7 @@ include core/$(CORE)/build.mk
 endif
 
 # Required chip modules
-chip-y+=clock.o gpio.o system.o hwtimer.o uart.o flash.o
+chip-y+=clock.o gpio.o system.o hwtimer.o uart.o flash.o ish_persistent_data.o
 chip-$(CONFIG_I2C)+=i2c.o
 chip-$(CONFIG_WATCHDOG)+=watchdog.o
 chip-$(CONFIG_HOSTCMD_HECI)+=host_command_heci.o
diff --git a/chip/ish/config_chip.h b/chip/ish/config_chip.h
index cf0bd75c1a..3f3f3ee275 100644
--- a/chip/ish/config_chip.h
+++ b/chip/ish/config_chip.h
@@ -9,6 +9,11 @@
 /* CPU core BFD configuration */
 #include "core/minute-ia/config_core.h"
 
+#ifndef __ASSEMBLER__
+/* Needed for PANIC_DATA_BASE */
+#include "ish_persistent_data.h"
+#endif
+
 /* Number of IRQ vectors on the ISH */
 #define CONFIG_IRQ_COUNT	(VEC_TO_IRQ(255) + 1)
 
@@ -54,13 +59,8 @@
 					 + CONFIG_AON_RAM_SIZE	\
 					 - CONFIG_AON_ROM_SIZE)
 
-/*
- * Store persistent panic data in AON memory. There are 256 bytes
- * available for ECOS use, and we need two software-defined REG32's at
- * the end.
- */
-#define CONFIG_PANIC_DATA_BASE		CONFIG_AON_ROM_BASE
-#define CONFIG_PANIC_DATA_SIZE		(256 - (2 * sizeof(uint32_t)))
+/* Store persistent panic data in AON memory. */
+#define CONFIG_PANIC_DATA_BASE		(&(ish_persistent_data.panic_data))
 
 /* System stack size */
 #define CONFIG_STACK_SIZE		1024
diff --git a/chip/ish/ish_persistent_data.c b/chip/ish/ish_persistent_data.c
new file mode 100644
index 0000000000..c5168475d5
--- /dev/null
+++ b/chip/ish/ish_persistent_data.c
@@ -0,0 +1,59 @@
+/* Copyright 2019 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "common.h"
+#include "config.h"
+#include "hooks.h"
+#include "system.h"
+#include "ish_persistent_data.h"
+
+#define PERSISTENT_DATA_MAGIC 0x49534864 /* "ISHd" */
+
+struct ish_persistent_data ish_persistent_data = {
+	.magic = PERSISTENT_DATA_MAGIC,
+	.reset_flags = RESET_FLAG_POWER_ON,
+	.watchdog_counter = 0,
+	.panic_data = {0},
+};
+
+/*
+ * When AON task firmware is not available (perhaps in the early
+ * stages of bringing up a new board), we have no way to persist data
+ * across reset. Allocate a memory region for "persistent data" which
+ * will never persist, this way we can use ish_persistent_data in a
+ * consistent manner without having to worry if the AON task firmware
+ * is available.
+ *
+ * Otherwise (AON task firmware is available), the
+ * ish_persistent_data_aon symbol is exported by the linker script.
+ */
+#ifdef CONFIG_ISH_PM_AONTASK
+extern struct ish_persistent_data ish_persistent_data_aon;
+#else
+static struct ish_persistent_data ish_persistent_data_aon;
+#endif
+
+void ish_persistent_data_init(void)
+{
+	if (ish_persistent_data_aon.magic == PERSISTENT_DATA_MAGIC) {
+		/* Stored data is valid, load a copy */
+		memcpy(&ish_persistent_data,
+		       &ish_persistent_data_aon,
+		       sizeof(struct ish_persistent_data));
+
+		/* Invalidate stored data, in case commit fails to happen */
+		ish_persistent_data_aon.magic = 0;
+	}
+
+	/* Update the system module's copy of the reset flags */
+	system_set_reset_flags(chip_read_reset_flags());
+}
+
+void ish_persistent_data_commit(void)
+{
+	memcpy(&ish_persistent_data_aon,
+	       &ish_persistent_data,
+	       sizeof(struct ish_persistent_data));
+}
diff --git a/chip/ish/ish_persistent_data.h b/chip/ish/ish_persistent_data.h
new file mode 100644
index 0000000000..65a85203fb
--- /dev/null
+++ b/chip/ish/ish_persistent_data.h
@@ -0,0 +1,41 @@
+/* Copyright 2019 The Chromium OS Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef __CROS_EC_ISH_PERSISTENT_DATA_H
+#define __CROS_EC_ISH_PERSISTENT_DATA_H
+
+#include "panic.h"
+
+/*
+ * If you make backwards-incompatible changes to this struct, (that
+ * is, reading a previous version of the data would be incorrect),
+ * simply change the magic number in ish_persistent_data.c. This will
+ * cause the struct to be re-initialized when the firmware loads.
+ */
+struct ish_persistent_data {
+	uint32_t magic;
+	uint32_t reset_flags;
+	uint32_t watchdog_counter;
+	struct panic_data panic_data;
+};
+
+/*
+ * Local copy of persistent data, which is copied from AON memory only
+ * if the data in AON memory is valid.
+ */
+extern struct ish_persistent_data ish_persistent_data;
+
+/*
+ * Copy the AON persistent data into the local copy and initialize
+ * system reset flags, only if magic number is correct.
+ */
+void ish_persistent_data_init(void);
+
+/*
+ * Commit the local copy to the AON memory (to be called at reset).
+ */
+void ish_persistent_data_commit(void);
+
+#endif /* __CROS_EC_ISH_PERSISTENT_DATA_H */
diff --git a/chip/ish/power_mgt.c b/chip/ish/power_mgt.c
index 63d9b6bc5d..e61838b953 100644
--- a/chip/ish/power_mgt.c
+++ b/chip/ish/power_mgt.c
@@ -3,15 +3,15 @@
  * found in the LICENSE file.
  */
 
-#include <console.h>
-#include <task.h>
-#include <system.h>
-#include <hwtimer.h>
-#include <util.h>
-#include "interrupts.h"
 #include "aontaskfw/ish_aon_share.h"
-#include "power_mgt.h"
+#include "console.h"
+#include "hwtimer.h"
+#include "interrupts.h"
 #include "ish_dma.h"
+#include "power_mgt.h"
+#include "system.h"
+#include "task.h"
+#include "util.h"
 
 #ifdef CONFIG_ISH_PM_DEBUG
 #define CPUTS(outstr) cputs(CC_SYSTEM, outstr)
diff --git a/chip/ish/power_mgt.h b/chip/ish/power_mgt.h
index d592928744..bd2e707cb4 100644
--- a/chip/ish/power_mgt.h
+++ b/chip/ish/power_mgt.h
@@ -6,6 +6,8 @@
 #ifndef __CROS_EC_POWER_MGT_H
 #define __CROS_EC_POWER_MGT_H
 
+#include "registers.h"
+
 /* power states for ISH */
 enum {
 	/* D0 state: active mode */
@@ -57,12 +59,14 @@ static inline void ish_mia_reset(void)
 	__builtin_unreachable();
 }
 
-
-/**
- * ish low power management initialization,
- * should be called at system init stage before RTOS task scheduling start
- */
+/* Initialize power management module. */
+#ifdef CONFIG_LOW_POWER_IDLE
 void ish_pm_init(void);
+#else
+__maybe_unused static void ish_pm_init(void)
+{
+}
+#endif
 
 /**
  * reset ISH (reset minute-ia cpu core, and power off main SRAM)
diff --git a/chip/ish/registers.h b/chip/ish/registers.h
index 1ffd5fad75..a736093b3c 100644
--- a/chip/ish/registers.h
+++ b/chip/ish/registers.h
@@ -337,12 +337,6 @@ enum ish_i2c_port {
 #define ISH_SRAM_CTRL_ERASE_ADDR	REG32(ISH_SRAM_CTRL_BASE + 0x10)
 #define ISH_SRAM_CTRL_BANK_STATUS	REG32(ISH_SRAM_CTRL_BASE + 0x2c)
 
-/* Software defined registers */
-
-/* Persistent reset flags - placed directly at end of panic data */
-#define ISH_RESET_FLAGS		REG32(CONFIG_PANIC_DATA_BASE	\
-					      + CONFIG_PANIC_DATA_SIZE)
-
 #if defined(CHIP_FAMILY_ISH3)
 /* on ISH3, reused ISH2PMC IPC message registers */
 #define SNOWBALL_BASE			IPC_ISH2PMC_MSG_BASE
diff --git a/chip/ish/system.c b/chip/ish/system.c
index 1af3499f62..0d778eaa5e 100644
--- a/chip/ish/system.c
+++ b/chip/ish/system.c
@@ -3,30 +3,23 @@
  * found in the LICENSE file.
  */
 
-/* System module ISH (Not implemented) */
-
 #include "clock.h"
 #include "common.h"
 #include "console.h"
 #include "cpu.h"
 #include "gpio.h"
+#include "hooks.h"
 #include "host_command.h"
 #include "ish_fwst.h"
+#include "ish_persistent_data.h"
+#include "power_mgt.h"
 #include "registers.h"
 #include "shared_mem.h"
+#include "spi.h"
 #include "system.h"
-#include "hooks.h"
 #include "task.h"
 #include "timer.h"
 #include "util.h"
-#include "spi.h"
-#include "power_mgt.h"
-
-/* Indices for hibernate data registers (RAM backed by VBAT) */
-enum hibdata_index {
-	HIBDATA_INDEX_SCRATCHPAD = 0,    /* General-purpose scratchpad */
-	HIBDATA_INDEX_SAVED_RESET_FLAGS  /* Saved reset flags */
-};
 
 int system_is_reboot_warm(void)
 {
@@ -37,35 +30,34 @@ int system_is_reboot_warm(void)
 void system_pre_init(void)
 {
 	ish_fwst_set_fw_status(FWSTS_FW_IS_RUNNING);
-
 	task_enable_irq(ISH_FABRIC_IRQ);
-
-	if (IS_ENABLED(CONFIG_LOW_POWER_IDLE))
-		ish_pm_init();
-
-	system_set_reset_flags(chip_read_reset_flags());
+	ish_pm_init();
+	ish_persistent_data_init();
 }
 
 void chip_save_reset_flags(uint32_t flags)
 {
-	ISH_RESET_FLAGS = flags;
+	ish_persistent_data.reset_flags = flags;
 }
 
 uint32_t chip_read_reset_flags(void)
 {
-	uint32_t flags = ISH_RESET_FLAGS;
-
-	if (flags)
-		return flags;
-
-	/* Flags are zero? Assume we came up from a cold reset */
-	return RESET_FLAG_POWER_ON;
+	return ish_persistent_data.reset_flags;
 }
 
 void system_reset(int flags)
 {
 	uint32_t save_flags;
 
+	/*
+	 * We can't save any data when we do an ish_mia_reset(). Take
+	 * the quick path out.
+	 */
+	if (!IS_ENABLED(CONFIG_ISH_PM_AONTASK) || flags & SYSTEM_RESET_HARD) {
+		ish_mia_reset();
+		__builtin_unreachable();
+	}
+
 	system_encode_save_flags(flags, &save_flags);
 
 	if (flags & SYSTEM_RESET_AP_WATCHDOG)
@@ -73,15 +65,8 @@ void system_reset(int flags)
 
 	chip_save_reset_flags(save_flags);
 
-	/*
-	 * ish_pm_reset() does more (poweroff main SRAM, etc) than
-	 * ish_mia_reset() which just resets the ISH minute-ia cpu core
-	 */
-	if (!IS_ENABLED(CONFIG_LOW_POWER_IDLE) || flags & SYSTEM_RESET_HARD)
-		ish_mia_reset();
-	else
-		ish_pm_reset();
-
+	ish_persistent_data_commit();
+	ish_pm_reset();
 	__builtin_unreachable();
 }
 
diff --git a/core/minute-ia/ec.lds.S b/core/minute-ia/ec.lds.S
index 55102d5ae5..2a303a1f94 100644
--- a/core/minute-ia/ec.lds.S
+++ b/core/minute-ia/ec.lds.S
@@ -206,4 +206,8 @@ SECTIONS
 
 	def_irq_low  = ABSOLUTE(default_int_handler) & 0xFFFF;
 	def_irq_high = ABSOLUTE(default_int_handler) >> 16;
+
+#ifdef CONFIG_ISH_PM_AONTASK
+	ish_persistent_data_aon = ABSOLUTE(CONFIG_AON_ROM_BASE);
+#endif
 }