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>

1 files changed, 129 insertions, 27 deletions

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);
 }