Re: [PATCH v2 2/2] serial: tegra-utc: Add driver for Tegra UART Trace Controller (UTC)
From: Jiri Slaby
Date: Tue Feb 11 2025 - 02:24:54 EST
On 11. 02. 25, 7:19, Kartik Rajput wrote:
The Tegra264 SoC supports the UTC (UART Trace Controller), which allows
multiple firmware clients (up to 16) to share a single physical UART.
Each client is provided with its own interrupt and has access to a
128-character wide FIFO for both transmit (TX) and receive (RX)
operations.
Add tegra-utc driver to support Tegra UART Trace Controller (UTC)
client.
Signed-off-by: Kartik Rajput <kkartik@xxxxxxxxxx>
--- /dev/null
+++ b/drivers/tty/serial/tegra-utc.c
@@ -0,0 +1,622 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: Copyright (c) 2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+/*
+ * NVIDIA Tegra UTC (UART Trace Controller) driver.
+ */
+
+#include <linux/console.h>
+#include <linux/kthread.h>
Do you really use kthread somewhere?
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/string.h>
What's the reason for string.h?
+#include <linux/tty.h>
+#include <linux/tty_flip.h>
+
+#define TEGRA_UTC_ENABLE 0x0
+#define TEGRA_UTC_ENABLE_CLIENT_ENABLE BIT(0)
+
+#define TEGRA_UTC_FIFO_THRESHOLD 0x8
+
+#define TEGRA_UTC_COMMAND 0xc
+#define TEGRA_UTC_COMMAND_FLUSH BIT(1)
+#define TEGRA_UTC_COMMAND_RESET BIT(0)
+
+#define TEGRA_UTC_DATA 0x20
+
+#define TEGRA_UTC_FIFO_STATUS 0x100
+#define TEGRA_UTC_FIFO_TIMEOUT BIT(4)
+#define TEGRA_UTC_FIFO_OVERFLOW BIT(3)
+#define TEGRA_UTC_FIFO_REQ BIT(2)
+#define TEGRA_UTC_FIFO_FULL BIT(1)
+#define TEGRA_UTC_FIFO_EMPTY BIT(0)
It looks a bit weird to order bits from MSB to LSB.
+#define TEGRA_UTC_FIFO_OCCUPANCY 0x104
+
+#define TEGRA_UTC_INTR_STATUS 0x108
+#define TEGRA_UTC_INTR_SET 0x10c
+#define TEGRA_UTC_INTR_MASK 0x110
+#define TEGRA_UTC_INTR_CLEAR 0x114
+#define TEGRA_UTC_INTR_TIMEOUT BIT(4)
+#define TEGRA_UTC_INTR_OVERFLOW BIT(3)
+#define TEGRA_UTC_INTR_REQ BIT(2)
+#define TEGRA_UTC_INTR_FULL BIT(1)
+#define TEGRA_UTC_INTR_EMPTY BIT(0)
+
+#define UART_NR 16
+
+struct tegra_utc_soc {
+ unsigned int fifosize;
What is this struct good for, given you use a single value?
+struct tegra_utc_port {
+ const struct tegra_utc_soc *soc;
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+ struct console console;
+#endif
+ struct uart_port port;
+
+ void __iomem *rx_base;
+ void __iomem *tx_base;
+
+ u32 tx_irqmask;
+ u32 rx_irqmask;
+
+ u32 tx_threshold;
+ u32 rx_threshold;
+ int irq;
Why can't uart_port::irq be used instead?
+static bool tegra_utc_tx_char(struct tegra_utc_port *tup, u8 c)
+{
+ if (tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL)
+ return false;
+
+ tegra_utc_tx_writel(tup, c, TEGRA_UTC_DATA);
+
+ return true;
+}
+
+static bool tegra_utc_tx_chars(struct tegra_utc_port *tup)
To the least, you do not account TX stats. Why not to use uart_port_tx()?
+{
+ struct tty_port *tport = &tup->port.state->port;
+ u8 c;
+
+ if (tup->port.x_char) {
+ if (!tegra_utc_tx_char(tup, tup->port.x_char))
+ return true;
+
+ tup->port.x_char = 0;
+ }
+
+ if (kfifo_is_empty(&tport->xmit_fifo) || uart_tx_stopped(&tup->port)) {
+ tegra_utc_stop_tx(&tup->port);
+ return false;
+ }
+
+ while (kfifo_peek(&tport->xmit_fifo, &c)) {
+ if (!tegra_utc_tx_char(tup, c))
+ break;
+
+ kfifo_skip(&tport->xmit_fifo);
+ }
+
+ if (kfifo_len(&tport->xmit_fifo) < WAKEUP_CHARS)
+ uart_write_wakeup(&tup->port);
+
+ if (kfifo_is_empty(&tport->xmit_fifo)) {
+ tegra_utc_stop_tx(&tup->port);
+ return false;
+ }
+
+ return true;
+}
+
+static void tegra_utc_rx_chars(struct tegra_utc_port *tup)
+{
+ struct tty_port *port = &tup->port.state->port;
+ unsigned int max_chars = 256;
+ unsigned int flag;
Useless variable.
+ u32 status;
+ int sysrq;
+ u32 ch;
+
+ while (--max_chars) {
+ status = tegra_utc_rx_readl(tup, TEGRA_UTC_FIFO_STATUS);
+ if (status & TEGRA_UTC_FIFO_EMPTY)
+ break;
+
+ ch = tegra_utc_rx_readl(tup, TEGRA_UTC_DATA);
+ flag = TTY_NORMAL;
+ tup->port.icount.rx++;
+
+ if (status & TEGRA_UTC_FIFO_OVERFLOW)
+ tup->port.icount.overrun++;
+
+ uart_port_unlock(&tup->port);
+ sysrq = uart_handle_sysrq_char(&tup->port, ch & 0xff);
No need for "& 0xff".
+ uart_port_lock(&tup->port);
+
+ if (!sysrq)
+ tty_insert_flip_char(port, ch, flag);
You do not mask 'ch' here either. Both functions take 'u8'.
+ }
+
+ tty_flip_buffer_push(port);
+}
+
+static irqreturn_t tegra_utc_isr(int irq, void *dev_id)
+{
+ struct tegra_utc_port *tup = dev_id;
+ unsigned long flags;
+ u32 status;
+
+ uart_port_lock_irqsave(&tup->port, &flags);
+
+ /* Process RX_REQ and RX_TIMEOUT interrupts. */
+ do {
+ status = tegra_utc_rx_readl(tup, TEGRA_UTC_INTR_STATUS) & tup->rx_irqmask;
+ if (status) {
+ tegra_utc_rx_writel(tup, tup->rx_irqmask, TEGRA_UTC_INTR_CLEAR);
+ tegra_utc_rx_chars(tup);
+ }
+ } while (status);
+
+ /* Process TX_REQ interrupt. */
+ do {
+ status = tegra_utc_tx_readl(tup, TEGRA_UTC_INTR_STATUS) & tup->tx_irqmask;
+ if (status) {
+ tegra_utc_tx_writel(tup, tup->tx_irqmask, TEGRA_UTC_INTR_CLEAR);
+ tegra_utc_tx_chars(tup);
+ }
+ } while (status);
+
+ uart_port_unlock_irqrestore(&tup->port, flags);
+
+ return IRQ_HANDLED;
You do not let the irq subsystem to kill this IRQ if you do not handle
it above (in case HW gets mad, triggers IRQ, but does not set rx/tx
flags). That is, return IRQ_HANDLED only when you really handled it
(some status above was nonzero).
+}
+static int tegra_utc_startup(struct uart_port *port)
+{
+ struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+ int ret;
+
+ tegra_utc_hw_init(tup);
+
+ ret = request_irq(tup->irq, tegra_utc_isr, 0, dev_name(port->dev), tup);
Just asking: so it can never be shared, right?
+ if (ret < 0) {
+ dev_err(port->dev, "failed to register interrupt handler\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static void tegra_utc_shutdown(struct uart_port *port)
+{
+ struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+ tegra_utc_rx_writel(tup, 0x0, TEGRA_UTC_ENABLE);
Writes cannot be posted on this bus, right?
+ free_irq(tup->irq, tup);
+}
...
+static int tegra_utc_get_poll_char(struct uart_port *port)
+{
+ struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+ while (tegra_utc_rx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_EMPTY)
+ cpu_relax();
Hmm, there should be a timeout. Can't you use read_poll_timeout_atomic()?
+ return tegra_utc_rx_readl(tup, TEGRA_UTC_DATA);
+}
+
+static void tegra_utc_put_poll_char(struct uart_port *port, unsigned char ch)
+{
+ struct tegra_utc_port *tup = container_of(port, struct tegra_utc_port, port);
+
+ while (tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_STATUS) & TEGRA_UTC_FIFO_FULL)
+ cpu_relax();
Detto.
+ tegra_utc_tx_writel(tup, ch, TEGRA_UTC_DATA);
+}
+
+#endif
+static void tegra_utc_console_write(struct console *cons, const char *s, unsigned int count)
+{
+ struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+ unsigned long flags;
+ int locked = 1;
+
+ if (tup->port.sysrq || oops_in_progress)
+ locked = uart_port_trylock_irqsave(&tup->port, &flags);
+ else
+ uart_port_lock_irqsave(&tup->port, &flags);
+
+ while (count) {
+ u32 burst_size = tup->soc->fifosize;
+
+ burst_size -= tegra_utc_tx_readl(tup, TEGRA_UTC_FIFO_OCCUPANCY);
+ if (count < burst_size)
+ burst_size = count;
+
+ uart_console_write(&tup->port, s, burst_size, tegra_utc_console_putchar);
+
+ count -= burst_size;
+ s += burst_size;
+ };
+
+ if (locked)
+ uart_port_unlock_irqrestore(&tup->port, flags);
+}
+
+static int tegra_utc_console_setup(struct console *cons, char *options)
+{
+ struct tegra_utc_port *tup = container_of(cons, struct tegra_utc_port, console);
+
+ tegra_utc_init_tx(tup);
+
+ return 0;
+}
+#endif
+
+static struct uart_driver tegra_utc_driver = {
+ .driver_name = "tegra-utc",
+ .dev_name = "ttyUTC",
+ .nr = UART_NR
+};
+
+static void tegra_utc_setup_port(struct device *dev, struct tegra_utc_port *tup)
+{
+ tup->port.dev = dev;
+ tup->port.fifosize = tup->soc->fifosize;
+ tup->port.flags = UPF_BOOT_AUTOCONF;
+ tup->port.iotype = UPIO_MEM;
+ tup->port.ops = &tegra_utc_uart_ops;
+ tup->port.type = PORT_TEGRA_TCU;
+ tup->port.private_data = tup;
+
+#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
+ strscpy(tup->console.name, "ttyUTC", sizeof(tup->console.name));
+ tup->console.write = tegra_utc_console_write;
+ tup->console.device = uart_console_device;
+ tup->console.setup = tegra_utc_console_setup;
+ tup->console.flags = CON_PRINTBUFFER | CON_CONSDEV | CON_ANYTIME;
New code shall be CON_NBCON compatible. You should implement
::write_atomic/thread et al. instead of bare ::write.
+ tup->console.data = &tegra_utc_driver;
+#endif
+
+ uart_read_port_properties(&tup->port);
+}
+static void tegra_utc_remove(struct platform_device *pdev)
+{
+ struct tegra_utc_port *tup = platform_get_drvdata(pdev);
+
No unregister_console()?
+ uart_remove_one_port(&tegra_utc_driver, &tup->port);
+}
thanks,
--
js
suse labs