usb_pd_timer: use atomic_*_bit functions

There are many atomic operations on the timer_active and timer_disabled
variables. Change their type to atomic_t and use atomic_*_bit functions.

It simplifies and unifies the bit operations. Also, the change saves
memory 240-512B of FLASH depending on a board.

BUG=b:208435177
TEST=zmake testall & make buildall & run faft_pd and make sure there is
no regression
BRANCH=none

Signed-off-by: Dawid Niedzwiecki <dn@semihalf.com>
Change-Id: I880d07b2fe51b27bea1aacba884eacaede6476c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/3488374
Reviewed-by: Denis Brockus <dbrockus@chromium.org>
Commit-Queue: Dawid Niedzwiecki <dawidn@google.com>

3 files changed, 74 insertions, 153 deletions

diff --git a/common/usbc/usb_pd_timer.c b/common/usbc/usb_pd_timer.c
index c4837c1448..d215590371 100644
--- a/common/usbc/usb_pd_timer.c
+++ b/common/usbc/usb_pd_timer.c
@@ -5,6 +5,7 @@
 
 #include "assert.h"
 #include "atomic.h"
+#include "atomic_bit.h"
 #include "common.h"
 #include "console.h"
 #include "limits.h"
@@ -21,42 +22,29 @@
 #define NO_TIMEOUT	(-1)
 #define EXPIRE_NOW	(0)
 
-#define PD_SET_ACTIVE(p, m)	pd_timer_atomic_op(			\
-					atomic_or,			\
-					(atomic_t *)timer_active[p],	\
-					(m))
-#define PD_CLR_ACTIVE(p, m)	pd_timer_atomic_op(			\
-					atomic_clear_bits,		\
-					(atomic_t *)timer_active[p],	\
-					(m))
-#define PD_CHK_ACTIVE(p, m)	((timer_active[p][0] & ((m) >> 32)) |	\
-				 (timer_active[p][1] & (m)))
-
-#define PD_SET_DISABLED(p, m)	pd_timer_atomic_op(			\
-					atomic_or,			\
-					(atomic_t *)timer_disabled[p],	\
-					(m))
-#define PD_CLR_DISABLED(p, m)	pd_timer_atomic_op(			\
-					atomic_clear_bits,		\
-					(atomic_t *)timer_disabled[p],	\
-					(m))
-#define PD_CHK_DISABLED(p, m)	((timer_disabled[p][0] & ((m) >> 32)) | \
-				 (timer_disabled[p][1] & (m)))
-
-#define TIMER_FIELD_NUM_UINT32S 2
+#define PD_SET_ACTIVE(p, bit) \
+	atomic_set_bit(timer_active, (p) * PD_TIMER_COUNT + (bit))
+
+#define PD_CLR_ACTIVE(p, bit) \
+	atomic_clear_bit(timer_active, (p) * PD_TIMER_COUNT + (bit))
+
+#define PD_CHK_ACTIVE(p, bit) \
+	atomic_test_bit(timer_active, (p) * PD_TIMER_COUNT + (bit))
+
+#define PD_SET_DISABLED(p, bit) \
+	atomic_set_bit(timer_disabled, (p) * PD_TIMER_COUNT + (bit))
+
+#define PD_CLR_DISABLED(p, bit) \
+	atomic_clear_bit(timer_disabled, (p) * PD_TIMER_COUNT + (bit))
+
+#define PD_CHK_DISABLED(p, bit) \
+	atomic_test_bit(timer_disabled, (p) * PD_TIMER_COUNT + (bit))
 
-/*
- * Use uint32_t for timer_active and timer_disabled instead of atomic_t,
- * because mixing types signed and unsigned around shifting may lead to
- * undefined behavior.
- */
 test_mockable_static
-uint32_t timer_active[MAX_PD_PORTS][TIMER_FIELD_NUM_UINT32S];
+ATOMIC_DEFINE(timer_active, PD_TIMER_COUNT * MAX_PD_PORTS);
 test_mockable_static
