zephyr: use interrupt based RX buffering

Poll-based RX was technically incorrect since Zephyr was setting up
the UART to FIFO mode (instead of byte mode). This change replaces
polling the RX UART with interrupt based callbacks. When calling
uart_shell_stop(), the ISR will be replaced and will instead queue
items to a ring buffer which can be read from uart_getc() (it can
also be cleared via the uart_clear_input() function).

BRANCH=none
BUG=b:181352041
TEST=Build volteer, run, see expected 0xec07 from the GSC.

Cq-Depend: chromium:2730870, chromium:2730869
Signed-off-by: Yuval Peress <peress@chromium.org>
Change-Id: I5d2b61e914b56f678a259b373969522da87e8df3
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2728824
Reviewed-by: Keith Short <keithshort@chromium.org>
Commit-Queue: Keith Short <keithshort@chromium.org>

5 files changed, 141 insertions, 43 deletions

diff --git a/common/vboot/efs2.c b/common/vboot/efs2.c
index b01b82bf8c..7c04d2460e 100644
--- a/common/vboot/efs2.c
+++ b/common/vboot/efs2.c
@@ -71,7 +71,11 @@ static enum cr50_comm_err send_to_cr50(const uint8_t *data, size_t size)
 	uart_flush_output();
 	uart_clear_input();
 
-	uart_shell_stop();
+	if (uart_shell_stop()) {
+		/* Failed to stop the shell. */
+		enable_packet_mode(false);
+		return CR50_COMM_ERR_UNKNOWN;
+	}
 
 	/*
 	 * Send packet. No traffic control, assuming Cr50 consumes stream much
@@ -106,18 +110,7 @@ static enum cr50_comm_err send_to_cr50(const uint8_t *data, size_t size)
 				res.error = res.error | c << (i*8);
 				break;
 			}
-			/*
-			 * TODO(b:181352041) Implement proper RX buffering.
-			 * Zephyr's shell (even when "stopped") can steal some
-			 * bytes from the uart RX. Skipping the sleep here
-			 * appears to always let us capture the response. Once
-			 * we're able to fork the shell RX, we'll be able to
-			 * buffer the response and add the sleep back into the
-			 * Zephyr builds here (or alternatively use event
-			 * signals).
-			 */
-			if (!IS_ENABLED(CONFIG_ZEPHYR))
-				msleep(1);
+			msleep(1);
 			timeout = timestamp_expired(until, NULL);
 		}
 	}
diff --git a/include/console.h b/include/console.h
index f517d3e074..91800103b6 100644
--- a/include/console.h
+++ b/include/console.h
@@ -23,11 +23,12 @@
  * briefly taking control of the uart.
  */
 #ifdef CONFIG_ZEPHYR
-void uart_shell_stop(void);
+int uart_shell_stop(void);
 void uart_shell_start(void);
 #else
