zephyr/hooks: don't store hooks in RAM

This saves 8 bytes times the number of hook handlers in RAM, which amounts
to nearly 1KB in most configurations. It trades that RAM for higher
cost in calling handlers since the hook list must be traversed multiple
times for each notification, but that matches the ECOS behavior which
implies the performance cost is acceptable.

BUG=b:223044986
TEST=zmake testall; 944 bytes of RAM are saved on Nereid
BRANCH=none

Signed-off-by: Peter Marheine <pmarheine@chromium.org>
Change-Id: I823f2a974faf69fa5195f11c645b569fb57854a9
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3529602
Reviewed-by: Denis Brockus <dbrockus@chromium.org>

5 files changed, 166 insertions, 69 deletions

diff --git a/zephyr/linker/iterables-ram.ld b/zephyr/linker/iterables-ram.ld
index 58318a0e78..e69de29bb2 100644
--- a/zephyr/linker/iterables-ram.ld
+++ b/zephyr/linker/iterables-ram.ld
@@ -1,3 +0,0 @@
-#ifdef CONFIG_PLATFORM_EC_HOOKS
-ITERABLE_SECTION_RAM(zephyr_shim_hook_list, 4)
-#endif
diff --git a/zephyr/linker/iterables-rom.ld b/zephyr/linker/iterables-rom.ld
index b8e451a085..25a9a4c7b7 100644
--- a/zephyr/linker/iterables-rom.ld
+++ b/zephyr/linker/iterables-rom.ld
@@ -5,3 +5,11 @@ ITERABLE_SECTION_ROM(host_command, 4)
 #ifdef CONFIG_PLATFORM_EC_MKBP_EVENT
 ITERABLE_SECTION_ROM(mkbp_event_source, 4)
 #endif
+
+#ifdef CONFIG_PLATFORM_EC_HOOKS
+#include "../shim/include/hook_types.h"
+#define HOOK_TYPE_SECTION(type) \
+	ITERABLE_SECTION_ROM(zephyr_shim_hook_##type, 4)
+FOR_EACH(HOOK_TYPE_SECTION, (
+), HOOK_TYPES_LIST)
+#endif
\ No newline at end of file
diff --git a/zephyr/shim/include/hook_types.h b/zephyr/shim/include/hook_types.h
new file mode 100644
index 0000000000..363fa01a94
--- /dev/null
+++ b/zephyr/shim/include/hook_types.h
@@ -0,0 +1,52 @@
+/* Copyright 2022 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_HOOK_TYPES_H_
+#define __CROS_EC_HOOK_TYPES_H_
+
+#include <sys/util_macro.h>
+
+/*
+ * Some config macros are defined but without any value for some boards, so
+ * IF_ENABLED checks cannot be used. In addition, config values may not be set
+ * when generating the linker script: in that case we emit every possible hook
+ * type, since doing so won't bloat the output if no corresponding sections
+ * were generated.
+ */
+#if defined(TEST_BUILD) || defined(_LINKER)
+#define HOOK_TYPES_TEST_BUILD HOOK_TEST_1, HOOK_TEST_2, HOOK_TEST_3,
+#else
+#define HOOK_TYPES_TEST_BUILD
+#endif
+
+#if defined(CONFIG_USB_SUSPEND) || defined(_LINKER)
+#define HOOK_TYPES_USB_SUSPEND HOOK_USB_PM_CHANGE
+#else
+#define HOOK_TYPES_USB_SUSPEND
+#endif
+
+/*
+ * HOOK_TYPES_LIST is a sequence of tokens that expands to every enabled
+ * `enum hook_type` value.
+ *
+ * If the enum definition is changed, this macro must also be changed.
+ */
+#define HOOK_TYPES_LIST                                                    \
+	LIST_DROP_EMPTY(                                                   \
+		HOOK_INIT, HOOK_PRE_FREQ_CHANGE, HOOK_FREQ_CHANGE,         \
+		HOOK_SYSJUMP, HOOK_CHIPSET_PRE_INIT, HOOK_CHIPSET_STARTUP, \
+		HOOK_CHIPSET_RESUME, HOOK_CHIPSET_SUSPEND,                 \
+		IF_ENABLED(CONFIG_PLATFORM_EC_CHIPSET_RESUME_INIT_HOOK,    \
+			   (HOOK_CHIPSET_RESUME_INIT,                      \
+			    HOOK_CHIPSET_SUSPEND_COMPLETE, ))              \
+			HOOK_CHIPSET_SHUTDOWN,                             \
+		HOOK_CHIPSET_SHUTDOWN_COMPLETE, HOOK_CHIPSET_HARD_OFF,     \
+		HOOK_CHIPSET_RESET, HOOK_AC_CHANGE, HOOK_LID_CHANGE,       \
+		HOOK_TABLET_MODE_CHANGE, HOOK_BASE_ATTACHED_CHANGE,        \
+		HOOK_POWER_BUTTON_CHANGE, HOOK_BATTERY_SOC_CHANGE,         \
+		HOOK_TYPES_USB_SUSPEND, HOOK_TICK, HOOK_SECOND,            \
+		HOOK_USB_PD_DISCONNECT, HOOK_USB_PD_CONNECT,               \
+		HOOK_TYPES_TEST_BUILD)
+
+#endif
diff --git a/zephyr/shim/include/zephyr_hooks_shim.h b/zephyr/shim/include/zephyr_hooks_shim.h
index e233e1a6af..7b56487076 100644
--- a/zephyr/shim/include/zephyr_hooks_shim.h
+++ b/zephyr/shim/include/zephyr_hooks_shim.h
@@ -35,33 +35,30 @@ int hook_call_deferred(const struct deferred_data *data, int us);
 	}
 
 /**
- * Internal linked-list structure used to store hook lists.
- *
- * The info is constant, so storing a pointer to it in RAM rather than a copy
- * saves 4 bytes of RAM per entry (assuming 32-bit pointers and alignment).
+ * Record describing a single hook routine.
  */
 struct zephyr_shim_hook_info {
 	void (*routine)(void);
 	uint16_t priority; /* HOOK_PRIO_LAST = 9999 */
-	enum hook_type type;
 };
 