-uint32_t timer_disabled[MAX_PD_PORTS][TIMER_FIELD_NUM_UINT32S];
-static uint64_t timer_expires[MAX_PD_PORTS][MAX_PD_TIMERS];
-BUILD_ASSERT(sizeof(timer_active[0]) * CHAR_BIT >= PD_TIMER_COUNT);
-BUILD_ASSERT(sizeof(timer_disabled[0]) * CHAR_BIT >= PD_TIMER_COUNT);
+ATOMIC_DEFINE(timer_disabled, PD_TIMER_COUNT * MAX_PD_PORTS);
+static uint64_t timer_expires[MAX_PD_PORTS][PD_TIMER_COUNT];
 
 /*
  * CONFIG_CMD_PD_TIMER debug variables
@@ -113,73 +101,25 @@ __maybe_unused static __const_data const char * const pd_timer_names[] = {
  * will not adjust the task scheduling timeout value.
  */
 
-/*
- * Performs an atomic operation on a PD timer bit field. Atomic operations
- * require 32-bit operands, but there are more than 32 timers, so choose the
- * correct operand and modify the mask accordingly.
- *
- * @param op          Atomic operation function to call
- * @param timer_field Array of timer fields to operate on
- * @param mask_val    64-bit mask to apply to the timer field
- */
-test_mockable_static void pd_timer_atomic_op(
-		atomic_val_t (*op)(atomic_t*, atomic_val_t),
-		atomic_t *const timer_field, const uint64_t mask_val)
-{
-	atomic_t *atomic_timer_field;
-	union mask64_t {
-		struct {
-#if (__BYTE_ORDER__  == __ORDER_LITTLE_ENDIAN__)
-			uint32_t lo;
-			uint32_t hi;
-#elif (__BYTE_ORDER__ == __ORDER_BIG_ENDIAN__)
-			uint32_t hi;
-			uint32_t lo;
-#endif
-		};
-		uint64_t val;
-	} mask;
-
-	/*
-	 * High-order mask bits correspond to field [0]. Low-order mask bits
-	 * correspond to field [1].
-	 */
-	mask.val = mask_val;
-	if (mask.hi) {
-		atomic_timer_field = timer_field;
-		(void)op(atomic_timer_field, mask.hi);
-	}
-	if (mask.lo) {
-		atomic_timer_field = timer_field + 1;
-		(void)op(atomic_timer_field, mask.lo);
-	}
-}
-
 static void pd_timer_inactive(int port, enum pd_task_timer timer)
 {
-	uint64_t mask = bitmask_uint64(timer);
-
-	if (PD_CHK_ACTIVE(port, mask)) {
-		PD_CLR_ACTIVE(port, mask);
+	if (PD_CHK_ACTIVE(port, timer)) {
+		PD_CLR_ACTIVE(port, timer);
 
 		if (IS_ENABLED(CONFIG_CMD_PD_TIMER))
 			count[port]--;
 	}
-	PD_CLR_DISABLED(port, mask);
+	PD_CLR_DISABLED(port, timer);
 }
 
 static bool pd_timer_is_active(int port, enum pd_task_timer timer)
 {
-	uint64_t mask = bitmask_uint64(timer);
-
-	return PD_CHK_ACTIVE(port, mask);
+	return PD_CHK_ACTIVE(port, timer);
 }
 
 static bool pd_timer_is_inactive(int port, enum pd_task_timer timer)
 {
-	uint64_t mask = bitmask_uint64(timer);
-
-	return !PD_CHK_ACTIVE(port, mask) && !PD_CHK_DISABLED(port, mask);
+	return !PD_CHK_ACTIVE(port, timer) && !PD_CHK_DISABLED(port, timer);
 }
 
 /*****************************************************************************
@@ -190,16 +130,20 @@ void pd_timer_init(int port)
 	if (IS_ENABLED(CONFIG_CMD_PD_TIMER))
 		count[port] = 0;
 
-	PD_CLR_ACTIVE(port, PD_TIMERS_ALL_MASK);
-	PD_SET_DISABLED(port, PD_TIMERS_ALL_MASK);
+	/*
+	 * timer_active and timer_disabled are atomic_t global arrays.
+	 * Set them to the initial state.
+	 */
+	for (int i = 0; i < ARRAY_SIZE(timer_active); i++) {
+		*(timer_active + i) = 0;
+		*(timer_disabled + i) = ~0;
+	}
 }
 
 void pd_timer_enable(int port, enum pd_task_timer timer, uint32_t expires_us)
 {
-	uint64_t mask = bitmask_uint64(timer);
-
-	if (!PD_CHK_ACTIVE(port, mask)) {
-		PD_SET_ACTIVE(port, mask);
+	if (!PD_CHK_ACTIVE(port, timer)) {
+		PD_SET_ACTIVE(port, timer);
 
 		if (IS_ENABLED(CONFIG_CMD_PD_TIMER)) {
 			count[port]++;
@@ -207,21 +151,19 @@ void pd_timer_enable(int port, enum pd_task_timer timer, uint32_t expires_us)
 				max_count[port] = count[port];
 		}
 	}
-	PD_CLR_DISABLED(port, mask);
+	PD_CLR_DISABLED(port, timer);
 	timer_expires[port][timer] = get_time().val + expires_us;
 }
 
 void pd_timer_disable(int port, enum pd_task_timer timer)
 {
-	uint64_t mask = bitmask_uint64(timer);
-
-	if (PD_CHK_ACTIVE(port, mask)) {
-		PD_CLR_ACTIVE(port, mask);
+	if (PD_CHK_ACTIVE(port, timer)) {
+		PD_CLR_ACTIVE(port, timer);
 
 		if (IS_ENABLED(CONFIG_CMD_PD_TIMER))
 			count[port]--;
 	}
-	PD_SET_DISABLED(port, mask);
+	PD_SET_DISABLED(port, timer);
 }
 
 void pd_timer_disable_range(int port, enum pd_timer_range range)
@@ -252,9 +194,7 @@ void pd_timer_disable_range(int port, enum pd_timer_range range)
 
 bool pd_timer_is_disabled(int port, enum pd_task_timer timer)
 {
-	uint64_t mask = bitmask_uint64(timer);
-
-	return PD_CHK_DISABLED(port, mask);
+	return PD_CHK_DISABLED(port, timer);
 }
 
 bool pd_timer_is_expired(int port, enum pd_task_timer timer)
@@ -273,11 +213,10 @@ void pd_timer_manage_expired(int port)
 {
 	int timer;
 
-	if (timer_active[port])
-		for (timer = 0; timer < MAX_PD_TIMERS; ++timer)
-			if (pd_timer_is_active(port, timer) &&
-			    pd_timer_is_expired(port, timer))
-				pd_timer_inactive(port, timer);
+	for (timer = 0; timer < PD_TIMER_COUNT; ++timer)
+		if (pd_timer_is_active(port, timer) &&
+		    pd_timer_is_expired(port, timer))
+			pd_timer_inactive(port, timer);
 }
 
 int pd_timer_next_expiration(int port)
@@ -286,7 +225,7 @@ int pd_timer_next_expiration(int port)
 	int ret_value = MAX_EXPIRE;
 	uint64_t now = get_time().val;
 
-	for (timer = 0; timer < MAX_PD_TIMERS; ++timer) {
+	for (timer = 0; timer < PD_TIMER_COUNT; ++timer) {
 		/* Only use active timers for the next expired value */
 		if (pd_timer_is_active(port, timer)) {
 			int delta;
@@ -318,7 +257,7 @@ test_mockable_static void pd_timer_dump(int port)
 	ccprints("Timers(%d): cur=%d max=%d",
 		port, count[port], max_count[port]);
 
-	for (timer = 0; timer < MAX_PD_TIMERS; ++timer) {
+	for (timer = 0; timer < PD_TIMER_COUNT; ++timer) {
 		if (pd_timer_is_disabled(port, timer)) {
 			continue;
 		} else if (pd_timer_is_active(port, timer)) {
diff --git a/include/usb_pd_timer.h b/include/usb_pd_timer.h
index 1ed268f14d..3a3f388b22 100644
--- a/include/usb_pd_timer.h
+++ b/include/usb_pd_timer.h
@@ -11,6 +11,7 @@
 #include <stdbool.h>
 
 #include "atomic.h"
+#include "atomic_bit.h"
 
 /*
  * List of all timers that will be managed by usb_pd_timer
@@ -333,43 +334,28 @@ void pd_timer_dump(int port);
 
 /* exported: number of USB-C ports */
 #define MAX_PD_PORTS		CONFIG_USB_PD_PORT_MAX_COUNT
-/* exported: number of uint32_t fields for bit mask to all timers */
-#define TIMER_FIELD_NUM_UINT32S	2
 
 /* PD timers have three possible states: Active, Inactive and Disabled */
 /* exported: timer_active indicates if a timer is currently active */
-extern uint32_t timer_active[MAX_PD_PORTS][TIMER_FIELD_NUM_UINT32S];
+extern ATOMIC_DEFINE(timer_active, PD_TIMER_COUNT * MAX_PD_PORTS);
 /* exported: timer_disabled indicates if a timer is currently disabled */
-extern uint32_t timer_disabled[MAX_PD_PORTS][TIMER_FIELD_NUM_UINT32S];
-
-/* exported: do not call directly, only for the defined macros */
-extern void pd_timer_atomic_op(
-		atomic_val_t (*op)(atomic_t*, atomic_val_t),
-		atomic_t *const timer_field, const uint64_t mask);
+extern ATOMIC_DEFINE(timer_disabled, PD_TIMER_COUNT * MAX_PD_PORTS);
 
 /* exported: set/clear/check the current timer_active for a timer */
-#define PD_SET_ACTIVE(p, m)	pd_timer_atomic_op(			\
-					atomic_or,			\
-					(atomic_t *)timer_active[p],	\
-					(m))
-#define PD_CLR_ACTIVE(p, m)	pd_timer_atomic_op(			\
-					atomic_clear_bits,		\
-					(atomic_t *)timer_active[p],	\
-					(m))
-#define PD_CHK_ACTIVE(p, m)	((timer_active[p][0] & ((m) >> 32)) | \
-				 (timer_active[p][1] & (m)))
+#define PD_SET_ACTIVE(p, bit) \
+	atomic_set_bit(timer_active, (p) * PD_TIMER_COUNT + (bit))
+#define PD_CLR_ACTIVE(p, bit) \
+	atomic_clear_bit(timer_active, (p) * PD_TIMER_COUNT + (bit))
+#define PD_CHK_ACTIVE(p, bit) \
+	atomic_test_bit(timer_active, (p) * PD_TIMER_COUNT + (bit))
 
 /* exported: set/clear/check the current timer_disabled for a timer */
-#define PD_SET_DISABLED(p, m)	pd_timer_atomic_op(			\
-					atomic_or,			\
-					(atomic_t *)timer_disabled[p],	\
-					(m))
-#define PD_CLR_DISABLED(p, m)	pd_timer_atomic_op(			\
-					atomic_clear_bits,		\
-					(atomic_t *)timer_disabled[p],	\
-					(m))
-#define PD_CHK_DISABLED(p, m)	((timer_disabled[p][0] & ((m) >> 32)) | \
-				 (timer_disabled[p][1] & (m)))
+#define PD_SET_DISABLED(p, bit) \
+	atomic_set_bit(timer_disabled, (p) * PD_TIMER_COUNT + (bit))
+#define PD_CLR_DISABLED(p, bit) \
+	atomic_clear_bit(timer_disabled, (p) * PD_TIMER_COUNT + (bit))
+#define PD_CHK_DISABLED(p, bit) \
+	atomic_test_bit(timer_disabled, (p) * PD_TIMER_COUNT + (bit))
 
 #endif /* TEST_BUILD */
 
diff --git a/test/usb_pd_timer.c b/test/usb_pd_timer.c
index 38ed0cda78..b6378ca1f8 100644
--- a/test/usb_pd_timer.c
+++ b/test/usb_pd_timer.c
@@ -26,26 +26,24 @@ int test_pd_timers_bit_ops(void)
 	 */
 	pd_timer_init(port);
 	for (bit = 0; bit < PD_TIMER_COUNT; ++bit)
-		TEST_EQ(PD_CHK_ACTIVE(port, 1ULL << bit), 0ULL, "%llu");
+		TEST_EQ(PD_CHK_ACTIVE(port, bit), 0, "%d");
 	for (bit = 0; bit < PD_TIMER_COUNT; ++bit)
-		TEST_NE(PD_CHK_DISABLED(port, 1ULL << bit), 0ULL, "%llu");
+		TEST_NE(PD_CHK_DISABLED(port, bit), 0, "%d");
 
 	/*
 	 * Set one active bit at a time and verify it is the only bit set. Reset
 	 * the bit on each iteration of the bit loop.
 	 */
 	for (bit = 0; bit < PD_TIMER_COUNT; ++bit) {
-		TEST_EQ(PD_CHK_ACTIVE(port, 1ULL << bit), 0ULL, "%llu");
-		PD_SET_ACTIVE(port, 1ULL << bit);
+		TEST_EQ(PD_CHK_ACTIVE(port, bit), 0, "%d");
+		PD_SET_ACTIVE(port, bit);
 		for (int i = 0; i < PD_TIMER_COUNT; ++i) {
 			if (i != bit)
-				TEST_EQ(PD_CHK_ACTIVE(port, 1ULL << i), 0ULL,
-						"%llu");
+				TEST_EQ(PD_CHK_ACTIVE(port, i), 0, "%d");
 			else
-				TEST_NE(PD_CHK_ACTIVE(port, 1ULL << i), 0ULL,
-						"%llu");
+				TEST_NE(PD_CHK_ACTIVE(port, i), 0, "%d");
 		}
-		PD_CLR_ACTIVE(port, 1ULL << bit);
+		PD_CLR_ACTIVE(port, bit);
 	}
 
 	/*
@@ -53,17 +51,15 @@ int test_pd_timers_bit_ops(void)
 	 * Reset the bit on each iteration of the bit loop.
 	 */
 	for (bit = 0; bit < PD_TIMER_COUNT; ++bit) {
-		TEST_NE(PD_CHK_DISABLED(port, 1ULL << bit), 0ULL, "%llu");
-		PD_CLR_DISABLED(port, 1ULL << bit);
+		TEST_NE(PD_CHK_DISABLED(port, bit), 0, "%d");
+		PD_CLR_DISABLED(port, bit);
 		for (int i = 0; i < PD_TIMER_COUNT; ++i) {
 			if (i != bit)
-				TEST_NE(PD_CHK_DISABLED(port, 1ULL << i), 0ULL,
-						"%llu");
+				TEST_NE(PD_CHK_DISABLED(port, i), 0, "%d");
 			else
-				TEST_EQ(PD_CHK_DISABLED(port, 1ULL << i), 0ULL,
-						"%llu");
+				TEST_EQ(PD_CHK_DISABLED(port, i), 0, "%d");
 		}
-		PD_SET_DISABLED(port, 1ULL << bit);
+		PD_SET_DISABLED(port, bit);
 	}
 
 	return EC_SUCCESS;