-static inline void uart_shell_stop(void)
+static inline int uart_shell_stop(void)
 {
+	return 0;
 }
 static inline void uart_shell_start(void)
 {
diff --git a/zephyr/Kconfig.stacks b/zephyr/Kconfig.stacks
index 6bcb52fd18..bd86810dac 100644
--- a/zephyr/Kconfig.stacks
+++ b/zephyr/Kconfig.stacks
@@ -9,7 +9,7 @@ config ISR_STACK_SIZE
 	default 1024
 
 config SHELL_STACK_SIZE
-	default 960
+	default 1536
 
 # Chromium EC stack sizes
 
diff --git a/zephyr/projects/volteer/volteer/prj.conf b/zephyr/projects/volteer/volteer/prj.conf
index b2a68bebd8..32a2ac8118 100644
--- a/zephyr/projects/volteer/volteer/prj.conf
+++ b/zephyr/projects/volteer/volteer/prj.conf
@@ -4,6 +4,8 @@
 
 CONFIG_CROS_EC=y
 
+CONFIG_WATCHDOG=y
+
 # SoC configuration
 CONFIG_AP=y
 CONFIG_AP_X86_INTEL_TGL=y
diff --git a/zephyr/shim/src/console.c b/zephyr/shim/src/console.c
index c4311761f3..22272a9c8d 100644
--- a/zephyr/shim/src/console.c
+++ b/zephyr/shim/src/console.c
@@ -5,53 +5,152 @@
 
 #include <device.h>
 #include <drivers/uart.h>
-#include <init.h>
-#include <kernel.h>
 #include <shell/shell.h>
 #include <shell/shell_uart.h>
 #include <stdbool.h>
 #include <string.h>
 #include <sys/printk.h>
+#include <sys/ring_buffer.h>
 #include <zephyr.h>
 
 #include "console.h"
 #include "printf.h"
 #include "uart.h"
 
-static const struct device *uart_dev;
-#ifdef CONFIG_UART_CONSOLE_ON_DEV_NAME
-static int init_uart_dev(const struct device *unused)
+static struct k_poll_signal shell_uninit_signal;
+static struct k_poll_signal shell_init_signal;
+RING_BUF_DECLARE(rx_buffer, CONFIG_UART_RX_BUF_SIZE);
+
+static void uart_rx_handle(const struct device *dev)
 {
-	ARG_UNUSED(unused);
-	uart_dev = device_get_binding(CONFIG_UART_CONSOLE_ON_DEV_NAME);
-	return 0;
+	static uint8_t scratch;
+	static uint8_t *data;
+	static uint32_t len, rd_len;
+
+	do {
+		/* Get some bytes on the ring buffer */
+		len = ring_buf_put_claim(&rx_buffer, &data, rx_buffer.size);
+		if (len > 0) {
+			/* Read from the FIFO up to `len` bytes */
+			rd_len = uart_fifo_read(dev, data, len);
+
+			/* Put `rd_len` bytes on the ring buffer */
+			ring_buf_put_finish(&rx_buffer, rd_len);
+		} else {
+			/* There's no room on the ring buffer, throw away 1
+			 * byte.
+			 */
+			rd_len = uart_fifo_read(dev, &scratch, 1);
+		}
+	} while (rd_len != 0 && rd_len == len);
 }
-SYS_INIT(init_uart_dev, POST_KERNEL, 50);
-#endif
 
-void uart_shell_stop(void)
+static void uart_callback(const struct device *dev, void *user_data)
 {
-	/* Disable interrupts for the uart. */
-	if (uart_dev) {
-		uart_irq_tx_disable(uart_dev);
-		uart_irq_rx_disable(uart_dev);
+	uart_irq_update(dev);
+
+	if (uart_irq_rx_ready(dev))
+		uart_rx_handle(dev);
+}
+
+static void shell_uninit_callback(const struct shell *shell, int res)
+{
+	const struct device *dev =
+		device_get_binding(CONFIG_UART_SHELL_ON_DEV_NAME);
+
+	if (!res) {
+		/* Set the new callback */
+		uart_irq_callback_user_data_set(dev, uart_callback, NULL);
+
+		/* Disable TX interrupts. We don't actually use TX but for some
+		 * reason none of this works without this line.
+		 */
+		uart_irq_tx_disable(dev);
+
+		/* Enable RX interrupts */
+		uart_irq_rx_enable(dev);
 	}
 
-	/* Stop the shell and process all pending operations. */
-	shell_stop(shell_backend_uart_get_ptr());
-	shell_process(shell_backend_uart_get_ptr());
+	/* Notify the uninit signal that we finished */
+	k_poll_signal_raise(&shell_uninit_signal, res);
 }
 
-void uart_shell_start(void)
+int uart_shell_stop(void)
 {
-	/* Restart the shell. */
-	shell_start(shell_backend_uart_get_ptr());
+	struct k_poll_event event = K_POLL_EVENT_INITIALIZER(
+		K_POLL_TYPE_SIGNAL, K_POLL_MODE_NOTIFY_ONLY,
+		&shell_uninit_signal);
+	const struct device *dev =
+		device_get_binding(CONFIG_UART_SHELL_ON_DEV_NAME);
+
+	/* Clear all pending input */
+	uart_clear_input();
+
+	/* Disable RX and TX interrupts */
+	uart_irq_rx_disable(dev);
+	uart_irq_tx_disable(dev);
+
+	/* Initialize the uninit signal */
+	k_poll_signal_init(&shell_uninit_signal);
+
+	/* Stop the shell */
+	shell_uninit(shell_backend_uart_get_ptr(), shell_uninit_callback);
 
-	/* Re-enable interrupts for the uart. */
-	if (uart_dev) {
-		uart_irq_rx_enable(uart_dev);
-		uart_irq_tx_enable(uart_dev);
+	/* Wait for the shell to be turned off, the signal will wake us */
+	k_poll(&event, 1, K_FOREVER);
+
+	/* Event was signaled, return the result */
+	return event.signal->result;
+}
+
+static void shell_init_from_work(struct k_work *work)
+{
+	const struct device *dev =
+		device_get_binding(CONFIG_UART_SHELL_ON_DEV_NAME);
+	bool log_backend = CONFIG_SHELL_BACKEND_SERIAL_LOG_LEVEL > 0;
+	uint32_t level;
+	ARG_UNUSED(work);
+
+	if (CONFIG_SHELL_BACKEND_SERIAL_LOG_LEVEL > LOG_LEVEL_DBG) {
+		level = CONFIG_LOG_MAX_LEVEL;
+	} else {
+		level = CONFIG_SHELL_BACKEND_SERIAL_LOG_LEVEL;
 	}
+
+	/* Initialize the shell and re-enable both RX and TX */
+	shell_init(shell_backend_uart_get_ptr(), dev, false, log_backend,
+		   level);
+	uart_irq_rx_enable(dev);
+	uart_irq_tx_enable(dev);
+
+	/* Notify the init signal that initialization is complete */
+	k_poll_signal_raise(&shell_init_signal, 0);
+}
+
+void uart_shell_start(void)
+{
+	static struct k_work shell_init_work;
+	const struct device *dev =
+		device_get_binding(CONFIG_UART_SHELL_ON_DEV_NAME);
+	struct k_poll_event event = K_POLL_EVENT_INITIALIZER(
+		K_POLL_TYPE_SIGNAL, K_POLL_MODE_NOTIFY_ONLY,
+		&shell_init_signal);
+
+	/* Disable RX and TX interrupts */
+	uart_irq_rx_disable(dev);
+	uart_irq_tx_disable(dev);
+
+	/* Initialize k_work to call shell init (this makes it thread safe) */
+	k_work_init(&shell_init_work, shell_init_from_work);
+
+	/* Initialize the init signal to make sure we're read to listen */
+	k_poll_signal_init(&shell_init_signal);
+
+	/* Submit the work to be run by the kernel */
+	k_work_submit(&shell_init_work);
+
+	/* Wait for initialization to be run, the signal will wake us */
+	k_poll(&event, 1, K_FOREVER);
 }
 
 int zshim_run_ec_console_command(int (*handler)(int argc, char **argv),
@@ -136,12 +235,15 @@ int uart_getc(void)
 {
 	uint8_t c;
 
-	if (uart_dev && !uart_poll_in(uart_dev, &c))
+	if (ring_buf_get(&rx_buffer, &c, 1)) {
 		return c;
+	}
 	return -1;
 }
 
 void uart_clear_input(void)
 {
-	/* Not needed since we're not stopping the shell. */
+	/* Clear any remaining shell processing. */
+	shell_process(shell_backend_uart_get_ptr());
+	ring_buf_reset(&rx_buffer);
 }