+/*
+ * List of hook routines.
+ *
+ * Values are contiguous, where *start is the first and *(end - 1) is the last.
+ */
 struct zephyr_shim_hook_list {
-	const struct zephyr_shim_hook_info *info;
-	struct zephyr_shim_hook_list *next;
+	const struct zephyr_shim_hook_info *start;
+	const struct zephyr_shim_hook_info *end;
 };
 
 /**
  * See include/hooks.h for documentation.
  */
-#define DECLARE_HOOK(_hooktype, _routine, _priority)                     \
-	static const struct zephyr_shim_hook_info                        \
-		_cros_hook_##_hooktype##_info_##_routine = {             \
-			.type = _hooktype,                               \
-			.priority = _priority,                           \
-			.routine = _routine,                             \
-		};                                                       \
-	STRUCT_SECTION_ITERABLE(zephyr_shim_hook_list,                   \
-				_cros_hook_##_hooktype##_##_routine) = { \
-		.info = &_cros_hook_##_hooktype##_info_##_routine,       \
+#define DECLARE_HOOK(_hooktype, _routine, _priority)                 \
+	STRUCT_SECTION_ITERABLE_ALTERNATE(                           \
+		zephyr_shim_hook_##_hooktype, zephyr_shim_hook_info, \
+		_cros_hook_##_hooktype##_##_routine) = {             \
+		.routine = _routine,                                 \
+		.priority = _priority,                               \
 	}
diff --git a/zephyr/shim/src/hooks.c b/zephyr/shim/src/hooks.c
index 7efb221f40..c5b2c0ec41 100644
--- a/zephyr/shim/src/hooks.c
+++ b/zephyr/shim/src/hooks.c
@@ -12,14 +12,40 @@
 #include "console.h"
 #include "ec_tasks.h"
 #include "hooks.h"
+#include "hook_types.h"
 #include "task.h"
 #include "timer.h"
 
+/*
+ * hook_registry maps each hook_type to the list of handlers for that hook type.
+ *
+ * Because this structure is not supported by the usual STRUCT_SECTION_FOREACH,
+ * this code must manually generate references to the symbols generated by
+ * STRUCT_SECTION_ITERABLE_ALTERNATE in zephyr_hooks_shim.h.
+ */
+#define HOOK_LIST_EXTERNS(type)                              \
+	extern const struct zephyr_shim_hook_info            \
+		_zephyr_shim_hook_##type##_list_start[]; \
+	extern const struct zephyr_shim_hook_info            \
+		_zephyr_shim_hook_##type##_list_end[];
+FOR_EACH(HOOK_LIST_EXTERNS, (), HOOK_TYPES_LIST)
+
+#define HOOK_LIST_ENTRY(type)                                       \
+	[type] = {                                                  \
+		.start = _zephyr_shim_hook_##type##_list_start, \
+		.end = _zephyr_shim_hook_##type##_list_end,     \
+	}
+static const struct zephyr_shim_hook_list hook_registry[] = {
+	FOR_EACH(HOOK_LIST_ENTRY, (,), HOOK_TYPES_LIST)
+};
+BUILD_ASSERT(ARRAY_SIZE(hook_registry) == HOOK_TYPE_COUNT,
+	     "All defined hook types must be represented in hook_registry");
+BUILD_ASSERT(NUM_VA_ARGS_LESS_1(HOOK_TYPES_LIST) + 1 == HOOK_TYPE_COUNT,
+	     "At least one hook type is missing from HOOK_TYPES_LIST");
+
 static void hook_second_work(struct k_work *work);
 static void hook_tick_work(struct k_work *work);
 
-static struct zephyr_shim_hook_list *hook_registry[HOOK_TYPE_COUNT];
-
 static K_WORK_DELAYABLE_DEFINE(hook_seconds_work_data, hook_second_work);
 static K_WORK_DELAYABLE_DEFINE(hook_ticks_work_data, hook_tick_work);
 
@@ -74,20 +100,6 @@ static int zephyr_shim_setup_hooks(const struct device *unused)
 {
 	int rv;
 
-	STRUCT_SECTION_FOREACH(zephyr_shim_hook_list, entry) {
-		struct zephyr_shim_hook_list **loc =
-			&hook_registry[entry->info->type];
-
-		/* Find the correct place to put the entry in the registry. */
-		while (*loc && (*loc)->info->priority < entry->info->priority)
-			loc = &((*loc)->next);
-
-		entry->next = *loc;
-
-		/* Insert the entry. */
-		*loc = entry;
-	}
-
 	/* Startup the HOOK_TICK and HOOK_SECOND recurring work */
 	rv = k_work_reschedule(&hook_seconds_work_data, K_SECONDS(1));
 	if (rv < 0)
@@ -105,10 +117,45 @@ SYS_INIT(zephyr_shim_setup_hooks, APPLICATION, 1);
 
 void hook_notify(enum hook_type type)
 {
-	struct zephyr_shim_hook_list *p;
+	const struct zephyr_shim_hook_info *start = hook_registry[type].start;
+	const struct zephyr_shim_hook_info *end = hook_registry[type].end;
+	int last_prio = HOOK_PRIO_FIRST - 1;
+
+	__ASSERT(type >= 0 && type < HOOK_TYPE_COUNT,
+		 "hook type %d is out of range (maximum hook_type value %d)",
+		 type, HOOK_TYPE_COUNT);
+
+	while (1) {
+		/*
+		 * Find the lowest priority that is larger than the last pass'
+		 * priority. That is, the next highest uncalled priority.
+		 */
+		int prio = INT_MAX;
+
+		for (const struct zephyr_shim_hook_info *p = start; p != end;
+		     p++) {
+			__ASSERT(
+				IN_RANGE(p->priority, HOOK_PRIO_FIRST,
+					 HOOK_PRIO_LAST),
+				"Hook priority %d (handler %p) is out of range",
+				p->priority, p->routine);
+			if (p->priority > last_prio)
+				prio = MIN(prio, p->priority);
+		}
 
-	for (p = hook_registry[type]; p; p = p->next)
-		p->info->routine();
+		if (prio == INT_MAX) {
+			/* No more handlers of higher priority */
+			break;
+		}
+		last_prio = prio;
+
+		/* Call each handler with the located priority */
+		for (const struct zephyr_shim_hook_info *p = start; p != end;
+		     p++) {
+			if (p->priority == prio)
+				p->routine();
+		}
+	};
 }
 
 int hook_call_deferred(const struct deferred_data *data, int us)
@@ -151,23 +198,23 @@ static void ev_handler(struct ap_power_ev_callback *cb,
 	default:
 		break;
 
-#define CASE_HOOK(h)				\
-	case AP_POWER_##h:				\
-		hook_notify(HOOK_CHIPSET_##h);	\
+#define CASE_HOOK(h)                           \
+	case AP_POWER_##h:                     \
+		hook_notify(HOOK_CHIPSET_##h); \
 		break
 
-	CASE_HOOK(PRE_INIT);
-	CASE_HOOK(STARTUP);
-	CASE_HOOK(RESUME);
-	CASE_HOOK(SUSPEND);
+		CASE_HOOK(PRE_INIT);
+		CASE_HOOK(STARTUP);
+		CASE_HOOK(RESUME);
+		CASE_HOOK(SUSPEND);
 #ifdef CONFIG_CHIPSET_RESUME_INIT_HOOK
-	CASE_HOOK(RESUME_INIT);
-	CASE_HOOK(SUSPEND_COMPLETE);
+		CASE_HOOK(RESUME_INIT);
+		CASE_HOOK(SUSPEND_COMPLETE);
 #endif
-	CASE_HOOK(SHUTDOWN);
-	CASE_HOOK(SHUTDOWN_COMPLETE);
-	CASE_HOOK(HARD_OFF);
-	CASE_HOOK(RESET);
+		CASE_HOOK(SHUTDOWN);
+		CASE_HOOK(SHUTDOWN_COMPLETE);
+		CASE_HOOK(HARD_OFF);
+		CASE_HOOK(RESET);
 	}
 }
 
@@ -181,19 +228,15 @@ static int zephyr_shim_ap_power_event(const struct device *unused)
 	/*
 	 * Register for all events.
 	 */
-	ap_power_ev_init_callback(&cb, ev_handler,
-				  AP_POWER_PRE_INIT |
-				  AP_POWER_STARTUP |
-				  AP_POWER_RESUME |
-				  AP_POWER_SUSPEND |
+	ap_power_ev_init_callback(
+		&cb, ev_handler,
+		AP_POWER_PRE_INIT | AP_POWER_STARTUP | AP_POWER_RESUME |
+			AP_POWER_SUSPEND |
 #ifdef CONFIG_CHIPSET_RESUME_INIT_HOOK
-				  AP_POWER_RESUME_INIT |
-				  AP_POWER_SUSPEND_COMPLETE |
+			AP_POWER_RESUME_INIT | AP_POWER_SUSPEND_COMPLETE |
 #endif
-				  AP_POWER_SHUTDOWN |
-				  AP_POWER_SHUTDOWN_COMPLETE |
-				  AP_POWER_HARD_OFF |
-				  AP_POWER_RESET);
+			AP_POWER_SHUTDOWN | AP_POWER_SHUTDOWN_COMPLETE |
+			AP_POWER_HARD_OFF | AP_POWER_RESET);
 	ap_power_ev_add_callback(&cb);
 	return 0;
 }
@@ -204,12 +247,12 @@ SYS_INIT(zephyr_shim_ap_power_event, APPLICATION, 1);
 /*
  * Events received from the hooks and sent to the AP power event callbacks.
  */
-#define EV_HOOK(h)						\
-static void hook_##h(void)					\
-{								\
-	ap_power_ev_send_callbacks(AP_POWER_##h);			\
-}								\
-DECLARE_HOOK(HOOK_CHIPSET_##h, hook_##h, HOOK_PRIO_DEFAULT)
+#define EV_HOOK(h)                                        \
+	static void hook_##h(void)                        \
+	{                                                 \
+		ap_power_ev_send_callbacks(AP_POWER_##h); \
+	}                                                 \
+	DECLARE_HOOK(HOOK_CHIPSET_##h, hook_##h, HOOK_PRIO_DEFAULT)
 
 EV_HOOK(PRE_INIT);
 EV_HOOK(STARTUP);